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: ignored nodes in indent rule (fixes #9392) #9393
Conversation
Thanks for the pull request, @robinhouston! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
a61a7dd
to
90ae854
Compare
LGTM |
lib/rules/indent.js
Outdated
// If the token is ignored, set the desired indent to an IgnoredTokenIndent | ||
const observedIndent = this._tokenInfo.getTokenIndent(token).length / this._indentSize; | ||
|
||
this._desiredIndentCache.set(token, new IgnoredTokenIndent(observedIndent)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emm.. I'm not sure class IgnoredTokenIndent is a necessary. I think the easiest way is setting to be some different value, e.g. -1.
in this way, no need to calc observedIndent 😄, and you can simply check like this:
function validateTokenIndent(token, desiredIndentLevel) {
if (desiredIndentLevel < 0) {
return true;
}
// ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried something like that at first, and it resulted in a large number of test failures. So I don’t think this will work.
The reason is – as I understand it, which may be imperfectly – that the indentation of an ignored line may nevertheless influence the expected indentation of subsequent lines. In code terms, the value is used here.
Perhaps someone who understands this better can give a clearer explanation; but in any case having unsuccessfully tried something like this I don’t think it’s a goer, sadly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aladdin-add You can see the sort of problems your proposal would cause by changing the valueOf()
method of class IgnoredTokenIndent
to return 0 rather than this._indent
.
diff --git a/lib/rules/indent.js b/lib/rules/indent.js
index 3c22ad0..936d1d0 100644
--- a/lib/rules/indent.js
+++ b/lib/rules/indent.js
@@ -243,7 +243,7 @@ class IgnoredTokenIndent {
* @returns {number} The indent
*/
valueOf() {
- return this._indent;
+ return 0; //this._indent;
}
}
Here’s what I get when I run the tests with that change:
$ npm test
> eslint@4.8.0 test /Users/robin/code/eslint
> node Makefile.js test
Validating Makefile.js
Validating JSON Files
Validating Markdown Files
Validating JavaScript files
/Users/robin/code/eslint/lib/ast-utils.js
539:1 error Expected indentation of 4 spaces but found 16 indent
542:1 error Expected indentation of 0 spaces but found 12 indent
/Users/robin/code/eslint/lib/code-path-analysis/code-path-state.js
474:1 error Expected indentation of 0 spaces but found 16 indent
/Users/robin/code/eslint/lib/rules/array-bracket-newline.js
194:1 error Expected indentation of 4 spaces but found 20 indent
197:1 error Expected indentation of 0 spaces but found 16 indent
/Users/robin/code/eslint/lib/rules/array-bracket-spacing.js
193:1 error Expected indentation of 4 spaces but found 20 indent
199:1 error Expected indentation of 4 spaces but found 20 indent
/Users/robin/code/eslint/lib/rules/array-element-newline.js
193:1 error Expected indentation of 4 spaces but found 20 indent
195:1 error Expected indentation of 0 spaces but found 16 indent
/Users/robin/code/eslint/lib/rules/indent.js
245:12 error Expected 'this' to be used by class method 'valueOf' class-methods-use-this
246:19 error Expected exception block, space or tab after '//' in comment spaced-comment
/Users/robin/code/eslint/lib/rules/keyword-spacing.js
495:1 error Expected indentation of 4 spaces but found 20 indent
497:1 error Expected indentation of 0 spaces but found 16 indent
/Users/robin/code/eslint/lib/rules/no-cond-assign.js
92:1 error Expected indentation of 4 spaces but found 20 indent
93:1 error Expected indentation of 4 spaces but found 20 indent
94:1 error Expected indentation of 0 spaces but found 16 indent
/Users/robin/code/eslint/lib/rules/no-extra-parens.js
375:1 error Expected indentation of 4 spaces but found 24 indent
376:1 error Expected indentation of 4 spaces but found 24 indent
377:1 error Expected indentation of 4 spaces but found 24 indent
379:1 error Expected indentation of 0 spaces but found 20 indent
467:1 error Expected indentation of 4 spaces but found 20 indent
469:1 error Expected indentation of 4 spaces but found 24 indent
472:1 error Expected indentation of 0 spaces but found 20 indent
474:1 error Expected indentation of 0 spaces but found 16 indent
570:1 error Expected indentation of 4 spaces but found 32 indent
571:1 error Expected indentation of 0 spaces but found 28 indent
614:1 error Expected indentation of 4 spaces but found 24 indent
616:1 error Expected indentation of 4 spaces but found 28 indent
620:1 error Expected indentation of 0 spaces but found 24 indent
621:1 error Expected indentation of 0 spaces but found 20 indent
/Users/robin/code/eslint/lib/rules/no-implicit-coercion.js
81:1 error Expected indentation of 4 spaces but found 12 indent
84:1 error Expected indentation of 0 spaces but found 8 indent
/Users/robin/code/eslint/lib/rules/no-lonely-if.js
57:1 error Expected indentation of 4 spaces but found 36 indent
61:1 error Expected indentation of 0 spaces but found 32 indent
/Users/robin/code/eslint/lib/rules/no-mixed-operators.js
121:1 error Expected indentation of 4 spaces but found 20 indent
123:1 error Expected indentation of 0 spaces but found 16 indent
/Users/robin/code/eslint/lib/rules/no-multi-spaces.js
92:1 error Expected indentation of 4 spaces but found 28 indent
94:1 error Expected indentation of 0 spaces but found 24 indent
/Users/robin/code/eslint/lib/rules/no-unused-vars.js
385:1 error Expected indentation of 4 spaces but found 20 indent
387:1 error Expected indentation of 0 spaces but found 16 indent
391:1 error Expected indentation of 4 spaces but found 20 indent
394:1 error Expected indentation of 0 spaces but found 16 indent
/Users/robin/code/eslint/lib/rules/no-useless-call.js
25:1 error Expected indentation of 4 spaces but found 12 indent
27:1 error Expected indentation of 0 spaces but found 8 indent
/Users/robin/code/eslint/lib/rules/no-useless-constructor.js
133:1 error Expected indentation of 4 spaces but found 12 indent
135:1 error Expected indentation of 0 spaces but found 8 indent
/Users/robin/code/eslint/lib/rules/object-curly-newline.js
141:1 error Expected indentation of 4 spaces but found 20 indent
144:1 error Expected indentation of 0 spaces but found 16 indent
/Users/robin/code/eslint/lib/rules/padding-line-between-statements.js
116:1 error Expected indentation of 4 spaces but found 12 indent
118:1 error Expected indentation of 4 spaces but found 16 indent
120:1 error Expected indentation of 0 spaces but found 12 indent
121:1 error Expected indentation of 0 spaces but found 8 indent
/Users/robin/code/eslint/lib/rules/prefer-const.js
51:1 error Expected indentation of 4 spaces but found 12 indent
54:1 error Expected indentation of 0 spaces but found 8 indent
/Users/robin/code/eslint/lib/rules/space-before-function-paren.js
73:1 error Expected indentation of 4 spaces but found 24 indent
76:1 error Expected indentation of 0 spaces but found 20 indent
77:1 error Expected indentation of 0 spaces but found 16 indent
/Users/robin/code/eslint/lib/rules/vars-on-top.js
59:1 error Expected indentation of 4 spaces but found 20 indent
62:1 error Expected indentation of 0 spaces but found 16 indent
✖ 59 problems (59 errors, 0 warnings)
58 errors, 0 warnings potentially fixable with the `--fix` option.
/Users/robin/code/eslint/node_modules/shelljs/src/common.js:417
if (config.fatal) throw e;
^
Error: exec: internal error
at Object.error (/Users/robin/code/eslint/node_modules/shelljs/src/common.js:128:27)
at _exec (/Users/robin/code/eslint/node_modules/shelljs/src/exec.js:292:12)
at /Users/robin/code/eslint/node_modules/shelljs/src/common.js:350:23
at Function.target.lint (/Users/robin/code/eslint/Makefile.js:532:18)
at Object.global.target.(anonymous function) [as lint] (/Users/robin/code/eslint/node_modules/shelljs/make.js:36:40)
at Function.target.test (/Users/robin/code/eslint/Makefile.js:585:12)
at Object.global.target.(anonymous function) [as test] (/Users/robin/code/eslint/node_modules/shelljs/make.js:36:40)
at /Users/robin/code/eslint/node_modules/shelljs/make.js:48:27
at Array.forEach (native)
at Timeout._onTimeout (/Users/robin/code/eslint/node_modules/shelljs/make.js:46:10)
npm ERR! Test failed. See above for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you are right! this is more complicated than I thought. ovvo
But I'm afraid it's not a good idea to store an object like this -- it's so strange! maybe there is something wrong withgetTokenIndent(token)
, we need to figure out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think there’s anything wrong with getTokenIndent(token)
, which seems to be working as intended.
I guess it’s a matter of taste whether one considers the use of a marker class to be strange. I’m certainly open to alternative implementations if you have any suggestions. The only thing I really care about is getting this fixed somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I’ve added a link to an alternative implementation in a top-level comment below.)
@aladdin-add I have written an alternative solution which you may prefer, in branch If you prefer that solution, I can update this branch with that code instead. |
great! I prefer that one. @robinhouston so we need a member to review. thanks for doing this! |
90ae854
to
8af1f44
Compare
LGTM |
Okay, I’ve updated this branch with the alternative implementation. For the record, so the above discussion makes sense, my original implementation is now in the branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I think the current implementation isn't quite correct. As you noted in #9393 (comment), the indentation of an ignored token is sometimes used to compute the indentation of other tokens in the file. Generally, this is used to ensure cases like this are linted correctly:
var x = foo &&
function() {
foo();
bar();
}
In the example above, the function
token is considered to be ignored, and any indentation would be allowed for that line. However, the indentations of the foo()
and bar()
statements in the function are validated. This ensures that foo()
and bar()
are indented correctly relative to the function
token, even though the function
token could have any indentation.
Normally, this works by checking the actual indentation of the function
token (as a number of spaces), and setting its desired offset to the same number of spaces, to ensure that the desired indentation and the actual indentation always match. Unfortunately, this doesn't work when the function
token is indented with tabs, because the rule just returns the expected number of indentation characters, and the type of those indentation characters (spaces or tabs) is handled at a higher level.
The problem with the implementation in this PR is that it doesn't respect dependent token offsets correctly. For example:
/* eslint indent: [error, tab, { ArrayExpression: first, ignoredNodes: [CallExpression] }] */
[
foo(),
bar
]
In this example, the bar
token is supposed to be aligned with the foo
token (since the ArrayExpression: first
option is used. Additionally, note that the rule is configured with tab indentation while lines 2 and 3 are indented with spaces, and the indentation of the foo()
expression is ignored.
This example should not report an error, because bar
is aligned with foo()
, and foo
is ignored. However, it currently does report an error with the implementation in this PR, because the indentation of foo()
is ignored in the new if
statement that you've added, rather than being ignored in the "regular" way where dependent offsets are adjusted correctly.
I think the best way to solve this problem would be to:
- Update
getDesiredIndent
to return a string containing the actual desired indentation, rather than a number containing the desired indent level. - Update
validateTokenIndent
to accept a string rather than an indent level - Update
report
to accept a string rather than an indent level. - Update the places where
getDesiredIndent
is called, to adjust for the fact that a string is returned.
I've created an implementation of this which seems to work, but feel free to test it out more and check for anything I've missed. Here's the diff from the original version of indent
on master:
diff
diff --git a/lib/rules/indent.js b/lib/rules/indent.js
index 0f6468a7..d33ed1ab 100644
--- a/lib/rules/indent.js
+++ b/lib/rules/indent.js
@@ -235,10 +235,12 @@ class OffsetStorage {
/**
* @param {TokenInfo} tokenInfo a TokenInfo instance
* @param {number} indentSize The desired size of each indentation level
+ * @param {string} indentType The indentation character
*/
- constructor(tokenInfo, indentSize) {
+ constructor(tokenInfo, indentSize, indentType) {
this._tokenInfo = tokenInfo;
this._indentSize = indentSize;
+ this._indentType = indentType;
this._tree = new BinarySearchTree();
this._tree.insert(0, { offset: 0, from: null, force: false });
@@ -408,7 +410,7 @@ class OffsetStorage {
/**
* Gets the desired indent of a token
* @param {Token} token The token
- * @returns {number} The desired indent of the token
+ * @returns {string} The desired indent of the token
*/
getDesiredIndent(token) {
if (!this._desiredIndentCache.has(token)) {
@@ -417,7 +419,10 @@ class OffsetStorage {
// If the token is ignored, use the actual indent of the token as the desired indent.
// This ensures that no errors are reported for this token.
- this._desiredIndentCache.set(token, this._tokenInfo.getTokenIndent(token).length / this._indentSize);
+ this._desiredIndentCache.set(
+ token,
+ this._tokenInfo.getTokenIndent(token)
+ );
} else if (this._lockedFirstTokens.has(token)) {
const firstToken = this._lockedFirstTokens.get(token);
@@ -428,7 +433,7 @@ class OffsetStorage {
this.getDesiredIndent(this._tokenInfo.getFirstTokenOfLine(firstToken)) +
// (space between the start of the first element's line and the first element)
- (firstToken.loc.start.column - this._tokenInfo.getFirstTokenOfLine(firstToken).loc.start.column) / this._indentSize
+ this._indentType.repeat(firstToken.loc.start.column - this._tokenInfo.getFirstTokenOfLine(firstToken).loc.start.column)
);
} else {
const offsetInfo = this._getOffsetDescriptor(token);
@@ -436,9 +441,12 @@ class OffsetStorage {
offsetInfo.from &&
offsetInfo.from.loc.start.line === token.loc.start.line &&
!offsetInfo.force
- ) ? 0 : offsetInfo.offset;
+ ) ? 0 : offsetInfo.offset * this._indentSize;
- this._desiredIndentCache.set(token, offset + (offsetInfo.from ? this.getDesiredIndent(offsetInfo.from) : 0));
+ this._desiredIndentCache.set(
+ token,
+ (offsetInfo.from ? this.getDesiredIndent(offsetInfo.from) : "") + this._indentType.repeat(offset)
+ );
}
}
return this._desiredIndentCache.get(token);
@@ -655,7 +663,7 @@ module.exports = {
const sourceCode = context.getSourceCode();
const tokenInfo = new TokenInfo(sourceCode);
- const offsets = new OffsetStorage(tokenInfo, indentSize);
+ const offsets = new OffsetStorage(tokenInfo, indentSize, indentType === "space" ? " " : "\t");
const parameterParens = new WeakSet();
/**
@@ -688,27 +696,24 @@ module.exports = {
/**
* Reports a given indent violation
* @param {Token} token Token violating the indent rule
- * @param {int} neededIndentLevel Expected indentation level
- * @param {int} gottenSpaces Actual number of indentation spaces for the token
- * @param {int} gottenTabs Actual number of indentation tabs for the token
+ * @param {string} neededIndent Expected indentation string
* @returns {void}
*/
- function report(token, neededIndentLevel) {
+ function report(token, neededIndent) {
const actualIndent = Array.from(tokenInfo.getTokenIndent(token));
const numSpaces = actualIndent.filter(char => char === " ").length;
const numTabs = actualIndent.filter(char => char === "\t").length;
- const neededChars = neededIndentLevel * indentSize;
context.report({
node: token,
- message: createErrorMessage(neededChars, numSpaces, numTabs),
+ message: createErrorMessage(neededIndent.length, numSpaces, numTabs),
loc: {
start: { line: token.loc.start.line, column: 0 },
end: { line: token.loc.start.line, column: token.loc.start.column }
},
fix(fixer) {
const range = [token.range[0] - token.loc.start.column, token.range[0]];
- const newText = (indentType === "space" ? " " : "\t").repeat(neededChars);
+ const newText = neededIndent;
return fixer.replaceTextRange(range, newText);
}
@@ -718,14 +723,13 @@ module.exports = {
/**
* Checks if a token's indentation is correct
* @param {Token} token Token to examine
- * @param {int} desiredIndentLevel needed indent level
+ * @param {string} desiredIndent Desired indentation of the string
* @returns {boolean} `true` if the token's indentation is correct
*/
- function validateTokenIndent(token, desiredIndentLevel) {
+ function validateTokenIndent(token, desiredIndent) {
const indentation = tokenInfo.getTokenIndent(token);
- const expectedChar = indentType === "space" ? " " : "\t";
- return indentation === expectedChar.repeat(desiredIndentLevel * indentSize) ||
+ return indentation === desiredIndent ||
// To avoid conflicts with no-mixed-spaces-and-tabs, don't report mixed spaces and tabs.
indentation.includes(" ") && indentation.includes("\t");
@not-an-aardvark Thanks for the detailed review! This is great. Thanks for explaining the dependent token offsets: I was conscious that I didn’t really understand what was going on there. I guess we need some more tests as well. Shall I add some tests and replace my changes to the rule with your patch, or would you prefer to close this PR and commit your fix separately? |
Add test suggested by @not-an-aardvark in: eslint#9393 (review)
8af1f44
to
3b9daa7
Compare
LGTM |
When a node is ignored by the indent rule, it ought not to matter how it’s indented. But the ignoring of nodes was implemented in such a way that the *type* of indentation (tabs vs spaces) was being checked. For example in "tab" mode, an ignored line indented by four spaces would cause the error “Expected indentation of 4 tabs but found 4 spaces”. In particular, this is a problem with “tabs for indentation, spaces for alignment” styles, where we want to allow code like: var x = 1, y = 2; where the second line is aligned using four spaces. The implementation is taken from @not-an-aardvark’s comment eslint#9393 (review) All tests pass. Fixes eslint#9392.
I’ve updated this PR with the test case and implementation from @not-an-aardvark’s review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Leaving this open for another day in case any other team members want to review it.
When a node is ignored by the indent rule, it ought not to matter how it’s indented. But the ignoring of nodes was implemented in such a way that the *type* of indentation (tabs vs spaces) was being checked. For example in "tab" mode, an ignored line indented by four spaces would cause the error “Expected indentation of 4 tabs but found 4 spaces”. In particular, this is a problem with “tabs for indentation, spaces for alignment” styles, where we want to allow code like: var x = 1, y = 2; where the second line is aligned using four spaces. The implementation is taken from @not-an-aardvark’s comment eslint#9393 (review) All tests pass. Fixes eslint#9392.
3b9daa7
to
bf7a3ed
Compare
LGTM |
Thanks for contributing! |
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
(I shan’t include the bug report template here unless you would especially like me to, since I have already filed the corresponding issue as #9392.)
When a node is ignored by the indent rule, it ought not to matter how it’s indented. But the ignoring of nodes was implemented in such a way that the type of indentation (tabs vs spaces) was being checked. For example in "tab" mode, an ignored line indented by four spaces would cause the error “Expected indentation of 4 tabs but found 4 spaces”.
In particular, this is a problem with “tabs for indentation, spaces for alignment” styles, where we want to allow code like:
where the second line is aligned using four spaces.
This commit marks ignored indents by making them instances of the
IgnoredTokenIndent
class, and explicitly ignores the indentation of such lines.All tests pass.
Is there anything you'd like reviewers to focus on?
Nothing particular comes to mind.