Bug: rules should not introduce new directives

[fasttime]

Environment

Node version: v18.15.0
npm version: v9.5.0
Local ESLint version: v8.36.0 (Currently used)
Global ESLint version: Not found
Operating System: darwin 22.3.0

What parser are you using?

Default (Espree)

What did you do?

ECMAScript 5 or later has a concept of directives such as "use strict" and implementation-specific directives like "use asm" and a few more. Directives follow special syntax rules and can change the semantics of a program.

Apparently, some built-it rules do not take the syntax of directives into account.
If these rules have an autofix, they may occasionally introduce a new directive where there was none or turn an existing directive into a different one.

Configuration

{
    "parserOptions": {
        "ecmaVersion": "latest",
        "sourceType": "script"
    }
}

/* eslint no-extra-parens: "error" */
function test1() {
    ("use strict");
    return 007;
}

/* eslint no-extra-semi: "error" */
function test2() {
    ;"use strict";
    return 007;
}

/* eslint no-unused-labels: "error" */
function test3() {
    NoDirective: "use strict";
    return 007;
}

/* eslint no-useless-escape: "error" */
function test4() {
    "use\ strict";
    return 007;
}

REPRO

What did you expect to happen?

Autofixes and suggestions should not add new directives unless intended.

What actually happened?

Considering the examples above, the rules no-extra-parens, no-extra-semi and no-unused-labels have autofixes that cause a "use strict" literal to become part of a directive in some cases, resulting in a syntax error (but other undesirable outcomes are also possible).
Similarly, the rule no-useless-escape suggests to change an unrecognized "use\ strict" directive into "use strict", incorrectly stating that this would maintain the existing functionality.
There could be other rules with similar problems as well, I will update this issue if I find more.

On a critical note, if a rule happens to accidentally insert an additional directive by removing or changing what's around, it's likely that the original code was affected by a substantial problem, like a typo, a copy-paste mishap or a misunderstanding of the language syntax. Although the autofix doesn't make sense, removing it wouldn't necessarily make things any better.

Should we suppress the reports that produce incorrect autofixes and suggestions? Anything else we should do here?

Participation

• I am willing to submit a pull request for this issue.

Additional comments

If the team decides that any changes are necessary, there should be probably separate pull requests for each rule. I would be available to do parts of the work.

[mdjermanovic]

I think the mentioned rules should keep reporting these problems. For example, ;"use strict"; might be a mistake in editing the code that users would probably like to be warned about. Even if it's intentional, the code altogether has no effect and looks like a way to temporarily remove the directive while working. And even if such code is going to be checked in as a way to mark intent to not use the directive, I don't think it's a common practice so allowing it would rather be behind an option than by default.

As for the fixes, suggestions are allowed to change the behavior so I think no-useless-escape is fine.

On the other hand, autofixes should never change the behavior of the code, so it seems we should update no-extra-parens, no-extra-semi and no-unused-labels to not provide autofixes in these cases.

[fasttime]

Thanks for the quick reply @mdjermanovic.

On the other hand, autofixes should never change the behavior of the code, so it seems we should update no-extra-parens, no-extra-semi and no-unused-labels to not provide autofixes in these cases.

Disabling the autofixes while keeping the rules reporting on the problems sounds very reasonable to me.

As for the fixes, suggestions are allowed to change the behavior so I think no-useless-escape is fine.

Shall we at least change the suggestion text to remove the misleading part? For example:

Current text

• Remove the `\`. This maintains the current functionality.

New text

• Remove the `\` if it was inserted by mistake.

[mdjermanovic]

New text

• Remove the \ if it was inserted by mistake.

Sounds good to me.

[fasttime]

Thanks @mdjermanovic. I've already started to dig into the code of the rules in question and I have a strategy in my mind to ensure that no more than one autofix per program or function body is disabled: whenever a sequence of problems reported by the rules no-extra-parens, no-extra-semi, no-unused-labels and quotes would introduce a new directive (if all fixes were applied), then only the first fix in the sequence should be disabled.

So for instance in this example all rules should report (as it is now the case), but no-extra-semi should not autofix.

/*
eslint
no-extra-parens: "error",
no-extra-semi: "error",
no-unused-labels: "error",
quotes: "error",
*/

; Foo: (`use strict`)

and the intended autofix would be:

/*
eslint
no-extra-parens: "error",
no-extra-semi: "error",
no-unused-labels: "error",
quotes: "error",
*/

; "use strict"

In the end, ; "use strict" is no better than Foo: "use strict", ("use strict") or `use strict`. My assumption is that leaving the token in place that marks the end of the directive prologue makes the most sense.

Clearly, rules have no knowledge about the settings of other rules, so they should always operate under the assumption that all other rules are active and fixing.

[mdjermanovic]

I've already started to dig into the code of the rules in question and I have a strategy in my mind to ensure that no more than one autofix per program or function body is disabled: whenever a sequence of problems reported by the rules no-extra-parens, no-extra-semi, no-unused-labels and quotes would introduce a new directive (if all fixes were applied), then only the first fix in the sequence should be disabled.

I think that just checking whether the potentially problematic statement is directive-like (ExpressionStatement whose expression is a string literal) is quite good enough for these edge cases.

For example, this wouldn't be autofixed because the extra semi is followed by a directive-like statement, although it wouldn't really become a directive when the extra semi is removed:

/* eslint no-extra-semi: "error" */

foo();
; "use strict";

[fasttime]

I think that just checking whether the potentially problematic statement is directive-like (ExpressionStatement whose expression is a string literal) is quite good enough for these edge cases.

I like the simplicity of this approach, but I think that the definition of directive-like should be extended to account for fixes provided by the quotes rule, i.e.:

ExpressionStatement whose expression is a string Literal or a TemplateLiteral with no expressions.

Following this definition, `use strict`; and ("use strict"); would be considered both directive-like, and detected as such by a preceding extra semicolon or unused label. So they could be both safely autofixed to "use strict"; by quotes and no-extra-parens respectively, except when at the start of a program or function body.

[fasttime]

I prepared a fix for no-extra-parens in #17022.