From f394a1dfc5eb4874f899b7bc19685896893af7b8 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Mon, 19 Nov 2018 10:06:50 -0800 Subject: [PATCH] Chore: Extract config comment parsing (#11091) * Chore: Extract config comment parsing * Add more debug messages * Fix linting errors * Remove unused tests and constructor * Add comment to updated test * Fix lint issue --- lib/linter.js | 119 +----------- lib/util/config-comment-parser.js | 144 +++++++++++++++ tests/lib/util/config-comment-parser.js | 229 ++++++++++++++++++++++++ tests/lib/util/npm-utils.js | 8 + 4 files changed, 388 insertions(+), 112 deletions(-) create mode 100644 lib/util/config-comment-parser.js create mode 100644 tests/lib/util/config-comment-parser.js diff --git a/lib/linter.js b/lib/linter.js index edcf741069c..84a53784a5c 100644 --- a/lib/linter.js +++ b/lib/linter.js @@ -11,7 +11,6 @@ const eslintScope = require("eslint-scope"), evk = require("eslint-visitor-keys"), - levn = require("levn"), lodash = require("lodash"), CodePathAnalyzer = require("./code-path-analysis/code-path-analyzer"), ConfigOps = require("./config/config-ops"), @@ -25,6 +24,7 @@ const eslintScope = require("eslint-scope"), createReportTranslator = require("./report-translator"), Rules = require("./rules"), timing = require("./util/timing"), + ConfigCommentParser = require("./util/config-comment-parser"), astUtils = require("./util/ast-utils"), pkg = require("../package.json"), SourceCodeFixer = require("./util/source-code-fixer"); @@ -32,6 +32,7 @@ const eslintScope = require("eslint-scope"), const debug = require("debug")("eslint:linter"); const MAX_AUTOFIX_PASSES = 10; const DEFAULT_PARSER_NAME = "espree"; +const commentParser = new ConfigCommentParser(); //------------------------------------------------------------------------------ // Typedefs @@ -59,112 +60,6 @@ const DEFAULT_PARSER_NAME = "espree"; // Helpers //------------------------------------------------------------------------------ -/** - * Parses a list of "name:boolean_value" or/and "name" options divided by comma or - * whitespace. - * @param {string} string The string to parse. - * @param {Comment} comment The comment node which has the string. - * @returns {Object} Result map object of names and boolean values - */ -function parseBooleanConfig(string, comment) { - const items = {}; - - // Collapse whitespace around `:` and `,` to make parsing easier - const trimmedString = string.replace(/\s*([:,])\s*/g, "$1"); - - trimmedString.split(/\s|,+/).forEach(name => { - if (!name) { - return; - } - - // value defaults to "false" (if not provided), e.g: "foo" => ["foo", "false"] - const [key, value = "false"] = name.split(":"); - - items[key] = { - value: value === "true", - comment - }; - }); - return items; -} - -/** - * Parses a JSON-like config. - * @param {string} string The string to parse. - * @param {Object} location Start line and column of comments for potential error message. - * @returns {({success: true, config: Object}|{success: false, error: Problem})} Result map object - */ -function parseJsonConfig(string, location) { - let items = {}; - - // Parses a JSON-like comment by the same way as parsing CLI option. - try { - items = levn.parse("Object", string) || {}; - - // Some tests say that it should ignore invalid comments such as `/*eslint no-alert:abc*/`. - // Also, commaless notations have invalid severity: - // "no-alert: 2 no-console: 2" --> {"no-alert": "2 no-console: 2"} - // Should ignore that case as well. - if (ConfigOps.isEverySeverityValid(items)) { - return { - success: true, - config: items - }; - } - } catch (ex) { - - // ignore to parse the string by a fallback. - } - - /* - * Optionator cannot parse commaless notations. - * But we are supporting that. So this is a fallback for that. - */ - items = {}; - const normalizedString = string.replace(/([a-zA-Z0-9\-/]+):/g, "\"$1\":").replace(/(]|[0-9])\s+(?=")/, "$1,"); - - try { - items = JSON.parse(`{${normalizedString}}`); - } catch (ex) { - return { - success: false, - error: { - ruleId: null, - fatal: true, - severity: 2, - message: `Failed to parse JSON from '${normalizedString}': ${ex.message}`, - line: location.start.line, - column: location.start.column + 1 - } - }; - - } - - return { - success: true, - config: items - }; -} - -/** - * Parses a config of values separated by comma. - * @param {string} string The string to parse. - * @returns {Object} Result map of values and true values - */ -function parseListConfig(string) { - const items = {}; - - // Collapse whitespace around , - string.replace(/\s*,\s*/g, ",").split(/,+/).forEach(name => { - const trimmedName = name.trim(); - - if (trimmedName) { - items[trimmedName] = true; - } - }); - return items; -} - /** * Ensures that variables representing built-in properties of the Global Object, * and any globals declared by special block comments, are present in the global @@ -243,7 +138,7 @@ function addDeclaredGlobals(globalScope, configGlobals, commentDirectives) { * @returns {DisableDirective[]} Directives from the comment */ function createDisableDirectives(type, loc, value) { - const ruleIds = Object.keys(parseListConfig(value)); + const ruleIds = Object.keys(commentParser.parseListConfig(value)); const directiveRules = ruleIds.length ? ruleIds : [null]; return directiveRules.map(ruleId => ({ type, line: loc.line, column: loc.column + 1, ruleId })); @@ -296,12 +191,12 @@ function getDirectiveComments(filename, ast, ruleMapper) { } else if (comment.type === "Block") { switch (match[1]) { case "exported": - Object.assign(exportedVariables, parseBooleanConfig(directiveValue, comment)); + Object.assign(exportedVariables, commentParser.parseBooleanConfig(directiveValue, comment)); break; case "globals": case "global": - Object.assign(enabledGlobals, parseBooleanConfig(directiveValue, comment)); + Object.assign(enabledGlobals, commentParser.parseBooleanConfig(directiveValue, comment)); break; case "eslint-disable": @@ -313,7 +208,7 @@ function getDirectiveComments(filename, ast, ruleMapper) { break; case "eslint": { - const parseResult = parseJsonConfig(directiveValue, comment.loc); + const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc); if (parseResult.success) { Object.keys(parseResult.config).forEach(name => { @@ -393,7 +288,7 @@ function findEslintEnv(text) { eslintEnvPattern.lastIndex = 0; while ((match = eslintEnvPattern.exec(text))) { - retv = Object.assign(retv || {}, parseListConfig(match[1])); + retv = Object.assign(retv || {}, commentParser.parseListConfig(match[1])); } return retv; diff --git a/lib/util/config-comment-parser.js b/lib/util/config-comment-parser.js new file mode 100644 index 00000000000..88504caf1ca --- /dev/null +++ b/lib/util/config-comment-parser.js @@ -0,0 +1,144 @@ +/** + * @fileoverview Config Comment Parser + * @author Nicholas C. Zakas + */ + +/* eslint-disable class-methods-use-this*/ +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const levn = require("levn"), + ConfigOps = require("../config/config-ops"); + +const debug = require("debug")("eslint:config-comment-parser"); + +//------------------------------------------------------------------------------ +// Public Interface +//------------------------------------------------------------------------------ + +/** + * Object to parse ESLint configuration comments inside JavaScript files. + * @name ConfigCommentParser + */ +module.exports = class ConfigCommentParser { + + /** + * Parses a list of "name:boolean_value" or/and "name" options divided by comma or + * whitespace. Used for "global" and "exported" comments. + * @param {string} string The string to parse. + * @param {Comment} comment The comment node which has the string. + * @returns {Object} Result map object of names and boolean values + */ + parseBooleanConfig(string, comment) { + debug("Parsing Boolean config"); + + const items = {}; + + // Collapse whitespace around `:` and `,` to make parsing easier + const trimmedString = string.replace(/\s*([:,])\s*/g, "$1"); + + trimmedString.split(/\s|,+/).forEach(name => { + if (!name) { + return; + } + + // value defaults to "false" (if not provided), e.g: "foo" => ["foo", "false"] + const [key, value = "false"] = name.split(":"); + + items[key] = { + value: value === "true", + comment + }; + }); + return items; + } + + /** + * Parses a JSON-like config. + * @param {string} string The string to parse. + * @param {Object} location Start line and column of comments for potential error message. + * @returns {({success: true, config: Object}|{success: false, error: Problem})} Result map object + */ + parseJsonConfig(string, location) { + debug("Parsing JSON config"); + + let items = {}; + + // Parses a JSON-like comment by the same way as parsing CLI option. + try { + items = levn.parse("Object", string) || {}; + + // Some tests say that it should ignore invalid comments such as `/*eslint no-alert:abc*/`. + // Also, commaless notations have invalid severity: + // "no-alert: 2 no-console: 2" --> {"no-alert": "2 no-console: 2"} + // Should ignore that case as well. + if (ConfigOps.isEverySeverityValid(items)) { + return { + success: true, + config: items + }; + } + } catch (ex) { + + debug("Levn parsing failed; falling back to manual parsing."); + + // ignore to parse the string by a fallback. + } + + /* + * Optionator cannot parse commaless notations. + * But we are supporting that. So this is a fallback for that. + */ + items = {}; + const normalizedString = string.replace(/([a-zA-Z0-9\-/]+):/g, "\"$1\":").replace(/(]|[0-9])\s+(?=")/, "$1,"); + + try { + items = JSON.parse(`{${normalizedString}}`); + } catch (ex) { + debug("Manual parsing failed."); + + return { + success: false, + error: { + ruleId: null, + fatal: true, + severity: 2, + message: `Failed to parse JSON from '${normalizedString}': ${ex.message}`, + line: location.start.line, + column: location.start.column + 1 + } + }; + + } + + return { + success: true, + config: items + }; + } + + /** + * Parses a config of values separated by comma. + * @param {string} string The string to parse. + * @returns {Object} Result map of values and true values + */ + parseListConfig(string) { + debug("Parsing list config"); + + const items = {}; + + // Collapse whitespace around commas + string.replace(/\s*,\s*/g, ",").split(/,+/).forEach(name => { + const trimmedName = name.trim(); + + if (trimmedName) { + items[trimmedName] = true; + } + }); + return items; + } + +}; diff --git a/tests/lib/util/config-comment-parser.js b/tests/lib/util/config-comment-parser.js new file mode 100644 index 00000000000..6dbe9381aff --- /dev/null +++ b/tests/lib/util/config-comment-parser.js @@ -0,0 +1,229 @@ +/** + * @fileoverview Tests for ConfigCommentParser object. + * @author Nicholas C. Zakas + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const assert = require("chai").assert, + ConfigCommentParser = require("../../../lib/util/config-comment-parser"); + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +describe("ConfigCommentParser", () => { + + let commentParser; + const location = { + start: { + line: 1, + column: 0 + } + }; + + beforeEach(() => { + commentParser = new ConfigCommentParser(); + }); + + describe("parseJsonConfig", () => { + + it("should parse JSON config with one item", () => { + const code = "no-alert:0"; + const result = commentParser.parseJsonConfig(code, location); + + + assert.deepStrictEqual(result, { + success: true, + config: { + "no-alert": 0 + } + }); + }); + + it("should parse JSON config with two items", () => { + const code = "no-alert:0 semi: 2"; + const result = commentParser.parseJsonConfig(code, location); + + + assert.deepStrictEqual(result, { + success: true, + config: { + "no-alert": 0, + semi: 2 + } + }); + }); + + it("should parse JSON config with two comma-separated items", () => { + const code = "no-alert:0,semi: 2"; + const result = commentParser.parseJsonConfig(code, location); + + + assert.deepStrictEqual(result, { + success: true, + config: { + "no-alert": 0, + semi: 2 + } + }); + }); + + it("should parse JSON config with two items and a string severity", () => { + const code = "no-alert:off,semi: 2"; + const result = commentParser.parseJsonConfig(code, location); + + + assert.deepStrictEqual(result, { + success: true, + config: { + "no-alert": "off", + semi: 2 + } + }); + }); + + it("should parse JSON config with two items and options", () => { + const code = "no-alert:off, semi: [2, always]"; + const result = commentParser.parseJsonConfig(code, location); + + + assert.deepStrictEqual(result, { + success: true, + config: { + "no-alert": "off", + semi: [2, "always"] + } + }); + }); + + it("should parse JSON config with two items and options from plugins", () => { + const code = "plugin/no-alert:off, plugin/semi: [2, always]"; + const result = commentParser.parseJsonConfig(code, location); + + assert.deepStrictEqual(result, { + success: true, + config: { + "plugin/no-alert": "off", + "plugin/semi": [2, "always"] + } + }); + }); + + + }); + + describe("parseBooleanConfig", () => { + + const comment = {}; + + it("should parse Boolean config with one item", () => { + const code = "a: true"; + const result = commentParser.parseBooleanConfig(code, comment); + + assert.deepStrictEqual(result, { + a: { + value: true, + comment + } + }); + }); + + it("should parse Boolean config with one item and no value", () => { + const code = "a"; + const result = commentParser.parseBooleanConfig(code, comment); + + assert.deepStrictEqual(result, { + a: { + value: false, + comment + } + }); + }); + + it("should parse Boolean config with two items", () => { + const code = "a: true b:false"; + const result = commentParser.parseBooleanConfig(code, comment); + + assert.deepStrictEqual(result, { + a: { + value: true, + comment + }, + b: { + value: false, + comment + } + }); + }); + + it("should parse Boolean config with two comma-separated items", () => { + const code = "a: true, b:false"; + const result = commentParser.parseBooleanConfig(code, comment); + + assert.deepStrictEqual(result, { + a: { + value: true, + comment + }, + b: { + value: false, + comment + } + }); + }); + + it("should parse Boolean config with two comma-separated items and no values", () => { + const code = "a , b"; + const result = commentParser.parseBooleanConfig(code, comment); + + assert.deepStrictEqual(result, { + a: { + value: false, + comment + }, + b: { + value: false, + comment + } + }); + }); + }); + + describe("parseListConfig", () => { + + it("should parse list config with one item", () => { + const code = "a"; + const result = commentParser.parseListConfig(code); + + assert.deepStrictEqual(result, { + a: true + }); + }); + + it("should parse list config with two items", () => { + const code = "a, b"; + const result = commentParser.parseListConfig(code); + + assert.deepStrictEqual(result, { + a: true, + b: true + }); + }); + + it("should parse list config with two items and exta whitespace", () => { + const code = " a , b "; + const result = commentParser.parseListConfig(code); + + assert.deepStrictEqual(result, { + a: true, + b: true + }); + }); + }); + +}); diff --git a/tests/lib/util/npm-utils.js b/tests/lib/util/npm-utils.js index 4c615abc61f..5c8e4cc845e 100644 --- a/tests/lib/util/npm-utils.js +++ b/tests/lib/util/npm-utils.js @@ -162,6 +162,14 @@ describe("npmUtils", () => { }); it("should return false if package.json does not exist", () => { + + /* + * Checking local file system directly, which does not + * contains package.json. This is necessary because mock-fs + * doesn't work in Node.js 11: it throws an error when + * a file is missing rather than returning false. + * TODO: Find a replacement for mock-fs that works in Node 11. + */ assert.strictEqual(npmUtils.checkPackageJson("/"), false); }); });