Skip to content
Permalink
Browse files

Fix: more autofix token-combining bugs (#8394)

There are cases in `dot-notation` and `no-floating-decimal` where tokens can be inadvertently combined by an autofix, resulting in incorrect code. This commit adds a helper function to ast-utils to detect whether tokens can be safely combined, and updates existing autofixers to use that function.
  • Loading branch information...
not-an-aardvark authored and kaicataldo committed Apr 7, 2017
1 parent f5a7e42 commit 275414198a41368438478b3c046e4bc762e2614b
@@ -10,6 +10,7 @@
//------------------------------------------------------------------------------

const esutils = require("esutils");
const espree = require("espree");

//------------------------------------------------------------------------------
// Helpers
@@ -1254,5 +1255,52 @@ module.exports = {
* `node.regex` instead. Also see: https://github.com/eslint/eslint/issues/8020
*/
return node.type === "Literal" && node.value === null && !node.regex;
},

/**
* Determines whether two tokens can safely be placed next to each other without merging into a single token
* @param {Token|string} leftValue The left token. If this is a string, it will be tokenized and the last token will be used.
* @param {Token|string} rightValue The right token. If this is a string, it will be tokenized and the first token will be used.
* @returns {boolean} If the tokens cannot be safely placed next to each other, returns `false`. If the tokens can be placed
* next to each other, behavior is undefined (although it should return `true` in most cases).
*/
canTokensBeAdjacent(leftValue, rightValue) {
let leftToken;

if (typeof leftValue === "string") {
const leftTokens = espree.tokenize(leftValue, { ecmaVersion: 2015 });

leftToken = leftTokens[leftTokens.length - 1];
} else {
leftToken = leftValue;
}

const rightToken = typeof rightValue === "string" ? espree.tokenize(rightValue, { ecmaVersion: 2015 })[0] : rightValue;

if (leftToken.type === "Punctuator" || rightToken.type === "Punctuator") {
if (leftToken.type === "Punctuator" && rightToken.type === "Punctuator") {
const PLUS_TOKENS = new Set(["+", "++"]);
const MINUS_TOKENS = new Set(["-", "--"]);

return !(
PLUS_TOKENS.has(leftToken.value) && PLUS_TOKENS.has(rightToken.value) ||
MINUS_TOKENS.has(leftToken.value) && MINUS_TOKENS.has(rightToken.value)
);
}
return true;
}

if (
leftToken.type === "String" || rightToken.type === "String" ||
leftToken.type === "Template" || rightToken.type === "Template"
) {
return true;
}

if (leftToken.type !== "Numeric" && rightToken.type === "Numeric" && rightToken.value.startsWith(".")) {
return true;
}

return false;
}
};
@@ -9,7 +9,6 @@
//------------------------------------------------------------------------------

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

//------------------------------------------------------------------------------
// Rule Definition
@@ -240,7 +239,7 @@ module.exports = {
// e.g. `do{foo()} while (bar)` should be corrected to `do foo() while (bar)`
const needsPrecedingSpace = node.type === "DoWhileStatement" &&
sourceCode.getTokenBefore(bodyNode).end === bodyNode.start &&
esUtils.code.isIdentifierPartES6(sourceCode.getText(bodyNode).charCodeAt(1));
!astUtils.canTokensBeAdjacent("do", sourceCode.getFirstToken(bodyNode, { skip: 1 }));

const openingBracket = sourceCode.getFirstToken(bodyNode);
const closingBracket = sourceCode.getLastToken(bodyNode);
@@ -79,11 +79,17 @@ module.exports = {
return null;
}

const tokenAfterProperty = sourceCode.getTokenAfter(rightBracket);
const needsSpaceAfterProperty = tokenAfterProperty &&
rightBracket.range[1] === tokenAfterProperty.range[0] &&
!astUtils.canTokensBeAdjacent(String(node.property.value), tokenAfterProperty);

const textBeforeDot = astUtils.isDecimalInteger(node.object) ? " " : "";
const textAfterProperty = needsSpaceAfterProperty ? " " : "";

return fixer.replaceTextRange(
[leftBracket.range[0], rightBracket.range[1]],
`${textBeforeDot}.${node.property.value}`
`${textBeforeDot}.${node.property.value}${textAfterProperty}`
);
}
});
@@ -9,7 +9,6 @@
//------------------------------------------------------------------------------

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

module.exports = {
meta: {
@@ -250,28 +249,10 @@ module.exports = {
const tokenBeforeLeftParen = sourceCode.getTokenBefore(node, 1);
const firstToken = sourceCode.getFirstToken(node);

// If there is already whitespace before the previous token, don't add more.
if (!tokenBeforeLeftParen || tokenBeforeLeftParen.end !== leftParenToken.start) {
return false;
}

// If the parens are preceded by a keyword (e.g. `typeof(0)`), a space should be inserted (`typeof 0`)
const precededByIdentiferPart = esUtils.code.isIdentifierPartES6(tokenBeforeLeftParen.value.slice(-1).charCodeAt(0));

// However, a space should not be inserted unless the first character of the token is an identifier part
// e.g. `typeof([])` should be fixed to `typeof[]`
const startsWithIdentifierPart = esUtils.code.isIdentifierPartES6(firstToken.value.charCodeAt(0));

// If the parens are preceded by and start with a unary plus/minus (e.g. `+(+foo)`), a space should be inserted (`+ +foo`)
const precededByUnaryPlus = tokenBeforeLeftParen.type === "Punctuator" && tokenBeforeLeftParen.value === "+";
const precededByUnaryMinus = tokenBeforeLeftParen.type === "Punctuator" && tokenBeforeLeftParen.value === "-";

const startsWithUnaryPlus = firstToken.type === "Punctuator" && firstToken.value === "+";
const startsWithUnaryMinus = firstToken.type === "Punctuator" && firstToken.value === "-";

return (precededByIdentiferPart && startsWithIdentifierPart) ||
(precededByUnaryPlus && startsWithUnaryPlus) ||
(precededByUnaryMinus && startsWithUnaryMinus);
return tokenBeforeLeftParen &&
tokenBeforeLeftParen.range[1] === leftParenToken.range[0] &&
leftParenToken.range[1] === firstToken.range[0] &&
!astUtils.canTokensBeAdjacent(tokenBeforeLeftParen, firstToken);
}

/**
@@ -5,6 +5,12 @@

"use strict";

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

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

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
@@ -23,16 +29,24 @@ module.exports = {
},

create(context) {
const sourceCode = context.getSourceCode();

return {
Literal(node) {

if (typeof node.value === "number") {
if (node.raw.indexOf(".") === 0) {
if (node.raw.startsWith(".")) {
context.report({
node,
message: "A leading decimal point can be confused with a dot.",
fix: fixer => fixer.insertTextBefore(node, "0")
fix(fixer) {
const tokenBefore = sourceCode.getTokenBefore(node);
const needsSpaceBefore = tokenBefore &&
tokenBefore.range[1] === node.range[0] &&
!astUtils.canTokensBeAdjacent(tokenBefore, `0${node.raw}`);

return fixer.insertTextBefore(node, needsSpaceBefore ? " 0" : "0");
}
});
}
if (node.raw.indexOf(".") === node.raw.length - 1) {
@@ -6,7 +6,6 @@
"use strict";

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

//------------------------------------------------------------------------------
// Helpers
@@ -215,8 +214,7 @@ module.exports = {
if (
tokenBefore &&
tokenBefore.range[1] === node.range[0] &&
esUtils.code.isIdentifierPartES6(tokenBefore.value.slice(-1).charCodeAt(0)) &&
esUtils.code.isIdentifierPartES6(recommendation.charCodeAt(0))
!astUtils.canTokensBeAdjacent(tokenBefore, recommendation)
) {
return fixer.replaceText(node, ` ${recommendation}`);
}
@@ -9,7 +9,6 @@
//------------------------------------------------------------------------------

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

//------------------------------------------------------------------------------
// Rule Definition
@@ -61,8 +60,7 @@ module.exports = {

// Insert a space before the key to avoid changing identifiers, e.g. ({ get[2]() {} }) to ({ get2() {} })
const needsSpaceBeforeKey = tokenBeforeLeftBracket.range[1] === leftSquareBracket.range[0] &&
esUtils.code.isIdentifierPartES6(tokenBeforeLeftBracket.value.slice(-1).charCodeAt(0)) &&
esUtils.code.isIdentifierPartES6(key.raw.charCodeAt(0));
!astUtils.canTokensBeAdjacent(tokenBeforeLeftBracket, sourceCode.getFirstToken(key));

const replacementKey = (needsSpaceBeforeKey ? " " : "") + key.raw;

@@ -4,6 +4,12 @@
*/
"use strict";

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

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

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
@@ -68,21 +74,6 @@ module.exports = {
return node.argument && node.argument.type && node.argument.type === "ObjectExpression";
}

/**
* Check if it is safe to remove the spaces between the two tokens in
* the context of a non-word prefix unary operator. For example, `+ +1`
* cannot safely be changed to `++1`.
* @param {Token} firstToken The operator for a non-word prefix unary operator
* @param {Token} secondToken The first token of its operand
* @returns {boolean} Whether or not the spacing between the tokens can be removed
*/
function canRemoveSpacesBetween(firstToken, secondToken) {
return !(
(firstToken.value === "+" && secondToken.value[0] === "+") ||
(firstToken.value === "-" && secondToken.value[0] === "-")
);
}

/**
* Checks if an override exists for a given operator.
* @param {ASTnode} node AST node
@@ -259,7 +250,7 @@ module.exports = {
operator: firstToken.value
},
fix(fixer) {
if (canRemoveSpacesBetween(firstToken, secondToken)) {
if (astUtils.canTokensBeAdjacent(firstToken, secondToken)) {
return fixer.removeRange([firstToken.range[1], secondToken.range[0]]);
}
return null;
@@ -1270,4 +1270,45 @@ describe("ast-utils", () => {
});
});
});

describe("canTokensBeAdjacent", () => {
const CASES = new Map([
[["foo", "bar"], false],
[[";foo", "bar"], false],
[[";", "bar"], true],
[[")", "bar"], true],
[["foo0", "bar"], false],
[["foo;", "bar"], true],
[["foo", "0"], false],
[["of", ".2"], true],
[["2", ".2"], false],
[["of", "'foo'"], true],
[["foo", "`bar`"], true],
[["`foo`", "in"], true],
[["of", "0.2"], false],
[["of", "0."], false],
[[".2", "foo"], false],
[["2.", "foo"], false],
[["+", "-"], true],
[["++", "-"], true],
[["+", "--"], true],
[["++", "--"], true],
[["-", "+"], true],
[["--", "+"], true],
[["-", "++"], true],
[["--", "++"], true],
[["+", "+"], false],
[["-", "-"], false],
[["++", "+"], false],
[["--", "-"], false],
[["+", "++"], false],
[["-", "--"], false]
]);

CASES.forEach((expectedResult, tokenStrings) => {
it(tokenStrings.join(", "), () => {
assert.strictEqual(astUtils.canTokensBeAdjacent(tokenStrings[0], tokenStrings[1]), expectedResult);
});
});
});
});
@@ -173,6 +173,11 @@ ruleTester.run("dot-notation", rule, {
code: "1['toString']",
output: "1 .toString",
errors: [{ message: "[\"toString\"] is better written in dot notation." }]
},
{
code: "foo['bar']instanceof baz",
output: "foo.bar instanceof baz",
errors: [{ message: "[\"bar\"] is better written in dot notation." }]
}
]
});
@@ -487,6 +487,7 @@ ruleTester.run("no-extra-parens", rule, {
invalid("typeof (0)", "typeof 0", "Literal"),
invalid("typeof([])", "typeof[]", "ArrayExpression"),
invalid("typeof ([])", "typeof []", "ArrayExpression"),
invalid("typeof( 0)", "typeof 0", "Literal"),
invalid("typeof(typeof 5)", "typeof typeof 5", "UnaryExpression"),
invalid("typeof (typeof 5)", "typeof typeof 5", "UnaryExpression"),
invalid("+(+foo)", "+ +foo", "UnaryExpression"),
@@ -43,6 +43,17 @@ ruleTester.run("no-floating-decimal", rule, {
code: "var x = -2.;",
output: "var x = -2.0;",
errors: [{ message: "A trailing decimal point can be confused with a dot.", type: "Literal" }]
},
{
code: "typeof.2",
output: "typeof 0.2",
errors: [{ message: "A leading decimal point can be confused with a dot.", type: "Literal" }]
},
{
code: "for(foo of.2);",
output: "for(foo of 0.2);",
parserOptions: { ecmaVersion: 2015 },
errors: [{ message: "A leading decimal point can be confused with a dot.", type: "Literal" }]
}
]
});

0 comments on commit 2754141

Please sign in to comment.
You can’t perform that action at this time.