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

no-constant-condition false positive #4737

Closed
MethodGrab opened this Issue Dec 18, 2015 · 7 comments

Comments

Projects
None yet
4 participants
@MethodGrab
Copy link
Contributor

MethodGrab commented Dec 18, 2015

eslint: v1.10.3
Config: {"rules": { "no-constant-condition": 2, "no-array-constructor": 2 }}

Problem:
no-constant-condition gives a false positive result for the following code:

if ( 'every' in [] ) { /* browser supports every */ }

Using Array() instead of [] will not result in the error but I have no-array-constructor so it still doesn't pass linting.

Expected:
The results for [] and Array() are consistent, linting passes.

Actual:
Linting fails.

Example Code:

if ( 'every' in [] ) { console.log( 1 ) } // → no-constant-condition error

if ( 'every' in Array() ) { console.log( 2 ) } // → no-array-constructor error

/* eslint-disable no-array-constructor */
if ( 'every' in Array() ) { console.log( 3 ) } // → pass
/* eslint-enable no-array-constructor */

if ( typeof [].every === 'function' ) { console.log( 4 ) } // → pass

ESLint Output:

1:1   error  Unexpected constant condition                 no-constant-condition
3:17  error  The array literal notation [] is preferrable  no-array-constructor
@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 18, 2015

Looks like a bug. Not really surprised, in operator is used very infrequently and we probably missed it in the rule all together.

@ilyavolodin ilyavolodin added accepted and removed evaluating labels Dec 18, 2015

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 18, 2015

But it is constant in a particular environment, assuming you're not mutating native prototypes. You should just disable the rule here.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 18, 2015

@michaelficarra You are absolutely correct about particular environment, however, JavaScript is executed in a lot of different environments (in this case "environment" is a broader term that includes different version of browsers). A specific property on the object might be available in one browser, but not in another, so the expression is not constant in cross-environment code.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 18, 2015

So code like if ("v" === "\v") ... else ... should not be warned either because some environments take one path and some take another? Eslint sticks to standard JS, not implementations.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 18, 2015

I agree with @ilyavolodin here. The intent of this rule is to flag conditions that will never change regardless of where code is executed (see the examples in the documentation).

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 18, 2015

@michaelficarra You example is correct, but extreme. We have to find middle ground, where the rule is helpful to users, but at the same time doesn't cover all possible variations. In this case, it's a well known fact that different versions of browsers have different attributes for native objects. Forking code based on existence of those attributes is a common practice. While "v" === "\v" might not be true in some cases, those are very much edge cases.

@alberto alberto closed this in 6f9a6ed Dec 19, 2015

nzakas added a commit that referenced this issue Dec 19, 2015

Merge pull request #4744 from eslint/issue4737
Fix: no-constant-condition false positive (fixes #4737)
@MethodGrab

This comment has been minimized.

Copy link
Contributor Author

MethodGrab commented Dec 21, 2015

Thanks for fixing this so quickly!

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.