Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix: unify LintMessage type (#17076)
* Update LintMessage type to reflect implementation

I made three changes to `LintMessage` and `SuppressedLintMessage`

1. Many places that create `LintMessage`s don't set `fatal`, so I made
   it optional. This would be a breaking change if we made types part of
   our exports, but in this case I believe it's updating JSDoc
   documentation to reflect reality.
2. Added an optional `messageId` property that's set by reports from rules.
3. Added an optional, nullable `nodeType` property.
    - Reports from rules set it to a value.
    - `applyDirectives()` explicitly sets it to `null` for unused
      disable directives.
    - `Linter`'s `createLintingProblem()` explicitly sets it to `null`.

* Add missing LintResult type import

* Use LintMessage type

All of these previously referenced a `Problem` type that does not have a
definition.

* Replace ReportInfo type with LintMessage type

`ReportInfo` was defined within `report-translator.js` to be very
similar to `LintMessage`. It had one extra property, `source`, which
wasn't ever set, so it would've been removed anyway. In
`Linter.runRules()`, the return value from `reportTranslator()` gets
pushed into a `lintingProblems` array and returned, and the return type
annotation is `LintMessage[]`, which gives me confidence that
`ReportInfo` was intended to be the same type as `LintMessage`
elsewhere.

* Make ruleId required but nullable

Originally, we talked about the reverse - making `ruleId` optional but
non-nullable and relying on `report-translator.js`'s `createProblem()`
to normalize it. However, the `LintMessage` type would differ from
`createProblem()`'s return value only by `null` vs `undefined` for
`ruleId`. So instead, I made `ruleId` required but nullable and
explicitly set it to `null` in the few remaining places that didn't
already.

* Add missing null nodeTypes

`LintMessage.nodeType` is currently defined as required but nullable.
Actual implementations explicitly set it to `null` in a couple places
and omit it in several.

After discussion in #16968, we initially leaned toward making it
non-nullable but optional. I pursued that, but it resulted in slightly
more runtime code changes, including some branching in
`report-translator` to set it conditionally.

Instead, I'm presenting the opposite solution of updating the remaining
implementations to match the existing type definition by explicitly
setting `nodeType` to `null`.

* Update LintMessage docs

This updates existing documented properties to match the updated `LintMessage`
type and implementations.

`messageId`, `nodeType`, and `suppressions` remain undocumented.
  • Loading branch information
btmills committed May 16, 2023
1 parent 7709b14 commit 94da96c
Show file tree
Hide file tree
Showing 14 changed files with 87 additions and 55 deletions.
10 changes: 5 additions & 5 deletions docs/src/extend/custom-processors.md
Expand Up @@ -59,19 +59,19 @@ Reported problems have the following location information in each lint message:
type LintMessage = {

/// The 1-based line number where the message occurs.
line: number;
line?: number;

/// The 1-based column number where the message occurs.
column: number;
column?: number;

/// The 1-based line number of the end location.
endLine: number;
endLine?: number;

/// The 1-based column number of the end location.
endColumn: number;
endColumn?: number;

/// If `true`, this is a fatal error.
fatal: boolean;
fatal?: boolean;

/// Information for an autofix.
fix: Fix;
Expand Down
4 changes: 3 additions & 1 deletion lib/cli-engine/cli-engine.js
Expand Up @@ -308,9 +308,11 @@ function createIgnoreResult(filePath, baseDir) {
filePath: path.resolve(filePath),
messages: [
{
ruleId: null,
fatal: false,
severity: 1,
message
message,
nodeType: null
}
],
suppressedMessages: [],
Expand Down
4 changes: 3 additions & 1 deletion lib/eslint/eslint-helpers.js
Expand Up @@ -607,9 +607,11 @@ function createIgnoreResult(filePath, baseDir) {
filePath: path.resolve(filePath),
messages: [
{
ruleId: null,
fatal: false,
severity: 1,
message
message,
nodeType: null
}
],
suppressedMessages: [],
Expand Down
1 change: 1 addition & 0 deletions lib/eslint/flat-eslint.js
Expand Up @@ -55,6 +55,7 @@ const LintResultCache = require("../cli-engine/lint-result-cache");
/** @typedef {import("../shared/types").ConfigData} ConfigData */
/** @typedef {import("../shared/types").DeprecatedRuleInfo} DeprecatedRuleInfo */
/** @typedef {import("../shared/types").LintMessage} LintMessage */
/** @typedef {import("../shared/types").LintResult} LintResult */
/** @typedef {import("../shared/types").ParserOptions} ParserOptions */
/** @typedef {import("../shared/types").Plugin} Plugin */
/** @typedef {import("../shared/types").ResultsMeta} ResultsMeta */
Expand Down
12 changes: 11 additions & 1 deletion lib/linter/apply-disable-directives.js
Expand Up @@ -5,6 +5,16 @@

"use strict";

//------------------------------------------------------------------------------
// Typedefs
//------------------------------------------------------------------------------

/** @typedef {import("../shared/types").LintMessage} LintMessage */

//------------------------------------------------------------------------------
// Module Definition
//------------------------------------------------------------------------------

const escapeRegExp = require("escape-string-regexp");

/**
Expand Down Expand Up @@ -196,7 +206,7 @@ function processUnusedDisableDirectives(allDirectives) {
* @param {Object} options options for applying directives. This is the same as the options
* for the exported function, except that `reportUnusedDisableDirectives` is not supported
* (this function always reports unused disable directives).
* @returns {{problems: Problem[], unusedDisableDirectives: Problem[]}} An object with a list
* @returns {{problems: LintMessage[], unusedDisableDirectives: LintMessage[]}} An object with a list
* of problems (including suppressed ones) and unused eslint-disable directives
*/
function applyDirectives(options) {
Expand Down
11 changes: 9 additions & 2 deletions lib/linter/config-comment-parser.js
Expand Up @@ -19,6 +19,12 @@ const levn = require("levn"),

const debug = require("debug")("eslint:config-comment-parser");

//------------------------------------------------------------------------------
// Typedefs
//------------------------------------------------------------------------------

/** @typedef {import("../shared/types").LintMessage} LintMessage */

//------------------------------------------------------------------------------
// Public Interface
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -61,7 +67,7 @@ module.exports = class ConfigCommentParser {
* 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
* @returns {({success: true, config: Object}|{success: false, error: LintMessage})} Result map object
*/
parseJsonConfig(string, location) {
debug("Parsing JSON config");
Expand Down Expand Up @@ -109,7 +115,8 @@ module.exports = class ConfigCommentParser {
severity: 2,
message: `Failed to parse JSON from '${normalizedString}': ${ex.message}`,
line: location.start.line,
column: location.start.column + 1
column: location.start.column + 1,
nodeType: null
}
};

Expand Down
23 changes: 14 additions & 9 deletions lib/linter/linter.js
Expand Up @@ -364,7 +364,7 @@ function extractDirectiveComment(value) {
* @param {ASTNode} ast The top node of the AST.
* @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: Problem[], disableDirectives: DisableDirective[]}}
* @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) {
Expand Down Expand Up @@ -775,7 +775,7 @@ function analyzeScope(ast, languageOptions, visitorKeys) {
* @param {string} text The text to parse.
* @param {LanguageOptions} languageOptions Options to pass to the parser
* @param {string} filePath The path to the file being parsed.
* @returns {{success: false, error: Problem}|{success: true, sourceCode: SourceCode}}
* @returns {{success: false, error: LintMessage}|{success: true, sourceCode: SourceCode}}
* An object containing the AST and parser services if parsing was successful, or the error if parsing failed
* @private
*/
Expand Down Expand Up @@ -851,7 +851,8 @@ function parse(text, languageOptions, filePath) {
severity: 2,
message,
line: ex.lineNumber,
column: ex.column
column: ex.column,
nodeType: null
}
};
}
Expand Down Expand Up @@ -921,7 +922,7 @@ const BASE_TRAVERSAL_CONTEXT = Object.freeze(
* @param {boolean} disableFixes If true, it doesn't make `fix` properties.
* @param {string | undefined} cwd cwd of the cli
* @param {string} physicalFilename The full path of the file on disk without any code block information
* @returns {Problem[]} An array of reported problems
* @returns {LintMessage[]} An array of reported problems
*/
function runRules(sourceCode, configuredRules, ruleMapper, parserName, languageOptions, settings, filename, disableFixes, cwd, physicalFilename) {
const emitter = createEmitter();
Expand Down Expand Up @@ -1253,7 +1254,8 @@ class Linter {
severity: 2,
message: `Configured parser '${config.parser}' was not found.`,
line: 0,
column: 0
column: 0,
nodeType: null
}];
}
parserName = config.parser;
Expand Down Expand Up @@ -1464,7 +1466,8 @@ class Linter {
severity: 2,
message,
line: ex.lineNumber,
column: ex.column
column: ex.column,
nodeType: null
}
];
}
Expand Down Expand Up @@ -1729,7 +1732,8 @@ class Linter {
severity: 1,
message: `No matching configuration found for ${filename}.`,
line: 0,
column: 0
column: 0,
nodeType: null
}
];
}
Expand Down Expand Up @@ -1794,7 +1798,8 @@ class Linter {
severity: 2,
message,
line: ex.lineNumber,
column: ex.column
column: ex.column,
nodeType: null
}
];
}
Expand Down Expand Up @@ -1840,7 +1845,7 @@ class Linter {
/**
* Given a list of reported problems, distinguish problems between normal messages and suppressed messages.
* The normal messages will be returned and the suppressed messages will be stored as lastSuppressedMessages.
* @param {Problem[]} problems A list of reported problems.
* @param {Array<LintMessage|SuppressedLintMessage>} problems A list of reported problems.
* @returns {LintMessage[]} A list of LintMessage.
*/
_distinguishSuppressedMessages(problems) {
Expand Down
23 changes: 4 additions & 19 deletions lib/linter/report-translator.js
Expand Up @@ -17,6 +17,8 @@ const interpolate = require("./interpolate");
// Typedefs
//------------------------------------------------------------------------------

/** @typedef {import("../shared/types").LintMessage} LintMessage */

/**
* An error message description
* @typedef {Object} MessageDescriptor
Expand All @@ -29,23 +31,6 @@ const interpolate = require("./interpolate");
* @property {Array<{desc?: string, messageId?: string, fix: Function}>} suggest Suggestion descriptions and functions to create a the associated fixes.
*/

/**
* Information about the report
* @typedef {Object} ReportInfo
* @property {string} ruleId The rule ID
* @property {(0|1|2)} severity Severity of the error
* @property {(string|undefined)} message The message
* @property {(string|undefined)} [messageId] The message ID
* @property {number} line The line number
* @property {number} column The column number
* @property {(number|undefined)} [endLine] The ending line number
* @property {(number|undefined)} [endColumn] The ending column number
* @property {(string|null)} nodeType Type of node
* @property {string} source Source text
* @property {({text: string, range: (number[]|null)}|null)} [fix] The fix object
* @property {Array<{text: string, range: (number[]|null)}|null>} [suggestions] Suggestion info
*/

//------------------------------------------------------------------------------
// Module Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -239,7 +224,7 @@ function mapSuggestions(descriptor, sourceCode, messages) {
* @param {{start: SourceLocation, end: (SourceLocation|null)}} options.loc Start and end location
* @param {{text: string, range: (number[]|null)}} options.fix The fix object
* @param {Array<{text: string, range: (number[]|null)}>} options.suggestions The array of suggestions objects
* @returns {function(...args): ReportInfo} Function that returns information about the report
* @returns {LintMessage} Information about the report
*/
function createProblem(options) {
const problem = {
Expand Down Expand Up @@ -314,7 +299,7 @@ function validateSuggestions(suggest, messages) {
* problem for the Node.js API.
* @param {{ruleId: string, severity: number, sourceCode: SourceCode, messageIds: Object, disableFixes: boolean}} metadata Metadata for the reported problem
* @param {SourceCode} sourceCode The `SourceCode` instance for the text being linted
* @returns {function(...args): ReportInfo} Function that returns information about the report
* @returns {function(...args): LintMessage} Function that returns information about the report
*/

module.exports = function createReportTranslator(metadata) {
Expand Down
8 changes: 6 additions & 2 deletions lib/shared/types.js
Expand Up @@ -96,10 +96,12 @@ module.exports = {};
* @property {number|undefined} column The 1-based column number.
* @property {number} [endColumn] The 1-based column number of the end location.
* @property {number} [endLine] The 1-based line number of the end location.
* @property {boolean} fatal If `true` then this is a fatal error.
* @property {boolean} [fatal] If `true` then this is a fatal error.
* @property {{range:[number,number], text:string}} [fix] Information for autofix.
* @property {number|undefined} line The 1-based line number.
* @property {string} message The error message.
* @property {string} [messageId] The ID of the message in the rule's meta.
* @property {(string|null)} nodeType Type of node
* @property {string|null} ruleId The ID of the rule which makes this message.
* @property {0|1|2} severity The severity of this message.
* @property {Array<{desc?: string, messageId?: string, fix: {range: [number, number], text: string}}>} [suggestions] Information for suggestions.
Expand All @@ -110,10 +112,12 @@ module.exports = {};
* @property {number|undefined} column The 1-based column number.
* @property {number} [endColumn] The 1-based column number of the end location.
* @property {number} [endLine] The 1-based line number of the end location.
* @property {boolean} fatal If `true` then this is a fatal error.
* @property {boolean} [fatal] If `true` then this is a fatal error.
* @property {{range:[number,number], text:string}} [fix] Information for autofix.
* @property {number|undefined} line The 1-based line number.
* @property {string} message The error message.
* @property {string} [messageId] The ID of the message in the rule's meta.
* @property {(string|null)} nodeType Type of node
* @property {string|null} ruleId The ID of the rule which makes this message.
* @property {0|1|2} severity The severity of this message.
* @property {Array<{kind: string, justification: string}>} suppressions The suppression info.
Expand Down
9 changes: 6 additions & 3 deletions tests/lib/cli-engine/cli-engine.js
Expand Up @@ -577,7 +577,8 @@ describe("CLIEngine", () => {
severity: 2,
message: "Parsing error: Unexpected token is",
line: 1,
column: 19
column: 19,
nodeType: null
}
],
suppressedMessages: [],
Expand Down Expand Up @@ -622,7 +623,8 @@ describe("CLIEngine", () => {
severity: 2,
message: "Parsing error: Unexpected token",
line: 1,
column: 10
column: 10,
nodeType: null
}
],
suppressedMessages: [],
Expand Down Expand Up @@ -713,7 +715,8 @@ describe("CLIEngine", () => {
severity: 2,
message: "Parsing error: Unexpected token is",
line: 1,
column: 19
column: 19,
nodeType: null
}
],
suppressedMessages: [],
Expand Down
9 changes: 6 additions & 3 deletions tests/lib/eslint/eslint.js
Expand Up @@ -690,7 +690,8 @@ describe("ESLint", () => {
severity: 2,
message: "Parsing error: Unexpected token is",
line: 1,
column: 19
column: 19,
nodeType: null
}
],
suppressedMessages: [],
Expand Down Expand Up @@ -730,7 +731,8 @@ describe("ESLint", () => {
severity: 2,
message: "Parsing error: Unexpected token",
line: 1,
column: 10
column: 10,
nodeType: null
}
],
suppressedMessages: [],
Expand Down Expand Up @@ -819,7 +821,8 @@ describe("ESLint", () => {
severity: 2,
message: "Parsing error: Unexpected token is",
line: 1,
column: 19
column: 19,
nodeType: null
}
],
suppressedMessages: [],
Expand Down
13 changes: 9 additions & 4 deletions tests/lib/eslint/flat-eslint.js
Expand Up @@ -506,7 +506,8 @@ describe("FlatESLint", () => {
severity: 2,
message: "Parsing error: Unexpected token is",
line: 1,
column: 19
column: 19,
nodeType: null
}
],
suppressedMessages: [],
Expand Down Expand Up @@ -546,7 +547,8 @@ describe("FlatESLint", () => {
severity: 2,
message: "Parsing error: Unexpected token",
line: 1,
column: 10
column: 10,
nodeType: null
}
],
suppressedMessages: [],
Expand Down Expand Up @@ -636,7 +638,8 @@ describe("FlatESLint", () => {
severity: 2,
message: "Parsing error: Unexpected token is",
line: 1,
column: 19
column: 19,
nodeType: null
}
],
suppressedMessages: [],
Expand Down Expand Up @@ -5088,9 +5091,11 @@ describe("FlatESLint", () => {
fixableWarningCount: 0,
messages: [
{
ruleId: null,
fatal: false,
message: "File ignored by default. Use \"--ignore-pattern '!node_modules/*'\" to override.",
severity: 1
severity: 1,
nodeType: null
}
],
usedDeprecatedRules: [],
Expand Down

0 comments on commit 94da96c

Please sign in to comment.