prefer-const not triggered (regression from 2.4.0)

[fabienjuif]

What version of ESLint are you using?
2.7.0

What parser (default, Babel-ESLint, etc.) are you using?

"babel-eslint": "~6.0.0"

Please show your full configuration:

{
  "parser": "babel-eslint",
  "extends": "airbnb",
  "rules": {
      "semi": [2, "never"],
      "arrow-body-style": 0
  }
}

What did you do? Please include the actual source code causing the issue.

import React, { PropTypes } from 'react'

const innerStyle = {
  color: '#343434',
  color2: 'white',
}

const ToolBar = ({ style, draw }) => {
  let dynStyle = {
    ...innerStyle,
    ...style,
  }

  dynStyle.color = draw ? dynStyle.color2 : dynStyle.color

  return (
    <div style={dynStyle} />
  )
}

ToolBar.propTypes = {
  style: PropTypes.object,
  draw: PropTypes.bool,
}

export default ToolBar

What did you expect to happen?
prefer-const should trigger and ask me to change let dynStyle to const dynStyle

What actually happened? Please include the actual, raw output from ESLint.
ESLint don't trigger the prefer-const rule.

---

Note I tried these versions :

• ~2.4.0 : Works
• ~2.5.0 : Doesn't work
• ~2.6.0 : Doesn't work
• ~2.7.0 : Doesn't work

---

Commits that are related to prefer-const (maybe I miss some) :

• 2.6: 9b73ffd Update: destructuring option of prefer-const rule (fixes prefer-const should somehow figure out mixed destructuring #5594) (Toru Nagashima)
• 2.5: 9a22625 Fix: prefer-const false positive at non-blocked if (fixes prefer-const false positive #5610) (Toru Nagashima)
• 2.5: 87d74b2 Fix: prefer-const got to not change scopes (refs Rule Proposal: prefer-smaller-scope #5284) (Toru Nagashima)

[ilyavolodin]

@mysticatea Could you take a look please?

[nzakas]

@fabienjuif
It looks like you're using babel-eslint as your parser. Can you try using the default parser instead? You can just delete the parser from your config file to switch to the default parser. Make sure you configure the parser options as well.

[fabienjuif]

Config file :

{
  "parserOptions": {
    "ecmaVersion": 7,
    "sourceType": "module",
    "ecmaFeatures": {
        "jsx": true,
        "experimentalObjectRestSpread": true
    }
  },
  "extends": "airbnb",
  "rules": {
      "semi": [2, "never"],
      "arrow-body-style": 0
  }
}

Code : Is the same
How do I run it ? : ./node_modules/eslint/bin/eslint.js [...]/ToolBar.js
Result : Blank.

[mysticatea]

Thank you for this issue.

Hm, I cannot reproduce this on my local environment.
prefer-const is warning dynStyle.

C:\Users\t-nagashima.AD\Documents\GitHub\eslint [master]> cat test.js
import React, { PropTypes } from 'react'

const innerStyle = {
  color: '#343434',
  color2: 'white',
}

const ToolBar = ({ style, draw }) => {
  let dynStyle = {
    ...innerStyle,
    ...style,
  }

  dynStyle.color = draw ? dynStyle.color2 : dynStyle.color

  return (
    <div style={dynStyle} />
  )
}

ToolBar.propTypes = {
  style: PropTypes.object,
  draw: PropTypes.bool,
}

export default ToolBar

C:\Users\t-nagashima.AD\Documents\GitHub\eslint [master]> eslint --version
v2.7.0
C:\Users\t-nagashima.AD\Documents\GitHub\eslint [master]> eslint test.js --rule prefer-const:2 --parser-options "{ecmaVersion:7,sourceType:'module',ecmaFeatures:{jsx:true,experimentalObjectRestSpread:true}}" --no-ignore --no-eslintrc

C:\Users\t-nagashima.AD\Documents\GitHub\eslint\test.js
  9:7  error  'dynStyle' is never reassigned, use 'const' instead  prefer-const

✖ 1 problem (1 error, 0 warnings)

[stevoland]

I had this problem and could reproduce the test case but it was fixed when I removed react/jsx-uses-vars https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/jsx-uses-vars.js from my config

[stevoland]

That plugin flags eslintUsed: https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/util/variable.js

[nzakas]

@stevoland interesting! @fabienjuif can you check that?

[lukeapage]

yes thats right - I raised this on the react plugin - jsx-eslint/eslint-plugin-react#716
I've created a test repo https://github.com/lukeapage/eslint-react-bug-prefer-const-716

after some investigation, as @stevoland suggested, eslint-plugin-react marks the variable with eslintUsed which then causes prefer-const to ignore the variable because of this line -

eslint/lib/rules/prefer-const.js

Line 86 in 421e4bf

if (variable.eslintUsed) {

and searching through the history there used to be a better explanation of why eslint ignores in this case

eslint/lib/rules/prefer-const.js

Line 184 in 361377f

// - If `eslintUsed` is true, we cannot know where it was used from.

from that file

// - If `eslintUsed` is true, we cannot know where it was used from.
//   In this case, if the scope of the variable would change, it
//   skips the variable.

so I presume that is to stop this

if (true) {
    var a = 1;
}
return (<div>{a}</div>);

because in that case, converting the var to a const would change the meaning of the code (a would no longer be defined).

maybe we can only ignore eslintUsed variables if they are var so that let variables can still be warned?

[mysticatea]

@lukeapage Thank you for investigating.

so I presume that is to stop this

prefer-const does not touch var declarations. :)
It's handling only let declarations.

Actually, the rule ignores variables if eslintUsed is true, because rules cannot distinguish reasons of used. (e.g., /*exported*/ comments set true to eslintUsed) (e.g., A plugin can set true to eslintUsed by another reason.)

(Please never mind this sentence, I'm searching my head...)

[lukeapage]

How does the reasons of usage change whether prefer-const should be applied? Can you give an example?
I was going to follow up that eslintUsed could maybe instead be an array of scopes.. but do you have a better idea?

[mysticatea]

I'm sorry, I was confused. I found my memory.
Originally, prefer-const was warning the following patterns:

let a;
for (let i = 0; i < 10; ++i) {
    a = doSomething();
    doSomething2(a);
}

// it can be modified as below.

for (let i = 0; i < 10; ++i) {
    let a = doSomething();
    doSomething2(a);
}

But in the following case (i.e., if there is read before write), the rule should not warn the code.

let a;
for (let i = 0; i < 10; ++i) {
    doSomething2(a);  // this uses the value of the previous iteration.
    a = doSomething();
}

Now, in the following case, if variable a is "eslintUsed", the rule cannot check the location of the read.

let a;
for (let i = 0; i < 10; ++i) {
    /* a code to set `eslintUsed`. */
    a = doSomething();
}

So, I wrote the condition !(variable.eslintUsed && variable.scope !== writer.from).

[mysticatea]

But currently, prefer-const does not warn variables if the variable is initialized in another scope from the declaration.

let a;
for (let i = 0; i < 10; ++i) {
    a = doSomething(); // this is inside a different scope from `let a`, so this `a` is not warned.
}

So probably we can remove the check of eslintUsed simply.

[ljharb]

Just to confirm, prefer-const should however still not warn on:

let foo;
const recursive = () => {
  return foo;
};
foo = {
  dontAsk: recursive
};

[mysticatea]

@ljharb It would be warned by default. It can be modified to:

const foo = {
    dontAsk: () => {
        return foo
    }
}

Also, we can make the rule ignoring that case by ignoreReadBeforeAssign option.

[ljharb]

@mysticatea ah sorry. My mistake. #5822 Ended up being resolved by adding the option you mentioned.

[yannickcr]

So probably we can remove the check of eslintUsed simply.

@mysticatea any news on this? This check is breaking prefer-const when used with react/jsx-uses-vars (jsx-eslint/eslint-plugin-react#716).

[mysticatea]

Oh, I'm sorry.
I had lost the track of this issue.

I will fix this.

[mysticatea]

Oh, wait, but I'm not sure whether it's the issue of this original post or not.
dynStyle seems not the target of react/jsx-uses-vars rule.

Maybe should we separate issue?

[yannickcr]

The issue is that any variable that is marked as used by context.markVariableAsUsed(name) (which set eslintUsed to true on the variable) will be ignored by prefer-const.

It was the case for dynStyle that was marked as used by react/jsx-uses-vars (fixed now since ESLint seems to handle it correctly without any help jsx-eslint/eslint-plugin-react@d4dee48).

But we'll still have the same problem with JSXOpeningElement:

let App = <div />; // should warn "'App' is never reassigned. Use 'const' instead" but will not
<App />

So it is still the same issue, just on another node type.

But I can open another issue if it makes it easier for you.

[mysticatea]

Thank you for explaining.
I understood it.