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
Fix: no-constant-condition doesn't introspect arrays (fixes #12225) #12307
Conversation
Thanks for the PR! Marking as accepted as the issue is accepted. The solution seems correct for the #12225 false positive, but I think that it could also introduce some new false negatives? For example: /*eslint no-constant-condition: "error"*/
if ([a, b] === [c, d]) {
// ...
} |
It's okay to check at the beginning, e.g., Though, the espree bug comment can be removed, as it isn't a bug :) |
This does create some false positives as you noted, however trying to be exhaustive in the permutations seem unfeasible, as we would need an list of each binary operator and wether or not that operator is valid. I can either close the scope, allowing for only I am not sure if we would rather have more false negatives, or more false positives. Let me know what you guys would prefer |
When looking at these particular false positives and negatives, supporting the use case of comparing arrays by their string presentations at the cost of missing to report cases like There is also the following workaround: I'm not sure is there another use case with 'dynamic' arrays that could currently produce a false positive? Other than converting the array to string at some point. I'd vote to limit the scope and allow just |
Since no one else has weighed in and I agree with @mdjermanovic, I have update the PR to only support the narrow scope of being a child of a Noted this does now allow for |
I agree that we should scope this bug fix to the accepted bug only, and leave any further changes to a separate proposal. @tickct Do you mind signing the CLA? |
@tickct Friendly ping |
I have signed the CLA, I unfortunately had not configured git with my email until after I did the initial push, so now it is showing two authors one of which has not signed, I am not sure if there is a way to rectify that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, thanks for working on this! I left a couple of minor suggestions, mostly about tests.
I believe this is a good balance between false negatives, false positives and code complexity.
… unit tests include more paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just one detail in the tests.
errors: [{ messageId: "unexpected", type: "BinaryExpression" }] | ||
}, | ||
{ | ||
code: "if(+1) {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was meant to be +[a]
? (a test to ensure that the change in this PR applies only to binary +
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me 👍
I think we can merge this PR as is, and add a test later.
For clarity, are you referring to #12307 (comment)? |
Yes. This PR has been open for a long time because it wasn't clear what should be done to fix the related issue and not produce new false negatives at the same time. The scope of the change looks good to me now, the code which implements it also looks good and the change is well-tested, so I think that one nice-to-have test shouldn't block this. I can add the test later. |
@mdjermanovic Sounds good 👍 @tickct Apologies for the delay, and thank you contributing to ESLint! |
) (eslint#12307) * Fix: no-constant-condition doesn't introspect arrays (fixes eslint#12225) * Update: no-constant-condition allows for narrower scope of varriable arrays * Update: no-constant condition now requires direct parent to be binary expression * Update: no-contant-contidtion now checks parent type before operator, unit tests include more paths
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
#12225
What changes did you make? (Give an overview)
no-constant-condition uses a recursive function
isConstant
to detect if items are constants.Previously we were presuming that all arrays were always constant. Since an array can hold variables, if we are inside of a binary evaluation it is possible for the array to change and there for change the conditional.
Now, if we are in a binary expression, we will calculate isConstant on each element of the array, and only return true if all elements are constant
Is there anything you'd like reviewers to focus on?
Per the espree docs, array elements of sparse arrays have values of null. I have added a check at the beginning of the function to handle this, but I could see the argument to handle this situation in other ways such as a filter.