prefer-destructuring should not check non-numeric indexes

[aparajita]

Tell us about your environment

• ESLint Version: 4.14.0
• Node Version: 9.3.9
• npm Version: 5.6.0

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

Please show your full configuration:

Configuration

{
  "parserOptions": {
    "ecmaVersion": 6,
    "sourceType": "module"
  },

  "extends": [
    "airbnb-base"
  ],

  "rules": {
    "class-methods-use-this": "off",
    "import/no-extraneous-dependencies": ["off", {"devDependencies": ["webpack-config/*.js"]}],
    "no-prototype-builtins": "off",
    "no-param-reassign": [0],
    "prefer-default-export": "off"
  }
}

What rule do you want to change? prefer-destructuring

Does this change cause the rule to produce more or fewer warnings? Fewer

How will the change be implemented? (New option, new default behavior, etc.)? New default behavior

Please provide some example code that this change will affect:

const INTEGER_WORDS = {
  one: 1,
  two: 2,
  three: 3,
  four: 4,
 };

let num = 1;
num = INTEGER_WORDS[num]; 

What does the rule currently do for this code?

Generates a warning for num = INTEGER_WORDS[num];

What will the rule do after it's changed?

It will ignore array indexes/keys that are not constants. I would argue this should always be the case, as it must be considered bad practice to destructure from a computed index or key.

[j-f1]

You could do let { [num]: num } = INTEGER_WORDS, but I agree that that should be an exception.

[platinumazure]

Hi @aparajita, thanks for the issue. I agree with @j-f1 that the rule is currently working as designed but that we could consider an enhancement to avoid warning when a computed property access is being used.

Would you mind editing your original post to match our Rule Change Proposal template so we can evaluate the proposed enhancement more easily? Thanks!

[eslint-deprecated]

Hi @aparajita, thanks for the issue. It looks like there's not enough information for us to know how to help you.

Requesting a rule change? Please see Proposing a Rule Change for instructions.

If it's something else, please just provide as much additional information as possible. Thanks!

(edited by @platinumazure)

[aparajita]

@j-f1 That is invalid syntax; you're trying to declare and reference num at the same time. Even it were valid, it's a tortured syntax at best and indecipherable to mere mortals at worst.

const words = { one: 1, two: 2 };
let num = 1;
let { [num]: num } = words;

Run that, you get:

SyntaxError: Identifier 'num' has already been declared

Remove let num = 1:

ReferenceError: num is not defined

[not-an-aardvark]

The correct way to do this is

({ [num]: num} = INTEGER_WORDS);

[aparajita]

@not-an-aardvark Correct, but looking at it I still can't decipher what's happening without seriously scratching my head, and I've written Javascript parsers and compilers. So under normal circumstances I think prefer-destructuring should have pity on us mere mortals and not complain if we use num = INTEGER_WORDS[num]. And I still think it's bad practice to destructure a computed index/key.

[not-an-aardvark]

Oh, I see what's going on. The issue is that the enforceForRenamedProperties option for prefer-destructuring should consider any computed property access to be a "renaming".

The current behavior is intended to report something like this:

num = foo.num;

// should be replaced with

({ num } = foo);

But it should not report an issue when the right-hand side is foo[num] instead.

[aparajita]

@not-an-aardvark That explains it.

[aparajita]

The fix seems to be quite easy. I haven't thought through it deeply, but this works:

Change these lines to:

if (!rightNode.computed && ((property.type === "Literal" && leftNode.name === property.value) ||
    (property.type === "Identifier" && leftNode.name === property.name))) {

I added the check for rightNode.computed. If that's true, a non-literal expression is being used.