Change Request: Correct and unify LintMessage type

[btmills]

ESLint version

v8.35.0

What problem do you want to solve?

For the processor docs update, we need to know why the LintMessage JSDoc typedef differs from real lint messages I captured at runtime. The short version is "because it doesn't reflect the implementation." I'll update the typedef or implementation, whichever is appropriate, to make sure they match.

What do you think is the correct solution?

After examining the implementation, I'm confident in at least three changes to the LintMessage type:

 /**
  * @typedef {Object} LintMessage
  * @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.
  */

1. Many places that create LintMessages 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 not because it's updating JSDoc documentation to reflect the reality of the implementation.
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.

In addition, there are a couple types we can unify:

1. There are a handful of places that currently reference an undefined Problem type that really means LintMessage, and I'll update those.
2. The ReportInfo type duplicates LintMessage, and I'll update that to reference LintMessage instead.

Participation

• I am willing to submit a pull request for this change.

Additional comments

I have two open questions:

1. Should I make nodeType non-nullable and update the places in the implementation that explicitly set it to null to omit it instead? A few places already omit it for e.g. parse errors or ignored file messages, so API consumers already have to check that it might not be set.

2. line and column are currently required but possibly undefined.

   • Ignored file messages currently omit line and column.
   • Linter explicitly sets them to 0 for two configuration errors (configured parser not found and no configuration found for file).
   • Linter's createLintingProblem() has a DEFAULT_ERROR_LOC that defaults to line 1 column 0, but the default is only used for missing rule definition messages.

   Should I either:

   1. Keep line and column required but possibly undefined and update ignored file messages to explicitly set them to 0, following precedent elsewhere?
   2. Or make line and column optional and remove fallback 0/0 or 1/0 values from configuration issues?

   The second option is how I'd design the API from scratch because I wouldn't report a location unless we had a real location. But it might be a breaking change. Even though the type is specified as possibly undefined, in practice, some API consumers might only see messages that have real or fallback locations if they don't encounter ignored file messages. The second option would slightly increase the prevalence of location-less messages.

[fasttime]

I recall from a previous discussion that changes in JSDoc should not be treated as breaking, so there should be no problem in redeclaring line and column as optional. As for removing the different fallbacks, that's a change that could affect existing implementations. The same is true for omitting nodeType where it is being set to null. I'd like to know what the rest of the team thinks about this.

[nzakas]

Thanks for flagging this! I think updating the type definition so it reflects reality is the right way to go. Because we don't export types, this is non-breaking.

To answer your questions:

Should I make nodeType non-nullable and update the places in the implementation that explicitly set it to null to omit it instead?

I think this is okay because it appears we normalize nodeType before we expose it externally.

Keep line and column required but possibly undefined and update ignored file messages to explicitly set them to 0, following precedent elsewhere?

I also think this option is preferable to limit the amount of changes we are introducing into the codebase. I agree that if we were doing type checking from the start, we'd probably want the other option, but since we aren't, I'll favor the less invasive change.

@mdjermanovic what do you think?