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.

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

Fix: dot-location errors with parenthesized objects (fixes #11868) #11933

Merged
merged 3 commits into from Jul 21, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Next

Fix: dot-location errors with parenthesized objects (fixes #11868)

  • Loading branch information...
mdjermanovic committed Jul 2, 2019
commit 26b5987d21a4ae2aa56321f56fe79b077acb62da
@@ -43,7 +43,13 @@ Examples of **correct** code for the default `"object"` option:
var foo = object.
property;
var bar = object.property;
var bar = (
object
).
property;
var baz = object.property;
```

### property
@@ -54,29 +54,36 @@ module.exports = {
*/
function checkDotLocation(obj, prop, node) {
const dot = sourceCode.getTokenBefore(prop);
const textBeforeDot = sourceCode.getText().slice(obj.range[1], dot.range[0]);

/* istanbul ignore if */
if (astUtils.isNotDotToken(dot)) {
This conversation was marked as resolved by platinumazure

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Jul 4, 2019

Member

What's the purpose of this conditional? Is it just a sanity check?

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Jul 4, 2019

Author Member

Yes, it's a bit strange now.

This should never happen, I couldn't find an example in proposals, typescript etc. It's basically inherited from the previous version, the purpose was most likely to check for [] (computed key), which isn't necessary now.

But, it was also a check for an unknown syntax, so I wasn't sure should it stay or not.

Anyway, I completely agree that the purpose of this line is not clear to a reader, and it's better to do something about it. Should I remove the check, or add a comment?

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Jul 5, 2019

Author Member

I've added the comment.

If it would be better, I can remove the whole conditional, too.

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Jul 6, 2019

Member

Unless we can find a test case where this guard is necessary, I’d be more inclined to remove it for future maintainers’ sake.

return;
}

// `obj` expression can be parenthesized, but those paren tokens are not a part of the `obj` node.
const tokenBeforeDot = sourceCode.getTokenBefore(dot);

const textBeforeDot = sourceCode.getText().slice(tokenBeforeDot.range[1], dot.range[0]);
const textAfterDot = sourceCode.getText().slice(dot.range[1], prop.range[0]);

if (dot.type === "Punctuator" && dot.value === ".") {
if (onObject) {
if (!astUtils.isTokenOnSameLine(obj, dot)) {
const neededTextAfterObj = astUtils.isDecimalInteger(obj) ? " " : "";

context.report({
node,
loc: dot.loc.start,
messageId: "expectedDotAfterObject",
fix: fixer => fixer.replaceTextRange([obj.range[1], prop.range[0]], `${neededTextAfterObj}.${textBeforeDot}${textAfterDot}`)
});
}
} else if (!astUtils.isTokenOnSameLine(dot, prop)) {
if (onObject) {
if (!astUtils.isTokenOnSameLine(tokenBeforeDot, dot)) {
const neededTextAfterToken = astUtils.isDecimalIntegerNumericToken(tokenBeforeDot) ? " " : "";

context.report({
node,
loc: dot.loc.start,
messageId: "expectedDotBeforeProperty",
fix: fixer => fixer.replaceTextRange([obj.range[1], prop.range[0]], `${textBeforeDot}${textAfterDot}.`)
messageId: "expectedDotAfterObject",
fix: fixer => fixer.replaceTextRange([tokenBeforeDot.range[1], prop.range[0]], `${neededTextAfterToken}.${textBeforeDot}${textAfterDot}`)
});
}
} else if (!astUtils.isTokenOnSameLine(dot, prop)) {
context.report({
node,
loc: dot.loc.start,
messageId: "expectedDotBeforeProperty",
fix: fixer => fixer.replaceTextRange([tokenBeforeDot.range[1], prop.range[0]], `${textBeforeDot}${textAfterDot}.`)
});
}
}

@@ -86,7 +93,9 @@ module.exports = {
* @returns {void}
*/
function checkNode(node) {
checkDotLocation(node.object, node.property, node);
if (!node.computed) {
checkDotLocation(node.object, node.property, node);
}
}

return {
@@ -37,6 +37,8 @@ const LINEBREAKS = new Set(["\r\n", "\r", "\n", "\u2028", "\u2029"]);
// A set of node types that can contain a list of statements
const STATEMENT_LIST_PARENTS = new Set(["Program", "BlockStatement", "SwitchCase"]);

const DECIMAL_INTEGER_PATTERN = /^(0|[1-9]\d*)$/u;

/**
* Checks reference if is non initializer and writable.
* @param {Reference} reference - A reference to check.
@@ -283,6 +285,16 @@ function isCommaToken(token) {
return token.value === "," && token.type === "Punctuator";
}

/**
* Checks if the given token is a dot token or not.
*
* @param {Token} token - The token to check.
* @returns {boolean} `true` if the token is a dot token.
*/
function isDotToken(token) {
return token.value === "." && token.type === "Punctuator";
}

/**
* Checks if the given token is a semicolon token or not.
*
@@ -462,12 +474,14 @@ module.exports = {
isColonToken,
isCommaToken,
isCommentToken,
isDotToken,
isKeywordToken,
isNotClosingBraceToken: negate(isClosingBraceToken),
isNotClosingBracketToken: negate(isClosingBracketToken),
isNotClosingParenToken: negate(isClosingParenToken),
isNotColonToken: negate(isColonToken),
isNotCommaToken: negate(isCommaToken),
isNotDotToken: negate(isDotToken),
isNotOpeningBraceToken: negate(isOpeningBraceToken),
isNotOpeningBracketToken: negate(isOpeningBracketToken),
isNotOpeningParenToken: negate(isOpeningParenToken),
@@ -988,7 +1002,18 @@ module.exports = {
* '5' // false
*/
isDecimalInteger(node) {
return node.type === "Literal" && typeof node.value === "number" && /^(0|[1-9]\d*)$/u.test(node.raw);
return node.type === "Literal" && typeof node.value === "number" &&
DECIMAL_INTEGER_PATTERN.test(node.raw);
},

/**
* Determines whether this token is a decimal integer numeric token.
* This is similar to isDecimalInteger(), but for tokens.
* @param {Token} token - The token to check.
* @returns {boolean} `true` if this token is a decimal integer.
*/
isDecimalIntegerNumericToken(token) {
return token.type === "Numeric" && DECIMAL_INTEGER_PATTERN.test(token.value);
},

/**
@@ -37,6 +37,81 @@ ruleTester.run("dot-location", rule, {
{
code: "(obj)\n.prop",
options: ["property"]
},
{
code: "obj . prop",
options: ["object"]
},
{
code: "obj /* a */ . prop",
options: ["object"]
},
{
code: "obj . \nprop",
options: ["object"]
},
{
code: "obj . prop",
options: ["property"]
},
{
code: "obj . /* a */ prop",
options: ["property"]
},
{
code: "obj\n. prop",
options: ["property"]
},
{
code: "f(a\n).prop",
options: ["object"]
},
{
code: "`\n`.prop",
options: ["object"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "obj[prop]",
options: ["object"]
},
{
code: "obj[prop]",
options: ["property"]
},

// https://github.com/eslint/eslint/issues/11868 (also in invalid)
{
code: "(obj).prop",
options: ["object"]
},
{
code: "(obj).\nprop",
options: ["object"]
},
{
code: "(obj\n).\nprop",
options: ["object"]
},
{
code: "(\nobj\n).\nprop",
options: ["object"]
},
{
code: "((obj\n)).\nprop",
options: ["object"]
},
{
code: "(f(a)\n).\nprop",
options: ["object"]
},
{
code: "((obj\n)\n).\nprop",
options: ["object"]
},
{
code: "(\na &&\nb()\n).toString()",
options: ["object"]
}
],
invalid: [
@@ -81,6 +156,81 @@ ruleTester.run("dot-location", rule, {
output: "foo. /* a */ \n /* b */ /* c */ bar",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 2, column: 10 }]
},
{
code: "f(a\n)\n.prop",
output: "f(a\n).\nprop",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 3, column: 1 }]
},
{
code: "`\n`\n.prop",
output: "`\n`.\nprop",
options: ["object"],
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 3, column: 1 }]
},

// https://github.com/eslint/eslint/issues/11868 (also in valid)
{
code: "(a\n)\n.prop",
output: "(a\n).\nprop",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 3, column: 1 }]
},
{
code: "(a\n)\n.\nprop",
output: "(a\n).\n\nprop",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 3, column: 1 }]
},
{
code: "(f(a)\n)\n.prop",
output: "(f(a)\n).\nprop",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 3, column: 1 }]
},
{
code: "(f(a\n)\n)\n.prop",
output: "(f(a\n)\n).\nprop",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 4, column: 1 }]
},
{
code: "((obj\n))\n.prop",
output: "((obj\n)).\nprop",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 3, column: 1 }]
},
{
code: "((obj\n)\n)\n.prop",
output: "((obj\n)\n).\nprop",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 4, column: 1 }]
},
{
code: "(a\n) /* a */ \n.prop",
output: "(a\n). /* a */ \nprop",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 3, column: 1 }]
},
{
code: "(a\n)\n/* a */\n.prop",
output: "(a\n).\n/* a */\nprop",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 4, column: 1 }]
},
{
code: "(a\n)\n/* a */.prop",
output: "(a\n).\n/* a */prop",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 3, column: 8 }]
},
{
code: "(5)\n.toExponential()",
output: "(5).\ntoExponential()",
options: ["object"],
errors: [{ messageId: "expectedDotAfterObject", type: "MemberExpression", line: 2, column: 1 }]
}
]
});
@@ -629,7 +629,7 @@ describe("ast-utils", () => {
});
});

describe("isDecimalInteger", () => {
{
const expectedResults = {
5: true,
0: true,
@@ -642,12 +642,22 @@ describe("ast-utils", () => {
"'5'": false
};

Object.keys(expectedResults).forEach(key => {
it(`should return ${expectedResults[key]} for ${key}`, () => {
assert.strictEqual(astUtils.isDecimalInteger(espree.parse(key).body[0].expression), expectedResults[key]);
describe("isDecimalInteger", () => {
Object.keys(expectedResults).forEach(key => {
it(`should return ${expectedResults[key]} for ${key}`, () => {
assert.strictEqual(astUtils.isDecimalInteger(espree.parse(key).body[0].expression), expectedResults[key]);
});
});
});
});

describe("isDecimalIntegerNumericToken", () => {
Object.keys(expectedResults).forEach(key => {
it(`should return ${expectedResults[key]} for ${key}`, () => {
assert.strictEqual(astUtils.isDecimalIntegerNumericToken(espree.tokenize(key)[0]), expectedResults[key]);
});
});
});
}

describe("getFunctionNameWithKind", () => {
const expectedResults = {
@@ -993,6 +1003,28 @@ describe("ast-utils", () => {
});
}

{
const code = "const obj = {foo: 1.5, bar: a.b};";
const tokens = espree.parse(code, { ecmaVersion: 6, tokens: true }).tokens;
const expected = [false, false, false, false, false, false, false, false, false, false, false, true, false, false, false];

describe("isDotToken", () => {
tokens.forEach((token, index) => {
it(`should return ${expected[index]} for '${token.value}'.`, () => {
assert.strictEqual(astUtils.isDotToken(token), expected[index]);
});
});
});

describe("isNotDotToken", () => {
tokens.forEach((token, index) => {
it(`should return ${!expected[index]} for '${token.value}'.`, () => {
assert.strictEqual(astUtils.isNotDotToken(token), !expected[index]);
});
});
});
}

describe("isCommentToken", () => {
const code = "const obj = /*block*/ {foo: 1, bar: 2}; //line";
const ast = espree.parse(code, { ecmaVersion: 6, tokens: true, comment: true });
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.