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: no-dupe-keys false negative (fixes #6801) #6863

Merged
merged 1 commit into from
Aug 10, 2016
Merged

Fix: no-dupe-keys false negative (fixes #6801) #6863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
122 changes: 98 additions & 24 deletions lib/rules/no-dupe-keys.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,79 @@

"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const astUtils = require("../ast-utils");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

const GET_KIND = /^(?:init|get)$/;
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm not sure what the first part of this regEx is supposed to check?

Copy link
Member Author

@mysticatea mysticatea Aug 9, 2016 •
edited

Choose a reason for hiding this comment

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

Normal property's kind is "init". This regexp is handling normal properties as getter/setter pair. As a result, this rule will address {a: 1, get a() {}} as duplicate keys. This is the purpose of this PR :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK, now I understand, thanks.

const SET_KIND = /^(?:init|set)$/;

/**
* The class which stores properties' information of an object.
*/
class ObjectInfo {

/**
* @param {ObjectInfo|null} upper - The information of the outer object.
* @param {ASTNode} node - The ObjectExpression node of this information.
*/
constructor(upper, node) {
this.upper = upper;
this.node = node;
this.properties = new Map();
}

/**
* Gets the information of the given Property node.
* @param {ASTNode} node - The Property node to get.
* @returns {{get: boolean, set: boolean}} The information of the property.
*/
getPropertyInfo(node) {
const name = astUtils.getStaticPropertyName(node);

if (!this.properties.has(name)) {
this.properties.set(name, {get: false, set: false});
}
return this.properties.get(name);
}

/**
* Checks whether the given property has been defined already or not.
* @param {ASTNode} node - The Property node to check.
* @returns {boolean} `true` if the property has been defined.
*/
isPropertyDefined(node) {
const entry = this.getPropertyInfo(node);

return (
(GET_KIND.test(node.kind) && entry.get) ||
(SET_KIND.test(node.kind) && entry.set)
);
}

/**
* Defines the given property.
* @param {ASTNode} node - The Property node to define.
* @returns {void}
*/
defineProperty(node) {
const entry = this.getPropertyInfo(node);

if (GET_KIND.test(node.kind)) {
entry.get = true;
}
if (SET_KIND.test(node.kind)) {
entry.set = true;
}
}
}

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand All @@ -20,37 +93,38 @@ module.exports = {
schema: []
},

create: function(context) {
create(context) {
let info = null;

return {
ObjectExpression(node) {
info = new ObjectInfo(info, node);
},
"ObjectExpression:exit"() {
info = info.upper;
},

ObjectExpression: function(node) {

// Object that will be a map of properties--safe because we will
// prefix all of the keys.
const nodeProps = Object.create(null);
Property(node) {
const name = astUtils.getStaticPropertyName(node);

node.properties.forEach(function(property) {
// Skip if the name is not static.
if (!name) {
return;
}

if (property.type !== "Property") {
return;
}

const keyName = property.key.name || property.key.value,
key = property.kind + "-" + keyName,
checkProperty = (!property.computed || property.key.type === "Literal");

if (checkProperty) {
if (nodeProps[key]) {
context.report(node, property.loc.start, "Duplicate key '{{key}}'.", { key: keyName });
} else {
nodeProps[key] = true;
}
}
});
// Reports if the name is defined already.
if (info.isPropertyDefined(node)) {
context.report({
node: info.node,
loc: node.key.loc,
message: "Duplicate key '{{name}}'.",
data: {name},
});
}

// Update info.
info.defineProperty(node);
}
};

}
};
9 changes: 7 additions & 2 deletions tests/lib/rules/no-dupe-keys.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,18 @@ ruleTester.run("no-dupe-keys", rule, {
"var x = { foo: 1, bar: 2 };",
"+{ get a() { }, set a(b) { } };",
{ code: "var x = { a: b, [a]: b };", parserOptions: { ecmaVersion: 6 }},
{ code: "var x = { a: b, ...c }", parserOptions: { ecmaVersion: 6, ecmaFeatures: { experimentalObjectRestSpread: true } }}
{ code: "var x = { a: b, ...c }", parserOptions: { ecmaVersion: 6, ecmaFeatures: { experimentalObjectRestSpread: true } }},
{ code: "var x = { get a() {}, set a (value) {} };", parserOptions: { ecmaVersion: 6 }},
{ code: "var x = { a: 1, b: { a: 2 } };", parserOptions: { ecmaVersion: 6 }},
],
invalid: [
{ code: "var x = { a: b, ['a']: b };", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Duplicate key 'a'.", type: "ObjectExpression"}] },
{ code: "var x = { y: 1, y: 2 };", errors: [{ message: "Duplicate key 'y'.", type: "ObjectExpression"}] },
{ code: "var foo = { 0x1: 1, 1: 2};", errors: [{ message: "Duplicate key '1'.", type: "ObjectExpression"}] },
{ code: "var x = { \"z\": 1, z: 2 };", errors: [{ message: "Duplicate key 'z'.", type: "ObjectExpression"}] },
{ code: "var foo = {\n bar: 1,\n bar: 1,\n}", errors: [{ message: "Duplicate key 'bar'.", line: 3, column: 3}]}
{ code: "var foo = {\n bar: 1,\n bar: 1,\n}", errors: [{ message: "Duplicate key 'bar'.", line: 3, column: 3}]},
{ code: "var x = { a: 1, get a() {} };", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Duplicate key 'a'.", type: "ObjectExpression"}] },
{ code: "var x = { a: 1, set a(value) {} };", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Duplicate key 'a'.", type: "ObjectExpression"}] },
{ code: "var x = { a: 1, b: { a: 2 }, get b() {} };", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Duplicate key 'b'.", type: "ObjectExpression"}] },
]
});