feat: make no-irregular-whitespace language-agnostic#150
Conversation
|
|
||
| Specifically: | ||
|
|
||
| 1. **Standardize `comments` property on `SourceCode`:** - A `comments` property is to be added to the JavaScript `SourceCode` class. The `CSSSourceCode` and `JSONSourceCode` classes already expose this property. `MarkdownSourceCode` does not expose it, as Markdown does not have a direct equivalent to comments. |
There was a problem hiding this comment.
If we standardize the comments property (for which I am in favor of), we should also update TextSourceCodeBase from @eslint/plugin-kit (and update the source codes of the our languages respectively). For Markdown this would either be setting it to an empty array or extracting the HTML comments.
There was a problem hiding this comment.
@DMartens can you provide more details on what should be updated in TextSourceCodeBase?
|
|
||
| ## Documentation | ||
|
|
||
| - [Custom Rules documentation](https://eslint.org/docs/latest/extend/custom-rules) should be updated to document the new `comments` property on the `SourceCode` object. |
There was a problem hiding this comment.
The documentation for creating languages should also be updated to require the comments property if accepted.
| checkForIrregularWhitespace(node); | ||
| checkForIrregularLineTerminators(node); |
There was a problem hiding this comment.
A peculiarity of the invalid characters "\u2028" and "\u2029" is that JavaScript treats them as line separators, whereas CSS, JSON, and Markdown do not. To calculate report locations correctly for different languages, checkForIrregularLineTerminators would need to account for language-specific line-separators (instead of relying on the hardcoded and JavaScript-specific regular expressions LINE_BREAK and IRREGULAR_LINE_TERMINATORS).
A simpler approach could be to calculate start and end locations from character indices rather than line and column separately, and at that point we could merge checkForIrregularWhitespace and checkForIrregularLineTerminators into a single implementation. For example:
function checkForIrregularWhitespaceOrLineTerminators(node) {
let match;
while (
(match = IRREGULAR_CHARACTERS.exec(sourceCode.text)) !== null
) {
errors.push({
node,
messageId: "noIrregularWhitespace",
loc: {
start: sourceCode.getLocFromIndex(match.index),
end: sourceCode.getLocFromIndex(match.index + match[0].length),
},
});
}
}The alternative, I think, would involve exposing the line separators for each language as a new property, but using sourceCode.getLocFromIndex() seems simpler, and it should work out of the box.
There was a problem hiding this comment.
I'm in favor of the approach with checkForIrregularWhitespaceOrLineTerminators(node).
|
|
||
| ## Drawbacks | ||
|
|
||
| The existing JS-specific options (`skipStrings`, `skipTemplates`, `skipRegExps`, `skipJSXText`) will have no effect when the rule is used on non-JS files, since the corresponding AST node types (`Literal`, `TemplateElement`, `JSXText`) do not exist in CSS, JSON, or Markdown ASTs. The listeners are registered but never triggered. The new `skipNodes` option is the intended mechanism for non-JS exclusions. |
There was a problem hiding this comment.
This would work, but it seem a bit fragile because it assumes that JavaScript is the only language with node types named Literal, TemplateElement or JSXText. Maybe a safer solution is checking (somehow) if the language is JavaScript or TypeScript, and registering the visitors only in that case.
|
|
||
| This crash occurs because the rule assumes that the `SourceCode` object will always expose a `getAllComments()` method, which is JavaScript-specific and not implemented by the `@eslint/css`, `@eslint/json`, or `@eslint/markdown` language plugins. Furthermore, the rule assumes specific JS-specific AST node types (like `Literal`, `TemplateElement`, `JSXText`) to filter out whitespace inside strings and templates, and assumes the root AST node is always `Program`. | ||
|
|
||
| The goal is to make this rule language-agnostic in its basic operation, ensuring it checks for irregular whitespace characters across any language without crashing. This requires unifying how rules access comment nodes by standardizing on a `comments` property across all ESLint language plugins. |
There was a problem hiding this comment.
I also think that standardizing a comments property is the most versatile solution.
|
|
||
| ## Open Questions | ||
|
|
||
| With the addition of the `comments` property to the JavaScript `SourceCode` class, its existing `getAllComments()` method becomes redundant. Should a formal deprecation (via JSDoc `@deprecated` tag and documentation updates) be included in the scope of this RFC, or should it be deferred to avoid immediate ecosystem churn? |
There was a problem hiding this comment.
If there is consensus on generalizing the comments property as proposed in the RFC, I think it's also safe to deprecate getAllComments() at the same time. ESLint deprecations are documentation-only, so not much will change for existing consumers.
There was a problem hiding this comment.
I'm in favor of generalizing the comments property and deprecating getAllComments() on JS SourceCode.
Summary
Make the
no-irregular-whitespacerule language-agnostic to prevent crashes on non-JavaScript files (e.g., CSS, JSON, Markdown), while accurately reporting irregular whitespace where applicable.Related Issues
eslint/eslint#19805