fix: Remove no-extra-parens autofix for potential directives (#17022)

* fix: Remove `no-extra-parens` autofix for potential directives * Update lib/rules/no-extra-parens.js Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com> * Elaborate JSDoc for `isFixable` * Update docs for `no-extra-parens` * Update lib/rules/no-extra-parens.js Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com> * One empty line is enough 😊 * Review updates * Check for top-level ExpressionStatement * Review updates * Revert changes in `quotes` rule --------- Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
eslint · Jun 13, 2023 · 54383e6 · 54383e6
1 parent e0cf0d8
commit 54383e6
Show file tree

Hide file tree

Showing 7 changed files with 152 additions and 24 deletions.
diff --git a/docs/src/rules/no-extra-parens.md b/docs/src/rules/no-extra-parens.md
@@ -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.
+
 ## Options
 
 This rule has a string option:

diff --git a/lib/rules/no-extra-parens.js b/lib/rules/no-extra-parens.js
@@ -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
+            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
@@ -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
                 });
             }
 

diff --git a/lib/rules/no-unused-expressions.js b/lib/rules/no-unused-expressions.js
@@ -4,6 +4,8 @@
  */
 "use strict";
 
+const astUtils = require("./utils/ast-utils");
+
 //------------------------------------------------------------------------------
 // Rule Definition
 //------------------------------------------------------------------------------
@@ -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);
         }
 
         /**

diff --git a/lib/rules/padding-line-between-statements.js b/lib/rules/padding-line-between-statements.js
@@ -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)

diff --git a/lib/rules/utils/ast-utils.js b/lib/rules/utils/ast-utils.js
@@ -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
 //------------------------------------------------------------------------------
@@ -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.
@@ -2120,5 +2138,6 @@ module.exports = {
     isLogicalAssignmentOperator,
     getSwitchCaseColonToken,
     getModuleExportName,
-    isConstant
+    isConstant,
+    isTopLevelExpressionStatement
 };
diff --git a/tests/lib/rules/no-extra-parens.js b/tests/lib/rules/no-extra-parens.js
@@ -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),
+        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';")
     ]
 });
diff --git a/tests/lib/rules/utils/ast-utils.js b/tests/lib/rules/utils/ast-utils.js
@@ -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 } });
+            });
+        });
+    });
 });