Skip to content

Commit

Permalink
Update: support multiple fixes in a report (fixes #7348) (#8101)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mysticatea
mysticatea committed Jun 18, 2017
1 parent 7a1bc38 commit 673a58b
Show file tree
Hide file tree
Showing 9 changed files with 409 additions and 53 deletions.
10 changes: 10 additions & 0 deletions docs/developer-guide/working-with-rules.md
View file
Expand Up @@ -198,6 +198,15 @@ The `fixer` object has the following methods:
* `replaceText(nodeOrToken, text)` - replaces the text in the given node or token
* `replaceTextRange(range, text)` - replaces the text in the given range

The above methods return a `fixing` object.
The `fix()` function can return the following values:

* A `fixing` object.
* An array which includes `fixing` objects.
* An iterable object which enumerates `fixing` objects. Especially, the `fix()` function can be a generator.

If you make a `fix()` function which returns multiple `fixing` objects, those `fixing` objects must not be overlapped.

Best practices for fixes:

1. Avoid any fixes that could change the runtime behavior of code and cause it to stop working.
Expand Down Expand Up @@ -286,6 +295,7 @@ Once you have an instance of `SourceCode`, you can use the methods on it to work
* `getNodeByRangeIndex(index)` - returns the deepest node in the AST containing the given source index.
* `getLocFromIndex(index)` - returns an object with `line` and `column` properties, corresponding to the location of the given source index. `line` is 1-based and `column` is 0-based.
* `getIndexFromLoc(loc)` - returns the index of a given location in the source code, where `loc` is an object with a 1-based `line` key and a 0-based `column` key.
* `commentsExistBetween(nodeOrToken1, nodeOrToken2)` - returns `true` if comments exist between two nodes.

> `skipOptions` is an object which has 3 properties; `skip`, `includeComments`, and `filter`. Default is `{skip: 0, includeComments: false, filter: null}`.
> - `skip` is a positive integer, the number of skipping tokens. If `filter` option is given at the same time, it doesn't count filtered tokens as skipped.
Expand Down
77 changes: 71 additions & 6 deletions lib/rule-context.js
View file
Expand Up @@ -8,6 +8,7 @@
// Requirements
//------------------------------------------------------------------------------

const assert = require("assert");
const ruleFixer = require("./util/rule-fixer");

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -60,6 +61,75 @@ const PASSTHROUGHS = [
// Rule Definition
//------------------------------------------------------------------------------

/**
* Compares items in a fixes array by range.
* @param {Fix} a The first message.
* @param {Fix} b The second message.
* @returns {int} -1 if a comes before b, 1 if a comes after b, 0 if equal.
* @private
*/
function compareFixesByRange(a, b) {
return a.range[0] - b.range[0] || a.range[1] - b.range[1];
}

/**
* Merges the given fixes array into one.
* @param {Fix[]} fixes The fixes to merge.
* @param {SourceCode} sourceCode The source code object to get the text between fixes.
* @returns {void}
*/
function mergeFixes(fixes, sourceCode) {
if (fixes.length === 0) {
return null;
}
if (fixes.length === 1) {
return fixes[0];
}

fixes.sort(compareFixesByRange);

const originalText = sourceCode.text;
const start = fixes[0].range[0];
const end = fixes[fixes.length - 1].range[1];
let text = "";
let lastPos = Number.MIN_SAFE_INTEGER;

for (const fix of fixes) {
assert(fix.range[0] >= lastPos, "Fix objects must not be overlapped in a report.");

if (fix.range[0] >= 0) {
text += originalText.slice(Math.max(0, start, lastPos), fix.range[0]);
}
text += fix.text;
lastPos = fix.range[1];
}
text += originalText.slice(Math.max(0, start, lastPos), end);

return { range: [start, end], text };
}

/**
* Gets one fix object from the given descriptor.
* If the descriptor retrieves multiple fixes, this merges those to one.
* @param {Object} descriptor The report descriptor.
* @param {SourceCode} sourceCode The source code object to get text between fixes.
* @returns {Fix} The got fix object.
*/
function getFix(descriptor, sourceCode) {
if (typeof descriptor.fix !== "function") {
return null;
}

// @type {null | Fix | Fix[] | IterableIterator<Fix>}
const fix = descriptor.fix(ruleFixer);

// Merge to one.
if (fix && Symbol.iterator in fix) {
return mergeFixes(Array.from(fix), sourceCode);
}
return fix;
}

/**
* Rule context class
* Acts as an abstraction layer between rules and the main eslint object.
Expand Down Expand Up @@ -120,12 +190,7 @@ class RuleContext {
// check to see if it's a new style call
if (arguments.length === 1) {
const descriptor = nodeOrDescriptor;
let fix = null;

// if there's a fix specified, get it
if (typeof descriptor.fix === "function") {
fix = descriptor.fix(ruleFixer);
}
const fix = getFix(descriptor, this.getSourceCode());

this.eslint.report(
this.id,
Expand Down
129 changes: 88 additions & 41 deletions lib/rules/arrow-body-style.js
View file
Expand Up @@ -65,6 +65,29 @@ module.exports = {
const requireReturnForObjectLiteral = options[1] && options[1].requireReturnForObjectLiteral;
const sourceCode = context.getSourceCode();

/**
* Checks whether the given node has ASI problem or not.
* @param {Token} token The token to check.
* @returns {boolean} `true` if it changes semantics if `;` or `}` followed by the token are removed.
*/
function hasASIProblem(token) {
return token && token.type === "Punctuator" && /^[([/`+-]/.test(token.value);
}

/**
* Gets the closing parenthesis which is the pair of the given opening parenthesis.
* @param {Token} token The opening parenthesis token to get.
* @returns {Token} The found closing parenthesis token.
*/
function findClosingParen(token) {
let node = sourceCode.getNodeByRangeIndex(token.range[1]);

while (!astUtils.isParenthesised(sourceCode, node)) {
node = node.parent;
}
return sourceCode.getTokenAfter(node);
}

/**
* Determines whether a arrow function body needs braces
* @param {ASTNode} node The arrow function node.
Expand All @@ -91,47 +114,55 @@ module.exports = {
loc: arrowBody.loc.start,
message: "Unexpected block statement surrounding arrow body.",
fix(fixer) {
if (blockBody.length !== 1 || blockBody[0].type !== "ReturnStatement" || !blockBody[0].argument) {
return null;
const fixes = [];

if (blockBody.length !== 1 ||
blockBody[0].type !== "ReturnStatement" ||
!blockBody[0].argument ||
hasASIProblem(sourceCode.getTokenAfter(arrowBody))
) {
return fixes;
}

const sourceText = sourceCode.getText();
const returnKeyword = sourceCode.getFirstToken(blockBody[0]);
const firstValueToken = sourceCode.getTokenAfter(returnKeyword);
let lastValueToken = sourceCode.getLastToken(blockBody[0]);

if (astUtils.isSemicolonToken(lastValueToken)) {

/* The last token of the returned value is the last token of the ReturnExpression (if
* the ReturnExpression has no semicolon), or the second-to-last token (if the ReturnExpression
* has a semicolon).
*/
lastValueToken = sourceCode.getTokenBefore(lastValueToken);
const openingBrace = sourceCode.getFirstToken(arrowBody);
const closingBrace = sourceCode.getLastToken(arrowBody);
const firstValueToken = sourceCode.getFirstToken(blockBody[0], 1);
const lastValueToken = sourceCode.getLastToken(blockBody[0]);
const commentsExist =
sourceCode.commentsExistBetween(openingBrace, firstValueToken) ||
sourceCode.commentsExistBetween(lastValueToken, closingBrace);

// Remove tokens around the return value.
// If comments don't exist, remove extra spaces as well.
if (commentsExist) {
fixes.push(
fixer.remove(openingBrace),
fixer.remove(closingBrace),
fixer.remove(sourceCode.getTokenAfter(openingBrace)) // return keyword
);
} else {
fixes.push(
fixer.removeRange([openingBrace.range[0], firstValueToken.range[0]]),
fixer.removeRange([lastValueToken.range[1], closingBrace.range[1]])
);
}

const tokenAfterArrowBody = sourceCode.getTokenAfter(arrowBody);

if (tokenAfterArrowBody && tokenAfterArrowBody.type === "Punctuator" && /^[([/`+-]/.test(tokenAfterArrowBody.value)) {
// If the first token of the reutrn value is `{`,
// enclose the return value by parentheses to avoid syntax error.
if (astUtils.isOpeningBraceToken(firstValueToken)) {
fixes.push(
fixer.insertTextBefore(firstValueToken, "("),
fixer.insertTextAfter(lastValueToken, ")")
);
}

// Don't do a fix if the next token would cause ASI issues when preceded by the returned value.
return null;
// If the last token of the return statement is semicolon, remove it.
// Non-block arrow body is an expression, not a statement.
if (astUtils.isSemicolonToken(lastValueToken)) {
fixes.push(fixer.remove(lastValueToken));
}

const textBeforeReturn = sourceText.slice(arrowBody.range[0] + 1, returnKeyword.range[0]);
const textBetweenReturnAndValue = sourceText.slice(returnKeyword.range[1], firstValueToken.range[0]);
const rawReturnValueText = sourceText.slice(firstValueToken.range[0], lastValueToken.range[1]);
const returnValueText = astUtils.isOpeningBraceToken(firstValueToken) ? `(${rawReturnValueText})` : rawReturnValueText;
const textAfterValue = sourceText.slice(lastValueToken.range[1], blockBody[0].range[1] - 1);
const textAfterReturnStatement = sourceText.slice(blockBody[0].range[1], arrowBody.range[1] - 1);

/*
* For fixes that only contain spaces around the return value, remove the extra spaces.
* This avoids ugly fixes that end up with extra spaces after the arrow, e.g. `() => 0 ;`
*/
return fixer.replaceText(
arrowBody,
(textBeforeReturn + textBetweenReturnAndValue).replace(/^\s*$/, "") + returnValueText + (textAfterValue + textAfterReturnStatement).replace(/^\s*$/, "")
);
return fixes;
}
});
}
Expand All @@ -142,21 +173,37 @@ module.exports = {
loc: arrowBody.loc.start,
message: "Expected block statement surrounding arrow body.",
fix(fixer) {
const lastTokenBeforeBody = sourceCode.getLastTokenBetween(sourceCode.getFirstToken(node), arrowBody, astUtils.isNotOpeningParenToken);
const firstBodyToken = sourceCode.getTokenAfter(lastTokenBeforeBody);

return fixer.replaceTextRange(
[firstBodyToken.range[0], node.range[1]],
`{return ${sourceCode.getText().slice(firstBodyToken.range[0], node.range[1])}}`
const fixes = [];
const arrowToken = sourceCode.getTokenBefore(arrowBody, astUtils.isArrowToken);
const firstBodyToken = sourceCode.getTokenAfter(arrowToken);
const lastBodyToken = sourceCode.getLastToken(node);
const isParenthesisedObjectLiteral =
astUtils.isOpeningParenToken(firstBodyToken) &&
astUtils.isOpeningBraceToken(sourceCode.getTokenAfter(firstBodyToken));

// Wrap the value by a block and a return statement.
fixes.push(
fixer.insertTextBefore(firstBodyToken, "{return "),
fixer.insertTextAfter(lastBodyToken, "}")
);

// If the value is object literal, remove parentheses which were forced by syntax.
if (isParenthesisedObjectLiteral) {
fixes.push(
fixer.remove(firstBodyToken),
fixer.remove(findClosingParen(firstBodyToken))
);
}

return fixes;
}
});
}
}
}

return {
ArrowFunctionExpression: validate
"ArrowFunctionExpression:exit": validate
};
}
};
6 changes: 4 additions & 2 deletions lib/testers/rule-tester.js
View file
Expand Up @@ -468,9 +468,11 @@ class RuleTester {
)
);

const hasMessageOfThisRule = messages.some(m => m.ruleId === ruleName);

for (let i = 0, l = item.errors.length; i < l; i++) {
assert.ok(!("fatal" in messages[i]), `A fatal parsing error occurred: ${messages[i].message}`);
assert.equal(messages[i].ruleId, ruleName, "Error rule name should be the same as the name of the rule being tested");
assert(!messages[i].fatal, `A fatal parsing error occurred: ${messages[i].message}`);
assert(hasMessageOfThisRule, "Error rule name should be the same as the name of the rule being tested");

if (typeof item.errors[i] === "string" || item.errors[i] instanceof RegExp) {

Expand Down
21 changes: 21 additions & 0 deletions lib/token-store/index.js
View file
Expand Up @@ -12,6 +12,7 @@ const assert = require("assert");
const cursors = require("./cursors");
const ForwardTokenCursor = require("./forward-token-cursor");
const PaddedTokenCursor = require("./padded-token-cursor");
const utils = require("./utils");
const astUtils = require("../ast-utils");

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -560,6 +561,26 @@ module.exports = class TokenStore {
).getAllTokens();
}

//--------------------------------------------------------------------------
// Others.
//--------------------------------------------------------------------------

/**
* Checks whether any comments exist or not between the given 2 nodes.
*
* @param {ASTNode} left - The node to check.
* @param {ASTNode} right - The node to check.
* @returns {boolean} `true` if one or more comments exist.
*/
commentsExistBetween(left, right) {
const index = utils.search(this[COMMENTS], left.range[1]);

return (
index < this[COMMENTS].length &&
this[COMMENTS][index].range[1] <= right.range[0]
);
}

/**
* Gets all comment tokens directly before the given node or token.
* @param {ASTNode|token} nodeOrToken The AST node or token to check for adjacent comment tokens.
Expand Down

0 comments on commit 673a58b

Please sign in to comment.