Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

if-statements allow multiple arguments and don't check for constant conditions after the first argument #60822

Open
MajorMeerkatThe3rd opened this issue Dec 19, 2024 · 7 comments Β· May be fixed by #60830

Comments

@MajorMeerkatThe3rd
Copy link

πŸ”Ž Search Terms

"if statement", "constant condition"

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about common bugs

⏯ Playground Link

https://www.typescriptlang.org/play/?#code/MYewdgzgLgBBIFsCmBhAFk4BrAlmA5gGICuYwUO4MAvDABQAOAhgE5MICMAXHFC3vgA0MZmwQAmHtH4FhIBhXBMANgAVW7AMw0YfYkgCUNAHwwA3gCgY1mAHpbMACYhexAGZurNlkijEWYLos+gDcFgC+Fhb2MCoA7kwAnhAwbioQSMI+EMTKsCBucIioGNgCJGSKgfFJKTj4YCA+jhY4bnTwyOiYuAQV5JRgdABEouwcw8KjGhLDBsJpyhlGljYwEVExjkgMSGDbYPmB2bn5hZ0lPeWkA+Ct7RfdZX03VSNjnJMw02LiX4vLFZeawRIA

πŸ’» Code

const someCheckingFunction = (param1: string, param2: string, optionalParam3 = true) => {
    // do stuff
    return true;
}

// always false, result of someCheckingFunction always ignored
if(someCheckingFunction("param1", "param2"), false) {
    
}

// dependent on result of someCheckingFunction
if(someCheckingFunction("param1", "param2", false)) {
    
}

πŸ™ Actual behavior

Due to a mistake in my bracket placement as seen in the example, I accidentally learned that multiple arguments in an if statement are possible.

Currently that means:

  1. only the last argument is used for the if statement check
  2. even for constant conditions there is not error visible

πŸ™‚ Expected behavior

What I would expect:

  1. don't allow multiple arguments, this only happens because someone made an error
  2. at least show an error if there is a constant condition

Additional information about the issue

No response

@MartinJohns
Copy link
Contributor

That sounds more like a linter-job, and there is a rule already: https://eslint.org/docs/latest/rules/no-sequences

@MajorMeerkatThe3rd
Copy link
Author

This rule seems to be very broad. For loops this behavior might make sense, I just don't see use cases for If-statements

@jcalz
Copy link
Contributor

jcalz commented Dec 19, 2024

I'm not a TS team member, this is just my opinion:

This isn't "multiple arguments"; you're objecting to the JavaScript comma operator being used inside an if statement. I don't think TS complains about the comma operator unless it's sure the initial operands have no side effects.

I agree that it seems code-smelly to write if (x(), y) {} instead of x(); if (y) {}, but is it really never used in practice? This still feels like something a linter would be better equipped to handle, without introducing a breaking change for TS code in general. I think typescript-eslint lets you write custom rules.

@kirkwaiblinger
Copy link

(typescript-eslint team member) Agreed that this is a linter-y request. You don't even need a full custom rule for the described request; you can just use no-restricted-syntax with selector "IfStatement > SequenceExpression".

Our FAQ
ESLint's no-restricted-syntax rule docs
Demo of this configured in our playground

FWIW I'd just use no-sequences though and disable it where you really want them for some reason.

@Josh-Cena
Copy link
Contributor

Do note however that TS already checks someCheckingFunction(...) && false and reports the if body as unreachable. I don't believe it's impossible for TS to realize that ..., false is also statically false, making the if body unreachable.

@Andarist
Copy link
Contributor

I think i makes sense to handle some of the cases around comma expressions in TS, that said - an improvement for this (#60830) won't help this exact case because this exact case involves boolean literals and those are special-cased by the current syntactic truthy semantics:

if (true) {} // ok

We could handle those differently within comma expressions but @RyanCavanaugh would have to green light it first.

@MajorMeerkatThe3rd
Copy link
Author

Thank you everyone for your input!

@kirkwaiblinger I'm now using the custom rule you provided, as within our code base there are instances of the comma-operator being used in other ways. My issue with it was specifically for if-statements. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants