-
Notifications
You must be signed in to change notification settings - Fork 91
fix!: remap locations for unused disable directives #669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
375a6c3
e86c647
4ae8ad0
68ae81e
f217132
32d4418
dbfbf94
d180d95
ac86d12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,8 @@ import { fromMarkdown } from "mdast-util-from-markdown"; | |
| * @import { LintMessage, RuleTextEdit, SourceRange } from "@eslint/core"; | ||
| * @import { Node, Parent, Code, Html } from "mdast"; | ||
| * @import { Block, RangeMap } from "./types.js"; | ||
| * @typedef { Block['comments'][number] } Comment | ||
| * @typedef {{ comment: Comment, jsOffset: number }} CommentMapping | ||
| */ | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
|
|
@@ -30,6 +32,8 @@ const UNSATISFIABLE_RULES = new Set([ | |
| const SUPPORTS_AUTOFIX = true; | ||
|
|
||
| const BOM = "\uFEFF"; | ||
| const unusedDirectiveMessagePattern = | ||
| /^Unused eslint-(?:disable|enable) directive/u; | ||
|
|
||
| /** | ||
| * @type {Map<string, Block[]>} | ||
|
|
@@ -138,7 +142,7 @@ function getIndentText(text, node) { | |
| * delta at the beginning of each line. | ||
| * @param {string} text The text of the file. | ||
| * @param {Code} node A Markdown code block AST node. | ||
| * @param {string[]} comments List of configuration comment strings that will be | ||
| * @param {Comment[]} comments List of configuration comment objects that will be | ||
| * inserted at the beginning of the code block. | ||
| * @returns {RangeMap[]} A list of offset-based adjustments, where lookups are | ||
| * done based on the `js` key, which represents the range in the linted JS, | ||
|
|
@@ -176,7 +180,7 @@ function getBlockRangeMap(text, node, comments) { | |
| * of the linted JS and start the JS offset lookup keys at this index. | ||
| */ | ||
| const commentLength = comments.reduce( | ||
| (len, comment) => len + comment.length + 1, | ||
| (len, comment) => len + comment.text.length + 1, | ||
| 0, | ||
| ); | ||
|
|
||
|
|
@@ -237,6 +241,67 @@ function getBlockRangeMap(text, node, comments) { | |
| return rangeMap; | ||
| } | ||
|
|
||
| /** | ||
| * Determines whether a message reports an unused directive. | ||
| * @param {LintMessage} message The message to check. | ||
| * @returns {boolean} True if the message reports an unused directive. | ||
| */ | ||
| function isUnusedDirectiveMessage(message) { | ||
| return ( | ||
| message.ruleId === null && | ||
| unusedDirectiveMessagePattern.test(message.message) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Adjusts an unused directive message in an inserted JS comment. | ||
| * @param {LintMessage} message The message to adjust. | ||
| * @param {Map<number, CommentMapping>} commentMappings Precomputed comment mappings, keyed by generated line. | ||
| * @returns {LintMessage} The adjusted message, if it can be mapped. | ||
| */ | ||
| function adjustCommentMessage(message, commentMappings) { | ||
| const mapping = commentMappings.get(message.line); | ||
|
|
||
| if (!mapping) { | ||
| return message; | ||
| } | ||
|
|
||
| const { comment: foundComment, jsOffset } = mapping; | ||
| const { start, end } = foundComment.position; | ||
| const { fix, ...messageWithoutFix } = message; | ||
|
|
||
| const adjustedMessage = /** @type {LintMessage} */ ({ | ||
| ...messageWithoutFix, | ||
| line: start.line, | ||
| column: start.column, | ||
| endLine: end.line, | ||
| endColumn: end.column, | ||
| }); | ||
|
xbinaryx marked this conversation as resolved.
|
||
|
|
||
| if (fix) { | ||
| const isFullRemoval = | ||
| fix.range[0] <= jsOffset && | ||
| fix.range[1] >= jsOffset + foundComment.text.length; | ||
|
|
||
| if (isFullRemoval) { | ||
| adjustedMessage.fix = { | ||
| range: [start.offset, end.offset], | ||
| text: fix.text, | ||
| }; | ||
| } else { | ||
| // '4' is the length of '<!--' and '2' is the length of '/*'. | ||
| const offsetDelta = start.offset + 4 - (jsOffset + 2); | ||
|
xbinaryx marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- src/processor.js: relevant region ---'
sed -n '240,325p' src/processor.js
printf '%s\n' '--- related symbols ---'
rg -n -C 3 'adjustCommentMessage|offsetDelta|comment\.text|start\.offset|unused|directive' src test tests 2>/dev/null || true
printf '%s\n' '--- repository files for focused fixtures ---'
git ls-files | rg '(^|/)(test|tests|spec|fixtures)(/|$)|processor'Repository: eslint/markdown Length of output: 50373 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- src/processor.js: comment creation and mapping ---'
sed -n '130,240p' src/processor.js
sed -n '340,385p' src/processor.js
sed -n '440,510p' src/processor.js
printf '%s\n' '--- multiline and partial-fix tests ---'
sed -n '576,735p' tests/plugin.test.js
printf '%s\n' '--- package versions and parser configuration ---'
rg -n -C 2 'mdast-util-from-markdown|micromark|remark|fromMarkdown|processor' package.json package-lock.json yarn.lock pnpm-lock.yaml 2>/dev/null || trueRepository: eslint/markdown Length of output: 12296 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- parser setup and HTML-node handling ---'
rg -n -C 5 'fromMarkdown|html\(|allowDangerousHtml|htmlComments|parse' src tests | head -260
printf '%s\n' '--- source-code type and parser imports ---'
sed -n '1,140p' src/language/markdown-source-code.js
printf '%s\n' '--- exact dependency resolution ---'
rg -n -A4 -B2 '"mdast-util-from-markdown"|"micromark"' package-lock.json npm-shrinkwrap.json yarn.lock pnpm-lock.yaml 2>/dev/null | head -120
printf '%s\n' '--- all existing nested/container comment fixtures ---'
rg -n -C 4 '^[[:space:]]*>|^[[:space:]]*[-+*][[:space:]]|eslint-disable|eslint-enable' tests/plugin.test.js | rg -n -C 2 '>|eslint-disable|eslint-enable' | head -260Repository: eslint/markdown Length of output: 30720 🌐 Web query:
💡 Result: The release of mdast-util-from-markdown version 2.0.2, published on October 25, 2024, was a maintenance release focused on internal refactoring [1][2][3]. Specifically, it updated internal type definitions and refactored the codebase to use modern Citations:
🌐 Web query:
💡 Result: In the context of parsing Markdown into a syntax tree (specifically using mdast and unified/remark), HTML elements—including HTML comments like —are typically treated as distinct nodes in the syntax tree [1][2]. When a blockquote contains an HTML comment, the parser (such as micromark, which powers remark-parse and mdast-util-from-markdown) typically handles the HTML block as a "black box" [2]. Within the resulting mdast (Markdown Abstract Syntax Tree), this is often represented by a node with the type 'html' [1]. Regarding your query about the value and position of such nodes: 1. Value: The value property of an 'html' node contains the raw string of the HTML, including the comment markers [1][3]. 2. Position: The position property provides offset information (start and end) in the source document [4]. Note that in complex structures like blockquotes, the positional offsets often include the raw source characters, such as the blockquote markers (e.g., '> ') used for line continuation, even if those markers are stripped from the resulting node's value [5]. This means the position span in the source text is wider than the length of the string found in the node's value [5]. If you are encountering issues where comments or other HTML structures are not behaving as expected within a blockquote, it is often due to how the parser handles the transition from Markdown syntax to HTML blocks. Processing such structures in the 'hast' (HTML Abstract Syntax Tree) space after parsing is generally recommended for safer and more reliable manipulation [6][3]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- exact fix-range and comment-position flow ---'
sed -n '1,130p' src/processor.js
sed -n '330,380p' src/processor.js
sed -n '400,445p' src/processor.js
printf '%s\n' '--- relevant test assertions after multiline case ---'
sed -n '705,750p' tests/plugin.test.jsRepository: eslint/markdown Length of output: 7814 Map continuation-line offsets for nested multiline comments.
🤖 Prompt for AI Agents
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One last suggestion: Regarding the AI review, the current autofix for nested blockquotes ( It would be helpful to fix this bug and add regression tests to prevent it from recurring.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- processor symbols ---'
ast-grep outline src/processor.js --items all --type function --match 'adjustCommentMessage|adjustBlock|getBlockRangeMap'
printf '%s\n' '--- mapping type definitions ---'
ast-grep outline src/types.ts --items all
rg -n -C 5 'CommentMapping|comments:|rangeMap|adjustCommentMessage|adjustBlock' src/processor.js src/types.ts
printf '%s\n' '--- unused-directive and autofix tests ---'
rg -n -C 12 'reportUnusedDisableDirectives|Unused eslint|eslint-disable-next-line|output|fix' tests/plugin.test.js
printf '%s\n' '--- nested Markdown container fixtures ---'
rg -n -C 8 '^[[:space:]]*>|^[[:space:]]*[-+*][[:space:]]|eslint-disable|eslint-enable' tests/plugin.test.jsLength of output: 96563
The fix should map each generated comment line to its own Markdown source offset. Please add regression tests in both the LegacyESLint and FlatESLint unused-directive autofix suites. The tests should cover:
🐇 You are interacting with an AI system. |
||
|
|
||
| adjustedMessage.fix = { | ||
| range: [fix.range[0] + offsetDelta, fix.range[1] + offsetDelta], | ||
| text: fix.text, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| return adjustedMessage; | ||
| } | ||
|
|
||
| const codeBlockFileNameRegex = /filename=(?<quote>["'])(?<filename>.*?)\1/u; | ||
|
|
||
| /** | ||
|
|
@@ -278,7 +343,7 @@ function preprocess(sourceText, filename) { | |
| * block immediately follows such a sequence, insert the comments at the | ||
| * top of the code block. Any non-ESLint comment or other node type breaks | ||
| * and empties the sequence. | ||
| * @type {string[]} | ||
| * @type {Comment[]} | ||
| */ | ||
| let htmlComments = []; | ||
|
|
||
|
|
@@ -294,16 +359,19 @@ function preprocess(sourceText, filename) { | |
| */ | ||
| code(node) { | ||
| if (node.lang) { | ||
| /** @type {string[]} */ | ||
| /** @type {Comment[]} */ | ||
| const comments = []; | ||
|
|
||
| for (const comment of htmlComments) { | ||
| if (comment.trim() === "eslint-skip") { | ||
| if (comment.text.trim() === "eslint-skip") { | ||
| htmlComments = []; | ||
| return; | ||
| } | ||
|
|
||
| comments.push(`/*${comment}*/`); | ||
| comments.push({ | ||
| text: `/*${comment.text}*/`, | ||
| position: comment.position, | ||
| }); | ||
| } | ||
|
|
||
| htmlComments = []; | ||
|
|
@@ -326,7 +394,7 @@ function preprocess(sourceText, filename) { | |
| const comment = getComment(node.value); | ||
|
|
||
| if (comment) { | ||
| htmlComments.push(comment); | ||
| htmlComments.push({ text: comment, position: node.position }); | ||
| } else { | ||
| htmlComments = []; | ||
| } | ||
|
|
@@ -345,7 +413,9 @@ function preprocess(sourceText, filename) { | |
|
|
||
| return { | ||
| filename: fileNameFromMeta(block) ?? `${index}.${fileExtension}`, | ||
| text: [...block.comments, block.value, ""].join("\n"), | ||
| text: [...block.comments.map(c => c.text), block.value, ""].join( | ||
| "\n", | ||
| ), | ||
| }; | ||
| }); | ||
| } | ||
|
|
@@ -386,10 +456,24 @@ function adjustFix(block, fix) { | |
| * @returns {(message: LintMessage) => LintMessage | null} A function that adjusts messages in a code block. | ||
| */ | ||
| function adjustBlock(block) { | ||
| const leadingCommentLines = block.comments.reduce( | ||
| (count, comment) => count + comment.split("\n").length, | ||
| 0, | ||
| ); | ||
| /** @type {Map<number, CommentMapping>} */ | ||
| const commentMappings = new Map(); | ||
| let currentLine = 1; | ||
| let jsOffset = 0; | ||
|
|
||
| for (const comment of block.comments) { | ||
| const commentLines = comment.text.split("\n").length; | ||
| const mapping = { comment, jsOffset }; | ||
|
|
||
| for (let i = 0; i < commentLines; i++) { | ||
| commentMappings.set(currentLine + i, mapping); | ||
| } | ||
|
|
||
| currentLine += commentLines; | ||
| jsOffset += comment.text.length + 1; | ||
| } | ||
|
|
||
| const leadingCommentLines = currentLine - 1; | ||
|
|
||
| const blockStart = block.position.start.line; | ||
|
|
||
|
|
@@ -410,7 +494,9 @@ function adjustBlock(block) { | |
| const lineInCode = message.line - leadingCommentLines; | ||
|
|
||
| if (lineInCode < 1 || lineInCode >= block.rangeMap.length) { | ||
| return null; | ||
| return isUnusedDirectiveMessage(message) | ||
| ? adjustCommentMessage(message, commentMappings) | ||
| : null; | ||
| } | ||
|
|
||
| /** @type {Pick<LintMessage, "line" | "column" | "endLine" | "suggestions">} */ | ||
|
|
||


Uh oh!
There was an error while loading. Please reload this page.