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

Fix: no-constant-condition doesn't introspect arrays (fixes #12225) #12307

Merged
merged 4 commits into from Jan 27, 2020

Conversation

tickct
Copy link
Contributor

@tickct tickct commented Sep 24, 2019

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.

@jsf-clabot
Copy link

jsf-clabot commented Sep 24, 2019

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Sep 24, 2019
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Sep 24, 2019
@mdjermanovic
Copy link
Member

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]) {
  // ...
}

@mdjermanovic
Copy link
Member

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.

It's okay to check at the beginning, e.g., no-self-assign handles this in a similar way.

Though, the espree bug comment can be removed, as it isn't a bug :)

@tickct
Copy link
Contributor Author

tickct commented Sep 30, 2019

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 + operators, which would fulfill this bug, but may cause the issue to reopen in the future if for some reason someone is using - for some reason, or I can make only direct equality operators ===,==,!==,!= fail and possibly give us other false negatives and deal with those as I come up.

I am not sure if we would rather have more false negatives, or more false positives. Let me know what you guys would prefer

@mdjermanovic
Copy link
Member

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 [a, b] === [c, d] doesn't seem like a good thing to me, personally. Or, at the cost of increasing the complexity too much.

There is also the following workaround: String([foo]) instead of '' + [foo] is valid by this rule.

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 string_literal + [dynamic_array] in this PR, but better wait for other opinions.

@tickct
Copy link
Contributor Author

tickct commented Oct 7, 2019

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 + operator.

Noted this does now allow for if('' + [a]){}, but in testing ""+[undefined] evaluates to "" so it is indeed possible for the condition to evaluate to both truthy and falsy.

@kaicataldo
Copy link
Member

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?

@kaicataldo
Copy link
Member

@tickct Friendly ping

@tickct
Copy link
Contributor Author

tickct commented Dec 23, 2019

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.

@kaicataldo
Copy link
Member

@tickct Understood. You should be able to follow the instructions here. Thanks!

Copy link
Member

@mdjermanovic mdjermanovic left a 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.

lib/rules/no-constant-condition.js Outdated Show resolved Hide resolved
lib/rules/no-constant-condition.js Show resolved Hide resolved
tests/lib/rules/no-constant-condition.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a 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) {}",
Copy link
Member

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 +)

Copy link
Member

@mdjermanovic mdjermanovic left a 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.

@kaicataldo
Copy link
Member

I think we can merge this PR as is, and add a test later.

For clarity, are you referring to #12307 (comment)?

@mdjermanovic
Copy link
Member

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.

@kaicataldo
Copy link
Member

@mdjermanovic Sounds good 👍

@tickct Apologies for the delay, and thank you contributing to ESLint!

@kaicataldo kaicataldo merged commit 80309c3 into eslint:master Jan 27, 2020
montmanu pushed a commit to montmanu/eslint that referenced this pull request Mar 4, 2020
) (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
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jul 27, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants