Fix: id-blacklist should ignore ObjectPatterns (fixes #12787) #12792

jpramassini · 2020-01-15T05:28:23Z

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

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

#12787

What changes did you make? (Give an overview)
I modified the should report logic to check that the effectiveParent's parent is a block statement and added tests for the case.

Is there anything you'd like reviewers to focus on?
Thoughts on any potentially cleaner ways to accomplish this? This is my first PR to ESLint so I want to make sure it's up to snuff.

…#12787)

yeonjuan · 2020-01-15T15:13:01Z

@jpramassini
Hello.
I'm not an eslint member but leave a comment.
Issue #12787 is related to the Destructuring assignment, while the codes in your test cases are Labeled statement.

eslint/tests/lib/rules/id-blacklist.js

Line 69 in ec3983c

code: "{foo: bar}",

Your changes should pass these test cases.

// valid
{
  code: “const { foo: bar } = baz”,
  options: [“foo”],
  parserOptions: { ecmaVersion: 6 }
}

// invalid
{
  code: “const { foo: bar } = baz”,
  options: [“bar”],
  parserOptions: { ecmaVersion: 6 },
  errors: [
     // ...
  ]
}

jpramassini · 2020-01-15T16:11:46Z

@yeonjuan This is a very good point, dumb mistake on my part. I'll update my test cases later today. Update: My code doesn't work with accurate test cases, so I'll revisit my approach here.

kaicataldo

Thanks for contributing! I don't think this current iteration is going to work since BlockStatement nodes aren't related to the bug in question. Here's what the AST looks like for a destructured assignment.

kaicataldo · 2020-01-15T20:54:48Z

I also want to note that the fix should be to not warn on destructured assignments at all. From the rule docs (emphasis my own):

It will not catch blacklisted identifiers that are:

function calls (so you can still use functions you do not have control over)

object properties (so you can still use objects you do not have control over)

Ideally, I do think the rule should warn when a property is renamed to a blacklisted identifier name in a destructuring assignment, but I also think that can (and probably should) be done in a separate PR as a separate bug fix.

jpramassini · 2020-01-16T01:48:33Z

@kaicataldo Thanks for the help on the AST! I'll take another crack at this.

jpramassini · 2020-01-16T03:14:34Z

@kaicataldo Updated to ignore in Object Pattern nodes, and removed the error test case as we want to ignore it for this PR.

kaicataldo · 2020-01-16T03:15:29Z

You got it! Feel free to reach out with any questions you have :)

lib/rules/id-blacklist.js

tests/lib/rules/id-blacklist.js

kaicataldo · 2020-01-16T03:21:40Z

We're getting closer! Thanks for working on this.

jpramassini · 2020-01-16T03:30:45Z

Do we want an error test case for function foo({ bar: baz }) {}? I.e. testing to make sure we're still getting the function name if foo is illegal?

kaicataldo · 2020-01-16T03:50:58Z

Sorry, that wasn't a great example. I think a test for the destructured parameters is sufficient. In the example above, that would be having bar blacklisted and not warning.

…eters.

kaicataldo · 2020-01-16T03:54:24Z

That being said, additional tests are always welcome :)

jpramassini · 2020-01-16T03:57:56Z

I think between the existing function name tests and the added nested and destructured arguments we should be covered, so I left it out.

kaicataldo · 2020-01-16T03:59:06Z

One last thing from me: do you mind updating the PR title to more accurately reflect the current iteration of this PR?

lib/rules/id-blacklist.js

kaicataldo · 2020-01-16T04:00:42Z

lib/rules/id-blacklist.js

                } else if (node.parent.type === "Property") {

                    if (shouldReport(effectiveParent, name)) {
                        report(node);
                    }

-                // Report anything that is a match and not a CallExpression
+                    // Report anything that is a match and not a CallExpression


Mind undoing this? This is referring to the following block and I think it's clearer to have it indented as it was.

Just pushed a commit that should fix this, the commit has these lined up properly but for some reason the diff doesn't seem to have updated.

Yeah, I see that. Strange :/ Seems like something on GitHub's end?

Seems like it. This is the first time I've had this happen. 🤷‍♂

kaicataldo · 2020-01-16T04:03:00Z

Re the commit message check that's failing now: How about changing it to Fix: id-blacklist should ignore ObjectPatterns (fixes #12787)?

jpramassini · 2020-01-16T04:05:10Z

Thanks for your patience and help, sorry for all the small gotchas here.

kaicataldo · 2020-01-16T04:08:24Z

You're doing great. We really appreciate the contribution!

kaicataldo

This LGTM. Thank you for contributing to ESLint!

jpramassini · 2020-01-16T04:13:58Z

Awesome, thanks again for all of your help!

yeonjuan · 2020-01-16T13:09:18Z

tests/lib/rules/id-blacklist.js

@@ -65,6 +65,26 @@ ruleTester.run("id-blacklist", rule, {
            code: "var obj = { key: foo.bar };",
            options: ["f", "fo", "fooo", "b", "ba", "barr", "bazz", "bingg"]
        },
+        {
+            code: "const {foo: bar} = baz",
+            options: ["foo", "bar"],


@jpramassini
~~Hi, In my opinion, this case should be included in invalid because it makes a new identifier bar, which is in the id-blacklist.~~

Ideally, I do think the rule should warn when a property is renamed to a blacklisted identifier name in a destructuring assignment, but I also think that can (and probably should) be done in a separate PR as a separate bug fix.

I got it

Yeah, this should definitely be invalid at some point due to the spirit of the rule. I added that to try and illustrate for now that that's intended behavior for now. Should I make a separate issue to make sure that gets tracked?

Should I make a separate issue to make sure that gets tracked?

Maybe it would be welcomed, but it is up to you. :)

An issue would be great!

btmills

LGTM, thanks for fixing this!

jpramassini · 2020-01-18T15:30:51Z

@kaicataldo Unsure of where to put this, this seems like the best place to follow up. For the issue to track the reassignment identifier warning for this rule, should that issue be categorized as a bug fix, or is it a rule change proposal?

kaicataldo · 2020-01-18T21:38:27Z

I think it should be categorized as a semver-minor bug fix (reported as a bug fix).

jpramassini · 2020-01-19T19:38:32Z

@kaicataldo New issue here: #12807

…slint#12792) * Fix: Ignore destructured object property reassignments. (fixes eslint#12787) * Updated to ignore blacklisted id's in ObjectPattern nodes. * Remove console.log. * Added tests for nested destructuring and destructuring function parameters. * Fix typo. * Undo accidentally auto-fixed comment indent.

Fix: Ignore destructured object property reassignments. (fixes eslint…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

ec3983c

…#12787)

eslint-deprecated bot added the triage label Jan 15, 2020

kaicataldo added accepted bug rule and removed triage labels Jan 15, 2020

kaicataldo suggested changes Jan 15, 2020

View reviewed changes

Updated to ignore blacklisted id's in ObjectPattern nodes.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

c25321a

Remove console.log.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

8e340c3

kaicataldo suggested changes Jan 16, 2020

View reviewed changes

lib/rules/id-blacklist.js Outdated Show resolved Hide resolved

tests/lib/rules/id-blacklist.js Show resolved Hide resolved

jpramassini added 2 commits January 15, 2020 22:52

Added tests for nested destructuring and destructuring function param…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

0e7b9a7

…eters.

Fix typo.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

871fc8c

jpramassini changed the title ~~Fix: Ignore destructured object property reassignments. (fixes #12787)~~ Fix: Ignore destructured object property reassignments and destructured object function arguments. (fixes #12787) Jan 16, 2020

kaicataldo reviewed Jan 16, 2020

View reviewed changes

lib/rules/id-blacklist.js Outdated Show resolved Hide resolved

kaicataldo reviewed Jan 16, 2020

View reviewed changes

Undo accidentally auto-fixed comment indent.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

bccd6eb

jpramassini changed the title ~~Fix: Ignore destructured object property reassignments and destructured object function arguments. (fixes #12787)~~ Fix: id-blacklist should ignore ObjectPatterns (fixes #12787) Jan 16, 2020

kaicataldo approved these changes Jan 16, 2020

View reviewed changes

yeonjuan reviewed Jan 16, 2020

View reviewed changes

btmills approved these changes Jan 17, 2020

View reviewed changes

btmills merged commit 756b95d into eslint:master Jan 17, 2020

jpramassini mentioned this pull request Jan 19, 2020

id-blacklist doesn't warn when renaming properties to blacklisted identifiers in destructuring assignment. #12807

Closed

eslint-deprecated bot locked and limited conversation to collaborators Jul 17, 2020

eslint-deprecated bot added the archived due to age label Jul 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Fix: id-blacklist should ignore ObjectPatterns (fixes #12787) #12792

Fix: id-blacklist should ignore ObjectPatterns (fixes #12787) #12792

jpramassini commented Jan 15, 2020

yeonjuan commented Jan 15, 2020 •

edited

Loading

jpramassini commented Jan 15, 2020 •

edited

Loading

kaicataldo left a comment

kaicataldo commented Jan 15, 2020 •

edited

Loading

jpramassini commented Jan 16, 2020

jpramassini commented Jan 16, 2020

kaicataldo commented Jan 16, 2020

kaicataldo commented Jan 16, 2020

jpramassini commented Jan 16, 2020 •

edited

Loading

kaicataldo commented Jan 16, 2020

kaicataldo commented Jan 16, 2020

jpramassini commented Jan 16, 2020

kaicataldo commented Jan 16, 2020

kaicataldo Jan 16, 2020 •

edited

Loading

jpramassini Jan 16, 2020

kaicataldo Jan 16, 2020

jpramassini Jan 16, 2020

kaicataldo commented Jan 16, 2020

jpramassini commented Jan 16, 2020

kaicataldo commented Jan 16, 2020

kaicataldo left a comment

jpramassini commented Jan 16, 2020

yeonjuan Jan 16, 2020 •

edited

Loading

jpramassini Jan 16, 2020

yeonjuan Jan 16, 2020

kaicataldo Jan 16, 2020

btmills left a comment

jpramassini commented Jan 18, 2020

kaicataldo commented Jan 18, 2020

jpramassini commented Jan 19, 2020

Fix: id-blacklist should ignore ObjectPatterns (fixes #12787) #12792

Fix: id-blacklist should ignore ObjectPatterns (fixes #12787) #12792

Conversation

jpramassini commented Jan 15, 2020

yeonjuan commented Jan 15, 2020 • edited Loading

jpramassini commented Jan 15, 2020 • edited Loading

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo commented Jan 15, 2020 • edited Loading

jpramassini commented Jan 16, 2020

jpramassini commented Jan 16, 2020

kaicataldo commented Jan 16, 2020

kaicataldo commented Jan 16, 2020

jpramassini commented Jan 16, 2020 • edited Loading

kaicataldo commented Jan 16, 2020

kaicataldo commented Jan 16, 2020

jpramassini commented Jan 16, 2020

kaicataldo commented Jan 16, 2020

kaicataldo Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

jpramassini Jan 16, 2020

Choose a reason for hiding this comment

kaicataldo Jan 16, 2020

Choose a reason for hiding this comment

jpramassini Jan 16, 2020

Choose a reason for hiding this comment

kaicataldo commented Jan 16, 2020

jpramassini commented Jan 16, 2020

kaicataldo commented Jan 16, 2020

kaicataldo left a comment

Choose a reason for hiding this comment

jpramassini commented Jan 16, 2020

yeonjuan Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

jpramassini Jan 16, 2020

Choose a reason for hiding this comment

yeonjuan Jan 16, 2020

Choose a reason for hiding this comment

kaicataldo Jan 16, 2020

Choose a reason for hiding this comment

btmills left a comment

Choose a reason for hiding this comment

jpramassini commented Jan 18, 2020

kaicataldo commented Jan 18, 2020

jpramassini commented Jan 19, 2020

yeonjuan commented Jan 15, 2020 •

edited

Loading

jpramassini commented Jan 15, 2020 •

edited

Loading

kaicataldo commented Jan 15, 2020 •

edited

Loading

jpramassini commented Jan 16, 2020 •

edited

Loading

kaicataldo Jan 16, 2020 •

edited

Loading

yeonjuan Jan 16, 2020 •

edited

Loading