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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from Jan 17, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ec3983c
Fix: Ignore destructured object property reassignments. (fixes #12787)
jpramassini Jan 15, 2020
c25321a
Updated to ignore blacklisted id's in ObjectPattern nodes.
jpramassini Jan 16, 2020
8e340c3
Remove console.log.
jpramassini Jan 16, 2020
0e7b9a7
Added tests for nested destructuring and destructuring function param…
jpramassini Jan 16, 2020
871fc8c
Fix typo.
jpramassini Jan 16, 2020
bccd6eb
Undo accidentally auto-fixed comment indent.
jpramassini Jan 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 9 additions & 6 deletions lib/rules/id-blacklist.js
View file
Open in desktop
Expand Up @@ -61,9 +61,12 @@ module.exports = {
* @returns {boolean} whether an error should be reported or not
*/
function shouldReport(effectiveParent, name) {
return effectiveParent.type !== "CallExpression" &&
return (
effectiveParent.type !== "CallExpression" &&
effectiveParent.type !== "NewExpression" &&
isInvalid(name);
effectiveParent.parent.type !== "ObjectPattern" &&
isInvalid(name)
);
}

/**
Expand Down Expand Up @@ -98,11 +101,11 @@ module.exports = {
report(node);
}

// Report AssignmentExpressions only if they are the left side of the assignment
// Report AssignmentExpressions only if they are the left side of the assignment
} else if (effectiveParent.type === "AssignmentExpression" &&
(effectiveParent.right.type !== "MemberExpression" ||
effectiveParent.left.type === "MemberExpression" &&
effectiveParent.left.property.name === node.name)) {
effectiveParent.left.type === "MemberExpression" &&
effectiveParent.left.property.name === node.name)) {
if (isInvalid(name)) {
report(node);
}
Expand All @@ -115,7 +118,7 @@ module.exports = {
report(node);
}

// Report anything that is a match and not a CallExpression
// Report anything that is a match and not a CallExpression
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if (shouldReport(effectiveParent, name)) {
report(node);
}
Expand Down
20 changes: 20 additions & 0 deletions tests/lib/rules/id-blacklist.js
View file
Open in desktop
Expand Up @@ -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",
kaicataldo marked this conversation as resolved.
Show resolved Hide resolved
options: ["foo", "bar"],
Copy link
Member

@yeonjuan yeonjuan Jan 16, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An issue would be great!

parserOptions: { ecmaVersion: 6 }
},
{
code: "const {foo: {bar: baz}} = qux",
options: ["foo", "bar", "baz"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "function foo({ bar: baz }) {}",
options: ["bar", "baz"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "function foo({ bar: {baz: qux} }) {}",
options: ["bar", "baz", "qux"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "var arr = [foo.bar];",
options: ["f", "fo", "fooo", "b", "ba", "barr", "bazz", "bingg"]
Expand Down