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

Fix false negatives with block-indentation rule #2318

Merged
merged 4 commits into from Jan 5, 2022
Merged

Fix false negatives with block-indentation rule #2318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b1e9586
fix: fix validateBlockStart to validate all the lines
Dec 20, 2021
1bb74e9
fix: fix indentation of first block when that blokc is on a single line
Jan 5, 2022
55e4294
fix: fix some issues
Jan 5, 2022
7088cf0
fix: fix indentation when tag has a leading content
Jan 5, 2022
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
177 changes: 103 additions & 74 deletions lib/rules/block-indentation.js
View file
Open in desktop
Expand Up @@ -192,18 +192,14 @@ module.exports = class BlockIndentation extends Rule {
this.seen = new Set();

return {
Template(node) {
if (this.mode === 'fix') {
if (
node.body[0] &&
node.body[0].type === 'TextNode' &&
!removeWhitespaceEnd(node.body[0].chars)
) {
// We need to remove the text node in this case
node.body.shift();
}
}
return node;
Template: {
enter(node) {
this.template = node;
return node;
},
exit() {
this.template = null;
},
},

BlockStatement(node) {
Expand All @@ -230,14 +226,16 @@ module.exports = class BlockIndentation extends Rule {
* @returns {Node}
*/
process(node, path) {
let fixedNode = this.validateBlockStart(node, path);

// Nodes that start and end on the same line cannot have any indentation
// issues (since the column of the start block was already checked in the
// parent's validateBlockChildren())
if (node.loc.start.line === node.loc.end.line || this.seen.has(node)) {
return node;
this.seen.add(fixedNode);
return fixedNode;
}

let fixedNode = this.validateBlockStart(node, path);
fixedNode = this.validateBlockElse(fixedNode, path);
fixedNode = this.validateBlockEnd(fixedNode, path);
fixedNode = this.validateBlockChildren(fixedNode, path);
Expand Down Expand Up @@ -281,7 +279,7 @@ module.exports = class BlockIndentation extends Rule {
}

// Recreate the node only if the source has changed, otherwise this would mean useless computation and possible mistake
if (sourceBeforeFix !== this.sourceEdited) {
if (sourceBeforeFix !== this.sourceEdited && this.sourceEdited) {
fixedNode = this.fixLine(
this.sourceEdited.split('\n'),
0,
Expand All @@ -297,25 +295,24 @@ module.exports = class BlockIndentation extends Rule {
return fixedNode;
}

validateBlockStart(node, path) {
if (!this.shouldValidateBlockEnd(node)) {
validateBlockStart(node) {
if (!this.shouldValidateBlockStart(node)) {
return node;
}

let startColumn = node.loc.start.column;
let startLine = node.loc.start.line;
const indexOfNodeInTemplate = this.template.body.indexOf(node);
const isDirectChildOfTemplate = indexOfNodeInTemplate !== -1;
const hasLeadingContent =
!isDirectChildOfTemplate || this.hasLeadingContent(node, this.template.body);
const startColumn = node.loc.start.column;

if (startLine === 1 && startColumn !== 0) {
// If it's not a direct child of the template the block start is already fixed by the validateBlockChildren function
if (isDirectChildOfTemplate && startColumn !== 0 && !hasLeadingContent) {
if (this.mode === 'fix') {
// If we are in a child (path set) we want to edit directly the source and not the source of the node
const elementSource = path
? this.sourceEdited || this.source.join('')
: sourceForLoc(this.sourceEdited || this.source, {
end: node.loc.end,
start: { line: node.loc.start.line, column: 0 },
});
const fixedNode = this.fixLine(elementSource.split('\n'), 0, startColumn, 0, path);
return fixedNode;
const previousNode = this.template.body[indexOfNodeInTemplate - 1];
if (previousNode.type === 'TextNode') {
previousNode.chars = removeWhitespaceEnd(previousNode.chars);
}
} else {
let isElementNode = node && node.type === 'ElementNode';
let displayName = isElementNode ? node.tag : node.path.original;
Expand Down Expand Up @@ -426,36 +423,7 @@ module.exports = class BlockIndentation extends Rule {
break;
}

// We might not actually be the first thing on the line. We might be
// preceded by another element or statement, or by some text. So walk
// backwards looking for something else on this line.
let hasLeadingContent = false;
for (let j = i - 2; j >= 0; j--) {
let sibling = children[j];
if (sibling.loc && sibling.type !== 'TextNode') {
// Found an element or statement. If it's on this line, then we
// have leading content, so set the flag and break. If it's not
// on this line, then we've scanned back to a previous line, so
// we can also break.
if (sibling.loc.end.line === child.loc.start.line) {
hasLeadingContent = true;
}
break;
} else {
let lines = sibling.chars.split(/[\n\r]/);
let lastLine = lines[lines.length - 1];
if (lastLine.trim()) {
// The last line in this text node has non-whitespace content, so
// set the flag.
hasLeadingContent = true;
}
if (lines.length > 1) {
// There are multiple lines meaning we've now scanned back to a
// previous line, so we can break.
break;
}
}
}
const hasLeadingContent = this.hasLeadingContent(child, children);

if (hasLeadingContent) {
// There's content before us on the same line, so we don't care about
Expand Down Expand Up @@ -589,6 +557,41 @@ module.exports = class BlockIndentation extends Rule {
return node;
}

hasLeadingContent(node, parentChildren) {
const currentIndex = parentChildren.indexOf(node);
// We might not actually be the first thing on the line. We might be
// preceded by another element or statement, or by some text. So walk
// backwards looking for something else on this line.
for (let j = currentIndex - 1; j >= 0; j--) {
let sibling = parentChildren[j];
if (sibling.loc && sibling.type !== 'TextNode') {
// Found an element or statement. If it's on this line, then we
// have leading content, so set the flag and break. If it's not
// on this line, then we've scanned back to a previous line, so
// we can also break.
if (sibling.loc.end.line === node.loc.start.line) {
return true;
}
break;
} else {
let lines = sibling.chars.split(/[\n\r]/);
let lastLine = lines[lines.length - 1];
if (lastLine.trim()) {
// The last line in this text node has non-whitespace content, so
// set the flag.
return true;
}
if (lines.length > 1) {
// There are multiple lines meaning we've now scanned back to a
// previous line, so we can break.
break;
}
}
}

return false;
}

getDisplayNameForNode(node) {
switch (node.type) {
case 'ElementNode':
Expand Down Expand Up @@ -618,7 +621,7 @@ module.exports = class BlockIndentation extends Rule {
const line = lines[lineNumber];

// If line starts with only white spaces...
if (line.startsWith(' '.repeat(actualColumn))) {
if (line.startsWith(' '.repeat(actualColumn)) || actualColumn === expectedColumn) {
// ... then it means we just need to fix the number of whitespace
lines[lineNumber] = actualColumn
? line.replace(/^ +/, ' '.repeat(expectedColumn))
Expand All @@ -630,7 +633,9 @@ module.exports = class BlockIndentation extends Rule {
)}${line.slice(Math.max(0, actualColumn))}`;
}

let node = parse(lines.join('\n')).body.find((node) => node.type !== 'TextNode');
this.sourceEdited = lines.join('\n');

let node = parse(this.sourceEdited).body.find((node) => node.type !== 'TextNode');
node = get(node, path);

// As we recreate a new node the _isElseIfBlock is not set anymore so we need to recompute it
Expand All @@ -640,8 +645,6 @@ module.exports = class BlockIndentation extends Rule {
elseBlockStatement._isElseIfBlock = true;
}

this.sourceEdited = lines.join('\n');

return node;
}

Expand All @@ -662,14 +665,28 @@ module.exports = class BlockIndentation extends Rule {
return false;
}

shouldValidateBlockEnd(node) {
sourceDoesNotMatchNode(node, shouldIncludeEndingToken) {
let source = sourceForLoc(this.sourceEdited || this.source, node.loc);
let endingToken = `/${node.path.original}`;
let indexOfEnding = source.lastIndexOf(endingToken);

// Do not validate if source doesn't match node (if vs iframe)
let charAfterEnding = source[indexOfEnding + endingToken.length];
if (indexOfEnding !== -1 && !VALID_FOLLOWING_CHARS.has(charAfterEnding)) {
return true;
}

return !shouldIncludeEndingToken || indexOfEnding !== -1;
}

shouldValidateBlockStart(node) {
if (node._isElseIfBlock) {
return false;
}

// do not validate indentation on VOID_TAG's
if (VOID_TAGS[node.tag]) {
return false;
return true;
}

if (this.isWithinIgnoredElement()) {
Expand All @@ -678,20 +695,32 @@ module.exports = class BlockIndentation extends Rule {

// do not validate nodes without children (whitespace will count as TextNodes)
if (node && node.type === 'ElementNode') {
return AstNodeInfo.hasChildren(node);
return true;
}

let source = this.sourceForNode(node);
let endingToken = `/${node.path.original}`;
let indexOfEnding = source.lastIndexOf(endingToken);
return this.sourceDoesNotMatchNode(node, false);
}

// Do not validate if source doesn't match node (if vs iframe)
let charAfterEnding = source[indexOfEnding + endingToken.length];
if (indexOfEnding !== -1 && !VALID_FOLLOWING_CHARS.has(charAfterEnding)) {
shouldValidateBlockEnd(node) {
if (node._isElseIfBlock) {
return false;
}

// do not validate indentation on VOID_TAG's
if (VOID_TAGS[node.tag]) {
return false;
}

return indexOfEnding !== -1;
if (this.isWithinIgnoredElement()) {
return false;
}

// do not validate nodes without children (whitespace will count as TextNodes)
if (node && node.type === 'ElementNode') {
return AstNodeInfo.hasChildren(node);
}

return this.sourceDoesNotMatchNode(node, true);
}

endingControlCharCount(node) {
Expand All @@ -700,7 +729,7 @@ module.exports = class BlockIndentation extends Rule {
return 3;
}

let source = this.sourceForNode(node);
let source = sourceForLoc(this.sourceEdited || this.source, node.loc);
let endingToken = `/${node.path.original}`;
let indexOfEnding = source.lastIndexOf(endingToken);

Expand Down