From 3a43264762905057e20d8048de6297521d42ad94 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Thu, 4 May 2023 14:07:50 -0700 Subject: [PATCH 01/28] Implement SourceCode#applyInlineConfig() --- lib/linter/linter.js | 85 ++++++-- lib/source-code/source-code.js | 343 ++++++++++++++++++++++++++++++++- 2 files changed, 402 insertions(+), 26 deletions(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 48b2bdbe5c3..135dd6035f0 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -588,6 +588,26 @@ function findEslintEnv(text) { return retv; } +const disallowedSourceCodeMethods = new Set(["applyLanguageOptions", "applyInlineConfig", "finalize"]); + +/** + * Creates a proxy object for a SourceCode object to ensure that rules can't + * call methods intended to be used by the core only. + * @param {SourceCode} sourceCode The SourceCode object to wrap. + * @returns {Proxy} The proxy object for the SourceCode. + */ +function createSourceCodeProxy(sourceCode) { + return new Proxy(sourceCode, { + get(target, key) { + if (disallowedSourceCodeMethods.has(key)) { + throw new TypeError("This method cannot be called inside of a rule."); + } + + return target[key]; + } + }); +} + /** * Convert "/path/to/" to "". * `CLIEngine#executeOnText()` method gives "/path/to/" if the filename @@ -1540,19 +1560,6 @@ class Linter { languageOptions.ecmaVersion ); - /* - * add configured globals and language globals - * - * using Object.assign instead of object spread for performance reasons - * https://github.com/eslint/eslint/issues/16302 - */ - const configuredGlobals = Object.assign( - {}, - getGlobalsForEcmaVersion(languageOptions.ecmaVersion), - languageOptions.sourceType === "commonjs" ? globals.commonjs : void 0, - languageOptions.globals - ); - // double check that there is a parser to avoid mysterious error messages if (!languageOptions.parser) { throw new TypeError(`No parser specified for ${options.filename}`); @@ -1608,6 +1615,42 @@ class Linter { } const sourceCode = slots.lastSourceCode; + + /* + * Make adjustments based on the language options. For JavaScript, + * this is primarily about adding variables into the global scope + * to account for ecmaVersion and configured globals. + */ + sourceCode.applyLanguageOptions(languageOptions); + + let inlineConfig = {}; + const inlineConfigProblems = []; + + /* + * Inline config can be either enabled or disabled. If disabled, it's possible + * to detect the inline config and emit a warning (though this is not required). + * So we first check to see if inline config is allowed at all, and if so, we + * need to check if it's a warning or not. + */ + if (options.allowInlineConfig) { + + // if inline config should warn then add the warnings + if (options.warnInlineConfig) { + sourceCode.getInlineConfigComments().forEach(comment => { + inlineConfigProblems.push(createLintingProblem({ + ruleId: null, + message: `'${comment.value}' has no effect because you have 'noInlineConfig' setting in ${options.warnInlineConfig}.`, + loc: comment.loc, + severity: 1 + })); + + }); + } else { + inlineConfig = sourceCode.applyInlineConfig(); + inlineConfigProblems.push(...inlineConfig.problems.map(createLintingProblem)); + } + } + const commentDirectives = options.allowInlineConfig ? getDirectiveComments( sourceCode.ast, @@ -1617,19 +1660,21 @@ class Linter { : { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] }; // augment global scope with declared global variables - addDeclaredGlobals( - sourceCode.scopeManager.scopes[0], - configuredGlobals, - { exportedVariables: commentDirectives.exportedVariables, enabledGlobals: commentDirectives.enabledGlobals } - ); + // addDeclaredGlobals( + // sourceCode.scopeManager.scopes[0], + // configuredGlobals, + // { exportedVariables: commentDirectives.exportedVariables, enabledGlobals: commentDirectives.enabledGlobals } + // ); const configuredRules = Object.assign({}, config.rules, commentDirectives.configuredRules); - let lintingProblems; + sourceCode.finalize(); + const proxySourceCode = createSourceCodeProxy(sourceCode); + try { lintingProblems = runRules( - sourceCode, + proxySourceCode, configuredRules, ruleId => getRuleFromConfig(ruleId, config), void 0, diff --git a/lib/source-code/source-code.js b/lib/source-code/source-code.js index 07c0d294829..a0e34763e07 100644 --- a/lib/source-code/source-code.js +++ b/lib/source-code/source-code.js @@ -12,7 +12,15 @@ const { isCommentToken } = require("@eslint-community/eslint-utils"), TokenStore = require("./token-store"), astUtils = require("../shared/ast-utils"), - Traverser = require("../shared/traverser"); + Traverser = require("../shared/traverser"), + globals = require("../../conf/globals"), + { + directivesPattern + } = require("../shared/directives"), + + /* eslint-disable-next-line n/no-restricted-require -- Too messy to figure out right now. */ + ConfigCommentParser = require("../linter/config-comment-parser"), + eslintScope = require("eslint-scope"); //------------------------------------------------------------------------------ // Type Definitions @@ -24,6 +32,8 @@ const // Private //------------------------------------------------------------------------------ +const commentParser = new ConfigCommentParser(); + /** * Validates that the given AST has the required information. * @param {ASTNode} ast The Program node of the AST to check. @@ -49,6 +59,29 @@ function validate(ast) { } } +/** + * Retrieves globals for the given ecmaVersion. + * @param {number} ecmaVersion The version to retrieve globals for. + * @returns {Object} The globals for the given ecmaVersion. + */ +function getGlobalsForEcmaVersion(ecmaVersion) { + + switch (ecmaVersion) { + case 3: + return globals.es3; + + case 5: + return globals.es5; + + default: + if (ecmaVersion < 2015) { + return globals[`es${ecmaVersion + 2009}`]; + } + + return globals[`es${ecmaVersion}`]; + } +} + /** * Check to see if its a ES6 export declaration. * @param {ASTNode} astNode An AST node. @@ -83,6 +116,36 @@ function sortedMerge(tokens, comments) { return result; } +/** + * Normalizes a value for a global in a config + * @param {(boolean|string|null)} configuredValue The value given for a global in configuration or in + * a global directive comment + * @returns {("readable"|"writeable"|"off")} The value normalized as a string + * @throws Error if global value is invalid + */ +function normalizeConfigGlobal(configuredValue) { + switch (configuredValue) { + case "off": + return "off"; + + case true: + case "true": + case "writeable": + case "writable": + return "writable"; + + case null: + case false: + case "false": + case "readable": + case "readonly": + return "readonly"; + + default: + throw new Error(`'${configuredValue}' is not a valid configuration for a global (use 'readonly', 'writable', or 'off')`); + } +} + /** * Determines if two nodes or tokens overlap. * @param {ASTNode|Token} first The first node or token to check. @@ -145,6 +208,116 @@ function isSpaceBetween(sourceCode, first, second, checkInsideOfJSXText) { return false; } +//----------------------------------------------------------------------------- +// Directive Comments +//----------------------------------------------------------------------------- + +/** + * Extract the directive and the justification from a given directive comment and trim them. + * @param {string} value The comment text to extract. + * @returns {{directivePart: string, justificationPart: string}} The extracted directive and justification. + */ +function extractDirectiveComment(value) { + const match = /\s-{2,}\s/u.exec(value); + + if (!match) { + return { directivePart: value.trim(), justificationPart: "" }; + } + + const directive = value.slice(0, match.index).trim(); + const justification = value.slice(match.index + match[0].length).trim(); + + return { directivePart: directive, justificationPart: justification }; +} + +/** + * Ensures that variables representing built-in properties of the Global Object, + * and any globals declared by special block comments, are present in the global + * scope. + * @param {Scope} globalScope The global scope. + * @param {Object} configGlobals The globals declared in configuration + * @param {Object|undefined} inlineGlobals The globals declared in the source code + * @returns {void} + */ +function addDeclaredGlobals(globalScope, configGlobals, inlineGlobals = {}) { + + // Define configured global variables. + for (const id of new Set([...Object.keys(configGlobals), ...Object.keys(inlineGlobals)])) { + + /* + * `ConfigOps.normalizeConfigGlobal` will throw an error if a configured global value is invalid. However, these errors would + * typically be caught when validating a config anyway (validity for inline global comments is checked separately). + */ + const configValue = configGlobals[id] === void 0 ? void 0 : normalizeConfigGlobal(configGlobals[id]); + const commentValue = inlineGlobals[id] && inlineGlobals[id].value; + const value = commentValue || configValue; + const sourceComments = inlineGlobals[id] && inlineGlobals[id].comments; + + if (value === "off") { + continue; + } + + let variable = globalScope.set.get(id); + + if (!variable) { + variable = new eslintScope.Variable(id, globalScope); + + globalScope.variables.push(variable); + globalScope.set.set(id, variable); + } + + variable.eslintImplicitGlobalSetting = configValue; + variable.eslintExplicitGlobal = sourceComments !== void 0; + variable.eslintExplicitGlobalComments = sourceComments; + variable.writeable = (value === "writable"); + } + + /* + * "through" contains all references which definitions cannot be found. + * Since we augment the global scope using configuration, we need to update + * references and remove the ones that were added by configuration. + */ + globalScope.through = globalScope.through.filter(reference => { + const name = reference.identifier.name; + const variable = globalScope.set.get(name); + + if (variable) { + + /* + * Links the variable and the reference. + * And this reference is removed from `Scope#through`. + */ + reference.resolved = variable; + variable.references.push(reference); + + return false; + } + + return true; + }); +} + +/** + * Sets the given variable names as exported so they won't be triggered by + * the `no-unused-vars` rule. + * @param {eslint.Scope} globalScope The global scope to define exports in. + * @param {Record} variables An object whose keys are the variable + * names to export. + * @returns {void} + */ +function markExportedVariables(globalScope, variables) { + + Object.keys(variables).forEach(name => { + const variable = globalScope.set.get(name); + + if (variable) { + variable.eslintUsed = true; + variable.eslintExported = true; + } + }); + +} + //------------------------------------------------------------------------------ // Public Interface //------------------------------------------------------------------------------ @@ -187,7 +360,8 @@ class SourceCode extends TokenStore { * General purpose caching for the class. */ this[caches] = new Map([ - ["scopes", new WeakMap()] + ["scopes", new WeakMap()], + ["vars", new Map()] ]); /** @@ -265,10 +439,6 @@ class SourceCode extends TokenStore { // Cache for comments found using getComments(). this._commentCache = new WeakMap(); - - // don't allow modification of this object - Object.freeze(this); - Object.freeze(this.lines); } /** @@ -724,6 +894,167 @@ class SourceCode extends TokenStore { } + /** + * Returns an array of all inline configuration comments found in the + * source code. + * @returns {Array} An array of all inline configuration comments. + */ + getInlineConfigComments() { + + return this.ast.comments.filter(comment => { + + // shebang comments are never directives + if (comment.type === "Shebang") { + return false; + } + + const { directivePart } = extractDirectiveComment(comment.value); + + const directiveMatch = directivesPattern.exec(directivePart); + + if (!directiveMatch) { + return false; + } + + // only certain comment types are supported as line comments + return comment.type === "Block" || !!/^eslint-disable-(next-)?line$/u.test(directiveMatch[1]); + }); + } + + /** + * Applies language options sent in from the core. + * @param {Object} languageOptions The language options for this run. + * @returns {void} + */ + applyLanguageOptions(languageOptions) { + + /* + * Add configured globals and language globals + * + * Using Object.assign instead of object spread for performance reasons + * https://github.com/eslint/eslint/issues/16302 + */ + const configGlobals = Object.assign( + {}, + getGlobalsForEcmaVersion(languageOptions.ecmaVersion), + languageOptions.sourceType === "commonjs" ? globals.commonjs : void 0, + languageOptions.globals + ); + const varsCache = this[caches].get("vars"); + + varsCache.set("configGlobals", configGlobals); + } + + /** + * Applies configuration found inside of the source code. This method is only + * called when ESLint is running with inline configuration allowed. + * @returns {{ok:boolean,problems:Array,config:FlatConfig}} Information + * that ESLint needs to further process the inline configuration. + */ + applyInlineConfig() { + + const problems = []; + const config = {}; + const exportedVariables = {}; + const inlineGlobals = Object.create(null); + + this.getInlineConfigComments().forEach(comment => { + + const { directivePart } = extractDirectiveComment(comment.value); + const match = directivesPattern.exec(directivePart); + const directiveText = match[1]; + const directiveValue = directivePart.slice(match.index + directiveText.length); + + switch (directiveText) { + case "exported": + Object.assign(exportedVariables, commentParser.parseStringConfig(directiveValue, comment)); + break; + + case "globals": + case "global": + for (const [id, { value }] of Object.entries(commentParser.parseStringConfig(directiveValue, comment))) { + let normalizedValue; + + try { + normalizedValue = normalizeConfigGlobal(value); + } catch (err) { + problems.push({ + ruleId: null, + loc: comment.loc, + message: err.message + }); + continue; + } + + if (inlineGlobals[id]) { + inlineGlobals[id].comments.push(comment); + inlineGlobals[id].value = normalizedValue; + } else { + inlineGlobals[id] = { + comments: [comment], + value: normalizedValue + }; + } + } + break; + + case "eslint": { + const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc); + + if (parseResult.success) { + config.rules = parseResult.config; + } else { + problems.push(parseResult.error); + } + + break; + } + + // no default + } + }); + + // save all the new variables for later + const varsCache = this[caches].get("vars"); + + varsCache.set("inlineGlobals", inlineGlobals); + varsCache.set("exportedVariables", exportedVariables); + + // now that we've gathered all of the directive-defined globals, let's add them + + return { + ok: problems.length === 0, + config, + problems + }; + } + + /** + * Called by ESLint core to indicate that it has finished providing + * information. We now add in all the missing variables and ensure that + * state-changing methods cannot be called by rules. + * @returns {void} + */ + finalize() { + + // Step 1: ensure that all of the necessary variables are up to date + const varsCache = this[caches].get("vars"); + const globalScope = this.scopeManager.scopes[0]; + const configGlobals = varsCache.get("configGlobals"); + const inlineGlobals = varsCache.get("inlineGlobals"); + const exportedVariables = varsCache.get("exportedVariables"); + + addDeclaredGlobals(globalScope, configGlobals, inlineGlobals); + + if (exportedVariables) { + markExportedVariables(globalScope, exportedVariables); + } + + // don't allow further modification of this object + Object.freeze(this); + Object.freeze(this.lines); + } + } module.exports = SourceCode; From 398a1152dfa6249c261e592e5990aead0e4c51c8 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Fri, 7 Jul 2023 13:10:46 -0400 Subject: [PATCH 02/28] Fix lint errors --- lib/linter/linter.js | 38 +++++--------------------------------- 1 file changed, 5 insertions(+), 33 deletions(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 135dd6035f0..e7e5e762ba4 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -50,7 +50,6 @@ const DEFAULT_ECMA_VERSION = 5; const commentParser = new ConfigCommentParser(); const DEFAULT_ERROR_LOC = { start: { line: 1, column: 0 }, end: { line: 1, column: 1 } }; const parserSymbol = Symbol.for("eslint.RuleTester.parser"); -const globals = require("../../conf/globals"); //------------------------------------------------------------------------------ // Typedefs @@ -145,29 +144,6 @@ function isEspree(parser) { return !!(parser === espree || parser[parserSymbol] === espree); } -/** - * Retrieves globals for the given ecmaVersion. - * @param {number} ecmaVersion The version to retrieve globals for. - * @returns {Object} The globals for the given ecmaVersion. - */ -function getGlobalsForEcmaVersion(ecmaVersion) { - - switch (ecmaVersion) { - case 3: - return globals.es3; - - case 5: - return globals.es5; - - default: - if (ecmaVersion < 2015) { - return globals[`es${ecmaVersion + 2009}`]; - } - - return globals[`es${ecmaVersion}`]; - } -} - /** * Ensures that variables representing built-in properties of the Global Object, * and any globals declared by special block comments, are present in the global @@ -604,6 +580,9 @@ function createSourceCodeProxy(sourceCode) { } return target[key]; + }, + set() { + return false; } }); } @@ -1344,12 +1323,12 @@ class Linter { ); const configuredRules = Object.assign({}, config.rules, commentDirectives.configuredRules); - let lintingProblems; + const proxySourceCode = createSourceCodeProxy(sourceCode); try { lintingProblems = runRules( - sourceCode, + proxySourceCode, configuredRules, ruleId => getRule(slots, ruleId), parserName, @@ -1659,13 +1638,6 @@ class Linter { ) : { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] }; - // augment global scope with declared global variables - // addDeclaredGlobals( - // sourceCode.scopeManager.scopes[0], - // configuredGlobals, - // { exportedVariables: commentDirectives.exportedVariables, enabledGlobals: commentDirectives.enabledGlobals } - // ); - const configuredRules = Object.assign({}, config.rules, commentDirectives.configuredRules); let lintingProblems; From 056ccedbf50132019e9006376ea181d3baefe1fa Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Mon, 10 Jul 2023 11:34:11 -0400 Subject: [PATCH 03/28] Add tests for forbidden methods --- tests/lib/linter/linter.js | 46 +++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 65957f82a75..1790b26355a 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -15184,7 +15184,6 @@ var a = "test2"; }); }); - describe("Error Conditions", () => { describe("when evaluating broken code", () => { const code = BROKEN_TEST_CODE; @@ -16259,6 +16258,51 @@ var a = "test2"; }); + describe("Disallowed SourceCode methods inside a rule", () => { + [ + "applyLanguageOptions", + "applyInlineConfig", + "finalize" + ].forEach(methodName => { + + it(`should throw error when accessing sourceCode.${methodName} inside a rule`, () => { + let spy; + const config = { + plugins: { + test: { + rules: { + checker: { + create(context) { + spy = sinon.spy(() => { + const sourceCode = context.sourceCode; + + assert.throws(() => { + sourceCode[methodName](); + }, /This method cannot be called inside of a rule/u); + }); + + return { Program: spy }; + } + } + } + } + }, + languageOptions: { + sourceType: "script" + }, + rules: { + "test/checker": "error" + } + }; + + linter.verify("foo", config); + assert(spy && spy.calledOnce); + }); + }); + + + }); + describe("when evaluating an empty string", () => { it("runs rules", () => { From 5ff2a9f81d5700b8c089681f2ab79c21ac5089e8 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Mon, 10 Jul 2023 11:35:01 -0400 Subject: [PATCH 04/28] Clean up naming --- lib/linter/linter.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index e7e5e762ba4..1a893871d19 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -1324,11 +1324,11 @@ class Linter { const configuredRules = Object.assign({}, config.rules, commentDirectives.configuredRules); let lintingProblems; - const proxySourceCode = createSourceCodeProxy(sourceCode); + const sourceCodeProxy = createSourceCodeProxy(sourceCode); try { lintingProblems = runRules( - proxySourceCode, + sourceCodeProxy, configuredRules, ruleId => getRule(slots, ruleId), parserName, @@ -1642,11 +1642,11 @@ class Linter { let lintingProblems; sourceCode.finalize(); - const proxySourceCode = createSourceCodeProxy(sourceCode); + const sourceCodeProxy = createSourceCodeProxy(sourceCode); try { lintingProblems = runRules( - proxySourceCode, + sourceCodeProxy, configuredRules, ruleId => getRuleFromConfig(ruleId, config), void 0, From dc94b597cba9642b4ba00c88775017fa54ba0df5 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Thu, 13 Jul 2023 11:54:31 -0400 Subject: [PATCH 05/28] Add more tests --- lib/source-code/source-code.js | 4 +- tests/lib/source-code/source-code.js | 256 +++++++++++++++++++++++++++ 2 files changed, 258 insertions(+), 2 deletions(-) diff --git a/lib/source-code/source-code.js b/lib/source-code/source-code.js index a0e34763e07..887c9b9586d 100644 --- a/lib/source-code/source-code.js +++ b/lib/source-code/source-code.js @@ -235,11 +235,11 @@ function extractDirectiveComment(value) { * and any globals declared by special block comments, are present in the global * scope. * @param {Scope} globalScope The global scope. - * @param {Object} configGlobals The globals declared in configuration + * @param {Object|undefined} configGlobals The globals declared in configuration * @param {Object|undefined} inlineGlobals The globals declared in the source code * @returns {void} */ -function addDeclaredGlobals(globalScope, configGlobals, inlineGlobals = {}) { +function addDeclaredGlobals(globalScope, configGlobals = {}, inlineGlobals = {}) { // Define configured global variables. for (const id of new Set([...Object.keys(configGlobals), ...Object.keys(inlineGlobals)])) { diff --git a/tests/lib/source-code/source-code.js b/tests/lib/source-code/source-code.js index dae9bcf4c3d..399f47be6f1 100644 --- a/tests/lib/source-code/source-code.js +++ b/tests/lib/source-code/source-code.js @@ -12,6 +12,7 @@ const fs = require("fs"), path = require("path"), assert = require("chai").assert, espree = require("espree"), + eslintScope = require("eslint-scope"), sinon = require("sinon"), { Linter } = require("../../../lib/linter"), SourceCode = require("../../../lib/source-code/source-code"), @@ -3787,4 +3788,259 @@ describe("SourceCode", () => { }); }); + + xdescribe("getInlineConfigComments()", () => { + + it("should return inline config comments", () => { + + const code = "/*eslint foo: 1*/ foo; /* non-config comment*/ /* eslint-disable bar */ bar; /* eslint-enable bar */"; + const ast = espree.parse(code, DEFAULT_CONFIG); + const sourceCode = new SourceCode(code, ast); + const configComments = sourceCode.getInlineConfigComments(); + + assert.deepStrictEqual(configComments, [ + { + type: "Block", + value: "eslint foo: 1", + start: 0, + end: 17, + range: [ + 0, + 17 + ], + loc: { + start: { + line: 1, + column: 0 + }, + end: { + line: 1, + column: 17 + } + } + }, + { + type: "Block", + value: " eslint-disable bar ", + start: 47, + end: 71, + range: [ + 47, + 71 + ], + loc: { + start: { + line: 1, + column: 47 + }, + end: { + line: 1, + column: 71 + } + } + }, + { + type: "Block", + value: " eslint-enable bar ", + start: 77, + end: 100, + range: [ + 77, + 100 + ], + loc: { + start: { + line: 1, + column: 77 + }, + end: { + line: 1, + column: 100 + } + } + } + ]); + + + }); + + }); + + describe("applyLanguageOptions()", () => { + + it("should add ES6 globals", () => { + + const code = "foo"; + const ast = espree.parse(code, DEFAULT_CONFIG); + const sourceCode = new SourceCode(code, ast); + const scopeManager = eslintScope.analyze(ast, { + ignoreEval: true, + ecmaVersion: 6 + }); + + sourceCode.applyLanguageOptions({ + ecmaVersion: 2015 + }); + + sourceCode.scopeManager = scopeManager; + sourceCode.finalize(); + + const globalScope = sourceCode.scopeManager.scopes[0]; + const variable = globalScope.set.get("Promise"); + + assert.isDefined(variable); + + }); + + it("should add custom globals", () => { + + const code = "foo"; + const ast = espree.parse(code, DEFAULT_CONFIG); + const sourceCode = new SourceCode(code, ast); + const scopeManager = eslintScope.analyze(ast, { + ignoreEval: true, + ecmaVersion: 6 + }); + + sourceCode.applyLanguageOptions({ + ecmaVersion: 2015, + globals: { + FOO: true + } + }); + + sourceCode.scopeManager = scopeManager; + sourceCode.finalize(); + + const globalScope = sourceCode.scopeManager.scopes[0]; + const variable = globalScope.set.get("FOO"); + + assert.isDefined(variable); + assert.isTrue(variable.writeable); + }); + + it("should add commonjs globals", () => { + + const code = "foo"; + const ast = espree.parse(code, DEFAULT_CONFIG); + const sourceCode = new SourceCode(code, ast); + const scopeManager = eslintScope.analyze(ast, { + ignoreEval: true, + nodejsScope: true, + ecmaVersion: 6 + }); + + sourceCode.applyLanguageOptions({ + ecmaVersion: 2015, + sourceType: "commonjs" + }); + + sourceCode.scopeManager = scopeManager; + sourceCode.finalize(); + + const globalScope = sourceCode.scopeManager.scopes[0]; + const variable = globalScope.set.get("require"); + + assert.isDefined(variable); + + }); + + }); + + describe("applyInlineConfig()", () => { + + it("should add inline globals", () => { + + const code = "/*global bar: true */ foo"; + const ast = espree.parse(code, DEFAULT_CONFIG); + const sourceCode = new SourceCode(code, ast); + const scopeManager = eslintScope.analyze(ast, { + ignoreEval: true, + ecmaVersion: 6 + }); + + const result = sourceCode.applyInlineConfig(); + + assert.isTrue(result.ok); + + sourceCode.scopeManager = scopeManager; + sourceCode.finalize(); + + const globalScope = sourceCode.scopeManager.scopes[0]; + const variable = globalScope.set.get("bar"); + + assert.isDefined(variable); + assert.isTrue(variable.writeable); + }); + + + it("should mark exported variables", () => { + + const code = "/*exported foo */ var foo;"; + const ast = espree.parse(code, DEFAULT_CONFIG); + const sourceCode = new SourceCode(code, ast); + const scopeManager = eslintScope.analyze(ast, { + ignoreEval: true, + ecmaVersion: 6 + }); + + const result = sourceCode.applyInlineConfig(); + + assert.isTrue(result.ok); + + sourceCode.scopeManager = scopeManager; + sourceCode.finalize(); + + const globalScope = sourceCode.scopeManager.scopes[0]; + const variable = globalScope.set.get("foo"); + + assert.isDefined(variable); + assert.isTrue(variable.eslintUsed); + assert.isTrue(variable.eslintExported); + }); + + it("should extract rule configuration", () => { + + const code = "/*eslint some-rule: 2 */ var foo;"; + const ast = espree.parse(code, DEFAULT_CONFIG); + const sourceCode = new SourceCode(code, ast); + const result = sourceCode.applyInlineConfig(); + + assert.isTrue(result.ok); + assert.strictEqual(result.config.rules["some-rule"], 2); + }); + + it("should extract multiple rule configurations", () => { + + const code = "/*eslint some-rule: 2, other-rule: [\"error\", { skip: true }] */ var foo;"; + const ast = espree.parse(code, DEFAULT_CONFIG); + const sourceCode = new SourceCode(code, ast); + const result = sourceCode.applyInlineConfig(); + + assert.isTrue(result.ok); + assert.strictEqual(result.config.rules["some-rule"], 2); + assert.deepStrictEqual(result.config.rules["other-rule"], ["error", { skip: true }]); + }); + + it("should report problem with rule configuration parsing", () => { + + const code = "/*eslint some-rule::, */ var foo;"; + const ast = espree.parse(code, DEFAULT_CONFIG); + const sourceCode = new SourceCode(code, ast); + const result = sourceCode.applyInlineConfig(); + + assert.isFalse(result.ok); + assert.deepStrictEqual(result.problems, [ + { + column: 1, + fatal: true, + line: 1, + message: "Failed to parse JSON from ' \"some-rule\"::,': Unexpected token : in JSON at position 14", + nodeType: null, + ruleId: null, + severity: 2 + } + ]); + }); + }); }); From 3cf661a1688bbdae10a4d8372a3345fb8c4f5591 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Thu, 13 Jul 2023 13:45:03 -0400 Subject: [PATCH 06/28] Fix last test --- tests/lib/source-code/source-code.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/lib/source-code/source-code.js b/tests/lib/source-code/source-code.js index 399f47be6f1..3b04274e68c 100644 --- a/tests/lib/source-code/source-code.js +++ b/tests/lib/source-code/source-code.js @@ -3789,7 +3789,7 @@ describe("SourceCode", () => { }); - xdescribe("getInlineConfigComments()", () => { + describe("getInlineConfigComments()", () => { it("should return inline config comments", () => { @@ -3798,7 +3798,8 @@ describe("SourceCode", () => { const sourceCode = new SourceCode(code, ast); const configComments = sourceCode.getInlineConfigComments(); - assert.deepStrictEqual(configComments, [ + // not sure why but without the JSON parse/stringify Chai won't see these as equal + assert.deepStrictEqual(JSON.parse(JSON.stringify(configComments)), [ { type: "Block", value: "eslint foo: 1", @@ -3861,7 +3862,6 @@ describe("SourceCode", () => { } ]); - }); }); From 7e02709f0715b72aee8f467992bc48970228efc6 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Tue, 18 Jul 2023 11:17:16 -0400 Subject: [PATCH 07/28] Fixing some bugs -- still tests failing --- lib/linter/linter.js | 175 +++++++++++++++++++++++++-- lib/source-code/source-code.js | 38 ++++-- tests/lib/linter/linter.js | 6 +- tests/lib/source-code/source-code.js | 4 +- 4 files changed, 197 insertions(+), 26 deletions(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 1a893871d19..8c47990e82f 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -337,13 +337,13 @@ function extractDirectiveComment(value) { * Parses comments in file to extract file-specific config of rules, globals * and environments and merges them with global config; also code blocks * where reporting is disabled or enabled and merges them with reporting config. - * @param {ASTNode} ast The top node of the AST. + * @param {SourceCode} sourceCode The SourceCode object to get comments from. * @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules * @param {string|null} warnInlineConfig If a string then it should warn directive comments as disabled. The string value is the config name what the setting came from. * @returns {{configuredRules: Object, enabledGlobals: {value:string,comment:Token}[], exportedVariables: Object, problems: LintMessage[], disableDirectives: DisableDirective[]}} * A collection of the directive comments that were found, along with any problems that occurred when parsing */ -function getDirectiveComments(ast, ruleMapper, warnInlineConfig) { +function getDirectiveComments(sourceCode, ruleMapper, warnInlineConfig) { const configuredRules = {}; const enabledGlobals = Object.create(null); const exportedVariables = {}; @@ -353,7 +353,7 @@ function getDirectiveComments(ast, ruleMapper, warnInlineConfig) { builtInRules: Rules }); - ast.comments.filter(token => token.type !== "Shebang").forEach(comment => { + sourceCode.getInlineConfigNodes().filter(token => token.type !== "Shebang").forEach(comment => { const { directivePart, justificationPart } = extractDirectiveComment(comment.value); const match = directivesPattern.exec(directivePart); @@ -487,6 +487,128 @@ function getDirectiveComments(ast, ruleMapper, warnInlineConfig) { }; } +/** + * Parses comments in file to extract file-specific config of rules, globals + * and environments and merges them with global config; also code blocks + * where reporting is disabled or enabled and merges them with reporting config. + * @param {SourceCode} sourceCode The SourceCode object to get comments from. + * @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules + * @param {string|null} warnInlineConfig If a string then it should warn directive comments as disabled. The string value is the config name what the setting came from. + * @returns {{configuredRules: Object, enabledGlobals: {value:string,comment:Token}[], exportedVariables: Object, problems: LintMessage[], disableDirectives: DisableDirective[]}} + * A collection of the directive comments that were found, along with any problems that occurred when parsing + */ +function getDirectiveCommentsForFlatConfig(sourceCode, ruleMapper, warnInlineConfig) { + const configuredRules = {}; + const enabledGlobals = Object.create(null); + const exportedVariables = {}; + const problems = []; + const disableDirectives = []; + const validator = new ConfigValidator({ + builtInRules: Rules + }); + + sourceCode.getInlineConfigNodes().filter(token => token.type !== "Shebang").forEach(comment => { + const { directivePart, justificationPart } = extractDirectiveComment(comment.value); + + const match = directivesPattern.exec(directivePart); + + if (!match) { + return; + } + const directiveText = match[1]; + const lineCommentSupported = /^eslint-disable-(next-)?line$/u.test(directiveText); + + if (comment.type === "Line" && !lineCommentSupported) { + return; + } + + if (warnInlineConfig) { + const kind = comment.type === "Block" ? `/*${directiveText}*/` : `//${directiveText}`; + + problems.push(createLintingProblem({ + ruleId: null, + message: `'${kind}' has no effect because you have 'noInlineConfig' setting in ${warnInlineConfig}.`, + loc: comment.loc, + severity: 1 + })); + return; + } + + if (directiveText === "eslint-disable-line" && comment.loc.start.line !== comment.loc.end.line) { + const message = `${directiveText} comment should not span multiple lines.`; + + problems.push(createLintingProblem({ + ruleId: null, + message, + loc: comment.loc + })); + return; + } + + const directiveValue = directivePart.slice(match.index + directiveText.length); + + switch (directiveText) { + case "eslint-disable": + case "eslint-enable": + case "eslint-disable-next-line": + case "eslint-disable-line": { + const directiveType = directiveText.slice("eslint-".length); + const options = { commentToken: comment, type: directiveType, value: directiveValue, justification: justificationPart, ruleMapper }; + const { directives, directiveProblems } = createDisableDirectives(options); + + disableDirectives.push(...directives); + problems.push(...directiveProblems); + break; + } + + // case "eslint": { + // const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc); + + // if (parseResult.success) { + // Object.keys(parseResult.config).forEach(name => { + // const rule = ruleMapper(name); + // const ruleValue = parseResult.config[name]; + + // if (!rule) { + // problems.push(createLintingProblem({ ruleId: name, loc: comment.loc })); + // return; + // } + + // try { + // validator.validateRuleOptions(rule, name, ruleValue); + // } catch (err) { + // problems.push(createLintingProblem({ + // ruleId: name, + // message: err.message, + // loc: comment.loc + // })); + + // // do not apply the config, if found invalid options. + // return; + // } + + // configuredRules[name] = ruleValue; + // }); + // } else { + // problems.push(parseResult.error); + // } + + // break; + // } + + // no default + } + }); + + return { + configuredRules, + enabledGlobals, + exportedVariables, + problems, + disableDirectives + }; +} + /** * Normalize ECMAScript version from the initial config * @param {Parser} parser The parser which uses this options. @@ -1312,7 +1434,7 @@ class Linter { const sourceCode = slots.lastSourceCode; const commentDirectives = options.allowInlineConfig - ? getDirectiveComments(sourceCode.ast, ruleId => getRule(slots, ruleId), options.warnInlineConfig) + ? getDirectiveComments(sourceCode, ruleId => getRule(slots, ruleId), options.warnInlineConfig) : { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] }; // augment global scope with declared global variables @@ -1602,7 +1724,9 @@ class Linter { */ sourceCode.applyLanguageOptions(languageOptions); - let inlineConfig = {}; + const mergedInlineConfig = { + rules: {} + }; const inlineConfigProblems = []; /* @@ -1615,7 +1739,7 @@ class Linter { // if inline config should warn then add the warnings if (options.warnInlineConfig) { - sourceCode.getInlineConfigComments().forEach(comment => { + sourceCode.getInlineConfigNodes().forEach(comment => { inlineConfigProblems.push(createLintingProblem({ ruleId: null, message: `'${comment.value}' has no effect because you have 'noInlineConfig' setting in ${options.warnInlineConfig}.`, @@ -1625,20 +1749,48 @@ class Linter { }); } else { - inlineConfig = sourceCode.applyInlineConfig(); - inlineConfigProblems.push(...inlineConfig.problems.map(createLintingProblem)); + const inlineConfigResult = sourceCode.applyInlineConfig(); + + inlineConfigProblems.push(...inlineConfigResult.problems.map(createLintingProblem)); + + // need to verify rule existence options + if (inlineConfigResult.ok) { + for (const { config: inlineConfig, comment } of inlineConfigResult.configs) { + + Object.keys(inlineConfig.rules).forEach(ruleId => { + const rule = getRuleFromConfig(ruleId, config); + const ruleValue = inlineConfig.rules[ruleId]; + + if (!rule) { + inlineConfigProblems.push(createLintingProblem({ ruleId: ruleId, loc: comment.loc })); + return; + } + + try { + validator.validateRuleOptions(rule, ruleId, ruleValue); + mergedInlineConfig.rules[ruleId] = ruleValue; + } catch (err) { + inlineConfigProblems.push(createLintingProblem({ + ruleId: ruleId, + message: err.message, + loc: comment.loc + })); + } + }); + } + } } } const commentDirectives = options.allowInlineConfig - ? getDirectiveComments( - sourceCode.ast, + ? getDirectiveCommentsForFlatConfig( + sourceCode, ruleId => getRuleFromConfig(ruleId, config), options.warnInlineConfig ) : { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] }; - const configuredRules = Object.assign({}, config.rules, commentDirectives.configuredRules); + const configuredRules = Object.assign({}, config.rules, mergedInlineConfig.rules, commentDirectives.configuredRules); let lintingProblems; sourceCode.finalize(); @@ -1684,6 +1836,7 @@ class Linter { disableFixes: options.disableFixes, problems: lintingProblems .concat(commentDirectives.problems) + .concat(inlineConfigProblems) .sort((problemA, problemB) => problemA.line - problemB.line || problemA.column - problemB.column), reportUnusedDisableDirectives: options.reportUnusedDisableDirectives }); diff --git a/lib/source-code/source-code.js b/lib/source-code/source-code.js index 887c9b9586d..047da91f390 100644 --- a/lib/source-code/source-code.js +++ b/lib/source-code/source-code.js @@ -361,7 +361,8 @@ class SourceCode extends TokenStore { */ this[caches] = new Map([ ["scopes", new WeakMap()], - ["vars", new Map()] + ["vars", new Map()], + ["configNodes", void 0] ]); /** @@ -895,13 +896,21 @@ class SourceCode extends TokenStore { /** - * Returns an array of all inline configuration comments found in the + * Returns an array of all inline configuration nodes found in the * source code. - * @returns {Array} An array of all inline configuration comments. + * @returns {Array} An array of all inline configuration nodes. */ - getInlineConfigComments() { + getInlineConfigNodes() { - return this.ast.comments.filter(comment => { + // check the cache first + let configNodes = this[caches].get("configNodes"); + + if (configNodes) { + return configNodes; + } + + // calculate fresh config nodes + configNodes = this.ast.comments.filter(comment => { // shebang comments are never directives if (comment.type === "Shebang") { @@ -919,6 +928,10 @@ class SourceCode extends TokenStore { // only certain comment types are supported as line comments return comment.type === "Block" || !!/^eslint-disable-(next-)?line$/u.test(directiveMatch[1]); }); + + this[caches].set("configNodes", configNodes); + + return configNodes; } /** @@ -948,17 +961,17 @@ class SourceCode extends TokenStore { /** * Applies configuration found inside of the source code. This method is only * called when ESLint is running with inline configuration allowed. - * @returns {{ok:boolean,problems:Array,config:FlatConfig}} Information + * @returns {{ok:boolean,problems:Array,configs:{config:FlatConfigArray,comment:ASTNode}}} Information * that ESLint needs to further process the inline configuration. */ applyInlineConfig() { const problems = []; - const config = {}; + const configs = []; const exportedVariables = {}; const inlineGlobals = Object.create(null); - this.getInlineConfigComments().forEach(comment => { + this.getInlineConfigNodes().forEach(comment => { const { directivePart } = extractDirectiveComment(comment.value); const match = directivesPattern.exec(directivePart); @@ -1002,7 +1015,12 @@ class SourceCode extends TokenStore { const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc); if (parseResult.success) { - config.rules = parseResult.config; + configs.push({ + config: { + rules: parseResult.config + }, + comment + }) } else { problems.push(parseResult.error); } @@ -1024,7 +1042,7 @@ class SourceCode extends TokenStore { return { ok: problems.length === 0, - config, + configs, problems }; } diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 1790b26355a..8844d27bd9c 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -11875,12 +11875,12 @@ describe("Linter with FlatConfigArray", () => { const messages = linter.verify(code, config, filename); const suppressedMessages = linter.getSuppressedMessages(); - assert.strictEqual(messages.length, 1); + assert.strictEqual(messages.length, 1, "Incorrect message length"); assert.strictEqual(messages[0].ruleId, "no-alert"); assert.strictEqual(messages[0].message, "Unexpected alert."); assert.include(messages[0].nodeType, "CallExpression"); - assert.strictEqual(suppressedMessages.length, 0); + assert.strictEqual(suppressedMessages.length, 0,"Incorrect suppressed message length"); }); it("rules should not change initial config", () => { @@ -11968,7 +11968,7 @@ describe("Linter with FlatConfigArray", () => { }); describe("when evaluating code with invalid comments to enable rules", () => { - it("should report a violation when the config is not a valid rule configuration", () => { + it.only("should report a violation when the config is not a valid rule configuration", () => { const messages = linter.verify("/*eslint no-alert:true*/ alert('test');", {}); const suppressedMessages = linter.getSuppressedMessages(); diff --git a/tests/lib/source-code/source-code.js b/tests/lib/source-code/source-code.js index 3b04274e68c..3ed916dfa8f 100644 --- a/tests/lib/source-code/source-code.js +++ b/tests/lib/source-code/source-code.js @@ -3789,14 +3789,14 @@ describe("SourceCode", () => { }); - describe("getInlineConfigComments()", () => { + describe("getInlineConfigNodes()", () => { it("should return inline config comments", () => { const code = "/*eslint foo: 1*/ foo; /* non-config comment*/ /* eslint-disable bar */ bar; /* eslint-enable bar */"; const ast = espree.parse(code, DEFAULT_CONFIG); const sourceCode = new SourceCode(code, ast); - const configComments = sourceCode.getInlineConfigComments(); + const configComments = sourceCode.getInlineConfigNodes(); // not sure why but without the JSON parse/stringify Chai won't see these as equal assert.deepStrictEqual(JSON.parse(JSON.stringify(configComments)), [ From 189516856df3b91123a0fb32997cd64ef9a35fdd Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Thu, 20 Jul 2023 16:48:26 -0400 Subject: [PATCH 08/28] Further refactoring and bug fixes --- lib/config/flat-config-schema.js | 10 ++ lib/linter/linter.js | 149 ++++++++++++++++----------- lib/source-code/source-code.js | 6 +- tests/lib/eslint/flat-eslint.js | 2 +- tests/lib/linter/linter.js | 12 +-- tests/lib/source-code/source-code.js | 21 +++- 6 files changed, 124 insertions(+), 76 deletions(-) diff --git a/lib/config/flat-config-schema.js b/lib/config/flat-config-schema.js index 3922e8a94fe..d06cb5a8700 100644 --- a/lib/config/flat-config-schema.js +++ b/lib/config/flat-config-schema.js @@ -533,3 +533,13 @@ exports.flatConfigSchema = { plugins: pluginsSchema, rules: rulesSchema }; + +//----------------------------------------------------------------------------- +// Exports +//----------------------------------------------------------------------------- + +module.exports = { + flatConfigSchema, + assertIsRuleSeverity, + assertIsRuleOptions +}; diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 8c47990e82f..068b0b73d6d 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -42,7 +42,8 @@ const ruleReplacements = require("../../conf/replacements.json"); const { getRuleFromConfig } = require("../config/flat-config-helpers"); const { FlatConfigArray } = require("../config/flat-config-array"); - +const { RuleValidator } = require("../config/rule-validator"); +const { assertIsRuleOptions, assertIsRuleSeverity } = require("../config/flat-config-schema"); const debug = require("debug")("eslint:linter"); const MAX_AUTOFIX_PASSES = 10; const DEFAULT_PARSER_NAME = "espree"; @@ -493,19 +494,15 @@ function getDirectiveComments(sourceCode, ruleMapper, warnInlineConfig) { * where reporting is disabled or enabled and merges them with reporting config. * @param {SourceCode} sourceCode The SourceCode object to get comments from. * @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules - * @param {string|null} warnInlineConfig If a string then it should warn directive comments as disabled. The string value is the config name what the setting came from. * @returns {{configuredRules: Object, enabledGlobals: {value:string,comment:Token}[], exportedVariables: Object, problems: LintMessage[], disableDirectives: DisableDirective[]}} * A collection of the directive comments that were found, along with any problems that occurred when parsing */ -function getDirectiveCommentsForFlatConfig(sourceCode, ruleMapper, warnInlineConfig) { +function getDirectiveCommentsForFlatConfig(sourceCode, ruleMapper) { const configuredRules = {}; const enabledGlobals = Object.create(null); const exportedVariables = {}; const problems = []; const disableDirectives = []; - const validator = new ConfigValidator({ - builtInRules: Rules - }); sourceCode.getInlineConfigNodes().filter(token => token.type !== "Shebang").forEach(comment => { const { directivePart, justificationPart } = extractDirectiveComment(comment.value); @@ -522,18 +519,6 @@ function getDirectiveCommentsForFlatConfig(sourceCode, ruleMapper, warnInlineCon return; } - if (warnInlineConfig) { - const kind = comment.type === "Block" ? `/*${directiveText}*/` : `//${directiveText}`; - - problems.push(createLintingProblem({ - ruleId: null, - message: `'${kind}' has no effect because you have 'noInlineConfig' setting in ${warnInlineConfig}.`, - loc: comment.loc, - severity: 1 - })); - return; - } - if (directiveText === "eslint-disable-line" && comment.loc.start.line !== comment.loc.end.line) { const message = `${directiveText} comment should not span multiple lines.`; @@ -561,40 +546,54 @@ function getDirectiveCommentsForFlatConfig(sourceCode, ruleMapper, warnInlineCon break; } - // case "eslint": { - // const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc); - - // if (parseResult.success) { - // Object.keys(parseResult.config).forEach(name => { - // const rule = ruleMapper(name); - // const ruleValue = parseResult.config[name]; - - // if (!rule) { - // problems.push(createLintingProblem({ ruleId: name, loc: comment.loc })); - // return; - // } - - // try { - // validator.validateRuleOptions(rule, name, ruleValue); - // } catch (err) { - // problems.push(createLintingProblem({ - // ruleId: name, - // message: err.message, - // loc: comment.loc - // })); - - // // do not apply the config, if found invalid options. - // return; - // } - - // configuredRules[name] = ruleValue; - // }); - // } else { - // problems.push(parseResult.error); - // } - - // break; - // } + /* + * case "eslint": { + * const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc); + */ + + /* + * if (parseResult.success) { + * Object.keys(parseResult.config).forEach(name => { + * const rule = ruleMapper(name); + * const ruleValue = parseResult.config[name]; + */ + + /* + * if (!rule) { + * problems.push(createLintingProblem({ ruleId: name, loc: comment.loc })); + * return; + * } + */ + + /* + * try { + * validator.validateRuleOptions(rule, name, ruleValue); + * } catch (err) { + * problems.push(createLintingProblem({ + * ruleId: name, + * message: err.message, + * loc: comment.loc + * })); + */ + + /* + * // do not apply the config, if found invalid options. + * return; + * } + */ + + /* + * configuredRules[name] = ruleValue; + * }); + * } else { + * problems.push(parseResult.error); + * } + */ + + /* + * break; + * } + */ // no default } @@ -1739,11 +1738,11 @@ class Linter { // if inline config should warn then add the warnings if (options.warnInlineConfig) { - sourceCode.getInlineConfigNodes().forEach(comment => { + sourceCode.getInlineConfigNodes().forEach(node => { inlineConfigProblems.push(createLintingProblem({ ruleId: null, - message: `'${comment.value}' has no effect because you have 'noInlineConfig' setting in ${options.warnInlineConfig}.`, - loc: comment.loc, + message: `'${sourceCode.text.slice(node.range[0], node.range[1])}' has no effect because you have 'noInlineConfig' setting in ${options.warnInlineConfig}.`, + loc: node.loc, severity: 1 })); @@ -1755,25 +1754,50 @@ class Linter { // need to verify rule existence options if (inlineConfigResult.ok) { - for (const { config: inlineConfig, comment } of inlineConfigResult.configs) { + + const ruleValidator = new RuleValidator(); + + for (const { config: inlineConfig, node } of inlineConfigResult.configs) { Object.keys(inlineConfig.rules).forEach(ruleId => { const rule = getRuleFromConfig(ruleId, config); const ruleValue = inlineConfig.rules[ruleId]; if (!rule) { - inlineConfigProblems.push(createLintingProblem({ ruleId: ruleId, loc: comment.loc })); + inlineConfigProblems.push(createLintingProblem({ ruleId, loc: node.loc })); return; } try { - validator.validateRuleOptions(rule, ruleId, ruleValue); + + const ruleOptions = Array.isArray(ruleValue) ? ruleValue : [ruleValue]; + + assertIsRuleOptions(ruleId, ruleValue); + assertIsRuleSeverity(ruleId, ruleOptions[0]); + + ruleValidator.validate({ + plugins: config.plugins, + rules: { + [ruleId]: ruleOptions + } + }); mergedInlineConfig.rules[ruleId] = ruleValue; } catch (err) { + + let baseMessage = err.message.slice( + err.message.startsWith("Key \"rules\":") + ? err.message.indexOf(":", 12) + 1 + : err.message.indexOf(":") + ).trim(); + + if (err.messageTemplate) { + baseMessage += ` You passed "${ruleValue}".`; + } + inlineConfigProblems.push(createLintingProblem({ - ruleId: ruleId, - message: err.message, - loc: comment.loc + ruleId, + message: `Inline configuration for rule "${ruleId}" is invalid:\n\t${baseMessage}\n`, + loc: node.loc })); } }); @@ -1785,8 +1809,7 @@ class Linter { const commentDirectives = options.allowInlineConfig ? getDirectiveCommentsForFlatConfig( sourceCode, - ruleId => getRuleFromConfig(ruleId, config), - options.warnInlineConfig + ruleId => getRuleFromConfig(ruleId, config) ) : { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] }; diff --git a/lib/source-code/source-code.js b/lib/source-code/source-code.js index 047da91f390..9735201c86f 100644 --- a/lib/source-code/source-code.js +++ b/lib/source-code/source-code.js @@ -961,7 +961,7 @@ class SourceCode extends TokenStore { /** * Applies configuration found inside of the source code. This method is only * called when ESLint is running with inline configuration allowed. - * @returns {{ok:boolean,problems:Array,configs:{config:FlatConfigArray,comment:ASTNode}}} Information + * @returns {{ok:boolean,problems:Array,configs:{config:FlatConfigArray,node:ASTNode}}} Information * that ESLint needs to further process the inline configuration. */ applyInlineConfig() { @@ -1019,8 +1019,8 @@ class SourceCode extends TokenStore { config: { rules: parseResult.config }, - comment - }) + node: comment + }); } else { problems.push(parseResult.error); } diff --git a/tests/lib/eslint/flat-eslint.js b/tests/lib/eslint/flat-eslint.js index 9e0ca12458f..f67e62e5c63 100644 --- a/tests/lib/eslint/flat-eslint.js +++ b/tests/lib/eslint/flat-eslint.js @@ -3531,7 +3531,7 @@ describe("FlatESLint", () => { const messages = results[0].messages; assert.strictEqual(messages.length, 1); - assert.strictEqual(messages[0].message, "'/*globals*/' has no effect because you have 'noInlineConfig' setting in your config."); + assert.strictEqual(messages[0].message, "'/* globals foo */' has no effect because you have 'noInlineConfig' setting in your config."); }); }); diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 8844d27bd9c..ecb15e3ba84 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -11880,7 +11880,7 @@ describe("Linter with FlatConfigArray", () => { assert.strictEqual(messages[0].message, "Unexpected alert."); assert.include(messages[0].nodeType, "CallExpression"); - assert.strictEqual(suppressedMessages.length, 0,"Incorrect suppressed message length"); + assert.strictEqual(suppressedMessages.length, 0, "Incorrect suppressed message length"); }); it("rules should not change initial config", () => { @@ -11968,7 +11968,7 @@ describe("Linter with FlatConfigArray", () => { }); describe("when evaluating code with invalid comments to enable rules", () => { - it.only("should report a violation when the config is not a valid rule configuration", () => { + it("should report a violation when the config is not a valid rule configuration", () => { const messages = linter.verify("/*eslint no-alert:true*/ alert('test');", {}); const suppressedMessages = linter.getSuppressedMessages(); @@ -11978,7 +11978,7 @@ describe("Linter with FlatConfigArray", () => { { severity: 2, ruleId: "no-alert", - message: "Configuration for rule \"no-alert\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed 'true').\n", + message: "Inline configuration for rule \"no-alert\" is invalid:\n\t: Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2. You passed \"true\".\n", line: 1, column: 1, endLine: 1, @@ -12001,7 +12001,7 @@ describe("Linter with FlatConfigArray", () => { { severity: 2, ruleId: "no-alert", - message: "Configuration for rule \"no-alert\" is invalid:\n\tValue [{\"nonExistentPropertyName\":true}] should NOT have more than 0 items.\n", + message: "Inline configuration for rule \"no-alert\" is invalid:\n\tValue [{\"nonExistentPropertyName\":true}] should NOT have more than 0 items.\n", line: 1, column: 1, endLine: 1, @@ -14025,7 +14025,7 @@ var a = "test2"; assert.deepStrictEqual(messages[0].fatal, void 0); assert.deepStrictEqual(messages[0].ruleId, null); assert.deepStrictEqual(messages[0].severity, 1); - assert.deepStrictEqual(messages[0].message, `'/*${directive.split(" ")[0]}*/' has no effect because you have 'noInlineConfig' setting in your config.`); + assert.deepStrictEqual(messages[0].message, `'/* ${directive} */' has no effect because you have 'noInlineConfig' setting in your config.`); assert.strictEqual(suppressedMessages.length, 0); }); @@ -14048,7 +14048,7 @@ var a = "test2"; assert.deepStrictEqual(messages[0].fatal, void 0); assert.deepStrictEqual(messages[0].ruleId, null); assert.deepStrictEqual(messages[0].severity, 1); - assert.deepStrictEqual(messages[0].message, `'//${directive.split(" ")[0]}' has no effect because you have 'noInlineConfig' setting in your config.`); + assert.deepStrictEqual(messages[0].message, `'// ${directive}' has no effect because you have 'noInlineConfig' setting in your config.`); assert.strictEqual(suppressedMessages.length, 0); }); diff --git a/tests/lib/source-code/source-code.js b/tests/lib/source-code/source-code.js index 3ed916dfa8f..eaedbac29f0 100644 --- a/tests/lib/source-code/source-code.js +++ b/tests/lib/source-code/source-code.js @@ -4007,7 +4007,8 @@ describe("SourceCode", () => { const result = sourceCode.applyInlineConfig(); assert.isTrue(result.ok); - assert.strictEqual(result.config.rules["some-rule"], 2); + assert.strictEqual(result.configs.length, 1); + assert.strictEqual(result.configs[0].config.rules["some-rule"], 2); }); it("should extract multiple rule configurations", () => { @@ -4018,8 +4019,22 @@ describe("SourceCode", () => { const result = sourceCode.applyInlineConfig(); assert.isTrue(result.ok); - assert.strictEqual(result.config.rules["some-rule"], 2); - assert.deepStrictEqual(result.config.rules["other-rule"], ["error", { skip: true }]); + assert.strictEqual(result.configs.length, 1); + assert.strictEqual(result.configs[0].config.rules["some-rule"], 2); + assert.deepStrictEqual(result.configs[0].config.rules["other-rule"], ["error", { skip: true }]); + }); + + it("should extract multiple comments into multiple configurations", () => { + + const code = "/*eslint some-rule: 2*/ /*eslint other-rule: [\"error\", { skip: true }] */ var foo;"; + const ast = espree.parse(code, DEFAULT_CONFIG); + const sourceCode = new SourceCode(code, ast); + const result = sourceCode.applyInlineConfig(); + + assert.isTrue(result.ok); + assert.strictEqual(result.configs.length, 2); + assert.strictEqual(result.configs[0].config.rules["some-rule"], 2); + assert.deepStrictEqual(result.configs[1].config.rules["other-rule"], ["error", { skip: true }]); }); it("should report problem with rule configuration parsing", () => { From d579475ff3ad49026dc45229e1b08a81ddba3935 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Thu, 20 Jul 2023 16:58:19 -0400 Subject: [PATCH 09/28] Fix test for Node.js 19 --- tests/lib/source-code/source-code.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/lib/source-code/source-code.js b/tests/lib/source-code/source-code.js index eaedbac29f0..8c17180eda6 100644 --- a/tests/lib/source-code/source-code.js +++ b/tests/lib/source-code/source-code.js @@ -4045,17 +4045,17 @@ describe("SourceCode", () => { const result = sourceCode.applyInlineConfig(); assert.isFalse(result.ok); - assert.deepStrictEqual(result.problems, [ - { - column: 1, - fatal: true, - line: 1, - message: "Failed to parse JSON from ' \"some-rule\"::,': Unexpected token : in JSON at position 14", - nodeType: null, - ruleId: null, - severity: 2 - } - ]); + + const problem = result.problems[0]; + + // Node.js 19 changes the JSON parsing error format, so we need to check each field separately to use a regex + assert.strictEqual(problem.column, 1); + assert.strictEqual(problem.line, 1); + assert.isTrue(problem.fatal); + assert.match(problem.message, /Failed to parse JSON from ' "some-rule"::,': Unexpected token '?:'?/u); + assert.isNull(problem.nodeType); + assert.isNull(problem.ruleId); + assert.strictEqual(problem.severity, 2); }); }); }); From e3e02821b286b0f183729b1c0c75fb62c0c0aed0 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Thu, 20 Jul 2023 17:05:48 -0400 Subject: [PATCH 10/28] Forgot to save the file --- lib/config/flat-config-schema.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/config/flat-config-schema.js b/lib/config/flat-config-schema.js index d06cb5a8700..a79c02d663b 100644 --- a/lib/config/flat-config-schema.js +++ b/lib/config/flat-config-schema.js @@ -507,7 +507,7 @@ const eslintrcKeys = [ // Full schema //----------------------------------------------------------------------------- -exports.flatConfigSchema = { +const flatConfigSchema = { // eslintrc-style keys that should always error ...Object.fromEntries(eslintrcKeys.map(key => [key, createEslintrcErrorSchema(key)])), From 10b5ecb6790a8548f2405a3f3fff2a87ff896364 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Wed, 30 Aug 2023 14:52:31 -0400 Subject: [PATCH 11/28] Remove proxy; update RuleTester/FlatRuleTester --- lib/linter/linter.js | 29 +------------- lib/rule-tester/flat-rule-tester.js | 48 ++++++++++++++++++++--- lib/rule-tester/rule-tester.js | 28 ++++++++++++- tests/lib/rule-tester/flat-rule-tester.js | 44 +++++++++++++++++++++ tests/lib/rule-tester/rule-tester.js | 44 +++++++++++++++++++++ 5 files changed, 160 insertions(+), 33 deletions(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 068b0b73d6d..e4483d5fc91 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -685,29 +685,6 @@ function findEslintEnv(text) { return retv; } -const disallowedSourceCodeMethods = new Set(["applyLanguageOptions", "applyInlineConfig", "finalize"]); - -/** - * Creates a proxy object for a SourceCode object to ensure that rules can't - * call methods intended to be used by the core only. - * @param {SourceCode} sourceCode The SourceCode object to wrap. - * @returns {Proxy} The proxy object for the SourceCode. - */ -function createSourceCodeProxy(sourceCode) { - return new Proxy(sourceCode, { - get(target, key) { - if (disallowedSourceCodeMethods.has(key)) { - throw new TypeError("This method cannot be called inside of a rule."); - } - - return target[key]; - }, - set() { - return false; - } - }); -} - /** * Convert "/path/to/" to "". * `CLIEngine#executeOnText()` method gives "/path/to/" if the filename @@ -1445,11 +1422,10 @@ class Linter { const configuredRules = Object.assign({}, config.rules, commentDirectives.configuredRules); let lintingProblems; - const sourceCodeProxy = createSourceCodeProxy(sourceCode); try { lintingProblems = runRules( - sourceCodeProxy, + sourceCode, configuredRules, ruleId => getRule(slots, ruleId), parserName, @@ -1817,11 +1793,10 @@ class Linter { let lintingProblems; sourceCode.finalize(); - const sourceCodeProxy = createSourceCodeProxy(sourceCode); try { lintingProblems = runRules( - sourceCodeProxy, + sourceCode, configuredRules, ruleId => getRuleFromConfig(ruleId, config), void 0, diff --git a/lib/rule-tester/flat-rule-tester.js b/lib/rule-tester/flat-rule-tester.js index d5f5981e67e..b695571ade9 100644 --- a/lib/rule-tester/flat-rule-tester.js +++ b/lib/rule-tester/flat-rule-tester.js @@ -133,6 +133,12 @@ const suggestionObjectParameters = new Set([ ]); const friendlySuggestionObjectParameterList = `[${[...suggestionObjectParameters].map(key => `'${key}'`).join(", ")}]`; +const forbiddenMethods = [ + "applyInlineConfig", + "applyLanguageOptions", + "finalize" +]; + const hasOwnProperty = Function.call.bind(Object.hasOwnProperty); /** @@ -291,6 +297,31 @@ function emitCodePathCurrentSegmentsWarning(ruleName) { } } +/** + * Function to replace forbidden `SourceCode` methods. Allows just one call per method. + * @param {string} methodName The name of the method to forbid. + * @param {Function} prototype The prototype with the original method to call. + * @returns {Function} The function that throws the error. + */ +function throwForbiddenMethodError(methodName, prototype) { + let called = false; + const original = prototype[methodName]; + + return function(...args) { + + if (!called) { + called = true; + + /* eslint-disable-next-line no-invalid-this -- needed to operate as a method. */ + return original.apply(this, args); + } + + throw new Error( + `\`SourceCode#${methodName}()\` cannot be called inside a rule.` + ); + }; +} + //------------------------------------------------------------------------------ // Public Interface //------------------------------------------------------------------------------ @@ -679,11 +710,6 @@ class FlatRuleTester { } } - // Verify the code. - const { getComments } = SourceCode.prototype; - const originalCurrentSegments = Object.getOwnPropertyDescriptor(CodePath.prototype, "currentSegments"); - let messages; - // check for validation errors try { configs.normalizeSync(); @@ -693,6 +719,11 @@ class FlatRuleTester { throw error; } + // Verify the code. + const { getComments, applyLanguageOptions, applyInlineConfig, finalize } = SourceCode.prototype; + const originalCurrentSegments = Object.getOwnPropertyDescriptor(CodePath.prototype, "currentSegments"); + let messages; + try { SourceCode.prototype.getComments = getCommentsDeprecation; Object.defineProperty(CodePath.prototype, "currentSegments", { @@ -702,10 +733,17 @@ class FlatRuleTester { } }); + forbiddenMethods.forEach(methodName => { + SourceCode.prototype[methodName] = throwForbiddenMethodError(methodName, SourceCode.prototype); + }); + messages = linter.verify(code, configs, filename); } finally { SourceCode.prototype.getComments = getComments; Object.defineProperty(CodePath.prototype, "currentSegments", originalCurrentSegments); + SourceCode.prototype.applyInlineConfig = applyInlineConfig; + SourceCode.prototype.applyLanguageOptions = applyLanguageOptions; + SourceCode.prototype.finalize = finalize; } diff --git a/lib/rule-tester/rule-tester.js b/lib/rule-tester/rule-tester.js index 82d79790a31..fbabdb562f6 100644 --- a/lib/rule-tester/rule-tester.js +++ b/lib/rule-tester/rule-tester.js @@ -163,6 +163,12 @@ const suggestionObjectParameters = new Set([ ]); const friendlySuggestionObjectParameterList = `[${[...suggestionObjectParameters].map(key => `'${key}'`).join(", ")}]`; +const forbiddenMethods = [ + "applyInlineConfig", + "applyLanguageOptions", + "finalize" +]; + const hasOwnProperty = Function.call.bind(Object.hasOwnProperty); const DEPRECATED_SOURCECODE_PASSTHROUGHS = { @@ -330,6 +336,19 @@ function getCommentsDeprecation() { ); } +/** + * Function to replace forbidden `SourceCode` methods. + * @param {string} methodName The name of the method to forbid. + * @returns {Function} The function that throws the error. + */ +function throwForbiddenMethodError(methodName) { + return () => { + throw new Error( + `\`SourceCode#${methodName}()\` cannot be called inside a rule.` + ); + }; +} + /** * Emit a deprecation warning if function-style format is being used. * @param {string} ruleName Name of the rule. @@ -761,7 +780,7 @@ class RuleTester { validate(config, "rule-tester", id => (id === ruleName ? rule : null)); // Verify the code. - const { getComments } = SourceCode.prototype; + const { getComments, applyLanguageOptions, applyInlineConfig, finalize } = SourceCode.prototype; const originalCurrentSegments = Object.getOwnPropertyDescriptor(CodePath.prototype, "currentSegments"); let messages; @@ -774,10 +793,17 @@ class RuleTester { } }); + forbiddenMethods.forEach(methodName => { + SourceCode.prototype[methodName] = throwForbiddenMethodError(methodName); + }); + messages = linter.verify(code, config, filename); } finally { SourceCode.prototype.getComments = getComments; Object.defineProperty(CodePath.prototype, "currentSegments", originalCurrentSegments); + SourceCode.prototype.applyInlineConfig = applyInlineConfig; + SourceCode.prototype.applyLanguageOptions = applyLanguageOptions; + SourceCode.prototype.finalize = finalize; } const fatalErrorMessage = messages.find(m => m.fatal); diff --git a/tests/lib/rule-tester/flat-rule-tester.js b/tests/lib/rule-tester/flat-rule-tester.js index 3e34c4c34bd..2285f446a0d 100644 --- a/tests/lib/rule-tester/flat-rule-tester.js +++ b/tests/lib/rule-tester/flat-rule-tester.js @@ -2620,6 +2620,50 @@ describe("FlatRuleTester", () => { }); }); + describe("SourceCode forbidden methods", () => { + + [ + "applyInlineConfig", + "applyLanguageOptions", + "finalize" + ].forEach(methodName => { + + const useForbiddenMethodRule = { + create: context => ({ + Program() { + const sourceCode = context.sourceCode; + + sourceCode[methodName](); + } + }) + }; + + it(`should throw if ${methodName} is called from a valid test case`, () => { + assert.throws(() => { + ruleTester.run("use-get-comments", useForbiddenMethodRule, { + valid: [""], + invalid: [] + }); + }, `\`SourceCode#${methodName}()\` cannot be called inside a rule.`); + }); + + it(`should throw if ${methodName} is called from an invalid test case`, () => { + assert.throws(() => { + ruleTester.run("use-get-comments", useForbiddenMethodRule, { + valid: [], + invalid: [{ + code: "", + errors: [{}] + }] + }); + }, `\`SourceCode#${methodName}()\` cannot be called inside a rule.`); + }); + + }); + + }); + + describe("Subclassing", () => { it("should allow subclasses to set the describe/it/itOnly statics and should correctly use those values", () => { const assertionDescribe = assertEmitted(ruleTesterTestEmitter, "custom describe", "this-is-a-rule-name"); diff --git a/tests/lib/rule-tester/rule-tester.js b/tests/lib/rule-tester/rule-tester.js index 68cf887adb9..389ef7646c7 100644 --- a/tests/lib/rule-tester/rule-tester.js +++ b/tests/lib/rule-tester/rule-tester.js @@ -2915,6 +2915,50 @@ describe("RuleTester", () => { }); }); + + describe("SourceCode forbidden methods", () => { + + [ + "applyInlineConfig", + "applyLanguageOptions", + "finalize" + ].forEach(methodName => { + + const useForbiddenMethodRule = { + create: context => ({ + Program() { + const sourceCode = context.sourceCode; + + sourceCode[methodName](); + } + }) + }; + + it(`should throw if ${methodName} is called from a valid test case`, () => { + assert.throws(() => { + ruleTester.run("use-get-comments", useForbiddenMethodRule, { + valid: [""], + invalid: [] + }); + }, `\`SourceCode#${methodName}()\` cannot be called inside a rule.`); + }); + + it(`should throw if ${methodName} is called from an invalid test case`, () => { + assert.throws(() => { + ruleTester.run("use-get-comments", useForbiddenMethodRule, { + valid: [], + invalid: [{ + code: "", + errors: [{}] + }] + }); + }, `\`SourceCode#${methodName}()\` cannot be called inside a rule.`); + }); + + }); + + }); + describe("Subclassing", () => { it("should allow subclasses to set the describe/it/itOnly statics and should correctly use those values", () => { From 81b75ebf19f6d2ac44e2c3810b9d0d0a02a36f54 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Wed, 6 Sep 2023 14:57:13 -0400 Subject: [PATCH 12/28] Clean up --- tests/lib/linter/linter.js | 46 ----------------------- tests/lib/rule-tester/flat-rule-tester.js | 44 ---------------------- 2 files changed, 90 deletions(-) diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index ecb15e3ba84..e0a36096b84 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -16257,52 +16257,6 @@ var a = "test2"; assert(spy.calledOnce); }); - - describe("Disallowed SourceCode methods inside a rule", () => { - [ - "applyLanguageOptions", - "applyInlineConfig", - "finalize" - ].forEach(methodName => { - - it(`should throw error when accessing sourceCode.${methodName} inside a rule`, () => { - let spy; - const config = { - plugins: { - test: { - rules: { - checker: { - create(context) { - spy = sinon.spy(() => { - const sourceCode = context.sourceCode; - - assert.throws(() => { - sourceCode[methodName](); - }, /This method cannot be called inside of a rule/u); - }); - - return { Program: spy }; - } - } - } - } - }, - languageOptions: { - sourceType: "script" - }, - rules: { - "test/checker": "error" - } - }; - - linter.verify("foo", config); - assert(spy && spy.calledOnce); - }); - }); - - - }); - describe("when evaluating an empty string", () => { it("runs rules", () => { diff --git a/tests/lib/rule-tester/flat-rule-tester.js b/tests/lib/rule-tester/flat-rule-tester.js index 2285f446a0d..3e34c4c34bd 100644 --- a/tests/lib/rule-tester/flat-rule-tester.js +++ b/tests/lib/rule-tester/flat-rule-tester.js @@ -2620,50 +2620,6 @@ describe("FlatRuleTester", () => { }); }); - describe("SourceCode forbidden methods", () => { - - [ - "applyInlineConfig", - "applyLanguageOptions", - "finalize" - ].forEach(methodName => { - - const useForbiddenMethodRule = { - create: context => ({ - Program() { - const sourceCode = context.sourceCode; - - sourceCode[methodName](); - } - }) - }; - - it(`should throw if ${methodName} is called from a valid test case`, () => { - assert.throws(() => { - ruleTester.run("use-get-comments", useForbiddenMethodRule, { - valid: [""], - invalid: [] - }); - }, `\`SourceCode#${methodName}()\` cannot be called inside a rule.`); - }); - - it(`should throw if ${methodName} is called from an invalid test case`, () => { - assert.throws(() => { - ruleTester.run("use-get-comments", useForbiddenMethodRule, { - valid: [], - invalid: [{ - code: "", - errors: [{}] - }] - }); - }, `\`SourceCode#${methodName}()\` cannot be called inside a rule.`); - }); - - }); - - }); - - describe("Subclassing", () => { it("should allow subclasses to set the describe/it/itOnly statics and should correctly use those values", () => { const assertionDescribe = assertEmitted(ruleTesterTestEmitter, "custom describe", "this-is-a-rule-name"); From 24d417ac0a6be5cfb5f3a5d68ef4abc0ca820619 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Mon, 11 Sep 2023 11:42:05 -0400 Subject: [PATCH 13/28] Use WeakSet to track method calls --- lib/rule-tester/flat-rule-tester.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/rule-tester/flat-rule-tester.js b/lib/rule-tester/flat-rule-tester.js index b695571ade9..300bcb7447e 100644 --- a/lib/rule-tester/flat-rule-tester.js +++ b/lib/rule-tester/flat-rule-tester.js @@ -139,6 +139,9 @@ const forbiddenMethods = [ "finalize" ]; +/** @type {Map} */ +const forbiddenMethodCalls = new Map(forbiddenMethods.map(methodName => ([methodName, new WeakSet()]))); + const hasOwnProperty = Function.call.bind(Object.hasOwnProperty); /** @@ -304,17 +307,20 @@ function emitCodePathCurrentSegmentsWarning(ruleName) { * @returns {Function} The function that throws the error. */ function throwForbiddenMethodError(methodName, prototype) { - let called = false; + const original = prototype[methodName]; return function(...args) { - if (!called) { - called = true; + const called = forbiddenMethodCalls.get(methodName); + + /* eslint-disable no-invalid-this -- needed to operate as a method. */ + if (!called.has(this)) { + called.add(this); - /* eslint-disable-next-line no-invalid-this -- needed to operate as a method. */ return original.apply(this, args); } + /* eslint-enable no-invalid-this -- not needed past this point */ throw new Error( `\`SourceCode#${methodName}()\` cannot be called inside a rule.` From c681c1140e371d0f7600129435b6225b71c931cf Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Wed, 13 Sep 2023 10:48:54 -0400 Subject: [PATCH 14/28] Update tests/lib/rule-tester/rule-tester.js Co-authored-by: Milos Djermanovic --- tests/lib/rule-tester/rule-tester.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/rule-tester/rule-tester.js b/tests/lib/rule-tester/rule-tester.js index 389ef7646c7..841b79bf7d0 100644 --- a/tests/lib/rule-tester/rule-tester.js +++ b/tests/lib/rule-tester/rule-tester.js @@ -2936,7 +2936,7 @@ describe("RuleTester", () => { it(`should throw if ${methodName} is called from a valid test case`, () => { assert.throws(() => { - ruleTester.run("use-get-comments", useForbiddenMethodRule, { + ruleTester.run("use-forbidden-method", useForbiddenMethodRule, { valid: [""], invalid: [] }); From 1af148b9eedb4e9cd356a7dd09b1dda07d0e6d89 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Wed, 13 Sep 2023 10:49:06 -0400 Subject: [PATCH 15/28] Update tests/lib/rule-tester/rule-tester.js Co-authored-by: Milos Djermanovic --- tests/lib/rule-tester/rule-tester.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/rule-tester/rule-tester.js b/tests/lib/rule-tester/rule-tester.js index 841b79bf7d0..6dc811bb468 100644 --- a/tests/lib/rule-tester/rule-tester.js +++ b/tests/lib/rule-tester/rule-tester.js @@ -2945,7 +2945,7 @@ describe("RuleTester", () => { it(`should throw if ${methodName} is called from an invalid test case`, () => { assert.throws(() => { - ruleTester.run("use-get-comments", useForbiddenMethodRule, { + ruleTester.run("use-forbidden-method", useForbiddenMethodRule, { valid: [], invalid: [{ code: "", From b4ba86d8c5b7af2a1632ed72e861811598ddb0b8 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Wed, 13 Sep 2023 10:52:25 -0400 Subject: [PATCH 16/28] Re-add tests for FlatRuleTester --- tests/lib/rule-tester/flat-rule-tester.js | 43 +++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/lib/rule-tester/flat-rule-tester.js b/tests/lib/rule-tester/flat-rule-tester.js index 3e34c4c34bd..a3be696d0a2 100644 --- a/tests/lib/rule-tester/flat-rule-tester.js +++ b/tests/lib/rule-tester/flat-rule-tester.js @@ -2620,6 +2620,49 @@ describe("FlatRuleTester", () => { }); }); + describe("SourceCode forbidden methods", () => { + + [ + "applyInlineConfig", + "applyLanguageOptions", + "finalize" + ].forEach(methodName => { + + const useForbiddenMethodRule = { + create: context => ({ + Program() { + const sourceCode = context.sourceCode; + + sourceCode[methodName](); + } + }) + }; + + it(`should throw if ${methodName} is called from a valid test case`, () => { + assert.throws(() => { + ruleTester.run("use-forbidden-method", useForbiddenMethodRule, { + valid: [""], + invalid: [] + }); + }, `\`SourceCode#${methodName}()\` cannot be called inside a rule.`); + }); + + it(`should throw if ${methodName} is called from an invalid test case`, () => { + assert.throws(() => { + ruleTester.run("use-forbidden-method", useForbiddenMethodRule, { + valid: [], + invalid: [{ + code: "", + errors: [{}] + }] + }); + }, `\`SourceCode#${methodName}()\` cannot be called inside a rule.`); + }); + + }); + + }); + describe("Subclassing", () => { it("should allow subclasses to set the describe/it/itOnly statics and should correctly use those values", () => { const assertionDescribe = assertEmitted(ruleTesterTestEmitter, "custom describe", "this-is-a-rule-name"); From 2180336b8159b541f1fd9512a0e557ebf113fa38 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Fri, 15 Sep 2023 12:16:49 -0400 Subject: [PATCH 17/28] Apply feedback --- lib/linter/linter.js | 59 +----------- tests/lib/linter/linter.js | 188 +++++++++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+), 57 deletions(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index e4483d5fc91..b8fb5334f19 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -494,13 +494,10 @@ function getDirectiveComments(sourceCode, ruleMapper, warnInlineConfig) { * where reporting is disabled or enabled and merges them with reporting config. * @param {SourceCode} sourceCode The SourceCode object to get comments from. * @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules - * @returns {{configuredRules: Object, enabledGlobals: {value:string,comment:Token}[], exportedVariables: Object, problems: LintMessage[], disableDirectives: DisableDirective[]}} + * @returns {{problems: LintMessage[], disableDirectives: DisableDirective[]}} * A collection of the directive comments that were found, along with any problems that occurred when parsing */ function getDirectiveCommentsForFlatConfig(sourceCode, ruleMapper) { - const configuredRules = {}; - const enabledGlobals = Object.create(null); - const exportedVariables = {}; const problems = []; const disableDirectives = []; @@ -546,63 +543,11 @@ function getDirectiveCommentsForFlatConfig(sourceCode, ruleMapper) { break; } - /* - * case "eslint": { - * const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc); - */ - - /* - * if (parseResult.success) { - * Object.keys(parseResult.config).forEach(name => { - * const rule = ruleMapper(name); - * const ruleValue = parseResult.config[name]; - */ - - /* - * if (!rule) { - * problems.push(createLintingProblem({ ruleId: name, loc: comment.loc })); - * return; - * } - */ - - /* - * try { - * validator.validateRuleOptions(rule, name, ruleValue); - * } catch (err) { - * problems.push(createLintingProblem({ - * ruleId: name, - * message: err.message, - * loc: comment.loc - * })); - */ - - /* - * // do not apply the config, if found invalid options. - * return; - * } - */ - - /* - * configuredRules[name] = ruleValue; - * }); - * } else { - * problems.push(parseResult.error); - * } - */ - - /* - * break; - * } - */ - // no default } }); return { - configuredRules, - enabledGlobals, - exportedVariables, problems, disableDirectives }; @@ -1782,7 +1727,7 @@ class Linter { } } - const commentDirectives = options.allowInlineConfig + const commentDirectives = options.allowInlineConfig && !options.warnInlineConfig ? getDirectiveCommentsForFlatConfig( sourceCode, ruleId => getRuleFromConfig(ruleId, config) diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index e0a36096b84..02eb709f4f9 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -2023,6 +2023,46 @@ describe("Linter", () => { assert.strictEqual(suppressedMessages.length, 0); }); + + it("should apply valid configuration even if there is an invalid configuration present", () => { + const code = [ + "/* eslint no-unused-vars: [ */ // <-- this one is invalid JSON", + "/* eslint no-undef: [\"error\"] */ // <-- this one is fine, and thus should apply", + "foo(); // <-- expected no-undef error here" + ].join("\n"); + + const messages = linter.verify(code); + const suppressedMessages = linter.getSuppressedMessages(); + + assert.deepStrictEqual( + messages, + [ + { + severity: 2, + fatal: true, + ruleId: null, + message: "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token } in JSON at position 21", + line: 1, + column: 1, + nodeType: null + }, + { + severity: 2, + ruleId: "no-undef", + message: "'foo' is not defined.", + messageId: "undef", + line: 3, + column: 1, + endLine: 3, + endColumn: 4, + nodeType: "Identifier" + } + ] + ); + + assert.strictEqual(suppressedMessages.length, 0); + }); + }); describe("when evaluating code with comments to disable rules", () => { @@ -4779,6 +4819,59 @@ var a = "test2"; }); }); + describe("warnInlineConfig option", () => { + + it("should report both a rule violation and a warning about inline config", () => { + const code = [ + "/* eslint-disable */ // <-- this should be inline config warning", + "foo(); // <-- this should be no-undef error" + ].join("\n"); + const config = { + rules: { + "no-undef": 2 + }, + noInlineConfig: true + }; + + const messages = linter.verify(code, config, { + filename, + warnInlineConfig: true + }); + const suppressedMessages = linter.getSuppressedMessages(); + + assert.strictEqual(messages.length, 2); + assert.deepStrictEqual( + messages, + [ + { + ruleId: null, + message: "'/*eslint-disable*/' has no effect because you have 'noInlineConfig' setting in your config.", + line: 1, + column: 1, + endLine: 1, + endColumn: 21, + severity: 1, + nodeType: null + }, + { + ruleId: "no-undef", + messageId: "undef", + message: "'foo' is not defined.", + line: 2, + endLine: 2, + column: 1, + endColumn: 4, + severity: 2, + nodeType: "Identifier" + } + ] + ); + + assert.strictEqual(suppressedMessages.length, 0); + }); + }); + + describe("when evaluating code with comments to change config when allowInlineConfig is disabled", () => { it("should not report a violation", () => { const code = [ @@ -12013,6 +12106,46 @@ describe("Linter with FlatConfigArray", () => { assert.strictEqual(suppressedMessages.length, 0); }); + + it("should apply valid configuration even if there is an invalid configuration present", () => { + const code = [ + "/* eslint no-unused-vars: [ */ // <-- this one is invalid JSON", + "/* eslint no-undef: [\"error\"] */ // <-- this one is fine, and thus should apply", + "foo(); // <-- expected no-undef error here" + ].join("\n"); + + const messages = linter.verify(code); + const suppressedMessages = linter.getSuppressedMessages(); + + assert.deepStrictEqual( + messages, + [ + { + severity: 2, + fatal: true, + ruleId: null, + message: "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token } in JSON at position 21", + line: 1, + column: 1, + nodeType: null + }, + { + severity: 2, + ruleId: "no-undef", + message: "'foo' is not defined.", + messageId: "undef", + line: 3, + column: 1, + endLine: 3, + endColumn: 4, + nodeType: "Identifier" + } + ] + ); + + assert.strictEqual(suppressedMessages.length, 0); + }); + }); describe("when evaluating code with comments to disable rules", () => { @@ -14093,6 +14226,61 @@ var a = "test2"; }); + + describe("warnInlineConfig option", () => { + + it("should report both a rule violation and a warning about inline config", () => { + const code = [ + "/* eslint-disable */ // <-- this should be inline config warning", + "foo(); // <-- this should be no-undef error" + ].join("\n"); + const config = { + rules: { + "no-undef": 2 + }, + linterOptions: { + noInlineConfig: true + } + }; + + const messages = linter.verify(code, config, { + filename, + warnInlineConfig: true + }); + const suppressedMessages = linter.getSuppressedMessages(); + + assert.strictEqual(messages.length, 2); + assert.deepStrictEqual( + messages, + [ + { + ruleId: null, + message: "'/* eslint-disable */' has no effect because you have 'noInlineConfig' setting in your config.", + line: 1, + column: 1, + endLine: 1, + endColumn: 21, + severity: 1, + nodeType: null + }, + { + ruleId: "no-undef", + messageId: "undef", + message: "'foo' is not defined.", + line: 2, + endLine: 2, + column: 1, + endColumn: 4, + severity: 2, + nodeType: "Identifier" + } + ] + ); + + assert.strictEqual(suppressedMessages.length, 0); + }); + }); + describe("reportUnusedDisableDirectives option", () => { it("reports problems for unused eslint-disable comments", () => { const messages = linter.verify("/* eslint-disable */", {}, { reportUnusedDisableDirectives: true }); From 271f8babd1d10814eeece328b0dcc4f7141a61d6 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Mon, 18 Sep 2023 17:33:45 -0400 Subject: [PATCH 18/28] Cleanup tests --- tests/lib/linter/linter.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 02eb709f4f9..350c8ce1d45 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -4819,7 +4819,7 @@ var a = "test2"; }); }); - describe("warnInlineConfig option", () => { + describe("config.noInlineConfig + options.allowInlineConfig", () => { it("should report both a rule violation and a warning about inline config", () => { const code = [ @@ -4835,7 +4835,7 @@ var a = "test2"; const messages = linter.verify(code, config, { filename, - warnInlineConfig: true + allowInlineConfig: true }); const suppressedMessages = linter.getSuppressedMessages(); @@ -14227,7 +14227,7 @@ var a = "test2"; }); - describe("warnInlineConfig option", () => { + describe("config.noInlineConfig + options.allowInlineConfig", () => { it("should report both a rule violation and a warning about inline config", () => { const code = [ @@ -14245,7 +14245,7 @@ var a = "test2"; const messages = linter.verify(code, config, { filename, - warnInlineConfig: true + allowInlineConfig: true }); const suppressedMessages = linter.getSuppressedMessages(); From 0fc3fe66ed5ec2b22717306e505db86a234709a8 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Tue, 19 Sep 2023 11:35:29 -0400 Subject: [PATCH 19/28] Update lib/source-code/source-code.js Co-authored-by: Milos Djermanovic --- lib/source-code/source-code.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/source-code/source-code.js b/lib/source-code/source-code.js index 9735201c86f..dbd8b177d66 100644 --- a/lib/source-code/source-code.js +++ b/lib/source-code/source-code.js @@ -245,7 +245,7 @@ function addDeclaredGlobals(globalScope, configGlobals = {}, inlineGlobals = {}) for (const id of new Set([...Object.keys(configGlobals), ...Object.keys(inlineGlobals)])) { /* - * `ConfigOps.normalizeConfigGlobal` will throw an error if a configured global value is invalid. However, these errors would + * `normalizeConfigGlobal` will throw an error if a configured global value is invalid. However, these errors would * typically be caught when validating a config anyway (validity for inline global comments is checked separately). */ const configValue = configGlobals[id] === void 0 ? void 0 : normalizeConfigGlobal(configGlobals[id]); From 645265d6b41a4d66ce9431f3f5f1a48ea82bc107 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Tue, 19 Sep 2023 11:38:45 -0400 Subject: [PATCH 20/28] Update lib/linter/linter.js Co-authored-by: Milos Djermanovic --- lib/linter/linter.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index b8fb5334f19..40b7215230d 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -1732,9 +1732,9 @@ class Linter { sourceCode, ruleId => getRuleFromConfig(ruleId, config) ) - : { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] }; + : { problems: [], disableDirectives: [] }; - const configuredRules = Object.assign({}, config.rules, mergedInlineConfig.rules, commentDirectives.configuredRules); + const configuredRules = Object.assign({}, config.rules, mergedInlineConfig.rules); let lintingProblems; sourceCode.finalize(); From 41f91df5d64a8864e304e9421e2987b323ac2e60 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Tue, 19 Sep 2023 11:47:56 -0400 Subject: [PATCH 21/28] Fix inline config problems in flat config mode --- lib/linter/linter.js | 86 ++++++++++++++++++++------------------ tests/lib/linter/linter.js | 5 ++- 2 files changed, 49 insertions(+), 42 deletions(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 40b7215230d..b33cd672cd8 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -1671,58 +1671,62 @@ class Linter { } else { const inlineConfigResult = sourceCode.applyInlineConfig(); - inlineConfigProblems.push(...inlineConfigResult.problems.map(createLintingProblem)); - - // need to verify rule existence options - if (inlineConfigResult.ok) { - - const ruleValidator = new RuleValidator(); - - for (const { config: inlineConfig, node } of inlineConfigResult.configs) { + inlineConfigProblems.push( + ...inlineConfigResult.problems + .map(createLintingProblem) + .map(problem => { + problem.fatal = true; + return problem; + }) + ); - Object.keys(inlineConfig.rules).forEach(ruleId => { - const rule = getRuleFromConfig(ruleId, config); - const ruleValue = inlineConfig.rules[ruleId]; + // next we need to verify information about the specified rules + const ruleValidator = new RuleValidator(); - if (!rule) { - inlineConfigProblems.push(createLintingProblem({ ruleId, loc: node.loc })); - return; - } + for (const { config: inlineConfig, node } of inlineConfigResult.configs) { - try { + Object.keys(inlineConfig.rules).forEach(ruleId => { + const rule = getRuleFromConfig(ruleId, config); + const ruleValue = inlineConfig.rules[ruleId]; - const ruleOptions = Array.isArray(ruleValue) ? ruleValue : [ruleValue]; + if (!rule) { + inlineConfigProblems.push(createLintingProblem({ ruleId, loc: node.loc })); + return; + } - assertIsRuleOptions(ruleId, ruleValue); - assertIsRuleSeverity(ruleId, ruleOptions[0]); + try { - ruleValidator.validate({ - plugins: config.plugins, - rules: { - [ruleId]: ruleOptions - } - }); - mergedInlineConfig.rules[ruleId] = ruleValue; - } catch (err) { + const ruleOptions = Array.isArray(ruleValue) ? ruleValue : [ruleValue]; - let baseMessage = err.message.slice( - err.message.startsWith("Key \"rules\":") - ? err.message.indexOf(":", 12) + 1 - : err.message.indexOf(":") - ).trim(); + assertIsRuleOptions(ruleId, ruleValue); + assertIsRuleSeverity(ruleId, ruleOptions[0]); - if (err.messageTemplate) { - baseMessage += ` You passed "${ruleValue}".`; + ruleValidator.validate({ + plugins: config.plugins, + rules: { + [ruleId]: ruleOptions } + }); + mergedInlineConfig.rules[ruleId] = ruleValue; + } catch (err) { + + let baseMessage = err.message.slice( + err.message.startsWith("Key \"rules\":") + ? err.message.indexOf(":", 12) + 1 + : err.message.indexOf(":") + ).trim(); - inlineConfigProblems.push(createLintingProblem({ - ruleId, - message: `Inline configuration for rule "${ruleId}" is invalid:\n\t${baseMessage}\n`, - loc: node.loc - })); + if (err.messageTemplate) { + baseMessage += ` You passed "${ruleValue}".`; } - }); - } + + inlineConfigProblems.push(createLintingProblem({ + ruleId, + message: `Inline configuration for rule "${ruleId}" is invalid:\n\t${baseMessage}\n`, + loc: node.loc + })); + } + }); } } } diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 350c8ce1d45..1ae60c0b11b 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -11766,6 +11766,7 @@ describe("Linter with FlatConfigArray", () => { column: 1, endLine: 1, endColumn: 39, + fatal: true, nodeType: null }, { @@ -12114,7 +12115,7 @@ describe("Linter with FlatConfigArray", () => { "foo(); // <-- expected no-undef error here" ].join("\n"); - const messages = linter.verify(code); + const messages = linter.verify(code, {}); const suppressedMessages = linter.getSuppressedMessages(); assert.deepStrictEqual( @@ -12127,6 +12128,8 @@ describe("Linter with FlatConfigArray", () => { message: "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token } in JSON at position 21", line: 1, column: 1, + endLine: 1, + endColumn: 2, nodeType: null }, { From 6b8e6566827d0c1ad18052d86970f40c52869df5 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Tue, 19 Sep 2023 12:09:12 -0400 Subject: [PATCH 22/28] Update JSON parse error message --- tests/lib/linter/linter.js | 280 ++++++++++++++++++++++++++++++++++++- 1 file changed, 278 insertions(+), 2 deletions(-) diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 1ae60c0b11b..845d956c97d 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -2031,6 +2031,18 @@ describe("Linter", () => { "foo(); // <-- expected no-undef error here" ].join("\n"); + + // Node.js changed its JSON parsing error message around Node.js v19, so check for that + let parseErrorMessage = "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token } in JSON at position 21"; + + try { + JSON.parse("{'}"); + } catch (error) { + if (error.message.includes("is not valid JSON")) { + parseErrorMessage = "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token '}', ...\"d-vars\": [}\" is not valid JSON"; + } + } + const messages = linter.verify(code); const suppressedMessages = linter.getSuppressedMessages(); @@ -2041,7 +2053,7 @@ describe("Linter", () => { severity: 2, fatal: true, ruleId: null, - message: "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token } in JSON at position 21", + message: parseErrorMessage, line: 1, column: 1, nodeType: null @@ -4869,6 +4881,129 @@ var a = "test2"; assert.strictEqual(suppressedMessages.length, 0); }); + + it("should report both a rule violation without warning about inline config when noInlineConfig is true and allowInlineConfig is false", () => { + const code = [ + "/* eslint-disable */ // <-- this should be inline config warning", + "foo(); // <-- this should be no-undef error" + ].join("\n"); + const config = { + rules: { + "no-undef": 2 + }, + noInlineConfig: true + }; + + const messages = linter.verify(code, config, { + filename, + allowInlineConfig: false + }); + const suppressedMessages = linter.getSuppressedMessages(); + + assert.strictEqual(messages.length, 1); + assert.deepStrictEqual( + messages, + [ + { + ruleId: "no-undef", + messageId: "undef", + message: "'foo' is not defined.", + line: 2, + endLine: 2, + column: 1, + endColumn: 4, + severity: 2, + nodeType: "Identifier" + } + ] + ); + + assert.strictEqual(suppressedMessages.length, 0); + }); + + it("should report both a rule violation without warning about inline config when both are false", () => { + const code = [ + "/* eslint-disable */ // <-- this should be inline config warning", + "foo(); // <-- this should be no-undef error" + ].join("\n"); + const config = { + rules: { + "no-undef": 2 + }, + noInlineConfig: false + }; + + const messages = linter.verify(code, config, { + filename, + allowInlineConfig: false + }); + const suppressedMessages = linter.getSuppressedMessages(); + + assert.strictEqual(messages.length, 1); + assert.deepStrictEqual( + messages, + [ + { + ruleId: "no-undef", + messageId: "undef", + message: "'foo' is not defined.", + line: 2, + endLine: 2, + column: 1, + endColumn: 4, + severity: 2, + nodeType: "Identifier" + } + ] + ); + + assert.strictEqual(suppressedMessages.length, 0); + }); + + it("should report one suppresed problem when noInlineConfig is false and allowInlineConfig is true", () => { + const code = [ + "/* eslint-disable */ // <-- this should be inline config warning", + "foo(); // <-- this should be no-undef error" + ].join("\n"); + const config = { + rules: { + "no-undef": 2 + }, + noInlineConfig: false + }; + + const messages = linter.verify(code, config, { + filename, + allowInlineConfig: true + }); + const suppressedMessages = linter.getSuppressedMessages(); + + assert.strictEqual(messages.length, 0); + assert.strictEqual(suppressedMessages.length, 1); + assert.deepStrictEqual( + suppressedMessages, + [ + { + ruleId: "no-undef", + messageId: "undef", + message: "'foo' is not defined.", + line: 2, + endLine: 2, + column: 1, + endColumn: 4, + severity: 2, + nodeType: "Identifier", + suppressions: [ + { + justification: "", + kind: "directive" + } + ] + } + ] + ); + + }); }); @@ -12118,6 +12253,17 @@ describe("Linter with FlatConfigArray", () => { const messages = linter.verify(code, {}); const suppressedMessages = linter.getSuppressedMessages(); + // Node.js changed its JSON parsing error message around Node.js v19, so check for that + let parseErrorMessage = "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token } in JSON at position 21"; + + try { + JSON.parse("{'}"); + } catch (error) { + if (error.message.includes("is not valid JSON")) { + parseErrorMessage = "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token '}', ...\"d-vars\": [}\" is not valid JSON"; + } + } + assert.deepStrictEqual( messages, [ @@ -12125,7 +12271,7 @@ describe("Linter with FlatConfigArray", () => { severity: 2, fatal: true, ruleId: null, - message: "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token } in JSON at position 21", + message: parseErrorMessage, line: 1, column: 1, endLine: 1, @@ -14282,6 +14428,136 @@ var a = "test2"; assert.strictEqual(suppressedMessages.length, 0); }); + + + it("should report both a rule violation without warning about inline config when noInlineConfig is true and allowInlineConfig is false", () => { + const code = [ + "/* eslint-disable */ // <-- this should be inline config warning", + "foo(); // <-- this should be no-undef error" + ].join("\n"); + const config = { + rules: { + "no-undef": 2 + }, + linterOptions: { + noInlineConfig: true + } + }; + + const messages = linter.verify(code, config, { + filename, + allowInlineConfig: false + }); + const suppressedMessages = linter.getSuppressedMessages(); + + assert.strictEqual(messages.length, 1); + assert.deepStrictEqual( + messages, + [ + { + ruleId: "no-undef", + messageId: "undef", + message: "'foo' is not defined.", + line: 2, + endLine: 2, + column: 1, + endColumn: 4, + severity: 2, + nodeType: "Identifier" + } + ] + ); + + assert.strictEqual(suppressedMessages.length, 0); + }); + + it("should report both a rule violation without warning about inline config when both are false", () => { + const code = [ + "/* eslint-disable */ // <-- this should be inline config warning", + "foo(); // <-- this should be no-undef error" + ].join("\n"); + const config = { + rules: { + "no-undef": 2 + }, + linterOptions: { + noInlineConfig: false + } + }; + + const messages = linter.verify(code, config, { + filename, + allowInlineConfig: false + }); + const suppressedMessages = linter.getSuppressedMessages(); + + assert.strictEqual(messages.length, 1); + assert.deepStrictEqual( + messages, + [ + { + ruleId: "no-undef", + messageId: "undef", + message: "'foo' is not defined.", + line: 2, + endLine: 2, + column: 1, + endColumn: 4, + severity: 2, + nodeType: "Identifier" + } + ] + ); + + assert.strictEqual(suppressedMessages.length, 0); + }); + + it("should report one suppresed problem when noInlineConfig is false and allowInlineConfig is true", () => { + const code = [ + "/* eslint-disable */ // <-- this should be inline config warning", + "foo(); // <-- this should be no-undef error" + ].join("\n"); + const config = { + rules: { + "no-undef": 2 + }, + linterOptions: { + noInlineConfig: false + } + }; + + const messages = linter.verify(code, config, { + filename, + allowInlineConfig: true + }); + const suppressedMessages = linter.getSuppressedMessages(); + + assert.strictEqual(messages.length, 0); + assert.strictEqual(suppressedMessages.length, 1); + assert.deepStrictEqual( + suppressedMessages, + [ + { + ruleId: "no-undef", + messageId: "undef", + message: "'foo' is not defined.", + line: 2, + endLine: 2, + column: 1, + endColumn: 4, + severity: 2, + nodeType: "Identifier", + suppressions: [ + { + justification: "", + kind: "directive" + } + ] + } + ] + ); + + }); }); describe("reportUnusedDisableDirectives option", () => { From 5c9f32f70a24ccc6e31758f74a5d83c9a78abf6a Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Tue, 19 Sep 2023 12:23:16 -0400 Subject: [PATCH 23/28] Fix JSON parse message again --- tests/lib/linter/linter.js | 111 ++++++++++++++----------------------- 1 file changed, 41 insertions(+), 70 deletions(-) diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 845d956c97d..c5a0d15fbfb 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -2031,45 +2031,31 @@ describe("Linter", () => { "foo(); // <-- expected no-undef error here" ].join("\n"); - - // Node.js changed its JSON parsing error message around Node.js v19, so check for that - let parseErrorMessage = "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token } in JSON at position 21"; - - try { - JSON.parse("{'}"); - } catch (error) { - if (error.message.includes("is not valid JSON")) { - parseErrorMessage = "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token '}', ...\"d-vars\": [}\" is not valid JSON"; - } - } - const messages = linter.verify(code); const suppressedMessages = linter.getSuppressedMessages(); + // different engines have different JSON parsing error messages + assert.match(messages[0].message, /Failed to parse JSON from ' "no-unused-vars": \['/u); + assert.strictEqual(messages[0].severity, 2); + assert.isTrue(messages[0].fatal); + assert.isNull(messages[0].ruleId); + assert.strictEqual(messages[0].line, 1); + assert.strictEqual(messages[0].column, 1); + assert.isNull(messages[0].nodeType); + assert.deepStrictEqual( - messages, - [ - { - severity: 2, - fatal: true, - ruleId: null, - message: parseErrorMessage, - line: 1, - column: 1, - nodeType: null - }, - { - severity: 2, - ruleId: "no-undef", - message: "'foo' is not defined.", - messageId: "undef", - line: 3, - column: 1, - endLine: 3, - endColumn: 4, - nodeType: "Identifier" - } - ] + messages[1], + { + severity: 2, + ruleId: "no-undef", + message: "'foo' is not defined.", + messageId: "undef", + line: 3, + column: 1, + endLine: 3, + endColumn: 4, + nodeType: "Identifier" + } ); assert.strictEqual(suppressedMessages.length, 0); @@ -12253,43 +12239,28 @@ describe("Linter with FlatConfigArray", () => { const messages = linter.verify(code, {}); const suppressedMessages = linter.getSuppressedMessages(); - // Node.js changed its JSON parsing error message around Node.js v19, so check for that - let parseErrorMessage = "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token } in JSON at position 21"; - - try { - JSON.parse("{'}"); - } catch (error) { - if (error.message.includes("is not valid JSON")) { - parseErrorMessage = "Failed to parse JSON from ' \"no-unused-vars\": [': Unexpected token '}', ...\"d-vars\": [}\" is not valid JSON"; - } - } + // different engines have different JSON parsing error messages + assert.match(messages[0].message, /Failed to parse JSON from ' "no-unused-vars": \['/u); + assert.strictEqual(messages[0].severity, 2); + assert.isTrue(messages[0].fatal); + assert.isNull(messages[0].ruleId); + assert.strictEqual(messages[0].line, 1); + assert.strictEqual(messages[0].column, 1); + assert.isNull(messages[0].nodeType); assert.deepStrictEqual( - messages, - [ - { - severity: 2, - fatal: true, - ruleId: null, - message: parseErrorMessage, - line: 1, - column: 1, - endLine: 1, - endColumn: 2, - nodeType: null - }, - { - severity: 2, - ruleId: "no-undef", - message: "'foo' is not defined.", - messageId: "undef", - line: 3, - column: 1, - endLine: 3, - endColumn: 4, - nodeType: "Identifier" - } - ] + messages[1], + { + severity: 2, + ruleId: "no-undef", + message: "'foo' is not defined.", + messageId: "undef", + line: 3, + column: 1, + endLine: 3, + endColumn: 4, + nodeType: "Identifier" + } ); assert.strictEqual(suppressedMessages.length, 0); From 36baf92ca29cb834c49d1780bb20fe692b9685d8 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Fri, 22 Sep 2023 11:09:44 -0400 Subject: [PATCH 24/28] Update lib/source-code/source-code.js Co-authored-by: Milos Djermanovic --- lib/source-code/source-code.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/source-code/source-code.js b/lib/source-code/source-code.js index dbd8b177d66..62325e3a2de 100644 --- a/lib/source-code/source-code.js +++ b/lib/source-code/source-code.js @@ -926,7 +926,7 @@ class SourceCode extends TokenStore { } // only certain comment types are supported as line comments - return comment.type === "Block" || !!/^eslint-disable-(next-)?line$/u.test(directiveMatch[1]); + return comment.type !== "Line" || !!/^eslint-disable-(next-)?line$/u.test(directiveMatch[1]); }); this[caches].set("configNodes", configNodes); From 3969b4122abb575d09985b2b09f6bba4cc69e0a4 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Fri, 22 Sep 2023 11:13:07 -0400 Subject: [PATCH 25/28] Update lib/linter/linter.js Co-authored-by: Milos Djermanovic --- lib/linter/linter.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index b33cd672cd8..3fbab46b799 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -489,9 +489,7 @@ function getDirectiveComments(sourceCode, ruleMapper, warnInlineConfig) { } /** - * Parses comments in file to extract file-specific config of rules, globals - * and environments and merges them with global config; also code blocks - * where reporting is disabled or enabled and merges them with reporting config. + * Parses comments in file to extract disable directives. * @param {SourceCode} sourceCode The SourceCode object to get comments from. * @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules * @returns {{problems: LintMessage[], disableDirectives: DisableDirective[]}} From 22a16a64cce851371f16461774daac94810d13ba Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Fri, 22 Sep 2023 11:14:29 -0400 Subject: [PATCH 26/28] Update lib/linter/linter.js Co-authored-by: Milos Djermanovic --- lib/linter/linter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 3fbab46b799..e195812e513 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -1711,7 +1711,7 @@ class Linter { let baseMessage = err.message.slice( err.message.startsWith("Key \"rules\":") ? err.message.indexOf(":", 12) + 1 - : err.message.indexOf(":") + : err.message.indexOf(":") + 1 ).trim(); if (err.messageTemplate) { From ff145b08506b5f4ed421f36c444de0c94917d075 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Fri, 22 Sep 2023 11:14:44 -0400 Subject: [PATCH 27/28] Update tests/lib/linter/linter.js Co-authored-by: Milos Djermanovic --- tests/lib/linter/linter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index c5a0d15fbfb..5b3d0682612 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -12193,7 +12193,7 @@ describe("Linter with FlatConfigArray", () => { { severity: 2, ruleId: "no-alert", - message: "Inline configuration for rule \"no-alert\" is invalid:\n\t: Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2. You passed \"true\".\n", + message: "Inline configuration for rule \"no-alert\" is invalid:\n\tExpected severity of \"off\", 0, \"warn\", 1, \"error\", or 2. You passed \"true\".\n", line: 1, column: 1, endLine: 1, From 14edf42ae6cbd4f1df5943173e457e785f0c14e9 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Fri, 22 Sep 2023 11:25:07 -0400 Subject: [PATCH 28/28] Apply feedback --- lib/source-code/source-code.js | 12 +++++------ tests/lib/source-code/source-code.js | 31 +++++++--------------------- 2 files changed, 12 insertions(+), 31 deletions(-) diff --git a/lib/source-code/source-code.js b/lib/source-code/source-code.js index dbd8b177d66..db4c50605f2 100644 --- a/lib/source-code/source-code.js +++ b/lib/source-code/source-code.js @@ -440,6 +440,10 @@ class SourceCode extends TokenStore { // Cache for comments found using getComments(). this._commentCache = new WeakMap(); + + // don't allow further modification of this object + Object.freeze(this); + Object.freeze(this.lines); } /** @@ -961,7 +965,7 @@ class SourceCode extends TokenStore { /** * Applies configuration found inside of the source code. This method is only * called when ESLint is running with inline configuration allowed. - * @returns {{ok:boolean,problems:Array,configs:{config:FlatConfigArray,node:ASTNode}}} Information + * @returns {{problems:Array,configs:{config:FlatConfigArray,node:ASTNode}}} Information * that ESLint needs to further process the inline configuration. */ applyInlineConfig() { @@ -1038,10 +1042,7 @@ class SourceCode extends TokenStore { varsCache.set("inlineGlobals", inlineGlobals); varsCache.set("exportedVariables", exportedVariables); - // now that we've gathered all of the directive-defined globals, let's add them - return { - ok: problems.length === 0, configs, problems }; @@ -1068,9 +1069,6 @@ class SourceCode extends TokenStore { markExportedVariables(globalScope, exportedVariables); } - // don't allow further modification of this object - Object.freeze(this); - Object.freeze(this.lines); } } diff --git a/tests/lib/source-code/source-code.js b/tests/lib/source-code/source-code.js index 8c17180eda6..763db27bc46 100644 --- a/tests/lib/source-code/source-code.js +++ b/tests/lib/source-code/source-code.js @@ -3872,17 +3872,16 @@ describe("SourceCode", () => { const code = "foo"; const ast = espree.parse(code, DEFAULT_CONFIG); - const sourceCode = new SourceCode(code, ast); const scopeManager = eslintScope.analyze(ast, { ignoreEval: true, ecmaVersion: 6 }); + const sourceCode = new SourceCode({ text: code, ast, scopeManager }); sourceCode.applyLanguageOptions({ ecmaVersion: 2015 }); - sourceCode.scopeManager = scopeManager; sourceCode.finalize(); const globalScope = sourceCode.scopeManager.scopes[0]; @@ -3896,11 +3895,11 @@ describe("SourceCode", () => { const code = "foo"; const ast = espree.parse(code, DEFAULT_CONFIG); - const sourceCode = new SourceCode(code, ast); const scopeManager = eslintScope.analyze(ast, { ignoreEval: true, ecmaVersion: 6 }); + const sourceCode = new SourceCode({ text: code, ast, scopeManager }); sourceCode.applyLanguageOptions({ ecmaVersion: 2015, @@ -3909,7 +3908,6 @@ describe("SourceCode", () => { } }); - sourceCode.scopeManager = scopeManager; sourceCode.finalize(); const globalScope = sourceCode.scopeManager.scopes[0]; @@ -3923,19 +3921,18 @@ describe("SourceCode", () => { const code = "foo"; const ast = espree.parse(code, DEFAULT_CONFIG); - const sourceCode = new SourceCode(code, ast); const scopeManager = eslintScope.analyze(ast, { ignoreEval: true, nodejsScope: true, ecmaVersion: 6 }); + const sourceCode = new SourceCode({ text: code, ast, scopeManager }); sourceCode.applyLanguageOptions({ ecmaVersion: 2015, sourceType: "commonjs" }); - sourceCode.scopeManager = scopeManager; sourceCode.finalize(); const globalScope = sourceCode.scopeManager.scopes[0]; @@ -3953,17 +3950,13 @@ describe("SourceCode", () => { const code = "/*global bar: true */ foo"; const ast = espree.parse(code, DEFAULT_CONFIG); - const sourceCode = new SourceCode(code, ast); const scopeManager = eslintScope.analyze(ast, { ignoreEval: true, ecmaVersion: 6 }); + const sourceCode = new SourceCode({ text: code, ast, scopeManager }); - const result = sourceCode.applyInlineConfig(); - - assert.isTrue(result.ok); - - sourceCode.scopeManager = scopeManager; + sourceCode.applyInlineConfig(); sourceCode.finalize(); const globalScope = sourceCode.scopeManager.scopes[0]; @@ -3978,17 +3971,13 @@ describe("SourceCode", () => { const code = "/*exported foo */ var foo;"; const ast = espree.parse(code, DEFAULT_CONFIG); - const sourceCode = new SourceCode(code, ast); const scopeManager = eslintScope.analyze(ast, { ignoreEval: true, ecmaVersion: 6 }); + const sourceCode = new SourceCode({ text: code, ast, scopeManager }); - const result = sourceCode.applyInlineConfig(); - - assert.isTrue(result.ok); - - sourceCode.scopeManager = scopeManager; + sourceCode.applyInlineConfig(); sourceCode.finalize(); const globalScope = sourceCode.scopeManager.scopes[0]; @@ -4006,7 +3995,6 @@ describe("SourceCode", () => { const sourceCode = new SourceCode(code, ast); const result = sourceCode.applyInlineConfig(); - assert.isTrue(result.ok); assert.strictEqual(result.configs.length, 1); assert.strictEqual(result.configs[0].config.rules["some-rule"], 2); }); @@ -4018,7 +4006,6 @@ describe("SourceCode", () => { const sourceCode = new SourceCode(code, ast); const result = sourceCode.applyInlineConfig(); - assert.isTrue(result.ok); assert.strictEqual(result.configs.length, 1); assert.strictEqual(result.configs[0].config.rules["some-rule"], 2); assert.deepStrictEqual(result.configs[0].config.rules["other-rule"], ["error", { skip: true }]); @@ -4031,7 +4018,6 @@ describe("SourceCode", () => { const sourceCode = new SourceCode(code, ast); const result = sourceCode.applyInlineConfig(); - assert.isTrue(result.ok); assert.strictEqual(result.configs.length, 2); assert.strictEqual(result.configs[0].config.rules["some-rule"], 2); assert.deepStrictEqual(result.configs[1].config.rules["other-rule"], ["error", { skip: true }]); @@ -4043,9 +4029,6 @@ describe("SourceCode", () => { const ast = espree.parse(code, DEFAULT_CONFIG); const sourceCode = new SourceCode(code, ast); const result = sourceCode.applyInlineConfig(); - - assert.isFalse(result.ok); - const problem = result.problems[0]; // Node.js 19 changes the JSON parsing error format, so we need to check each field separately to use a regex