Update: Handle locally unsupported regex in computed property keys

[mdjermanovic]

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix

Tell us about your environment

• ESLint Version: 6.1.0
• Node Version: 8.16.0
• npm Version: 6.4.1

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

default

Please show your full configuration:

Configuration

module.exports = {
  parserOptions: {
    ecmaVersion: 2018,
  },
};

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

This bug is observable in environments that don't support ES2018 regexp syntax, namely Node 8.

For the online demo use Firefox (current Firefox version is 68, still doesn't support this syntax), results are different when compared to Chrome.

This bug affects 5 rules:

---

no-dupe-keys false negative:

/*eslint no-dupe-keys: "error"*/
({ "/(?<zero>0)/": 1, [/(?<zero>0)/]: 2 })

Demo link (open in Firefox)

---

no-dupe-keys false positive:

/*eslint no-dupe-keys: "error"*/
({ null: 1, [/(?<zero>0)/]: 2 })

Demo link (open in Firefox)

---

sort-keys false negative:

/*eslint sort-keys: "error"*/
({ null: 1, [/(?<zero>0)/]: 2 })

Demo link (open in Firefox)

---

sort-keys false positive:

/*eslint sort-keys: "error"*/
({ [/(?<zero>0)/]: 1, "/(?<zero>0)/": 2 })

Demo link (open in Firefox)

---

no-self-assign false negative:

/*eslint no-self-assign: "error"*/
foo["/(?<zero>0)/"] = foo[/(?<zero>0)/]

Demo link (open in Firefox)

---

no-self-assign false positive:

/*eslint no-self-assign: "error"*/
foo.null = foo[/(?<zero>0)/];

Demo link (open in Firefox)

---

yoda false negative:

/*eslint yoda: ["error", "never", { "exceptRange": true }]*/
if (1 < foo.null && foo[/(?<zero>0)/] < 10) {}

Demo link (open in Firefox)

---

yoda false positive:

/*eslint yoda: ["error", "never", { "exceptRange": true }]*/
if (1 < foo["/(?<zero>0)/"] && foo[/(?<zero>0)/] < 10) {}

Demo link (open in Firefox)

---

no-restricted-properties false negative:

/*eslint no-restricted-properties: ["error", { "property": "/(?<zero>0)/" }]*/
foo[/(?<zero>0)/];

Demo link (open in Firefox)

---

no-restricted-properties false positive:

/*eslint no-restricted-properties: ["error", { "property": "null" }]*/
foo[/(?<zero>0)/];

Demo link (open in Firefox)

What did you expect to happen?

Warnings for 'false negative' examples, no warnings for 'false positive' examples.

What actually happened? Please include the actual, raw output from ESLint.

The opposite.

What changes did you make? (Give an overview)

Fixed astUtils.getStaticPropertyName

Is there anything you'd like reviewers to focus on?

Part of the code is extracted to new function getStaticStringValue, it's easier to test and it might be useful for something else.

Unrelated to this PR:

• The same fix should be added for bigint.
• accesor-pairs should use getStaticPropertyName, but this rule doesn't work well. It should enforce pairs per key, not per the whole literal. I'm working on this, a PR will be ready very soon.
• Some other rules might also use getStaticPropertyName instead of a partial/duplicate logic, I'm working on this.
• Some rules are treating empty string name as null, I'm working on this, will be ready very soon.

[platinumazure]

LGTM, thanks for contributing!

[kaicataldo]

LGTM, thanks!

[kaicataldo]

Sorry, to avoid confusion I usually wait to review until after a change is accepted. Code LGTM and I support this change, but we’ll need another 👍🏼 And a champion.

[mysticatea]

Would you try getPropertyName function of eslint-utils? I guess that the utility covers more cases.

[mdjermanovic]

Sorry, to avoid confusion I usually wait to review until after a change is accepted. Code LGTM and I support this change, but we’ll need another 👍🏼 And a champion.

Sorry, I should be more specific and the title/commit message indeed sounds like this is an enhancement.

This was supposed to be a bug fix. The title has 'Update' prefix because the fix can produce more warnings.

The function already supports regexp literals, but it doesn't 'support' RegExpLiteral extension of the Literal interface, i.e. the regex property.

The bug is in return String(prop.value) which will evaluate to "null" string if the ESLint is executed in an environment that doesn't support given regex syntax.

[mdjermanovic]

Would you try getPropertyName function of eslint-utils?

I'll try it on Node 8 with test cases from this PR to be sure, but I think it doesn't use regex properties?

Literal(node) {
        //istanbul ignore if : this is implementation-specific behavior.
        if (node.regex != null && node.value == null) {
            // It was a RegExp literal, but Node.js didn't support it.
            return null
        }
        return node
    },

instead of return null it might be something like

return { value: `/${node.regex.pattern}/${node.regex.flags}` }

or maybe there is a specific reason to return null in this scenario? I'm asking because it would apply to this PR as well.

I guess that the utility covers more cases.

Looks like a lot more, even without the scope argument, I'm not sure would it be a breaking change?

[mdjermanovic]

I think I see the reason now, getStaticValue() returns the value itself, which is impossible in this scenario. But then there is a problem with getStringIfConstant() which doesn't return the string in this scenario, although it would be possible.

[mdjermanovic]

Changed the title and commit message. I still believe this is a bug fix rather than enhancement :)

[kaicataldo]

@mysticatea Have your concerns been addressed?

[mysticatea]

I apologize for I lost track on this. LGTM.

[mdjermanovic]

@mysticatea no problem, but this will now collide with #12701, which handles this + bigints as well.

Might be easier to close this PR and do everything in #12701?

[mysticatea]

Thank you. I'm fine with either way... (1) we merge this PR to master then master to #12701, or (2) I cherry-pick this to #12701. Probably those are samely easy.

[mdjermanovic]

Rebased and fixed conflicts & JSDoc, for the case that this gets merged.

[mysticatea]

Thank you!