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

`prefer-const` and `init-declarations:never` conflict? #4474

Closed
PhilVargas opened this Issue Nov 18, 2015 · 9 comments

Comments

Projects
None yet
5 participants
@PhilVargas
Copy link
Contributor

PhilVargas commented Nov 18, 2015

The two rules prefer-const and init-declaration (set to never) don't really perform well together.
When i use init-declaration set to never i initialize all variables like so:

/*eslint init-declarations: [2, "never"]*/
function foo(){
  let bar;

  bar = 'bar';

  ...
}

however, when this setting is active, it pretty much never triggers prefer-const because an assignment IS set even though it is never re-set after definition. consider the function:

/* example 1 */
/*eslint init-declarations: [2, "never"]*/
/*eslint prefer-const: [2]*/
/* no errors are triggered */
function foo(){
  let bar;

  bar = 2;
  return bar * 5;
}

vs

/* example 2 */
/*eslint init-declarations: [0, "never"]*/
/*eslint prefer-const: [2]*/
/* prefer-const error triggered */
function foo(){
  let bar = 2;

  return bar * 5;
}

Essentially what is happening is that prefer-const doesnt throw an error when init-declarations is set to never, as seen in example 1 which raises no errors, but should raise a prefer-const error. If init-declarations is set to 2 in example 2, then the prefer-const error is overshadowed by the init-declarations error

ideally, i'd like to be able to have them both active, where prefer-const is able to check that if a variable is declared (but not assigned), and it is not assigned more than once, it should be declared as a const. I'd like example 1 to be able to identify that although bar is assigned it is assigned only once (away from declaration) and thus should be a const.

eslint v1.9.0

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Nov 18, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Nov 18, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 18, 2015

It's pretty tricky to track assignments for all declared variables, so I don't see us doing that.

When you say "doesn't work", what does that mean?

As the previous comment states, please paste the exact output from ESLint when discussing what you are expecting or not expecting.

@PhilVargas

This comment has been minimized.

Copy link
Contributor Author

PhilVargas commented Nov 18, 2015

i put the output from eslint in the comments of example 1 and 2. also edited the OP. when i say it doesnt work, i mean that the prefer-const rule doesnt throw an error when it should be throwing an error. I understand this would be a pretty hard ticket, and i had a peek at the prefer-const code and am pretty sure i wont be able to implement this suggestion, but nonetheless wanted to bring it up for discussion.

so for a recap:

example 1
currently: eslint throws no errors
expected: eslint throws a prefer-const error (the let is only assigned a single time and can be a const)

this is shown by example 2 where the function is rewritten with an initialized declaration and eslint correctly throws an prefer-const error (only after init-declarations is deactivated)

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 19, 2015

@mysticatea can you think of any easy way to do this?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 19, 2015

This should be simple. escope gives you a list of the write references for each variable.

@nzakas nzakas added enhancement rule accepted and removed triage labels Nov 20, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 20, 2015

Cool, let's do it

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 20, 2015

I think the following pattern should be excluded.

let foo;
try {
    foo = makeFoo();
} catch (err) {
    return null;
}
console.log(foo);
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 20, 2015

Is there any easy way to determine that?

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 21, 2015

Hmm, yes.

I thought variables which satisfies the following conditions can be replaced with const.

  • There is only one write-reference.
  • Other references are inside of the scope (or nested scopes) which has the write-reference.

If there is a read reference outside of the scope which has the write reference, the read reference cannot access the variable after the declaration was moved to the location of the write reference.

To check scope's parent‐child relationship is easy.
I will work on this.

@mysticatea mysticatea self-assigned this Nov 21, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Nov 23, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Nov 23, 2015

@nzakas nzakas closed this in #4522 Dec 1, 2015

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

@eslint eslint bot added the archived due to age label Feb 7, 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.