From 41617ec3c4bc8bd1ba5f66521185be1566e6f5f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=96=9B=E5=AE=9A=E8=B0=94=E7=9A=84=E7=8C=AB?= Date: Sat, 28 Aug 2021 03:20:49 +0800 Subject: [PATCH] Revert "allow all directives in line comments" (fixes #14960) (#14973) * Revert "Breaking: allow all directives in line comments (fixes #14575) (#14656)" This reverts commit 4c841b880b5649392a55c98ecc9af757bd213ff0. * 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 Co-authored-by: Milos Djermanovic --- docs/rules/no-unused-vars.md | 14 +- .../configuring/language-options.md | 12 +- docs/user-guide/configuring/rules.md | 18 +-- lib/linter/linter.js | 20 +-- lib/rules/newline-before-return.js | 2 +- lib/rules/utils/ast-utils.js | 9 +- tests/lib/linter/linter.js | 124 +----------------- tests/lib/rules/id-blacklist.js | 2 +- tests/lib/rules/id-denylist.js | 2 +- tests/lib/rules/no-inline-comments.js | 10 -- tests/lib/rules/no-promise-executor-return.js | 4 +- tests/lib/rules/no-setter-return.js | 4 +- tests/lib/rules/no-warning-comments.js | 5 +- tests/lib/rules/require-await.js | 2 +- tests/lib/rules/utils/ast-utils.js | 15 +-- 15 files changed, 42 insertions(+), 201 deletions(-) diff --git a/docs/rules/no-unused-vars.md b/docs/rules/no-unused-vars.md index 4058cad3a38..30647031eb1 100644 --- a/docs/rules/no-unused-vars.md +++ b/docs/rules/no-unused-vars.md @@ -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; ``` diff --git a/docs/user-guide/configuring/language-options.md b/docs/user-guide/configuring/language-options.md index 43595c09366..08b62aad571 100644 --- a/docs/user-guide/configuring/language-options.md +++ b/docs/user-guide/configuring/language-options.md @@ -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 @@ -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 diff --git a/docs/user-guide/configuring/rules.md b/docs/user-guide/configuring/rules.md index 5ee1cf7eb3a..2274b98ed02 100644 --- a/docs/user-guide/configuring/rules.md +++ b/docs/user-guide/configuring/rules.md @@ -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 @@ -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 */ @@ -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 @@ -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 */ diff --git a/lib/linter/linter.js b/lib/linter/linter.js index aa87557d107..4b28e196a76 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -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}`; @@ -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({ @@ -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. @@ -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; } diff --git a/lib/rules/newline-before-return.js b/lib/rules/newline-before-return.js index 93939024ca0..fd6341e67c0 100644 --- a/lib/rules/newline-before-return.js +++ b/lib/rules/newline-before-return.js @@ -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; diff --git a/lib/rules/utils/ast-utils.js b/lib/rules/utils/ast-utils.js index 4b031db4d64..7d4779fc143 100644 --- a/lib/rules/utils/ast-utils.js +++ b/lib/rules/utils/ast-utils.js @@ -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 + ) + ); }, /** diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index c6135faac69..d277ce1ea92 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -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 @@ -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 }; @@ -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
"; diff --git a/tests/lib/rules/id-blacklist.js b/tests/lib/rules/id-blacklist.js index a2f450798e6..4d13459acf6 100644 --- a/tests/lib/rules/id-blacklist.js +++ b/tests/lib/rules/id-blacklist.js @@ -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"], diff --git a/tests/lib/rules/id-denylist.js b/tests/lib/rules/id-denylist.js index 3d8f1f4d411..4e8248399e4 100644 --- a/tests/lib/rules/id-denylist.js +++ b/tests/lib/rules/id-denylist.js @@ -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"], diff --git a/tests/lib/rules/no-inline-comments.js b/tests/lib/rules/no-inline-comments.js index a055c1bc2ab..463c86eea66 100644 --- a/tests/lib/rules/no-inline-comments.js +++ b/tests/lib/rules/no-inline-comments.js @@ -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 = (
diff --git a/tests/lib/rules/no-promise-executor-return.js b/tests/lib/rules/no-promise-executor-return.js index 20cdf300ad7..a24629b6a44 100644 --- a/tests/lib/rules/no-promise-executor-return.js +++ b/tests/lib/rules/no-promise-executor-return.js @@ -86,7 +86,7 @@ ruleTester.run("no-promise-executor-return", rule, { "new Promise(foo, (resolve, reject) => { return 1; });", "new Promise(foo, (resolve, reject) => 1);", - // Global Promise doesn't exist + // global Promise doesn't exist "/* globals Promise:off */ new Promise(function (resolve, reject) { return 1; });", { code: "new Promise((resolve, reject) => { return 1; });", @@ -97,7 +97,7 @@ ruleTester.run("no-promise-executor-return", rule, { env: { es6: false } }, - // Global Promise is shadowed + // global Promise is shadowed "let Promise; new Promise(function (resolve, reject) { return 1; });", "function f() { new Promise((resolve, reject) => { return 1; }); var Promise; }", "function f(Promise) { new Promise((resolve, reject) => 1); }", diff --git a/tests/lib/rules/no-setter-return.js b/tests/lib/rules/no-setter-return.js index 366e65c2c84..ab5b196b9f3 100644 --- a/tests/lib/rules/no-setter-return.js +++ b/tests/lib/rules/no-setter-return.js @@ -225,7 +225,7 @@ ruleTester.run("no-setter-return", rule, { }, "object.create(foo, { bar: { set: function(val) { return 1; } } })", - // Global object doesn't exist + // global object doesn't exist "Reflect.defineProperty(foo, 'bar', { set(val) { if (val) { return 1; } } })", "/* globals Object:off */ Object.defineProperty(foo, 'bar', { set(val) { return 1; } })", { @@ -233,7 +233,7 @@ ruleTester.run("no-setter-return", rule, { globals: { Object: "off" } }, - // Global object is shadowed + // global object is shadowed "let Object; Object.defineProperty(foo, 'bar', { set(val) { return 1; } })", { code: "function f() { Reflect.defineProperty(foo, 'bar', { set(val) { if (val) { return 1; } } }); var Reflect;}", diff --git a/tests/lib/rules/no-warning-comments.js b/tests/lib/rules/no-warning-comments.js index b465de9a0a9..c2689defab1 100644 --- a/tests/lib/rules/no-warning-comments.js +++ b/tests/lib/rules/no-warning-comments.js @@ -37,10 +37,7 @@ ruleTester.run("no-warning-comments", rule, { { code: "// special regex characters don't cause problems", options: [{ terms: ["[aeiou]"], location: "anywhere" }] }, "/*eslint no-warning-comments: [2, { \"terms\": [\"todo\", \"fixme\", \"any other term\"], \"location\": \"anywhere\" }]*/\n\nvar x = 10;\n", { code: "/*eslint no-warning-comments: [2, { \"terms\": [\"todo\", \"fixme\", \"any other term\"], \"location\": \"anywhere\" }]*/\n\nvar x = 10;\n", options: [{ location: "anywhere" }] }, - { code: "foo", options: [{ terms: ["foo-bar"] }] }, - - // https://github.com/eslint/eslint/issues/14575 - "//eslint no-warning-comments: [2, {terms: [\"eslint\"]}]" + { code: "foo", options: [{ terms: ["foo-bar"] }] } ], invalid: [ { diff --git a/tests/lib/rules/require-await.js b/tests/lib/rules/require-await.js index 49bfc068418..5ed2fe09367 100644 --- a/tests/lib/rules/require-await.js +++ b/tests/lib/rules/require-await.js @@ -43,7 +43,7 @@ ruleTester.run("require-await", rule, { // for-await-of "async function foo() { for await (x of xs); }", - // Global await + // global await { code: "await foo()", parser: require.resolve("../../fixtures/parsers/typescript-parsers/global-await") diff --git a/tests/lib/rules/utils/ast-utils.js b/tests/lib/rules/utils/ast-utils.js index a6197c77af0..0d2954f5b6c 100644 --- a/tests/lib/rules/utils/ast-utils.js +++ b/tests/lib/rules/utils/ast-utils.js @@ -204,12 +204,7 @@ describe("ast-utils", () => { "// lalala I'm a normal comment", "// trying to confuse eslint ", "//trying to confuse eslint-directive-detection", - "//xxeslint-enable foo", - "//xxeslint-disable foo", - "//xxeslint foo: 2", - "//xxglobal foo", - "//xxglobals foo", - "//xxexported foo" + "//eslint is awesome" ].join("\n"); const ast = espree.parse(code, ESPREE_CONFIG); const sourceCode = new SourceCode(code, ast); @@ -237,13 +232,7 @@ describe("ast-utils", () => { "// eslint-disable-line no-undef", "// eslint-secret-directive 4 8 15 16 23 42 ", "// eslint-directive-without-argument", - "//eslint-directive-without-padding", - - // https://github.com/eslint/eslint/issues/14575 - "//eslint foo:0", - "//global foo", - "//eslint-enable foo", - "//eslint-disable foo" + "//eslint-directive-without-padding" ].join("\n"); const ast = espree.parse(code, ESPREE_CONFIG); const sourceCode = new SourceCode(code, ast);