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.

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: Remove no-extra-parens autofix for potential directives #17022

Merged
merged 10 commits into from Jun 13, 2023
19 changes: 19 additions & 0 deletions docs/src/rules/no-extra-parens.md
Expand Up @@ -21,6 +21,25 @@ This rule always ignores extra parentheses around the following:
* immediately-invoked function expressions (also known as IIFEs) such as `var x = (function () {})();` and `var x = (function () {}());` to avoid conflicts with the [wrap-iife](wrap-iife) rule
* arrow function arguments to avoid conflicts with the [arrow-parens](arrow-parens) rule

Problems reported by this rule can be fixed automatically, except when removing the parentheses would create a new directive, because that could change the semantics of the code.
For example, the following script prints `object` to the console, but if the parentheses around `"use strict"` were removed, it would print `undefined` instead.

```js
<!--
// this is a script
// -->

("use strict");

function test() {
console.log(typeof this);
}

test();
```

In this case, the rule will not try to remove the parentheses around `"use strict"` but will still report them as a problem.

fasttime marked this conversation as resolved.
Show resolved Hide resolved
## Options

This rule has a string option:
Expand Down
42 changes: 34 additions & 8 deletions lib/rules/no-extra-parens.js
Expand Up @@ -386,6 +386,30 @@ module.exports = {
return node && (node.type === "Identifier" || node.type === "MemberExpression");
}

/**
* Checks if a node is fixable.
* A node is fixable if removing a single pair of surrounding parentheses does not turn it
* into a directive after fixing other nodes.
* Almost all nodes are fixable, except if all of the following conditions are met:
* The node is a string Literal
* It has a single pair of parentheses
* It is the only child of an ExpressionStatement
* @param {ASTNode} node The node to evaluate.
* @returns {boolean} Whether or not the node is fixable.
* @private
*/
function isFixable(node) {

// if it's not a string literal it can be autofixed
fasttime marked this conversation as resolved.
Show resolved Hide resolved
if (node.type !== "Literal" || typeof node.value !== "string") {
return true;
}
if (isParenthesisedTwice(node)) {
return true;
}
return !astUtils.isTopLevelExpressionStatement(node.parent);
}

/**
* Report the node
* @param {ASTNode} node node to evaluate
Expand Down Expand Up @@ -429,14 +453,16 @@ module.exports = {
node,
loc: leftParenToken.loc,
messageId: "unexpected",
fix(fixer) {
const parenthesizedSource = sourceCode.text.slice(leftParenToken.range[1], rightParenToken.range[0]);

return fixer.replaceTextRange([
leftParenToken.range[0],
rightParenToken.range[1]
], (requiresLeadingSpace(node) ? " " : "") + parenthesizedSource + (requiresTrailingSpace(node) ? " " : ""));
}
fix: isFixable(node)
? fixer => {
const parenthesizedSource = sourceCode.text.slice(leftParenToken.range[1], rightParenToken.range[0]);

return fixer.replaceTextRange([
leftParenToken.range[0],
rightParenToken.range[1]
], (requiresLeadingSpace(node) ? " " : "") + parenthesizedSource + (requiresTrailingSpace(node) ? " " : ""));
}
: null
});
}

Expand Down
8 changes: 3 additions & 5 deletions lib/rules/no-unused-expressions.js
Expand Up @@ -4,6 +4,8 @@
*/
"use strict";

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

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -112,18 +114,14 @@ module.exports = {
* @returns {boolean} whether the given node is considered a directive in its current position
*/
function isDirective(node) {
const parent = node.parent,
grandparent = parent.parent;

/**
* https://tc39.es/ecma262/#directive-prologue
*
* Only `FunctionBody`, `ScriptBody` and `ModuleBody` can have directive prologue.
* Class static blocks do not have directive prologue.
*/
return (parent.type === "Program" || parent.type === "BlockStatement" &&
(/Function/u.test(grandparent.type))) &&
directives(parent).includes(node);
return astUtils.isTopLevelExpressionStatement(node) && directives(node.parent).includes(node);
}

/**
Expand Down
9 changes: 1 addition & 8 deletions lib/rules/padding-line-between-statements.js
Expand Up @@ -138,14 +138,7 @@ function isBlockLikeStatement(sourceCode, node) {
*/
function isDirective(node, sourceCode) {
return (
node.type === "ExpressionStatement" &&
(
node.parent.type === "Program" ||
(
node.parent.type === "BlockStatement" &&
astUtils.isFunction(node.parent.parent)
)
) &&
astUtils.isTopLevelExpressionStatement(node) &&
node.expression.type === "Literal" &&
typeof node.expression.value === "string" &&
!astUtils.isParenthesised(sourceCode, node.expression)
Expand Down
23 changes: 21 additions & 2 deletions lib/rules/utils/ast-utils.js
Expand Up @@ -986,6 +986,25 @@ function isConstant(scope, node, inBooleanPosition) {
return false;
}

/**
* Checks whether a node is an ExpressionStatement at the top level of a file or function body.
* A top-level ExpressionStatement node is a directive if it contains a single unparenthesized
* string literal and if it occurs either as the first sibling or immediately after another
* directive.
* @param {ASTNode} node The node to check.
* @returns {boolean} Whether or not the node is an ExpressionStatement at the top level of a
* file or function body.
*/
function isTopLevelExpressionStatement(node) {
if (node.type !== "ExpressionStatement") {
return false;
}
const parent = node.parent;

return parent.type === "Program" || (parent.type === "BlockStatement" && isFunction(parent.parent));

}

//------------------------------------------------------------------------------
// Public Interface
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -1501,7 +1520,6 @@ module.exports = {
return directives;
},


/**
* Determines whether this node is a decimal integer literal. If a node is a decimal integer literal, a dot added
* after the node will be parsed as a decimal point, rather than a property-access dot.
Expand Down Expand Up @@ -2120,5 +2138,6 @@ module.exports = {
isLogicalAssignmentOperator,
getSwitchCaseColonToken,
getModuleExportName,
isConstant
isConstant,
isTopLevelExpressionStatement
};
20 changes: 19 additions & 1 deletion tests/lib/rules/no-extra-parens.js
Expand Up @@ -3444,6 +3444,24 @@ ruleTester.run("no-extra-parens", rule, {
`a ${operator} function () {};`,
"Identifier"
)
)
),

// Potential directives (no autofix)
invalid("('use strict');", null),
invalid("function f() { ('abc'); }", null),
invalid("(function () { ('abc'); })();", null),
invalid("_ = () => { ('abc'); };", null),
invalid("'use strict';(\"foobar\");", null),
fasttime marked this conversation as resolved.
Show resolved Hide resolved
invalid("foo(); ('bar');", null),
invalid("foo = { bar() { ; (\"baz\"); } };", null),

// Directive lookalikes
invalid("(12345);", "12345;"),
invalid("(('foobar'));", "('foobar');"),
invalid("(`foobar`);", "`foobar`;"),
invalid("void ('foobar');", "void 'foobar';"),
invalid("_ = () => ('abc');", "_ = () => 'abc';"),
invalid("if (foo) ('bar');", "if (foo) 'bar';"),
invalid("const foo = () => ('bar');", "const foo = () => 'bar';")
]
});
55 changes: 55 additions & 0 deletions tests/lib/rules/utils/ast-utils.js
Expand Up @@ -1802,4 +1802,59 @@ describe("ast-utils", () => {
});
});
});

describe("isTopLevelExpressionStatement", () => {
it("should return false for a Program node", () => {
const node = { type: "Program", parent: null };

assert.strictEqual(astUtils.isTopLevelExpressionStatement(node), false);
});

it("should return false if the node is not an ExpressionStatement", () => {
linter.defineRule("checker", {
create: mustCall(() => ({
":expression": mustCall(node => {
assert.strictEqual(astUtils.isTopLevelExpressionStatement(node), false);
})
}))
});

linter.verify("var foo = () => \"use strict\";", { rules: { checker: "error" }, parserOptions: { ecmaVersion: 2022 } });
});

const expectedResults = [
["if (foo) { \"use strict\"; }", "\"use strict\";", false],
["{ \"use strict\"; }", "\"use strict\";", false],
["switch (foo) { case bar: \"use strict\"; }", "\"use strict\";", false],
["foo; bar;", "foo;", true],
["foo; bar;", "bar;", true],
["function foo() { bar; }", "bar;", true],
["var foo = function () { foo(); };", "foo();", true],
["var foo = () => { 'bar'; }", "'bar';", true],
["\"use strict\"", "\"use strict\"", true],
["(`use strict`)", "(`use strict`)", true]
];

expectedResults.forEach(([code, nodeText, expectedRetVal]) => {
it(`should return ${expectedRetVal} for \`${nodeText}\` in \`${code}\``, () => {
linter.defineRule("checker", {
create: mustCall(context => {
const assertForNode = mustCall(
node => assert.strictEqual(astUtils.isTopLevelExpressionStatement(node), expectedRetVal)
);

return ({
ExpressionStatement(node) {
if (context.sourceCode.getText(node) === nodeText) {
assertForNode(node);
}
}
});
})
});

linter.verify(code, { rules: { checker: "error" }, parserOptions: { ecmaVersion: 2022 } });
});
});
});
});