From 015d843cd9eb65678a00e8788d08d13375df4350 Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Sun, 27 Aug 2017 05:19:05 -0400 Subject: [PATCH 1/5] Chore: refactor reporting logic (refs #9161) --- lib/linter.js | 166 ++++------ lib/report-translator.js | 246 ++++++++++++++ lib/rule-context.js | 241 -------------- tests/lib/linter.js | 469 ++------------------------- tests/lib/report-translator.js | 567 +++++++++++++++++++++++++++++++++ tests/lib/rule-context.js | 328 ------------------- 6 files changed, 910 insertions(+), 1107 deletions(-) create mode 100644 lib/report-translator.js delete mode 100644 lib/rule-context.js create mode 100644 tests/lib/report-translator.js delete mode 100644 tests/lib/rule-context.js diff --git a/lib/linter.js b/lib/linter.js index 02be483a266..935257a1324 100755 --- a/lib/linter.js +++ b/lib/linter.js @@ -9,10 +9,10 @@ // Requirements //------------------------------------------------------------------------------ -const assert = require("assert"), - EventEmitter = require("events").EventEmitter, +const EventEmitter = require("events").EventEmitter, eslintScope = require("eslint-scope"), levn = require("levn"), + lodash = require("lodash"), blankScriptAST = require("../conf/blank-script.json"), defaultConfig = require("../conf/default-config-options.js"), replacements = require("../conf/replacements.json"), @@ -23,7 +23,7 @@ const assert = require("assert"), NodeEventGenerator = require("./util/node-event-generator"), SourceCode = require("./util/source-code"), Traverser = require("./util/traverser"), - RuleContext = require("./rule-context"), + createReportTranslator = require("./report-translator"), Rules = require("./rules"), timing = require("./timing"), astUtils = require("./ast-utils"), @@ -685,6 +685,34 @@ function parse(text, config, filePath, messages) { } } +const RULE_CONTEXT_PASSTHROUGHS = [ + "getAncestors", + "getDeclaredVariables", + "getFilename", + "getScope", + "getSourceCode", + "markVariableAsUsed", + + // DEPRECATED + "getAllComments", + "getComments", + "getFirstToken", + "getFirstTokens", + "getJSDocComment", + "getLastToken", + "getLastTokens", + "getNodeByRangeIndex", + "getSource", + "getSourceLines", + "getTokenAfter", + "getTokenBefore", + "getTokenByRangeStart", + "getTokens", + "getTokensAfter", + "getTokensBefore", + "getTokensBetween" +]; + //------------------------------------------------------------------------------ // Public Interface //------------------------------------------------------------------------------ @@ -833,45 +861,61 @@ class Linter extends EventEmitter { ConfigOps.normalize(config); // enable appropriate rules - Object.keys(config.rules).filter(key => getRuleSeverity(config.rules[key]) > 0).forEach(key => { + Object.keys(config.rules).filter(ruleId => getRuleSeverity(config.rules[ruleId]) > 0).forEach(ruleId => { let ruleCreator; - ruleCreator = this.rules.get(key); + ruleCreator = this.rules.get(ruleId); if (!ruleCreator) { - const replacementMsg = getRuleReplacementMessage(key); + const replacementMsg = getRuleReplacementMessage(ruleId); if (replacementMsg) { ruleCreator = createStubRule(replacementMsg); } else { - ruleCreator = createStubRule(`Definition for rule '${key}' was not found`); + ruleCreator = createStubRule(`Definition for rule '${ruleId}' was not found`); } - this.rules.define(key, ruleCreator); + this.rules.define(ruleId, ruleCreator); } - const severity = getRuleSeverity(config.rules[key]); - const options = getRuleOptions(config.rules[key]); + const severity = getRuleSeverity(config.rules[ruleId]); + const ruleContext = RULE_CONTEXT_PASSTHROUGHS.reduce( + (contextInfo, methodName) => Object.assign(contextInfo, { [methodName]: this[methodName].bind(this) }), + { + options: getRuleOptions(config.rules[ruleId]), + settings: config.settings, + parserOptions: config.parserOptions, + parserPath: config.parser, + parserServices: parseResult && parseResult.services || {}, + report: lodash.flow([ + createReportTranslator({ ruleId, severity, sourceCode: this.sourceCode }), + problem => { + if (problem.fix && ruleCreator.meta && !ruleCreator.meta.fixable) { + throw new Error("Fixable rules should export a `meta.fixable` property."); + } + if (!isDisabledByReportingConfig(this.reportingConfig, ruleId, problem)) { + this.messages.push(problem); + } + } + ]) + } + ); + + Object.freeze(ruleContext); try { - const ruleContext = new RuleContext( - key, this, severity, options, - config.settings, config.parserOptions, config.parser, - ruleCreator.meta, - (parseResult && parseResult.services ? parseResult.services : {}) - ); - - const rule = ruleCreator.create ? ruleCreator.create(ruleContext) + const rule = ruleCreator.create + ? ruleCreator.create(ruleContext) : ruleCreator(ruleContext); // add all the selectors from the rule as listeners Object.keys(rule).forEach(selector => { this.on(selector, timing.enabled - ? timing.time(key, rule[selector]) + ? timing.time(ruleId, rule[selector]) : rule[selector] ); }); } catch (ex) { - ex.message = `Error while loading rule '${key}': ${ex.message}`; + ex.message = `Error while loading rule '${ruleId}': ${ex.message}`; throw ex; } }); @@ -933,88 +977,6 @@ class Linter extends EventEmitter { return this.messages; } - /** - * Reports a message from one of the rules. - * @param {string} ruleId The ID of the rule causing the message. - * @param {number} severity The severity level of the rule as configured. - * @param {ASTNode} node The AST node that the message relates to. - * @param {Object=} location An object containing the error line and column - * numbers. If location is not provided the node's start location will - * be used. - * @param {string} message The actual message. - * @param {Object} opts Optional template data which produces a formatted message - * with symbols being replaced by this object's values. - * @param {Object} fix A fix command description. - * @param {Object} meta Metadata of the rule - * @returns {void} - */ - report(ruleId, severity, node, location, message, opts, fix, meta) { - if (node) { - assert.strictEqual(typeof node, "object", "Node must be an object"); - } - - let endLocation; - - if (typeof location === "string") { - assert.ok(node, "Node must be provided when reporting error if location is not provided"); - - meta = fix; - fix = opts; - opts = message; - message = location; - location = node.loc.start; - endLocation = node.loc.end; - } else { - endLocation = location.end; - } - - location = location.start || location; - - if (isDisabledByReportingConfig(this.reportingConfig, ruleId, location)) { - return; - } - - if (opts) { - message = message.replace(/\{\{\s*([^{}]+?)\s*\}\}/g, (fullMatch, term) => { - if (term in opts) { - return opts[term]; - } - - // Preserve old behavior: If parameter name not provided, don't replace it. - return fullMatch; - }); - } - - const problem = { - ruleId, - severity, - message, - line: location.line, - column: location.column + 1, // switch to 1-base instead of 0-base - nodeType: node && node.type, - source: this.sourceCode.lines[location.line - 1] || "" - }; - - // Define endLine and endColumn if exists. - if (endLocation) { - problem.endLine = endLocation.line; - problem.endColumn = endLocation.column + 1; // switch to 1-base instead of 0-base - } - - // ensure there's range and text properties, otherwise it's not a valid fix - if (fix && Array.isArray(fix.range) && (typeof fix.text === "string")) { - - // If rule uses fix, has metadata, but has no metadata.fixable, we should throw - if (meta && !meta.fixable) { - throw new Error("Fixable rules should export a `meta.fixable` property."); - } - - problem.fix = fix; - } - - this.messages.push(problem); - } - /** * Gets the SourceCode object representing the parsed source. * @returns {SourceCode} The SourceCode object. diff --git a/lib/report-translator.js b/lib/report-translator.js new file mode 100644 index 00000000000..0390d1f4c6f --- /dev/null +++ b/lib/report-translator.js @@ -0,0 +1,246 @@ +/** + * @fileoverview A helper that translates context.report() calls from the rule API into generic problem objects + * @author Teddy Katz + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const assert = require("assert"); +const lodash = require("lodash"); +const ruleFixer = require("./util/rule-fixer"); + +//------------------------------------------------------------------------------ +// Typedefs +//------------------------------------------------------------------------------ + +/** + * An error message description + * @typedef {Object} MessageDescriptor + * @property {ASTNode} [node] The reported node + * @property {Location} loc The location of the problem. + * @property {string} message The problem message. + * @property {Object} [data] Optional data to use to fill in placeholders in the + * message. + * @property {Function} [fix] The function to call that creates a fix command. + */ + +//------------------------------------------------------------------------------ +// Module Definition +//------------------------------------------------------------------------------ + + +/** + * Translates a multi-argument context.report() call into a single object argument call + * @param {...*} arguments A list of arguments passed to `context.report` + * @returns {MessageDescriptor} A normalized object containing report information + */ +function normalizeMultiArgReportCall() { + + // If there is one argument, it is considered to be a new-style call already. + if (arguments.length === 1) { + return arguments[0]; + } + + // If the second argument is a string, the arguments are interpreted as [node, message, data, fix]. + if (typeof arguments[1] === "string") { + return { + node: arguments[0], + message: arguments[1], + data: arguments[2], + fix: arguments[3] + }; + } + + // Otherwise, the arguments are interpreted as [node, loc, message, data, fix]. + return { + node: arguments[0], + loc: arguments[1], + message: arguments[2], + data: arguments[3], + fix: arguments[4] + }; +} + +/** + * Asserts that either a loc or a node was provided, and the node is valid if it was provided. + * @param {MessageDescriptor} descriptor A descriptor to validate + * @returns {MessageDescriptor} The same descriptor + * @throws AssertionError if neither a node nor a loc was provided, or if the node is not an object + */ +function assertValidNodeInfo(descriptor) { + if (descriptor.node) { + assert(typeof descriptor.node === "object", "Node must be an object"); + } else { + assert(descriptor.loc, "Node must be provided when reporting error if location is not provided"); + } + + return descriptor; +} + +/** + * Normalizes a MessageDescriptor to always have a `loc` with `start` and `end` properties + * @param {MessageDescriptor} descriptor A descriptor for the report from a rule + * @returns {MessageDescriptor} A new MessageDescriptor that inferred the `start` and `end` properties from + * the `node` of the old descriptor, or inferred the `start` from the `loc` of the old descriptor. + */ +function normalizeReportLoc(descriptor) { + if (descriptor.loc) { + if (descriptor.loc.start) { + return descriptor; + } + return Object.assign({}, descriptor, { loc: { start: descriptor.loc, end: null } }); + } + + return Object.assign({}, descriptor, { loc: descriptor.node.loc }); +} + +/** + * Interpolates data placeholders in report messages + * @param {MessageDescriptor} descriptor The report message descriptor + * @returns {MessageDescriptor} An new descriptor with a message containing the interpolated data + */ +function normalizeMessagePlaceholders(descriptor) { + if (!descriptor.data) { + return descriptor; + } + return Object.assign({}, descriptor, { + message: descriptor.message.replace(/\{\{\s*([^{}]+?)\s*\}\}/g, (fullMatch, term) => { + if (term in descriptor.data) { + return descriptor.data[term]; + } + + return fullMatch; + }) + }); +} + +/** + * Compares items in a fixes array by range. + * @param {Fix} a The first message. + * @param {Fix} b The second message. + * @returns {int} -1 if a comes before b, 1 if a comes after b, 0 if equal. + * @private + */ +function compareFixesByRange(a, b) { + return a.range[0] - b.range[0] || a.range[1] - b.range[1]; +} + +/** + * Merges the given fixes array into one. + * @param {Fix[]} fixes The fixes to merge. + * @param {SourceCode} sourceCode The source code object to get the text between fixes. + * @returns {void} + */ +function mergeFixes(fixes, sourceCode) { + if (fixes.length === 0) { + return null; + } + if (fixes.length === 1) { + return fixes[0]; + } + + fixes.sort(compareFixesByRange); + + const originalText = sourceCode.text; + const start = fixes[0].range[0]; + const end = fixes[fixes.length - 1].range[1]; + let text = ""; + let lastPos = Number.MIN_SAFE_INTEGER; + + for (const fix of fixes) { + assert(fix.range[0] >= lastPos, "Fix objects must not be overlapped in a report."); + + if (fix.range[0] >= 0) { + text += originalText.slice(Math.max(0, start, lastPos), fix.range[0]); + } + text += fix.text; + lastPos = fix.range[1]; + } + text += originalText.slice(Math.max(0, start, lastPos), end); + + return { range: [start, end], text }; +} + +/** + * Gets one fix object from the given descriptor. + * If the descriptor retrieves multiple fixes, this merges those to one. + * @param {SourceCode} sourceCode The source code object to get text between fixes. + * @param {Object} descriptor The report descriptor. + * @returns {Fix} The got fix object. + */ +function normalizeFixes(sourceCode, descriptor) { + if (typeof descriptor.fix !== "function") { + return Object.assign({}, descriptor, { fix: null }); + } + + // @type {null | Fix | Fix[] | IterableIterator} + const fix = descriptor.fix(ruleFixer); + + // Merge to one. + if (fix && Symbol.iterator in fix) { + return Object.assign({}, descriptor, { fix: mergeFixes(Array.from(fix), sourceCode) }); + } + return Object.assign({}, descriptor, { fix }); +} + +/** + * Returns a function that converts the arguments of a `context.report` call from a rule into a reported + * problem for the Node.js API. + * @param {Object} metadata Metadata that will be added to the reports. This cannot be modified + * by rules. + * @param {string} metadata.ruleId The rule that the reported messages should be associated with + * @param {0|1|2} metadata.severity The severity that the messages should have + * @param {SourceCode} metadata.sourceCode The `SourceCode` instance for the text being linted + * @returns {function(...args): { + * ruleId: string, + * severity: (0|1|2), + * message: string, + * line: number, + * column: number, + * endLine: (number|undefined), + * endColumn: (number|undefined), + * nodeType: (string|null), + * source: string, + * fix: ({text: string, range: [number, number]}|null) + * }} + * Information about the report + */ +module.exports = function createReportTranslator(metadata) { + const ruleId = metadata.ruleId; + const severity = metadata.severity; + const sourceCode = metadata.sourceCode; + + return lodash.flow([ + normalizeMultiArgReportCall, + assertValidNodeInfo, + normalizeReportLoc, + normalizeMessagePlaceholders, + lodash.partial(normalizeFixes, sourceCode), + descriptor => { + const problem = { + ruleId, + severity, + message: descriptor.message, + line: descriptor.loc.start.line, + column: descriptor.loc.start.column + 1, + nodeType: descriptor.node && descriptor.node.type || null, + source: sourceCode.lines[descriptor.loc.start.line - 1] || "" + }; + + if (descriptor.loc.end) { + problem.endLine = descriptor.loc.end.line; + problem.endColumn = descriptor.loc.end.column + 1; + } + + if (descriptor.fix) { + problem.fix = descriptor.fix; + } + + return problem; + } + ]); +}; diff --git a/lib/rule-context.js b/lib/rule-context.js deleted file mode 100644 index 0a2dc188d01..00000000000 --- a/lib/rule-context.js +++ /dev/null @@ -1,241 +0,0 @@ -/** - * @fileoverview RuleContext utility for rules - * @author Nicholas C. Zakas - */ -"use strict"; - -//------------------------------------------------------------------------------ -// Requirements -//------------------------------------------------------------------------------ - -const assert = require("assert"); -const ruleFixer = require("./util/rule-fixer"); - -//------------------------------------------------------------------------------ -// Constants -//------------------------------------------------------------------------------ - -const PASSTHROUGHS = [ - "getAncestors", - "getDeclaredVariables", - "getFilename", - "getScope", - "getSourceCode", - "markVariableAsUsed", - - // DEPRECATED - "getAllComments", - "getComments", - "getFirstToken", - "getFirstTokens", - "getJSDocComment", - "getLastToken", - "getLastTokens", - "getNodeByRangeIndex", - "getSource", - "getSourceLines", - "getTokenAfter", - "getTokenBefore", - "getTokenByRangeStart", - "getTokens", - "getTokensAfter", - "getTokensBefore", - "getTokensBetween" -]; - -//------------------------------------------------------------------------------ -// Typedefs -//------------------------------------------------------------------------------ - -/** - * An error message description - * @typedef {Object} MessageDescriptor - * @property {string} nodeType The type of node. - * @property {Location} loc The location of the problem. - * @property {string} message The problem message. - * @property {Object} [data] Optional data to use to fill in placeholders in the - * message. - * @property {Function} fix The function to call that creates a fix command. - */ - -//------------------------------------------------------------------------------ -// Module Definition -//------------------------------------------------------------------------------ - -/** - * Compares items in a fixes array by range. - * @param {Fix} a The first message. - * @param {Fix} b The second message. - * @returns {int} -1 if a comes before b, 1 if a comes after b, 0 if equal. - * @private - */ -function compareFixesByRange(a, b) { - return a.range[0] - b.range[0] || a.range[1] - b.range[1]; -} - -/** - * Merges the given fixes array into one. - * @param {Fix[]} fixes The fixes to merge. - * @param {SourceCode} sourceCode The source code object to get the text between fixes. - * @returns {void} - */ -function mergeFixes(fixes, sourceCode) { - if (fixes.length === 0) { - return null; - } - if (fixes.length === 1) { - return fixes[0]; - } - - fixes.sort(compareFixesByRange); - - const originalText = sourceCode.text; - const start = fixes[0].range[0]; - const end = fixes[fixes.length - 1].range[1]; - let text = ""; - let lastPos = Number.MIN_SAFE_INTEGER; - - for (const fix of fixes) { - assert(fix.range[0] >= lastPos, "Fix objects must not be overlapped in a report."); - - if (fix.range[0] >= 0) { - text += originalText.slice(Math.max(0, start, lastPos), fix.range[0]); - } - text += fix.text; - lastPos = fix.range[1]; - } - text += originalText.slice(Math.max(0, start, lastPos), end); - - return { range: [start, end], text }; -} - -/** - * Gets one fix object from the given descriptor. - * If the descriptor retrieves multiple fixes, this merges those to one. - * @param {Object} descriptor The report descriptor. - * @param {SourceCode} sourceCode The source code object to get text between fixes. - * @returns {Fix} The got fix object. - */ -function getFix(descriptor, sourceCode) { - if (typeof descriptor.fix !== "function") { - return null; - } - - // @type {null | Fix | Fix[] | IterableIterator} - const fix = descriptor.fix(ruleFixer); - - // Merge to one. - if (fix && Symbol.iterator in fix) { - return mergeFixes(Array.from(fix), sourceCode); - } - return fix; -} - -/** - * Rule context class - * Acts as an abstraction layer between rules and the main linter object. - */ -class RuleContext { - - /** - * @param {string} ruleId The ID of the rule using this object. - * @param {Linter} linter The linter object. - * @param {number} severity The configured severity level of the rule. - * @param {Array} options The configuration information to be added to the rule. - * @param {Object} settings The configuration settings passed from the config file. - * @param {Object} parserOptions The parserOptions settings passed from the config file. - * @param {Object} parserPath The parser setting passed from the config file. - * @param {Object} meta The metadata of the rule - * @param {Object} parserServices The parser services for the rule. - */ - constructor(ruleId, linter, severity, options, settings, parserOptions, parserPath, meta, parserServices) { - - // public. - this.id = ruleId; - this.options = options; - this.settings = settings; - this.parserOptions = parserOptions; - this.parserPath = parserPath; - this.meta = meta; - - // create a separate copy and freeze it (it's not nice to freeze other people's objects) - this.parserServices = Object.freeze(Object.assign({}, parserServices)); - - // private. - this._linter = linter; - this._severity = severity; - - Object.freeze(this); - } - - /** - * Passthrough to Linter#report() that automatically assigns the rule ID and severity. - * @param {ASTNode|MessageDescriptor} nodeOrDescriptor The AST node related to the message or a message - * descriptor. - * @param {Object=} location The location of the error. - * @param {string} message The message to display to the user. - * @param {Object} opts Optional template data which produces a formatted message - * with symbols being replaced by this object's values. - * @returns {void} - */ - report(nodeOrDescriptor, location, message, opts) { - - // check to see if it's a new style call - if (arguments.length === 1) { - const descriptor = nodeOrDescriptor; - const fix = getFix(descriptor, this.getSourceCode()); - - if (descriptor.loc) { - this._linter.report( - this.id, - this._severity, - descriptor.node, - descriptor.loc, - descriptor.message, - descriptor.data, - fix, - this.meta - ); - } else { - this._linter.report( - this.id, - this._severity, - descriptor.node, - - /* loc not provided */ - descriptor.message, - descriptor.data, - fix, - this.meta - ); - } - - } else { - - // old style call - this._linter.report( - this.id, - this._severity, - nodeOrDescriptor, - location, - message, - opts, - this.meta - ); - } - } -} - -// Copy over passthrough methods. -PASSTHROUGHS.forEach(name => { - Object.defineProperty(RuleContext.prototype, name, { - value() { - return this._linter[name].apply(this._linter, arguments); - }, - configurable: true, - writable: true, - enumerable: false - }); -}); - -module.exports = RuleContext; diff --git a/tests/lib/linter.js b/tests/lib/linter.js index 248ec72d7ca..36afe400d58 100644 --- a/tests/lib/linter.js +++ b/tests/lib/linter.js @@ -738,441 +738,6 @@ describe("Linter", () => { }); }); - describe("report()", () => { - - let config; - - beforeEach(() => { - config = { rules: { "test-rule": "error" } }; - }); - - it("should correctly parse a message when being passed all options", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report("test-rule", 2, node, node.loc.end, "hello {{dynamic}}", { dynamic: node.type }); - } - })); - - const messages = linter.verify("0", config, "", true); - - assert.deepEqual(messages[0], { - severity: 2, - ruleId: "test-rule", - message: "hello Program", - nodeType: "Program", - line: 1, - column: 2, - source: "0" - }); - }); - - it("should use the report the provided location when given", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report("test-rule", 2, node, { line: 42, column: 13 }, "hello world"); - } - })); - - const messages = linter.verify("0", config, "", true); - - assert.deepEqual(messages[0], { - severity: 2, - ruleId: "test-rule", - message: "hello world", - nodeType: "Program", - line: 42, - column: 14, - source: "" - }); - }); - - it("should not throw an error if node is provided and location is not", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report("test-rule", 2, node, "hello world"); - } - })); - - assert.doesNotThrow(() => { - linter.verify("0", config, "", true); - }); - }); - - it("should not throw an error if location is provided and node is not", () => { - linter.defineRule("test-rule", () => ({ - Program() { - linter.report("test-rule", 2, null, { line: 1, column: 1 }, "hello world"); - } - })); - - assert.doesNotThrow(() => { - linter.verify("0", config, "", true); - }); - }); - - it("should throw an error if neither node nor location is provided", () => { - linter.defineRule("test-rule", () => ({ - Program() { - linter.report("test-rule", 2, null, "hello world"); - } - })); - - assert.throws(() => { - linter.verify("0", config, "", true); - }, /Node must be provided when reporting error if location is not provided$/); - }); - - it("should throw an error if node is not an object", () => { - linter.defineRule("test-rule", () => ({ - Program() { - linter.report("test-rule", 2, "not a node", "hello world"); - } - })); - - assert.throws(() => { - linter.verify("0", config, "", true); - }, /Node must be an object$/); - }); - - it("should throw an error if fix is passed but meta has no `fixable` property", () => { - const meta = { - docs: {}, - schema: [] - }; - - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report("test-rule", 2, node, "hello world", [], { range: [1, 1], text: "" }, meta); - } - })); - - assert.throws(() => { - linter.verify("0", config, "", true); - }, /Fixable rules should export a `meta\.fixable` property.$/); - }); - - it("should not throw an error if fix is passed and no metadata is passed", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report("test-rule", 2, node, "hello world", [], { range: [1, 1], text: "" }); - } - })); - - assert.doesNotThrow(() => { - linter.verify("0", config, "", true); - }); - }); - - it("should correctly parse a message with object keys as numbers", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report("test-rule", 2, node, "my message {{name}}{{0}}", { 0: "!", name: "testing" }); - } - })); - - const messages = linter.verify("0", config, "", true); - - assert.deepEqual(messages[0], { - severity: 2, - ruleId: "test-rule", - message: "my message testing!", - nodeType: "Program", - line: 1, - column: 1, - endLine: 1, - endColumn: 2, - source: "0" - }); - }); - - it("should correctly parse a message with array", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report("test-rule", 2, node, "my message {{1}}{{0}}", ["!", "testing"]); - } - })); - - const messages = linter.verify("0", config, "", true); - - assert.deepEqual(messages[0], { - severity: 2, - ruleId: "test-rule", - message: "my message testing!", - nodeType: "Program", - line: 1, - column: 1, - endLine: 1, - endColumn: 2, - source: "0" - }); - }); - - it("should include a fix passed as the last argument when location is not passed", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report("test-rule", 2, node, "my message {{1}}{{0}}", ["!", "testing"], { range: [1, 1], text: "" }); - } - })); - - const messages = linter.verify("0", config, "", true); - - assert.deepEqual(messages[0], { - severity: 2, - ruleId: "test-rule", - message: "my message testing!", - nodeType: "Program", - line: 1, - column: 1, - endLine: 1, - endColumn: 2, - source: "0", - fix: { range: [1, 1], text: "" } - }); - }); - - it("should allow template parameter with inner whitespace", () => { - linter.defineRule("test-rule", context => ({ - Literal(node) { - context.report(node, "message {{parameter name}}", { - "parameter name": "yay!" - }); - } - })); - - config.rules["test-rule"] = 1; - - const messages = linter.verify("0", config); - - assert.equal(messages[0].message, "message yay!"); - }); - - it("should not crash if no template parameters are passed", () => { - linter.defineRule("test-rule", context => ({ - Literal(node) { - context.report(node, "message {{code}}"); - } - })); - - config.rules["test-rule"] = 1; - - const messages = linter.verify("0", config); - - assert.equal(messages[0].message, "message {{code}}"); - }); - - it("should allow template parameter with non-identifier characters", () => { - linter.defineRule("test-rule", context => ({ - Literal(node) { - context.report(node, "message {{parameter-name}}", { - "parameter-name": "yay!" - }); - } - })); - - config.rules["test-rule"] = 1; - - const messages = linter.verify("0", config); - - assert.equal(messages[0].message, "message yay!"); - }); - - it("should allow template parameter wrapped in braces", () => { - linter.defineRule("test-rule", context => ({ - Literal(node) { - context.report(node, "message {{{param}}}", { - param: "yay!" - }); - } - })); - - config.rules["test-rule"] = 1; - - const messages = linter.verify("0", config); - - assert.equal(messages[0].message, "message {yay!}"); - }); - - it("should ignore template parameter with no specified value", () => { - linter.defineRule("test-rule", context => ({ - Literal(node) { - context.report(node, "message {{parameter}}", {}); - } - })); - - config.rules["test-rule"] = 1; - - const messages = linter.verify("0", config); - - assert.equal(messages[0].message, "message {{parameter}}"); - }); - - it("should ignore template parameter with no specified value with warn severity", () => { - linter.defineRule("test-rule", context => ({ - Literal(node) { - context.report(node, "message {{parameter}}", {}); - } - })); - - config.rules["test-rule"] = "warn"; - - const messages = linter.verify("0", config); - - assert.equal(messages[0].severity, 1); - assert.equal(messages[0].message, "message {{parameter}}"); - }); - - it("should handle leading whitespace in template parameter", () => { - linter.defineRule("test-rule", context => ({ - Literal(node) { - context.report(node, "message {{ parameter}}", { - parameter: "yay!" - }); - } - })); - - config.rules["test-rule"] = 1; - - const messages = linter.verify("0", config); - - assert.equal(messages[0].message, "message yay!"); - }); - - it("should handle trailing whitespace in template parameter", () => { - linter.defineRule("test-rule", context => ({ - Literal(node) { - context.report(node, "message {{parameter }}", { - parameter: "yay!" - }); - } - })); - - config.rules["test-rule"] = 1; - - const messages = linter.verify("0", config); - - assert.equal(messages[0].message, "message yay!"); - }); - - it("should still allow inner whitespace as well as leading/trailing", () => { - linter.defineRule("test-rule", context => ({ - Literal(node) { - context.report(node, "message {{ parameter name }}", { - "parameter name": "yay!" - }); - } - })); - - config.rules["test-rule"] = 1; - - const messages = linter.verify("0", config); - - assert.equal(messages[0].message, "message yay!"); - }); - - it("should still allow non-identifier characters as well as leading/trailing whitespace", () => { - linter.defineRule("test-rule", context => ({ - Literal(node) { - context.report(node, "message {{ parameter-name }}", { - "parameter-name": "yay!" - }); - } - })); - - config.rules["test-rule"] = 1; - - const messages = linter.verify("0", config); - - assert.equal(messages[0].message, "message yay!"); - }); - - it("should include a fix passed as the last argument when location is passed", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report("test-rule", 2, node, { line: 42, column: 23 }, "my message {{1}}{{0}}", ["!", "testing"], { range: [1, 1], text: "" }); - } - })); - - const messages = linter.verify("0", config, "", true); - - assert.deepEqual(messages[0], { - severity: 2, - ruleId: "test-rule", - message: "my message testing!", - nodeType: "Program", - line: 42, - column: 24, - source: "", - fix: { range: [1, 1], text: "" } - }); - }); - - it("should have 'endLine' and 'endColumn' when there is not 'loc' property.", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report( - "test-rule", - 2, - node, - "test" - ); - } - })); - - const sourceText = "foo + bar;"; - - const messages = linter.verify(sourceText, config, "", true); - - assert.strictEqual(messages[0].endLine, 1); - assert.strictEqual(messages[0].endColumn, sourceText.length + 1); // (1-based column) - }); - - it("should have 'endLine' and 'endColumn' when 'loc' property has 'end' property.", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report( - "test-rule", - 2, - node, - node.loc, - "test" - ); - } - })); - - const messages = linter.verify("0", config, "", true); - - assert.strictEqual(messages[0].endLine, 1); - assert.strictEqual(messages[0].endColumn, 2); - }); - - it("should not have 'endLine' and 'endColumn' when 'loc' property does not have 'end' property.", () => { - linter.defineRule("test-rule", () => ({ - Program(node) { - linter.report( - "test-rule", - 2, - node, - node.loc.start, - "test" - ); - } - })); - - const messages = linter.verify("0", config, "", true); - - assert.strictEqual(messages[0].endLine, void 0); - assert.strictEqual(messages[0].endColumn, void 0); - }); - - it("should infer an 'endLine' and 'endColumn' property when using the object-based context.report API", () => { - const messages = linter.verify("foo", { rules: { "no-undef": "error" } }); - - assert.strictEqual(messages.length, 1); - assert.strictEqual(messages[0].endLine, 1); - assert.strictEqual(messages[0].endColumn, 4); - }); - }); - describe("when evaluating code", () => { const code = TEST_CODE; @@ -2739,7 +2304,7 @@ describe("Linter", () => { column: 1, severity: 1, source: "var answer = 6 * 7;", - nodeType: void 0 + nodeType: null } ); }); @@ -3870,6 +3435,38 @@ describe("Linter", () => { assert.strictEqual(fixResult.output, `${" ".repeat(10)}a`); assert.strictEqual(fixResult.messages.length, 1); }); + + it("should throw an error if fix is passed but meta has no `fixable` property", () => { + linter.defineRule("test-rule", { + meta: { + docs: {}, + schema: [] + }, + create: context => ({ + Program(node) { + context.report(node, "hello world", {}, () => ({ range: [1, 1], text: "" })); + } + }) + }); + + assert.throws(() => { + linter.verify("0", { rules: { "test-rule": "error" } }); + }, /Fixable rules should export a `meta\.fixable` property.$/); + }); + + it("should not throw an error if fix is passed and there is no metadata", () => { + linter.defineRule("test-rule", { + create: context => ({ + Program(node) { + context.report(node, "hello world", {}, () => ({ range: [1, 1], text: "" })); + } + }) + }); + + assert.doesNotThrow(() => { + linter.verify("0", { rules: { "test-rule": "error" } }); + }); + }); }); describe("Edge cases", () => { diff --git a/tests/lib/report-translator.js b/tests/lib/report-translator.js new file mode 100644 index 00000000000..56ca5ab5508 --- /dev/null +++ b/tests/lib/report-translator.js @@ -0,0 +1,567 @@ +/** + * @fileoverview Tests for createReportTranslator + * @author Teddy Katz + */ +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const assert = require("chai").assert; +const SourceCode = require("../../lib/util/source-code"); +const espree = require("espree"); +const createReportTranslator = require("../../lib/report-translator"); + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +describe("createReportTranslator", () => { + + /** + * Creates a SourceCode instance out of JavaScript text + * @param {string} text Source text + * @returns {SourceCode} A SourceCode instance for that text + */ + function createSourceCode(text) { + return new SourceCode( + text, + espree.parse( + text.replace(/^\uFEFF/, ""), + { + loc: true, + range: true, + raw: true, + tokens: true, + comment: true + } + )); + } + + let node, location, message, translateReport; + + beforeEach(() => { + const sourceCode = createSourceCode("foo\nbar"); + + node = sourceCode.ast.body[0]; + location = sourceCode.ast.body[1].loc.start; + message = "foo"; + translateReport = createReportTranslator({ ruleId: "foo-rule", severity: 2, sourceCode }); + }); + + describe("old-style call with location", () => { + it("should extract the location correctly", () => { + assert.deepEqual( + translateReport(node, location, message, {}), + { + ruleId: "foo-rule", + severity: 2, + message: "foo", + line: 2, + column: 1, + nodeType: "ExpressionStatement", + source: "bar" + } + ); + }); + }); + + describe("old-style call without location", () => { + it("should use the start location and end location of the node", () => { + assert.deepEqual( + translateReport(node, message, {}), + { + ruleId: "foo-rule", + severity: 2, + message: "foo", + line: 1, + column: 1, + endLine: 1, + endColumn: 4, + nodeType: "ExpressionStatement", + source: "foo" + } + ); + }); + }); + + describe("new-style call with all options", () => { + it("should include the new-style options in the report", () => { + const reportDescriptor = { + node, + loc: location, + message, + fix: () => ({ range: [1, 2], text: "foo" }) + }; + + assert.deepEqual( + translateReport(reportDescriptor), + { + ruleId: "foo-rule", + severity: 2, + message: "foo", + line: 2, + column: 1, + nodeType: "ExpressionStatement", + source: "bar", + fix: { + range: [1, 2], + text: "foo" + } + } + ); + }); + }); + describe("combining autofixes", () => { + it("should merge fixes to one if 'fix' function returns an array of fixes.", () => { + const reportDescriptor = { + node, + loc: location, + message, + fix: () => [{ range: [1, 2], text: "foo" }, { range: [4, 5], text: "bar" }] + }; + + assert.deepEqual( + translateReport(reportDescriptor), + { + ruleId: "foo-rule", + severity: 2, + message: "foo", + line: 2, + column: 1, + nodeType: "ExpressionStatement", + source: "bar", + fix: { + range: [1, 5], + text: "fooo\nbar" + } + } + ); + }); + + it("should merge fixes to one if 'fix' function returns an iterator of fixes.", () => { + const reportDescriptor = { + node, + loc: location, + message, + *fix() { + yield { range: [1, 2], text: "foo" }; + yield { range: [4, 5], text: "bar" }; + } + }; + + assert.deepEqual( + translateReport(reportDescriptor), + { + ruleId: "foo-rule", + severity: 2, + message: "foo", + line: 2, + column: 1, + nodeType: "ExpressionStatement", + source: "bar", + fix: { + range: [1, 5], + text: "fooo\nbar" + } + } + ); + }); + + it("should pass through fixes if only one is present", () => { + const reportDescriptor = { + node, + loc: location, + message, + *fix() { + yield { range: [1, 2], text: "foo" }; + yield { range: [4, 5], text: "bar" }; + } + }; + + assert.deepEqual( + translateReport(reportDescriptor), + { + ruleId: "foo-rule", + severity: 2, + message: "foo", + line: 2, + column: 1, + nodeType: "ExpressionStatement", + source: "bar", + fix: { + range: [1, 5], + text: "fooo\nbar" + } + } + ); + }); + + it("should handle inserting BOM correctly.", () => { + const reportDescriptor = { + node, + loc: location, + message, + fix: () => [{ range: [0, 3], text: "\uFEFFfoo" }, { range: [4, 5], text: "x" }] + }; + + assert.deepEqual( + translateReport(reportDescriptor), + { + ruleId: "foo-rule", + severity: 2, + message: "foo", + line: 2, + column: 1, + nodeType: "ExpressionStatement", + source: "bar", + fix: { + range: [0, 5], + text: "\uFEFFfoo\nx" + } + } + ); + }); + + + it("should handle removing BOM correctly.", () => { + const sourceCode = createSourceCode("\uFEFFfoo\nbar"); + + node = sourceCode.ast.body[0]; + + const reportDescriptor = { + node, + message, + fix: () => [{ range: [-1, 3], text: "\uFEFFfoo" }, { range: [4, 5], text: "x" }] + }; + + assert.deepEqual( + createReportTranslator({ ruleId: "foo-rule", severity: 1, sourceCode })(reportDescriptor), + { + ruleId: "foo-rule", + severity: 1, + message: "foo", + line: 1, + column: 1, + endLine: 1, + endColumn: 4, + nodeType: "ExpressionStatement", + source: "foo", + fix: { + range: [-1, 5], + text: "\uFEFFfoo\nx" + } + } + ); + }); + + it("should throw an assertion error if ranges are overlapped.", () => { + const reportDescriptor = { + node, + loc: location, + message, + fix: () => [{ range: [0, 3], text: "\uFEFFfoo" }, { range: [2, 5], text: "x" }] + }; + + assert.throws( + translateReport.bind(null, reportDescriptor), + "Fix objects must not be overlapped in a report." + ); + }); + + it("should include a fix passed as the last argument when location is passed", () => { + assert.deepEqual( + translateReport( + node, + { line: 42, column: 23 }, + "my message {{1}}{{0}}", + ["!", "testing"], + () => ({ range: [1, 1], text: "" }) + ), + { + ruleId: "foo-rule", + severity: 2, + message: "my message testing!", + line: 42, + column: 24, + nodeType: "ExpressionStatement", + source: "", + fix: { + range: [1, 1], + text: "" + } + } + ); + }); + + }); + + describe("message interpolation", () => { + it("should correctly parse a message when being passed all options in an old-style report", () => { + assert.deepEqual( + translateReport(node, node.loc.end, "hello {{dynamic}}", { dynamic: node.type }), + { + severity: 2, + ruleId: "foo-rule", + message: "hello ExpressionStatement", + nodeType: "ExpressionStatement", + line: 1, + column: 4, + source: "foo" + } + ); + }); + + it("should correctly parse a message when being passed all options in a new-style report", () => { + assert.deepEqual( + translateReport({ node, loc: node.loc.end, message: "hello {{dynamic}}", data: { dynamic: node.type } }), + { + severity: 2, + ruleId: "foo-rule", + message: "hello ExpressionStatement", + nodeType: "ExpressionStatement", + line: 1, + column: 4, + source: "foo" + } + ); + }); + + it("should correctly parse a message with object keys as numbers", () => { + assert.strictEqual( + translateReport(node, "my message {{name}}{{0}}", { 0: "!", name: "testing" }).message, + "my message testing!" + ); + }); + + it("should correctly parse a message with array", () => { + assert.strictEqual( + translateReport(node, "my message {{1}}{{0}}", ["!", "testing"]).message, + "my message testing!" + ); + }); + + it("should allow template parameter with inner whitespace", () => { + assert.strictEqual( + translateReport(node, "message {{parameter name}}", { "parameter name": "yay!" }).message, + "message yay!" + ); + }); + + it("should allow template parameter with non-identifier characters", () => { + assert.strictEqual( + translateReport(node, "message {{parameter-name}}", { "parameter-name": "yay!" }).message, + "message yay!" + ); + }); + + it("should allow template parameter wrapped in braces", () => { + assert.strictEqual( + translateReport(node, "message {{{param}}}", { param: "yay!" }).message, + "message {yay!}" + ); + }); + + it("should ignore template parameter with no specified value", () => { + assert.strictEqual( + translateReport(node, "message {{parameter}}", {}).message, + "message {{parameter}}" + ); + }); + + it("should handle leading whitespace in template parameter", () => { + assert.strictEqual( + translateReport({ node, message: "message {{ parameter}}", data: { parameter: "yay!" } }).message, + "message yay!" + ); + }); + + it("should handle trailing whitespace in template parameter", () => { + assert.strictEqual( + translateReport({ node, message: "message {{parameter }}", data: { parameter: "yay!" } }).message, + "message yay!" + ); + }); + + it("should still allow inner whitespace as well as leading/trailing", () => { + assert.strictEqual( + translateReport(node, "message {{ parameter name }}", { "parameter name": "yay!" }).message, + "message yay!" + ); + }); + + it("should still allow non-identifier characters as well as leading/trailing whitespace", () => { + assert.strictEqual( + translateReport(node, "message {{ parameter-name }}", { "parameter-name": "yay!" }).message, + "message yay!" + ); + }); + }); + + describe("location inference", () => { + it("should use the provided location when given in an old-style call", () => { + assert.deepEqual( + translateReport(node, { line: 42, column: 13 }, "hello world"), + { + severity: 2, + ruleId: "foo-rule", + message: "hello world", + nodeType: "ExpressionStatement", + line: 42, + column: 14, + source: "" + } + ); + }); + + it("should use the provided location when given in an new-style call", () => { + assert.deepEqual( + translateReport({ node, loc: { line: 42, column: 13 }, message: "hello world" }), + { + severity: 2, + ruleId: "foo-rule", + message: "hello world", + nodeType: "ExpressionStatement", + line: 42, + column: 14, + source: "" + } + ); + }); + + it("should extract the start and end locations from a node if no location is provided", () => { + assert.deepEqual( + translateReport(node, "hello world"), + { + severity: 2, + ruleId: "foo-rule", + message: "hello world", + nodeType: "ExpressionStatement", + line: 1, + column: 1, + endLine: 1, + endColumn: 4, + source: "foo" + } + ); + }); + + it("should have 'endLine' and 'endColumn' when 'loc' property has 'end' property.", () => { + assert.deepEqual( + translateReport({ loc: node.loc, message: "hello world" }), + { + severity: 2, + ruleId: "foo-rule", + message: "hello world", + nodeType: null, + line: 1, + column: 1, + endLine: 1, + endColumn: 4, + source: "foo" + } + ); + }); + + it("should not have 'endLine' and 'endColumn' when 'loc' property does not have 'end' property.", () => { + assert.deepEqual( + translateReport({ loc: node.loc.start, message: "hello world" }), + { + severity: 2, + ruleId: "foo-rule", + message: "hello world", + nodeType: null, + line: 1, + column: 1, + source: "foo" + } + ); + }); + + it("should infer an 'endLine' and 'endColumn' property when using the object-based context.report API", () => { + assert.deepEqual( + translateReport({ node, message: "hello world" }), + { + severity: 2, + ruleId: "foo-rule", + message: "hello world", + nodeType: "ExpressionStatement", + line: 1, + column: 1, + endLine: 1, + endColumn: 4, + source: "foo" + } + ); + }); + }); + + describe("converting old-style calls", () => { + it("should include a fix passed as the last argument when location is not passed", () => { + assert.deepEqual( + translateReport(node, "my message {{1}}{{0}}", ["!", "testing"], () => ({ range: [1, 1], text: "" })), + { + severity: 2, + ruleId: "foo-rule", + message: "my message testing!", + nodeType: "ExpressionStatement", + line: 1, + column: 1, + endLine: 1, + endColumn: 4, + source: "foo", + fix: { range: [1, 1], text: "" } + } + ); + }); + }); + + describe("validation", () => { + + it("should throw an error if node is not an object", () => { + assert.throws( + () => translateReport("not a node", "hello world"), + "Node must be an object" + ); + }); + + + it("should not throw an error if location is provided and node is not in an old-style call", () => { + assert.deepEqual( + translateReport(null, { line: 1, column: 1 }, "hello world"), + { + severity: 2, + ruleId: "foo-rule", + message: "hello world", + nodeType: null, + line: 1, + column: 2, + source: "foo" + } + ); + }); + + it("should not throw an error if location is provided and node is not in a new-style call", () => { + assert.deepEqual( + translateReport({ loc: { line: 1, column: 1 }, message: "hello world" }), + { + severity: 2, + ruleId: "foo-rule", + message: "hello world", + nodeType: null, + line: 1, + column: 2, + source: "foo" + } + ); + }); + + it("should throw an error if neither node nor location is provided", () => { + assert.throws( + () => translateReport(null, "hello world"), + "Node must be provided when reporting error if location is not provided" + ); + }); + }); +}); diff --git a/tests/lib/rule-context.js b/tests/lib/rule-context.js deleted file mode 100644 index 4195cb4a140..00000000000 --- a/tests/lib/rule-context.js +++ /dev/null @@ -1,328 +0,0 @@ -/** - * @fileoverview Tests for RuleContext object. - * @author Kevin Partington - */ - -"use strict"; - -//------------------------------------------------------------------------------ -// Requirements -//------------------------------------------------------------------------------ - -const sinon = require("sinon"), - assert = require("chai").assert, - leche = require("leche"), - Linter = require("../../lib/linter"), - RuleContext = require("../../lib/rule-context"); - -const realESLint = new Linter(); - -//------------------------------------------------------------------------------ -// Tests -//------------------------------------------------------------------------------ - -describe("RuleContext", () => { - const sandbox = sinon.sandbox.create(); - - describe("report()", () => { - let ruleContext, eslint; - - beforeEach(() => { - eslint = leche.fake(realESLint); - ruleContext = new RuleContext("fake-rule", eslint, 2, {}, {}, {}, "espree"); - }); - - describe("old-style call with location", () => { - it("should call eslint.report() with rule ID and severity prepended", () => { - const node = {}, - location = {}, - message = "Message", - messageOpts = {}; - - const mockESLint = sandbox.mock(eslint); - - mockESLint.expects("report") - .once() - .withArgs("fake-rule", 2, node, location, message, messageOpts); - - ruleContext.report(node, location, message, messageOpts); - - mockESLint.verify(); - }); - }); - - describe("old-style call without location", () => { - it("should call eslint.report() with rule ID and severity prepended", () => { - const node = {}, - message = "Message", - messageOpts = {}; - - const mockESLint = sandbox.mock(eslint); - - mockESLint.expects("report") - .once() - .withArgs("fake-rule", 2, node, message, messageOpts); - - ruleContext.report(node, message, messageOpts); - - mockESLint.verify(); - }); - }); - - describe("new-style call with all options", () => { - it("should call eslint.report() with rule ID and severity prepended and all new-style options", () => { - const node = {}, - location = {}, - message = "Message", - messageOpts = {}, - fixerObj = {}, - fix = sandbox.mock().returns(fixerObj).once(); - - const mockESLint = sandbox.mock(eslint); - - mockESLint.expects("report") - .once() - .withArgs("fake-rule", 2, node, location, message, messageOpts, fixerObj); - mockESLint.expects("getSourceCode") - .once() - .returns(null); - - ruleContext.report({ - node, - loc: location, - message, - data: messageOpts, - fix - }); - - fix.verify(); - mockESLint.verify(); - }); - - it("should merge fixes to one if 'fix' function returns an array of fixes.", () => { - const mockESLint = sandbox.mock(eslint); - - mockESLint.expects("getSourceCode") - .returns({ text: "var foo = 100;" }); - mockESLint.expects("report") - .once() - .withArgs( - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match({ - range: [4, 13], - text: "bar = 234" - }) - ); - - ruleContext.report({ - node: {}, - loc: {}, - message: "Message", - fix(fixer) { - return [ - fixer.replaceTextRange([10, 13], "234"), - fixer.replaceTextRange([4, 7], "bar") - ]; - } - }); - - mockESLint.verify(); - }); - - it("should merge fixes to one if 'fix' function returns an iterator of fixes.", () => { - const mockESLint = sandbox.mock(eslint); - - mockESLint.expects("getSourceCode").returns({ text: "var foo = 100;" }); - mockESLint.expects("report").once().withArgs( - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match({ - range: [4, 13], - text: "bar = 234" - }) - ); - - ruleContext.report({ - node: {}, - loc: {}, - message: "Message", - *fix(fixer) { - yield fixer.replaceTextRange([10, 13], "234"); - yield fixer.replaceTextRange([4, 7], "bar"); - } - }); - - mockESLint.verify(); - }); - - it("should pass through fixes if only one is present", () => { - const mockESLint = sandbox.mock(eslint); - - mockESLint.expects("getSourceCode").returns({ text: "var foo = 100;" }); - mockESLint.expects("report").once().withArgs( - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match({ - range: [10, 13], - text: "234" - }) - ); - - ruleContext.report({ - node: {}, - loc: {}, - message: "Message", - *fix(fixer) { - yield fixer.replaceTextRange([10, 13], "234"); - } - }); - - mockESLint.verify(); - }); - - it("should handle inserting BOM correctly.", () => { - const mockESLint = sandbox.mock(eslint); - - mockESLint.expects("getSourceCode") - .returns({ text: "var foo = 100;" }); - mockESLint.expects("report") - .once() - .withArgs( - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match({ - range: [0, 13], - text: "\uFEFFvar bar = 234" - }) - ); - - ruleContext.report({ - node: {}, - loc: {}, - message: "Message", - fix(fixer) { - return [ - fixer.insertTextBeforeRange([0, 1], "\uFEFF"), - fixer.replaceTextRange([10, 13], "234"), - fixer.replaceTextRange([4, 7], "bar") - ]; - } - }); - - mockESLint.verify(); - }); - - - it("should handle removing BOM correctly.", () => { - const mockESLint = sandbox.mock(eslint); - - mockESLint.expects("getSourceCode") - .returns({ text: "var foo = 100;" }); - mockESLint.expects("report") - .once() - .withArgs( - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match.any, - sinon.match({ - range: [-1, 13], - text: "var bar = 234" - }) - ); - - ruleContext.report({ - node: {}, - loc: {}, - message: "Message", - fix(fixer) { - return [ - fixer.removeRange([-1, 0]), - fixer.replaceTextRange([10, 13], "234"), - fixer.replaceTextRange([4, 7], "bar") - ]; - } - }); - - mockESLint.verify(); - }); - - it("should throw an assertion error if ranges are overlapped.", () => { - const mockESLint = sandbox.mock(eslint); - - mockESLint.expects("getSourceCode") - .returns({ text: "var foo = 100;" }); - mockESLint.expects("report") - .never(); - - assert.throws(() => { - ruleContext.report({ - node: {}, - loc: {}, - message: "Message", - fix(fixer) { - return [ - fixer.removeRange([-1, 0]), - fixer.removeRange([-1, 0]) - ]; - } - }); - }, "Fix objects must not be overlapped in a report."); - - mockESLint.verify(); - }); - - }); - }); - - describe("parserServices", () => { - - it("should pass through parserServices properties to context", () => { - const services = { - test: {} - }; - const ruleContext = new RuleContext("fake-rule", {}, 2, {}, {}, {}, "espree", {}, services); - - assert.equal(ruleContext.parserServices.test, services.test); - }); - - it("should copy parserServices properties to a new object", () => { - const services = { - test: {} - }; - const ruleContext = new RuleContext("fake-rule", {}, 2, {}, {}, {}, "espree", {}, services); - - assert.notEqual(ruleContext.parserServices, services); - }); - - it("should make context.parserServices a frozen object", () => { - const services = { - test: {} - }; - const ruleContext = new RuleContext("fake-rule", {}, 2, {}, {}, {}, "espree", {}, services); - - assert.ok(Object.isFrozen(ruleContext.parserServices)); - }); - - }); - -}); From 636067fda6a9e584d5a13e09f2fa56484426301f Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Tue, 29 Aug 2017 17:59:57 -0400 Subject: [PATCH 2/5] Ensure `id` is passed to rule contexts --- lib/linter.js | 1 + tests/lib/linter.js | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/lib/linter.js b/lib/linter.js index 935257a1324..c44c83bb2e0 100755 --- a/lib/linter.js +++ b/lib/linter.js @@ -881,6 +881,7 @@ class Linter extends EventEmitter { const ruleContext = RULE_CONTEXT_PASSTHROUGHS.reduce( (contextInfo, methodName) => Object.assign(contextInfo, { [methodName]: this[methodName].bind(this) }), { + id: ruleId, options: getRuleOptions(config.rules[ruleId]), settings: config.settings, parserOptions: config.parserOptions, diff --git a/tests/lib/linter.js b/tests/lib/linter.js index 36afe400d58..c175d331005 100644 --- a/tests/lib/linter.js +++ b/tests/lib/linter.js @@ -3004,6 +3004,17 @@ describe("Linter", () => { linter.verify("var foo", config); }); }); + + it("should pass 'id' to rule contexts with the rule id", () => { + const spy = sandbox.spy(context => { + assert.strictEqual(context.id, "foo-bar-baz"); + return {}; + }); + + linter.defineRule("foo-bar-baz", spy); + linter.verify("x", { rules: { "foo-bar-baz": "error" } }); + assert(spy.calledOnce); + }); }); describe("Variables and references", () => { From f516143bf0d05349983d8e2557e2963ccf7e1c13 Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Tue, 29 Aug 2017 18:28:37 -0400 Subject: [PATCH 3/5] Improve verify() performance --- lib/linter.js | 156 ++++++++++++++++++++------------------- lib/report-translator.js | 134 ++++++++++++++++++++------------- 2 files changed, 162 insertions(+), 128 deletions(-) diff --git a/lib/linter.js b/lib/linter.js index c44c83bb2e0..3bd315da529 100755 --- a/lib/linter.js +++ b/lib/linter.js @@ -685,33 +685,43 @@ function parse(text, config, filePath, messages) { } } -const RULE_CONTEXT_PASSTHROUGHS = [ - "getAncestors", - "getDeclaredVariables", - "getFilename", - "getScope", - "getSourceCode", - "markVariableAsUsed", - - // DEPRECATED - "getAllComments", - "getComments", - "getFirstToken", - "getFirstTokens", - "getJSDocComment", - "getLastToken", - "getLastTokens", - "getNodeByRangeIndex", - "getSource", - "getSourceLines", - "getTokenAfter", - "getTokenBefore", - "getTokenByRangeStart", - "getTokens", - "getTokensAfter", - "getTokensBefore", - "getTokensBetween" -]; +// methods that exist on SourceCode object +const DEPRECATED_SOURCECODE_PASSTHROUGHS = { + getSource: "getText", + getSourceLines: "getLines", + getAllComments: "getAllComments", + getNodeByRangeIndex: "getNodeByRangeIndex", + getComments: "getComments", + getCommentsBefore: "getCommentsBefore", + getCommentsAfter: "getCommentsAfter", + getCommentsInside: "getCommentsInside", + getJSDocComment: "getJSDocComment", + getFirstToken: "getFirstToken", + getFirstTokens: "getFirstTokens", + getLastToken: "getLastToken", + getLastTokens: "getLastTokens", + getTokenAfter: "getTokenAfter", + getTokenBefore: "getTokenBefore", + getTokenByRangeStart: "getTokenByRangeStart", + getTokens: "getTokens", + getTokensAfter: "getTokensAfter", + getTokensBefore: "getTokensBefore", + getTokensBetween: "getTokensBetween" +}; + +const BASE_TRAVERSAL_CONTEXT = Object.freeze( + Object.keys(DEPRECATED_SOURCECODE_PASSTHROUGHS).reduce( + (contextInfo, methodName) => + Object.assign(contextInfo, { + [methodName]() { + const sourceCode = this.getSourceCode(); + + return sourceCode[DEPRECATED_SOURCECODE_PASSTHROUGHS[methodName]].apply(sourceCode, arguments); + } + }), + {} + ) +); //------------------------------------------------------------------------------ // Public Interface @@ -860,6 +870,29 @@ class Linter extends EventEmitter { // ensure that severities are normalized in the config ConfigOps.normalize(config); + /* + * Create a frozen object with the ruleContext properties and methods that are shared by all rules. + * All rule contexts will inherit from this object. This avoids the performance penalty of copying all the + * properties once for each rule. + */ + const sharedTraversalContext = Object.freeze( + Object.assign( + Object.create(BASE_TRAVERSAL_CONTEXT), + { + getAncestors: this.getAncestors.bind(this), + getDeclaredVariables: this.getDeclaredVariables.bind(this), + getFilename: this.getFilename.bind(this), + getScope: this.getScope.bind(this), + getSourceCode: () => this.sourceCode, + markVariableAsUsed: this.markVariableAsUsed.bind(this), + parserOptions: config.parserOptions, + parserPath: config.parser, + parserServices: parseResult && parseResult.services || {}, + settings: config.settings + } + ) + ); + // enable appropriate rules Object.keys(config.rules).filter(ruleId => getRuleSeverity(config.rules[ruleId]) > 0).forEach(ruleId => { let ruleCreator; @@ -878,31 +911,27 @@ class Linter extends EventEmitter { } const severity = getRuleSeverity(config.rules[ruleId]); - const ruleContext = RULE_CONTEXT_PASSTHROUGHS.reduce( - (contextInfo, methodName) => Object.assign(contextInfo, { [methodName]: this[methodName].bind(this) }), - { - id: ruleId, - options: getRuleOptions(config.rules[ruleId]), - settings: config.settings, - parserOptions: config.parserOptions, - parserPath: config.parser, - parserServices: parseResult && parseResult.services || {}, - report: lodash.flow([ - createReportTranslator({ ruleId, severity, sourceCode: this.sourceCode }), - problem => { - if (problem.fix && ruleCreator.meta && !ruleCreator.meta.fixable) { - throw new Error("Fixable rules should export a `meta.fixable` property."); + const ruleContext = Object.freeze( + Object.assign( + Object.create(sharedTraversalContext), + { + id: ruleId, + options: getRuleOptions(config.rules[ruleId]), + report: lodash.flow([ + createReportTranslator({ ruleId, severity, sourceCode: this.sourceCode }), + problem => { + if (problem.fix && ruleCreator.meta && !ruleCreator.meta.fixable) { + throw new Error("Fixable rules should export a `meta.fixable` property."); + } + if (!isDisabledByReportingConfig(this.reportingConfig, ruleId, problem)) { + this.messages.push(problem); + } } - if (!isDisabledByReportingConfig(this.reportingConfig, ruleId, problem)) { - this.messages.push(problem); - } - } - ]) - } + ]) + } + ) ); - Object.freeze(ruleContext); - try { const rule = ruleCreator.create ? ruleCreator.create(ruleContext) @@ -1215,33 +1244,8 @@ class Linter extends EventEmitter { } } -// methods that exist on SourceCode object -const externalMethods = { - getSource: "getText", - getSourceLines: "getLines", - getAllComments: "getAllComments", - getNodeByRangeIndex: "getNodeByRangeIndex", - getComments: "getComments", - getCommentsBefore: "getCommentsBefore", - getCommentsAfter: "getCommentsAfter", - getCommentsInside: "getCommentsInside", - getJSDocComment: "getJSDocComment", - getFirstToken: "getFirstToken", - getFirstTokens: "getFirstTokens", - getLastToken: "getLastToken", - getLastTokens: "getLastTokens", - getTokenAfter: "getTokenAfter", - getTokenBefore: "getTokenBefore", - getTokenByRangeStart: "getTokenByRangeStart", - getTokens: "getTokens", - getTokensAfter: "getTokensAfter", - getTokensBefore: "getTokensBefore", - getTokensBetween: "getTokensBetween" -}; - -// copy over methods -Object.keys(externalMethods).forEach(methodName => { - const exMethodName = externalMethods[methodName]; +Object.keys(DEPRECATED_SOURCECODE_PASSTHROUGHS).forEach(methodName => { + const exMethodName = DEPRECATED_SOURCECODE_PASSTHROUGHS[methodName]; // Applies the SourceCode methods to the Linter prototype Object.defineProperty(Linter.prototype, methodName, { diff --git a/lib/report-translator.js b/lib/report-translator.js index 0390d1f4c6f..7afee872abb 100644 --- a/lib/report-translator.js +++ b/lib/report-translator.js @@ -10,7 +10,6 @@ //------------------------------------------------------------------------------ const assert = require("assert"); -const lodash = require("lodash"); const ruleFixer = require("./util/rule-fixer"); //------------------------------------------------------------------------------ @@ -42,7 +41,7 @@ function normalizeMultiArgReportCall() { // If there is one argument, it is considered to be a new-style call already. if (arguments.length === 1) { - return arguments[0]; + return Object.assign({}, arguments[0]); } // If the second argument is a string, the arguments are interpreted as [node, message, data, fix]. @@ -83,31 +82,33 @@ function assertValidNodeInfo(descriptor) { /** * Normalizes a MessageDescriptor to always have a `loc` with `start` and `end` properties - * @param {MessageDescriptor} descriptor A descriptor for the report from a rule - * @returns {MessageDescriptor} A new MessageDescriptor that inferred the `start` and `end` properties from - * the `node` of the old descriptor, or inferred the `start` from the `loc` of the old descriptor. + * @param {MessageDescriptor} descriptor A descriptor for the report from a rule. This descriptor may be mutated + * by this function. + * @returns {MessageDescriptor} The updated MessageDescriptor that infers the `start` and `end` properties from + * the `node` of the original descriptor, or infers the `start` from the `loc` of the original descriptor. */ function normalizeReportLoc(descriptor) { if (descriptor.loc) { if (descriptor.loc.start) { return descriptor; } - return Object.assign({}, descriptor, { loc: { start: descriptor.loc, end: null } }); + return Object.assign(descriptor, { loc: { start: descriptor.loc, end: null } }); } - return Object.assign({}, descriptor, { loc: descriptor.node.loc }); + return Object.assign(descriptor, { loc: descriptor.node.loc }); } /** * Interpolates data placeholders in report messages - * @param {MessageDescriptor} descriptor The report message descriptor + * @param {MessageDescriptor} descriptor The report message descriptor. This descriptor may be mutated + * by this function. * @returns {MessageDescriptor} An new descriptor with a message containing the interpolated data */ function normalizeMessagePlaceholders(descriptor) { if (!descriptor.data) { return descriptor; } - return Object.assign({}, descriptor, { + return Object.assign(descriptor, { message: descriptor.message.replace(/\{\{\s*([^{}]+?)\s*\}\}/g, (fullMatch, term) => { if (term in descriptor.data) { return descriptor.data[term]; @@ -168,13 +169,14 @@ function mergeFixes(fixes, sourceCode) { /** * Gets one fix object from the given descriptor. * If the descriptor retrieves multiple fixes, this merges those to one. + * @param {MessageDescriptor} descriptor The report descriptor. This descriptor may be mutated + * by this function. * @param {SourceCode} sourceCode The source code object to get text between fixes. - * @param {Object} descriptor The report descriptor. - * @returns {Fix} The got fix object. + * @returns {MessageDescriptor} The updated descriptor. */ -function normalizeFixes(sourceCode, descriptor) { +function normalizeFixes(descriptor, sourceCode) { if (typeof descriptor.fix !== "function") { - return Object.assign({}, descriptor, { fix: null }); + return Object.assign(descriptor, { fix: null }); } // @type {null | Fix | Fix[] | IterableIterator} @@ -182,19 +184,58 @@ function normalizeFixes(sourceCode, descriptor) { // Merge to one. if (fix && Symbol.iterator in fix) { - return Object.assign({}, descriptor, { fix: mergeFixes(Array.from(fix), sourceCode) }); + return Object.assign(descriptor, { fix: mergeFixes(Array.from(fix), sourceCode) }); } - return Object.assign({}, descriptor, { fix }); + return Object.assign(descriptor, { fix }); +} + +/** + * Creates information about the report from a descriptor + * @param {MessageDescriptor} descriptor The message descriptor + * @param {string} ruleId The rule ID of the problem + * @param {(0|1|2)} severity The severity of the problem + * @returns {function(...args): { + * ruleId: string, + * severity: (0|1|2), + * message: string, + * line: number, + * column: number, + * endLine: (number|undefined), + * endColumn: (number|undefined), + * nodeType: (string|null), + * source: string, + * fix: ({text: string, range: [number, number]}|null) + * }} Information about the report + */ +function createProblemFromDescriptor(descriptor, ruleId, severity) { + const problem = { + ruleId, + severity, + message: descriptor.message, + line: descriptor.loc.start.line, + column: descriptor.loc.start.column + 1, + nodeType: descriptor.node && descriptor.node.type || null + }; + + if (descriptor.loc.end) { + problem.endLine = descriptor.loc.end.line; + problem.endColumn = descriptor.loc.end.column + 1; + } + + if (descriptor.fix) { + problem.fix = descriptor.fix; + } + + return problem; } /** * Returns a function that converts the arguments of a `context.report` call from a rule into a reported - * problem for the Node.js API. - * @param {Object} metadata Metadata that will be added to the reports. This cannot be modified - * by rules. - * @param {string} metadata.ruleId The rule that the reported messages should be associated with - * @param {0|1|2} metadata.severity The severity that the messages should have - * @param {SourceCode} metadata.sourceCode The `SourceCode` instance for the text being linted + * problem for the Node.js API, without the `ruleId` and `severity` properties. The reported problem + * may be mutated by the claler after being returned from this function. However, none of the original arguments + * to this function should be mutated. + * @param {{ruleId: string, severity: number, sourceCode: SourceCode}} metadata Metadata for the reported problem + * @param {SourceCode} sourceCode The `SourceCode` instance for the text being linted * @returns {function(...args): { * ruleId: string, * severity: (0|1|2), @@ -209,38 +250,27 @@ function normalizeFixes(sourceCode, descriptor) { * }} * Information about the report */ + module.exports = function createReportTranslator(metadata) { - const ruleId = metadata.ruleId; - const severity = metadata.severity; - const sourceCode = metadata.sourceCode; - - return lodash.flow([ - normalizeMultiArgReportCall, - assertValidNodeInfo, - normalizeReportLoc, - normalizeMessagePlaceholders, - lodash.partial(normalizeFixes, sourceCode), - descriptor => { - const problem = { - ruleId, - severity, - message: descriptor.message, - line: descriptor.loc.start.line, - column: descriptor.loc.start.column + 1, - nodeType: descriptor.node && descriptor.node.type || null, - source: sourceCode.lines[descriptor.loc.start.line - 1] || "" - }; - - if (descriptor.loc.end) { - problem.endLine = descriptor.loc.end.line; - problem.endColumn = descriptor.loc.end.column + 1; - } - if (descriptor.fix) { - problem.fix = descriptor.fix; - } + /* + * `createReportTranslator` gets called once per enabled rule per file. It needs to be very performant. + * The report translator itself (i.e. the function that `createReportTranslator` returns) gets + * called every time a rule reports a problem, which happens much less frequently (usually, the vast + * majority of rules don't report any problems for a given file). + */ + return function() { + const descriptor = normalizeMultiArgReportCall.apply(null, arguments); - return problem; - } - ]); + assertValidNodeInfo(descriptor); + normalizeReportLoc(descriptor); + normalizeMessagePlaceholders(descriptor); + normalizeFixes(descriptor, metadata.sourceCode); + + const problem = createProblemFromDescriptor(descriptor, metadata.ruleId, metadata.severity); + + problem.source = metadata.sourceCode.lines[problem.line - 1] || ""; + + return problem; + }; }; From 600bda3de28f1d8f962c4c7382a5f37b1ce61065 Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Tue, 29 Aug 2017 21:08:50 -0400 Subject: [PATCH 4/5] Fix incorrect tests --- tests/lib/report-translator.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/lib/report-translator.js b/tests/lib/report-translator.js index 56ca5ab5508..3e04658ba39 100644 --- a/tests/lib/report-translator.js +++ b/tests/lib/report-translator.js @@ -174,10 +174,7 @@ describe("createReportTranslator", () => { node, loc: location, message, - *fix() { - yield { range: [1, 2], text: "foo" }; - yield { range: [4, 5], text: "bar" }; - } + fix: () => [{ range: [1, 2], text: "foo" }] }; assert.deepEqual( @@ -191,8 +188,8 @@ describe("createReportTranslator", () => { nodeType: "ExpressionStatement", source: "bar", fix: { - range: [1, 5], - text: "fooo\nbar" + range: [1, 2], + text: "foo" } } ); @@ -233,7 +230,7 @@ describe("createReportTranslator", () => { const reportDescriptor = { node, message, - fix: () => [{ range: [-1, 3], text: "\uFEFFfoo" }, { range: [4, 5], text: "x" }] + fix: () => [{ range: [-1, 3], text: "foo" }, { range: [4, 5], text: "x" }] }; assert.deepEqual( @@ -250,7 +247,7 @@ describe("createReportTranslator", () => { source: "foo", fix: { range: [-1, 5], - text: "\uFEFFfoo\nx" + text: "foo\nx" } } ); From 6973223c913db877199475dffad1f1444ac33dd5 Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Tue, 29 Aug 2017 21:19:08 -0400 Subject: [PATCH 5/5] Fix outdated JSDoc comment While investigating the performance impact of this change, I tried modifying the `createReportTranslator` API, but I forgot to change the JSDoc comment back after reverting that modification. --- lib/report-translator.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/report-translator.js b/lib/report-translator.js index 7afee872abb..fe59d9a75b5 100644 --- a/lib/report-translator.js +++ b/lib/report-translator.js @@ -231,9 +231,7 @@ function createProblemFromDescriptor(descriptor, ruleId, severity) { /** * Returns a function that converts the arguments of a `context.report` call from a rule into a reported - * problem for the Node.js API, without the `ruleId` and `severity` properties. The reported problem - * may be mutated by the claler after being returned from this function. However, none of the original arguments - * to this function should be mutated. + * problem for the Node.js API. * @param {{ruleId: string, severity: number, sourceCode: SourceCode}} metadata Metadata for the reported problem * @param {SourceCode} sourceCode The `SourceCode` instance for the text being linted * @returns {function(...args): {