eslint / markdown

Lint JavaScript code blocks in Markdown documents
MIT License
409 stars 63 forks source link

Bug: False positive `markdown/no-missing-label-refs` on GFM Alerts #294

Open midskyey opened 1 month ago

midskyey commented 1 month ago

Environment

ESLint version: 9.12.0 @eslint/markdown version: 6.2.1 Node version: 22.9.0 npm version: 10.8.3 Operating System: macOS 14.7

Which language are you using?

gfm

What did you do?

Configuration ```js import pluginMd from "@eslint/markdown" export default [ // Silly type narrowing for spreading in full-fledged typechecked `eslint.config.js` ...[pluginMd.configs?.recommended].filter(Boolean).flat(), { files: ["**/*.md"], language: "markdown/gfm", }, ] ```
> [!NOTE]
> Useful information that users should know, even when skimming content.

> [!TIP]
> Helpful advice for doing things better or more easily.

> [!IMPORTANT]
> Key information users need to know to achieve their goal.

> [!WARNING]
> Urgent info that needs immediate user attention to avoid problems.

> [!CAUTION]
> Advises about risks or negative outcomes of certain actions.

Which is being rendered on GH as:

[!NOTE] Useful information that users should know, even when skimming content.

[!TIP] Helpful advice for doing things better or more easily.

[!IMPORTANT] Key information users need to know to achieve their goal.

[!WARNING] Urgent info that needs immediate user attention to avoid problems.

[!CAUTION] Advises about risks or negative outcomes of certain actions.

What did you expect to happen?

No violations.

What actually happened?

   1:4  error  Label reference '!NOTE' not found       markdown/no-missing-label-refs
   4:4  error  Label reference '!TIP' not found        markdown/no-missing-label-refs
   7:4  error  Label reference '!IMPORTANT' not found  markdown/no-missing-label-refs
  10:4  error  Label reference '!WARNING' not found    markdown/no-missing-label-refs
  13:4  error  Label reference '!CAUTION' not found    markdown/no-missing-label-refs

Link to Minimal Reproducible Example

https://stackblitz.com/edit/118f65b5-5b7b-4190-9fbe-b023b0542a4e?file=package.json

Participation

Additional comments

More info on GFM Alerts:

mdjermanovic commented 1 month ago

Thanks for the issue! I can reproduce this with @eslint/markdown v6.2.1 (and v6.2.0 as well, so this isn't a regression introduced in v6.2.1).

I think this should be fixed in the parser as it treats [!NOTE] and others as just plain text, while it should probably be a new node type. @eslint/eslint-team thoughts?

nzakas commented 1 month ago

Yes, this is an issue with the parser not supporting GitHub alerts. It needs to be fixed there. Opened: https://github.com/syntax-tree/mdast-util-gfm/issues/2

nzakas commented 1 month ago

So it looks like alerts are not actually part of GFM and are just an HTML transform: https://github.com/syntax-tree/mdast-util-gfm/issues/2#issuecomment-2422827671

By "HTML transform", they mean that the parsing of the Markdown isn't any different, it's just the way that the Markdown is interpreted while transforming to HTML that changes.

Here's the GFM spec: https://github.github.com/gfm/

We have a couple options for what to do here:

  1. We could create a plugin that generates a separate AST for this syntax to make it more easily identifiable in the AST.
  2. We could add an option to the rule that filters out known GitHub alert labels.
JoshuaKGoldberg commented 1 month ago

Agreed on 1, this feels like a plugin to me. GitHub alert labels are just one example of a nonstandard extension to GFM. Folks writing .md for other platforms won't want them.

mdjermanovic commented 1 month ago
  1. We could create a plugin that generates a separate AST for this syntax to make it more easily identifiable in the AST.

We would then always use this plugin when parsing in gfm mode? That seems like a nice solution, though our parser would be doing something unique because other markdown parsers treat this as plain text, so that might be surprising to rule authors?

nzakas commented 1 month ago

We would then always use this plugin when parsing in gfm mode? That seems like a nice solution, though our parser would be doing something unique because other markdown parsers treat this as plain text, so that might be surprising to rule authors?

Yes. I think we should maintain the sanctity of what "GFM" means and ensure it matches the spec by default. If we want to create an AST representation of alerts, then I think that should be opt-in as a language option for markdown/gfm.