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

fasttime · 2023-03-26T21:42:13Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

References #16988

What changes did you make? (Give an overview)

This PR fixes an issue with the no-extra-parens rule that could cause the autofix to change the behavior of the code. The solution consists in disabling the autofix in situations where it would otherwise introduce a new directive.
This solution has been discussed in issue #16988, and I started working on the no-extra-parens because I think that there is sufficient consensus on how to handle the autofix in the case of this specific rule.

The problem addressed here is that a statement like ("use strict"); can mutate into a use strict directive if the parentheses are removed, and that would change the semantics of the code. To determine whether the autofix should be disabled, we decided to check if a string literal is the sole child of an ExpressionStatement.
Since there is no danger that the no-extra-parens and quotes rule fix the same node concurrently (see #17022 (comment)), it is safe for no-extra-parens to remove parentheses from a template literal.

Is there anything you'd like reviewers to focus on?

netlify · 2023-03-26T21:42:21Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`9d94ba4`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/64677f287913be00085b9eef

lib/rules/utils/ast-utils.js

nzakas

Overall LGTM. One spot where I thought we could clarify things a bit.

Can also update the documentation to note this change?

lib/rules/no-extra-parens.js

fasttime · 2023-03-28T14:49:53Z

Can also update the documentation to note this change?

Thanks @nzakas, I've added a sentence to the Rule Details. I think there is no special way to format a note like this?

lib/rules/no-extra-parens.js

mdjermanovic · 2023-03-29T17:45:47Z

lib/rules/no-extra-parens.js

+            if (isParenthesisedTwice(node)) {
+                return true;
+            }
+            return !astUtils.isExpressionInOrJustAfterDirectivePrologue(sourceCode, node.parent);


I thought we'd just check if the parent is ExpressionStatement as we don't know what fixes other rules are applying in the code that precedes it.

For example:

/* eslint no-extra-parens: "error" */ /* eslint prettier/prettier: "error" */ ; // ("use strict");

after --fix, "use strict" still ends up being a directive:

/* eslint no-extra-parens: "error" */ /* eslint prettier/prettier: "error" */ // "use strict";

Well, if compatibility with Prettier is a concern then we could just check the parent node as you suggest. I don't know how we could stop third-party rules from interfering if another autofix is applied to the ExpressionStatement node itself or to its children rather than to a sibling node (e.g. ("use " + "strict"); → ("use strict");), but from what I can tell, that is not what Prettier does.

In this case we should also change the behavior of the quotes rule, because fox example

/* eslint quotes: "error" */ /* eslint prettier/prettier: "error" */ ; // `use strict`;

autofixes to

/* eslint quotes: "error" */ /* eslint prettier/prettier: "error" */ // "use strict";

Well, if compatibility with Prettier is a concern then we could just check the parent node as you suggest.

Not specifically with Prettier, any custom or third-party rules.

I don't know how we could stop third-party rules from interfering if another autofix is applied to the ExpressionStatement node itself or to its children rather than to a sibling node (e.g. ("use " + "strict"); → ("use strict");),

That shouldn't cause problems because the ranges will be overlapping so ESLint would apply only one of the fixes:

https://eslint.org/docs/latest/extend/custom-rules#conflicting-fixes

I think that non-directive ExpressionStatement > Literal is a very edge case, and it's a meaningless code that should be manually fixed anyway, so I'd vote for the safest solution which is also the simplest for us. @nzakas what do you think?

/* eslint quotes: "error" */ /* eslint prettier/prettier: "error" */ ; // `use strict`;

Yes, I don't think this should be autofixed either.

That shouldn't cause problems because the ranges will be overlapping so ESLint would apply only one of the fixes:

https://eslint.org/docs/latest/extend/custom-rules#conflicting-fixes

Ah, understood. I didn't realize that removing the parentheses is handled as one single fix by the linter. Thanks for the explanation @mdjermanovic.

nzakas · 2023-04-12T21:33:49Z

docs/src/rules/no-extra-parens.md

@@ -21,6 +21,8 @@ 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.


I was thinking more that we need to show some examples, as I don't know that most people will understand what "removing the parentheses would create a new directive" actually means.

Thanks for the feedback @nzakas! Would it be also fine for you if we disable the autofix for all string literals that are direct children of an ExpressionStatement, as suggested in this comment by @mdjermanovic? This would improve interoperability with third-party rules at the expenses of a few autofixes in some rare edge cases.

Yup, that's fine with me. We don't guarantee that all autofixes will be applied, so it's fine to pick and choose.

github-actions · 2023-04-23T22:31:28Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

github-actions · 2023-05-05T22:31:31Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

Rec0iL99 · 2023-05-06T04:27:03Z

Don't close. Not stale.

Would it be also fine for you if we disable the autofix for all string literals that are direct children of an ExpressionStatement, as suggested #17022 (comment) by @mdjermanovic?

@nzakas there's a reply from @fasttime for you to look into here. Thanks.

nzakas · 2023-05-09T20:29:24Z

Thanks, replied!

lib/rules/quotes.js

fasttime · 2023-05-16T04:33:41Z

lib/rules/no-extra-parens.js

+            if (isParenthesisedTwice(node)) {
+                return true;
+            }
+            return node.parent.type !== "ExpressionStatement";


Shall we also check if the ExpressionStatement is at the top level of a program or function body?

I think yes, if it isn't at the top level of a program or function body then the fix seems safe.

I think yes, if it isn't at the top level of a program or function body then the fix seems safe.

Thanks! This is done in 7a2800d. I also took the time to refactor similar code in other rules that do the same kind of checks.

docs/src/rules/no-extra-parens.md

fasttime · 2023-05-16T07:55:05Z

Thanks for the suggestions @mdjermanovic @nzakas. I've updated the autofix logic as per discussion above and I've added an example to the rule docs that shows the effect of the disabled autofix. I've also reverted my changes in ast-utils and moved isExpressionInOrJustAfterDirectivePrologue() back into quotes.js, but still keeping the new name and the fix that checks for parentheses. Let me know if I overlooked something.

fasttime · 2023-05-18T21:05:28Z

Thanks for the suggestions @mdjermanovic, it's all updated in aa16e4c now.

mdjermanovic · 2023-05-19T11:08:08Z

lib/rules/quotes.js

@@ -212,7 +213,7 @@ module.exports = {

                // Directive Prologues.
                case "ExpressionStatement":
-                    return isPartOfDirectivePrologue(node);
+                    return !astUtils.isParenthesised(sourceCode, node) && isExpressionInOrJustAfterDirectivePrologue(node);


This checking for parentheses can produce more lint errors in existing code:

/* eslint quotes: ["error", "backtick"] */ ("use strict") // error after this change

So we should tag this as feat: per our semver polices. However, since the PR is about no-extra-parens, and this change isn't just refactoring, it might be better to extract all changes to the quotes rule into a separate PR?

However, since the PR is about no-extra-parens, and this change isn't just refactoring, it might be better to extract all changes to the quotes rule into a separate PR?

Agreed. These fixes in the quotes rule were not expected from my side, they came more as a side effect of refactoring. I'm going to revert these changes for now and I will submit a new PR to update quotes once the changes in ast-utils.js are merged.

Done in 9d94ba4.

mdjermanovic

LGTM, thanks! Leaving it open for @nzakas to verify.

mdjermanovic · 2023-06-08T12:57:40Z

@fasttime this should also be rebased because it includes a change in the docs.

Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>

linux-foundation-easycla · 2023-06-08T14:36:56Z

✅ login: fasttime / name: Francesco Trotta (9f8c7b0, 34e1af2, a91c5ce, 5fbf4d9, 17f8e91, 6fb9fdd, 5040c0e, 6288f5e, 613f414, 74004b9)
❌ The email address for the commit (34e1af2, 17f8e91) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

fasttime · 2023-06-08T14:57:30Z

@fasttime this should also be rebased because it includes a change in the docs.

Rebased on top of main. I have no idea why the EasyCLA check is failing.

snitin315 · 2023-06-10T02:12:24Z

/easycla

linux-foundation-easycla · 2023-06-10T02:12:32Z

✅ login: fasttime / name: Francesco Trotta (9f8c7b0, 34e1af2, a91c5ce, 5fbf4d9, 17f8e91, 6fb9fdd, 5040c0e, 6288f5e, 613f414, 74004b9)
❌ The email address for the commit (34e1af2, 17f8e91) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

snitin315 · 2023-06-10T02:25:54Z

I think the bug in CLA is when a commit is co-authored. Maybe try amending the commit to remove the co-author?

snitin315

LGTM, thanks!

fasttime · 2023-06-10T08:21:38Z

I think the bug in CLA is when a commit is co-authored. Maybe try amending the commit to remove the co-author?

Thanks @snitin315, I hadn't noticed that there is a problem with the EasyCLA bot. Now that you mention it, since this is not the only affected PR, maybe we should just wait until the issue is fixed?

mdjermanovic · 2023-06-13T10:53:54Z

/easycla

nzakas

LGTM. Thanks!

eslint-github-bot bot added the bug ESLint is working incorrectly label Mar 26, 2023

fasttime commented Mar 26, 2023

View reviewed changes

lib/rules/utils/ast-utils.js Outdated Show resolved Hide resolved

fasttime marked this pull request as ready for review March 26, 2023 21:59

fasttime requested a review from a team as a code owner March 26, 2023 21:59

fasttime mentioned this pull request Mar 26, 2023

Bug: rules should not introduce new directives #16988

Closed

1 task

fasttime added accepted There is consensus among the team that this change meets the criteria for inclusion rule Relates to ESLint's core rules autofix This change is related to ESLint's autofixing capabilities labels Mar 26, 2023

nzakas requested changes Mar 27, 2023

View reviewed changes

lib/rules/no-extra-parens.js Outdated Show resolved Hide resolved

nzakas reviewed Mar 28, 2023

View reviewed changes

lib/rules/no-extra-parens.js Show resolved Hide resolved

mdjermanovic reviewed Mar 29, 2023

View reviewed changes

Hosamph approved these changes Apr 2, 2023

View reviewed changes

nzakas reviewed Apr 12, 2023

View reviewed changes

github-actions bot added the Stale label Apr 23, 2023

Rec0iL99 removed the Stale label Apr 25, 2023

github-actions bot added the Stale label May 5, 2023

Rec0iL99 removed the Stale label May 6, 2023

fasttime self-assigned this May 6, 2023

fasttime commented May 16, 2023

View reviewed changes

lib/rules/quotes.js Outdated Show resolved Hide resolved

fasttime commented May 16, 2023

View reviewed changes

docs/src/rules/no-extra-parens.md Show resolved Hide resolved

fasttime requested review from nzakas and mdjermanovic May 16, 2023 07:56

mdjermanovic reviewed May 19, 2023

View reviewed changes

mdjermanovic approved these changes May 19, 2023

View reviewed changes

fasttime and others added 10 commits June 8, 2023 16:35

fix: Remove no-extra-parens autofix for potential directives

9f8c7b0

Update lib/rules/no-extra-parens.js

34e1af2

Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>

Elaborate JSDoc for isFixable

a91c5ce

Update docs for no-extra-parens

5fbf4d9

Update lib/rules/no-extra-parens.js

17f8e91

Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>

One empty line is enough 😊

6fb9fdd

Review updates

5040c0e

Check for top-level ExpressionStatement

6288f5e

Review updates

613f414

Revert changes in quotes rule

74004b9

fasttime force-pushed the issue16988-no-extra-parens branch from 9d94ba4 to 74004b9 Compare June 8, 2023 14:36

snitin315 approved these changes Jun 10, 2023

View reviewed changes

nzakas approved these changes Jun 13, 2023

View reviewed changes

nzakas merged commit 54383e6 into main Jun 13, 2023
17 checks passed

nzakas deleted the issue16988-no-extra-parens branch June 13, 2023 15:33

fasttime mentioned this pull request Jun 14, 2023

feat: directive prologue detection and autofix condition in quotes #17284

Merged

1 task

Hosamph approved these changes Aug 15, 2023

View reviewed changes

eslint-github-bot bot locked and limited conversation to collaborators Dec 11, 2023

eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fasttime commented Mar 26, 2023 •

edited

netlify bot commented Mar 26, 2023 •

edited

nzakas left a comment

fasttime commented Mar 28, 2023

mdjermanovic Mar 29, 2023

fasttime Mar 30, 2023

mdjermanovic Mar 31, 2023

fasttime Mar 31, 2023

nzakas Apr 12, 2023

fasttime Apr 13, 2023

nzakas May 9, 2023

github-actions bot commented Apr 23, 2023

github-actions bot commented May 5, 2023

Rec0iL99 commented May 6, 2023

nzakas commented May 9, 2023

fasttime May 16, 2023

mdjermanovic May 16, 2023

fasttime May 17, 2023

fasttime commented May 16, 2023

fasttime commented May 18, 2023

mdjermanovic May 19, 2023

fasttime May 19, 2023

fasttime May 19, 2023

mdjermanovic left a comment

mdjermanovic commented Jun 8, 2023

linux-foundation-easycla bot commented Jun 8, 2023

fasttime commented Jun 8, 2023

snitin315 commented Jun 10, 2023

linux-foundation-easycla bot commented Jun 10, 2023

snitin315 commented Jun 10, 2023

snitin315 left a comment

fasttime commented Jun 10, 2023

mdjermanovic commented Jun 13, 2023

nzakas left a comment

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

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

Conversation

fasttime commented Mar 26, 2023 • edited

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

netlify bot commented Mar 26, 2023 • edited

✅ Deploy Preview for docs-eslint canceled.

nzakas left a comment

Choose a reason for hiding this comment

fasttime commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 23, 2023

github-actions bot commented May 5, 2023

Rec0iL99 commented May 6, 2023

nzakas commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasttime commented May 16, 2023

fasttime commented May 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic commented Jun 8, 2023

linux-foundation-easycla bot commented Jun 8, 2023

fasttime commented Jun 8, 2023

snitin315 commented Jun 10, 2023

linux-foundation-easycla bot commented Jun 10, 2023

snitin315 commented Jun 10, 2023

snitin315 left a comment

Choose a reason for hiding this comment

fasttime commented Jun 10, 2023

mdjermanovic commented Jun 13, 2023

nzakas left a comment

Choose a reason for hiding this comment

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

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

fasttime commented Mar 26, 2023 •

edited

netlify bot commented Mar 26, 2023 •

edited