Skip to content

Commit

Permalink
Revert "allow all directives in line comments" (fixes #14960) (#14973)
Browse files Browse the repository at this point in the history 
* Revert "Breaking: allow all directives in line comments (fixes #14575) (#14656)"

This reverts commit 4c841b8.

* fix: codeql warning

refs: https://github.com/eslint/eslint/pull/14973/checks?check_run_id=3411736426

* chore: improve regex

* Update lib/linter/linter.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
  • Loading branch information
@aladdin-add @mdjermanovic
aladdin-add and mdjermanovic committed Aug 27, 2021
1 parent 05ca24c commit 41617ec
Show file tree
Hide file tree
Showing 15 changed files with 42 additions and 201 deletions.
14 changes: 5 additions & 9 deletions docs/rules/no-unused-vars.md
View file
Expand Up @@ -82,24 +82,20 @@ function getY([, y]) {

### exported

In environments outside of CommonJS or ECMAScript modules, you may use `var` to create a global variable that may be used by other scripts. You can use the comment `/* exported variableName */` or `// exported variableName` to indicate that this variable is being exported and therefore should not be considered unused.
In environments outside of CommonJS or ECMAScript modules, you may use `var` to create a global variable that may be used by other scripts. You can use the `/* exported variableName */` comment block to indicate that this variable is being exported and therefore should not be considered unused.

Note that the comment has no effect for any of the following:
Note that `/* exported */` has no effect for any of the following:

* when the environment is `node` or `commonjs`
* when `parserOptions.sourceType` is `module`
* when `ecmaFeatures.globalReturn` is `true`

Examples of **correct** code for `exported` operation:
The line comment `// exported variableName` will not work as `exported` is not line-specific.

```js
/* exported global_var */

var global_var = 42;
```
Examples of **correct** code for `/* exported variableName */` operation:

```js
// exported global_var
/* exported global_var */

var global_var = 42;
```
Expand Down
12 changes: 2 additions & 10 deletions docs/user-guide/configuring/language-options.md
View file
Expand Up @@ -44,16 +44,12 @@ Environments can be specified inside of a file, in configuration files or using

### Using configuration comments

To specify environments using a line or block comment inside of your JavaScript file, use the following format:
To specify environments using a comment inside of your JavaScript file, use the following format:

```js
/* eslint-env node, mocha */
```

```js
// eslint-env node, mocha
```

This enables Node.js and Mocha environments.

### Using configuration files
Expand Down Expand Up @@ -127,16 +123,12 @@ Some of ESLint's core rules rely on knowledge of the global variables available

### Using configuration comments

To specify globals using a line or block comment inside of your JavaScript file, use the following format:
To specify globals using a comment inside of your JavaScript file, use the following format:

```js
/* global var1, var2 */
```

```js
// global var1, var2
```

This defines two global variables, `var1` and `var2`. If you want to optionally specify that these global variables can be written to (rather than only being read), then you can set each with a `"writable"` flag:

```js
Expand Down
18 changes: 3 additions & 15 deletions docs/user-guide/configuring/rules.md
View file
Expand Up @@ -14,16 +14,12 @@ ESLint comes with a large number of built-in rules and you can add more rules th

### Using configuration comments

To configure rules inside of a file using configuration comments, use a line or block comment in the following format:
To configure rules inside of a file using configuration comments, use a comment in the following format:

```js
/* eslint eqeqeq: "off", curly: "error" */
```

```js
// eslint eqeqeq: "off", curly: "error"
```

In this example, [`eqeqeq`](https://eslint.org/docs/rules/eqeqeq) is turned off and [`curly`](.https://eslint.org/docs/rules/curly) is turned on as an error. You can also use the numeric equivalent for the rule severity:

```js
Expand Down Expand Up @@ -128,7 +124,7 @@ In these configuration files, the rule `plugin1/rule1` comes from the plugin nam

### Using configuration comments

To temporarily disable rule warnings in your file, use line or block comments in the following format:
To temporarily disable rule warnings in your file, use block comments in the following format:

```js
/* eslint-disable */
Expand All @@ -138,14 +134,6 @@ alert('foo');
/* eslint-enable */
```

```js
// eslint-disable

alert('foo');

// eslint-enable
```

You can also disable or enable warnings for specific rules:

```js
Expand All @@ -157,7 +145,7 @@ console.log('bar');
/* eslint-enable no-alert, no-console */
```

To disable rule warnings in an entire file, put a `/* eslint-disable */` or `// eslint-disable` at the top of the file:
To disable rule warnings in an entire file, put a `/* eslint-disable */` block comment at the top of the file:

```js
/* eslint-disable */
Expand Down
20 changes: 11 additions & 9 deletions lib/linter/linter.js
View file
Expand Up @@ -315,7 +315,11 @@ function getDirectiveComments(filename, ast, ruleMapper, warnInlineConfig) {
return;
}
const directiveText = match[1];
const mustBeOnSingleLine = /^eslint-disable-(next-)?line$/u.test(directiveText);
const lineCommentSupported = /^eslint-disable-(next-)?line$/u.test(directiveText);

if (comment.type === "Line" && !lineCommentSupported) {
return;
}

if (warnInlineConfig) {
const kind = comment.type === "Block" ? `/*${directiveText}*/` : `//${directiveText}`;
Expand All @@ -329,7 +333,7 @@ function getDirectiveComments(filename, ast, ruleMapper, warnInlineConfig) {
return;
}

if (mustBeOnSingleLine && comment.loc.start.line !== comment.loc.end.line) {
if (lineCommentSupported && comment.loc.start.line !== comment.loc.end.line) {
const message = `${directiveText} comment should not span multiple lines.`;

problems.push(createLintingProblem({
Expand Down Expand Up @@ -456,10 +460,7 @@ function normalizeEcmaVersion(parser, ecmaVersion) {
return ecmaVersion >= 2015 ? ecmaVersion - 2009 : ecmaVersion;
}

const eslintEnvPatterns = [
/\/\*\s*eslint-env\s(.+?)\*\//gsu, // block comments
/\/\/[^\S\r\n\u2028\u2029]*eslint-env[^\S\r\n\u2028\u2029](.+)/gu // line comments
];
const eslintEnvPattern = /\/\*\s*eslint-env\s(.+?)(?:\*\/|$)/gsu;

/**
* Checks whether or not there is a comment which has "eslint-env *" in a given text.
Expand All @@ -469,16 +470,17 @@ const eslintEnvPatterns = [
function findEslintEnv(text) {
let match, retv;

for (const eslintEnvPattern of eslintEnvPatterns) {
eslintEnvPattern.lastIndex = 0;
eslintEnvPattern.lastIndex = 0;

while ((match = eslintEnvPattern.exec(text)) !== null) {
while ((match = eslintEnvPattern.exec(text)) !== null) {
if (match[0].endsWith("*/")) {
retv = Object.assign(
retv || {},
commentParser.parseListConfig(stripDirectiveComment(match[1]))
);
}
}

return retv;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/rules/newline-before-return.js
View file
Expand Up @@ -133,7 +133,7 @@ module.exports = {
if (tokenBefore) {
lineNumTokenBefore = tokenBefore.loc.end.line;
} else {
lineNumTokenBefore = 0; // Global return at beginning of script
lineNumTokenBefore = 0; // global return at beginning of script
}

return lineNumTokenBefore;
Expand Down
9 changes: 8 additions & 1 deletion lib/rules/utils/ast-utils.js
View file
Expand Up @@ -875,7 +875,14 @@ module.exports = {
isDirectiveComment(node) {
const comment = node.value.trim();

return /^(?:eslint[- ]|(?:globals?|exported) )/u.test(comment);
return (
node.type === "Line" && comment.indexOf("eslint-") === 0 ||
node.type === "Block" && (
comment.indexOf("global ") === 0 ||
comment.indexOf("eslint ") === 0 ||
comment.indexOf("eslint-") === 0
)
);
},

/**
Expand Down
124 changes: 2 additions & 122 deletions tests/lib/linter/linter.js
View file
Expand Up @@ -23,7 +23,6 @@ const { Linter } = require("../../../lib/linter");

const TEST_CODE = "var answer = 6 * 7;",
BROKEN_TEST_CODE = "var;";
const linebreaks = ["\n", "\r\n", "\r", "\u2028", "\u2029"];

//------------------------------------------------------------------------------
// Helpers
Expand Down Expand Up @@ -1361,16 +1360,15 @@ describe("Linter", () => {
describe("when evaluating code containing a line comment", () => {
const code = "//global a \n function f() {}";

it("should introduce a global variable", () => {
it("should not introduce a global variable", () => {
const config = { rules: { checker: "error" } };
let spy;

linter.defineRule("checker", context => {
spy = sinon.spy(() => {
const scope = context.getScope();
const a = getVariable(scope, "a");

assert.strictEqual(a.eslintExplicitGlobal, true);
assert.strictEqual(getVariable(scope, "a"), null);
});

return { Program: spy };
Expand Down Expand Up @@ -5645,124 +5643,6 @@ var a = "test2";
});
});

// https://github.com/eslint/eslint/issues/14575
describe("directives in line comments", () => {
let messages;

it("//eslint-disable", () => {
const codes = [
...linebreaks.map(linebreak => `//eslint-disable no-debugger${linebreak}debugger;`),
...linebreaks.map(linebreak => `// eslint-disable no-debugger${linebreak}debugger;`)
];

for (const code of codes) {
messages = linter.verify(code, { rules: { "no-debugger": 2 } });
assert.strictEqual(messages.length, 0);

messages = linter.verify(code, { rules: { "no-debugger": 0 } });
assert.strictEqual(messages.length, 0);
}
});

it("//eslint-enable", () => {
const codes = [
...linebreaks.map(linebreak => `//eslint-disable no-debugger${linebreak}//eslint-enable no-debugger${linebreak}debugger;`),
...linebreaks.map(linebreak => `// eslint-disable no-debugger${linebreak}//eslint-enable no-debugger${linebreak}debugger;`)
];

for (const code of codes) {
messages = linter.verify(code, { rules: { "no-debugger": 2 } });
assert.strictEqual(messages.length, 1);
}
});

it("//eslint", () => {
const codes = [
...linebreaks.map(linebreak => `//eslint no-debugger:'error'${linebreak}debugger;`),
...linebreaks.map(linebreak => `// eslint no-debugger:'error'${linebreak}debugger;`)
];

for (const code of codes) {
messages = linter.verify(code, { rules: { "no-debugger": 0 } });
assert.strictEqual(messages.length, 1);
messages = linter.verify(code, { rules: { "no-debugger": 2 } });
assert.strictEqual(messages.length, 1);
}
});

it("//global(s)", () => {
const config = { rules: { "no-undef": 2 } };
const codes = [
...linebreaks.map(linebreak => `//globals foo: true${linebreak}foo;`),
...linebreaks.map(linebreak => `// globals foo: true${linebreak}foo;`),
...linebreaks.map(linebreak => `//global foo: true${linebreak}foo;`),
...linebreaks.map(linebreak => `// global foo: true${linebreak}foo;`)
];

for (const code of codes) {
messages = linter.verify(code, config, filename);
assert.strictEqual(messages.length, 0);
}
});
it("//exported", () => {
const codes = [
...linebreaks.map(linebreak => `//exported foo${linebreak}var foo = 0;`),
...linebreaks.map(linebreak => `// exported foo${linebreak}var foo = 0;`)
];
const config = { rules: { "no-unused-vars": 2 } };

for (const code of codes) {
messages = linter.verify(code, config, filename);
assert.strictEqual(messages.length, 0);
}
});

describe("//eslint-env", () => {
const config = { rules: { "no-undef": 2 } };

it("enable one env with different line breaks", () => {
const codes = [
...linebreaks.map(linebreak => `//${ESLINT_ENV} browser${linebreak}window;`),
...linebreaks.map(linebreak => `// ${ESLINT_ENV} browser${linebreak}window;`),
`window;//${ESLINT_ENV} browser` // no linebreaks
];

for (const code of codes) {
messages = linter.verify(code, config, filename);
assert.strictEqual(messages.length, 0);
}
});

it("multiple envs enabled with different line breaks", () => {
const codes = [
...linebreaks.map(linebreak => `//${ESLINT_ENV} browser,es6${linebreak}window;Promise;`),
...linebreaks.map(linebreak => `// ${ESLINT_ENV} browser,es6${linebreak}window;Promise;`),
...linebreaks.map(linebreak => `//${ESLINT_ENV} browser${linebreak}//${ESLINT_ENV} es6${linebreak}window;Promise;`),
...linebreaks.map(linebreak => `// ${ESLINT_ENV} browser${linebreak}//${ESLINT_ENV} es6${linebreak}window;Promise;`)
];

for (const code of codes) {
messages = linter.verify(code, config, filename);
assert.strictEqual(messages.length, 0);
}
});

it("no env enabled with different linebreaks", () => {
const codes = [
...linebreaks.map(linebreak => `//${ESLINT_ENV}${linebreak}browser${linebreak}window;`),
...linebreaks.map(linebreak => `// ${ESLINT_ENV}${linebreak}browser${linebreak}window;`),
...linebreaks.map(linebreak => `//${ESLINT_ENV}browser${linebreak}window;window;`),
...linebreaks.map(linebreak => `// ${ESLINT_ENV}browser${linebreak}window;window;`)
];

for (const code of codes) {
messages = linter.verify(code, config, filename);
assert.strictEqual(messages.length, 2);
}
});
});
});

describe("merging 'parserOptions'", () => {
it("should deeply merge 'parserOptions' from an environment with 'parserOptions' from the provided config", () => {
const code = "return <div/>";
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/rules/id-blacklist.js
View file
Expand Up @@ -1010,7 +1010,7 @@ ruleTester.run("id-blacklist", rule, {
]
},

// Globals declared in the given source code are not excluded from consideration
// globals declared in the given source code are not excluded from consideration
{
code: "const foo = 1; bar = foo;",
options: ["foo"],
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/rules/id-denylist.js
View file
Expand Up @@ -1022,7 +1022,7 @@ ruleTester.run("id-denylist", rule, {
]
},

// Globals declared in the given source code are not excluded from consideration
// globals declared in the given source code are not excluded from consideration
{
code: "const foo = 1; bar = foo;",
options: ["foo"],
Expand Down
10 changes: 0 additions & 10 deletions tests/lib/rules/no-inline-comments.js
View file
Expand Up @@ -40,16 +40,6 @@ ruleTester.run("no-inline-comments", rule, {
"var a = 1; // eslint-disable-line no-debugger",
"var a = 1; /* eslint-disable-line no-debugger */",

// https://github.com/eslint/eslint/issues/14575
"var i;//eslint no-debugger:2",
"var i;// eslint no-debugger:2",
"var i;//eslint-disable no-debugger",
"var i;// eslint-disable no-debugger",
"var i;//eslint-enable no-debugger",
"var i;// eslint-enable no-debugger",
"var i;//global foo",
"var i;// global foo",

// JSX exception
`var a = (
<div>
Expand Down

0 comments on commit 41617ec

Please sign in to comment.