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

Update: support multiple fixes in a report (fixes #7348) #8101

Merged
merged 9 commits into from Jun 18, 2017
Merged

Update: support multiple fixes in a report (fixes #7348) #8101

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
926b00a
Update: support multiple fixes in a report (refs #7348)
mysticatea Feb 28, 2017
7271a5e
Chore: refactor fix of arrow-body-style
mysticatea Feb 18, 2017
5dba973
Chore: add a test for semi
mysticatea Feb 16, 2017
cc5de10
Merge changes from master branch.
mysticatea May 4, 2017
c888f09
add docs
mysticatea Jun 16, 2017
3847673
fix for negative indices
mysticatea Jun 16, 2017
a4c3372
Merge remote-tracking branch 'origin/master' into multiple-fixes-per-…
mysticatea Jun 16, 2017
f85aee0
add docs
mysticatea Jun 16, 2017
11a9043
fix for overlapping of negative ranges
mysticatea Jun 17, 2017
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
75 changes: 69 additions & 6 deletions lib/rule-context.js
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@
// Requirements
//------------------------------------------------------------------------------

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

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

/**
* Compares items in a fixes array by range.
* @param {Fix} a The first message.
* @param {Fix} b The second message.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be "The first fix" and "The second fix"

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, it's my copy/paste mistake. Thank you!

* @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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth noting that mergeFixes could be made an internal utility instead of a public API if there's any controversy around this API addition.

Copy link
Member Author

Choose a reason for hiding this comment

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

As public API, this is simple enhancement; the fix functions support for an iterable object as their return value.

There are 2 topics here.

  • To not modify comments, this feature would be helpful.
  • To avoid autofix conflict, this feature would be helpful.

As I mentioned in this post, currently we have a pain to remain comments. The purpose of the fix functions is "to modify code", but we have to focus on "to not modify code" mainly in order to not remove comments. This enhancement would resolve the problem. I think this is valuable for plugin ecosystem as well.

In addition, we can address locked ranges in the same mechanism.

In fact, this is the 1st step of my plan.

Because the report of linting has fix properties which have a fix object, this PR is merging the multiple fixes before it makes the report for now. I have to avoid a breaking change about the fix property (the shape of the report also are public API).

However, I have a plan (though it's not accepted yet) to change the fix property to fixes property that has an array of fix objects, so this mergeFixes function would be removed in future. It implies that fixing process can distinguish between fixing ranges and locking ranges. I expect it to allow us smarter conflict check.

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 = start;

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

Choose a reason for hiding this comment

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

Requiring the ranges to be disjoint could make fixer.keep a lot less useful, I think. In #8067 for the no-else-return/no-useless-return fix (#8026), I intentionally want to keep all of an outer node (the function containing the fix) while modifying something inside of it.

With this API, I guess what I'd do is compute fixStart and fixEnd and yield that fix, then yield keepRange(functionStart, fixStart) and keepRange(fixEnd, functionEnd). That isn't so bad, I suppose, but seems a lot more clunky than saying "preserve the function and replace this text within the function".

One possible solution, if we're ok with it being a little magical, is to allow fixes to be completely contained within other fixes and always use the most specific one. Or maybe the fixes could be applied in order in a reasonable way, with later ones taking precedence.

(Although I recognize that there's maybe some disagreement about whether keep should be used that way in my use case anyway.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I think it's rare case if we need locking the outer node.
In no-else-return case, it needs locking only the previous if block.
In no-useless-return case, it needs locking only after the return statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, I can see how the fixes for no-else-return and no-useless-return would work in that case. (I'm still a little unsure about how to properly reason about the implementation of no-useless-return, but it certainly seems easy and correct to call keepRange from the end of the fix to the end of the function.)

I think locking the outer node can be nice because it's safer and makes the rule simpler, but I can see the argument that it's usually not necessary. And if the long-term plan is to allow a distinction between "fixing" and "locking" operations, then I think it shouldn't be hard to allow the two types to overlap, which would address my concerns there.


text += originalText.slice(lastPos, fix.range[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Some fix ranges, like unicode-bom, use -1 as the start to indicate that the fix should replace the BOM. I think that will cause a bug here because .slice(-1) will take the last character in the string.

text += fix.text;
lastPos = fix.range[1];
}
text += originalText.slice(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 +188,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
Open in desktop
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
Open in desktop
Expand Up @@ -469,9 +469,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
Open in desktop
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
68 changes: 68 additions & 0 deletions tests/lib/rule-context.js
View file
Open in desktop
Expand Up @@ -81,6 +81,9 @@ describe("RuleContext", () => {
mockESLint.expects("report")
.once()
.withArgs("fake-rule", 2, node, location, message, messageOpts, fixerObj);
mockESLint.expects("getSourceCode")
.once()
.returns(null);

ruleContext.report({
node,
Expand All @@ -93,6 +96,71 @@ describe("RuleContext", () => {
fix.verify();
mockESLint.verify();
});

it("should merge fixes to one if 'fix' function returns an array of fixes.", () => {
const mockESLint = sandbox.mock(eslint);

mockESLint.expects("getSourceCode")
.returns({ text: "var foo = 100;" });
mockESLint.expects("report")
.once()
.withArgs(
sinon.match.any,
sinon.match.any,
sinon.match.any,
sinon.match.any,
sinon.match.any,
sinon.match.any,
sinon.match({
range: [4, 13],
text: "bar = 234"
})
);

ruleContext.report({
node: {},
loc: {},
message: "Message",
fix(fixer) {
return [
fixer.replaceTextRange([10, 13], "234"),
fixer.replaceTextRange([4, 7], "bar")
];
}
});

mockESLint.verify();
});

it("should merge fixes to one if 'fix' function returns an iterator of fixes.", () => {
const mockESLint = sandbox.mock(eslint);

mockESLint.expects("getSourceCode").returns({ text: "var foo = 100;" });
mockESLint.expects("report").once().withArgs(
sinon.match.any,
sinon.match.any,
sinon.match.any,
sinon.match.any,
sinon.match.any,
sinon.match.any,
sinon.match({
range: [4, 13],
text: "bar = 234"
})
);

ruleContext.report({
node: {},
loc: {},
message: "Message",
*fix(fixer) {
yield fixer.replaceTextRange([10, 13], "234");
yield fixer.replaceTextRange([4, 7], "bar");
}
});

mockESLint.verify();
});
});
});

Expand Down