WIP Implement suppression of plugin diagnostics #4667

[wanghaoPolar]

This doc discuss changes related to Implement suppression of plugin diagnostics #4667

I'm new to biome and open source, please forgive any naive from my side. 🙇‍♂️

Below I list how suppression comment works in biome and possible changes need to make for #4667

Step 1: Parse suppression comment to {[{ category, value }], reason}

The parser for comments is at `/crates/biome_suppression/src/lib.rs``

fn parse_suppression_comment
fn parse_suppression_line

For each comment line that starts with 'biome-ignore', parse by syntax { <category> { (<value>) }? }+: <reason> and return { [ { category, value } ], reason}

For example, with comment // biome-ignore lint/a11y/noAccessKey(v) some reason for ignoring
Return {[{category="lint/a11y/noAccessKey", value="v"}], reason="some reason for ignoring"}

All categories are prebuild and defined in crates/biome_diagnostics_categories/src/categories.rs. if not included, will cause parse fail.

Task 1:

The initial proposed format to suppress plugin diagnostics is // biome-ignore plugin/<plugin-name> <reason>
category part is plugin/<plugin-name>, it's dynamic, we don't know the plugin-name at build time.

I assume that we should keep category static and predefined, so to handle dynamic plugin name, we can:

choice 1. add new slot for dynamic subcategory, in which we can put the plugin-name

change syntax to { <category> { [<dynamic-subcategory>] }? { (<value>) }? }+: <reason>,

e.g. // biome-ignore plugin[my-plugin](v) reason to ignore => {[{category: "plugin", dynamic_subcategory: "my-plugin", value: "v"}], reason: "reason to ignore"}

in this case will make change to parse_suppression_line method

choice 2. start with biome-ignore-plugin, then use custom syntax to parse

custom syntax for plugin: { <plugin-name> { (<value>) }? }+: <reason>
e.g. // biome-ignore-plugin my-plugin my-plugin-2(v) reason to ignore => {[{category: "plugin", plugin_name: "my-plugin"}, {category:"plugin", plugin_name: "my-plugin-2", value: "v"}], reason: "reason to ignore"}

int this case, will parse with new method if prefix with 'biome-ignore-plugin'

Personally I think should go with choice 1 because it's less hacky and ad-hoc and work with other suppressions.

Step 2: Generate SuppressionKind from {category, value}

after getting {category, value}, generate corresponding SuppressionKind, defined at crates/biome_analyze/src/lib.rs

pub enum SuppressionKind<'a> {
    /// A suppression disabling all lints eg. `// biome-ignore lint`
    Everything,
    /// A suppression disabling a specific rule eg. `// biome-ignore lint/style/useWhile`
    Rule(&'a str),
    /// A suppression to be evaluated by a specific rule eg. `// biome-ignore lint/correctness/useExhaustiveDependencies(foo)`
    RuleInstance(&'a str, &'a str),
    /// A suppression using the legacy syntax to disable a specific rule eg. `// biome-ignore lint(style/useWhile)`
    MaybeLegacy(&'a str),
    /// `rome-ignore` is legacy
    Deprecated,
}

Task 2

for this step, need to transform the info got from previous step into SuppressionKind

1. add two new kinds:

pub enum SuppressionKind<'a> {
    /// ...
    Plugin(&'a str)
    PluginInstance(&'a str, &'a str),
}

2. then extract the plugin name and value (if exist), to create SuppressionKind accordingly.

Step 3: Ignore signal by SuppressionKind

in crates/biome_analyze/src/lib.rs, at handle_comment method, it transform SuppressionKind to LineSuppression

struct LineSuppression {
    /// Line index this comment is suppressing lint rules for
    line_index: usize,
    /// Range of source text covered by the suppression comment
    comment_span: TextRange,
    /// Range of source text this comment is suppressing lint rules for
    text_range: TextRange,
    /// Set to true if this comment has set the `suppress_all` flag to true
    /// (must be restored to false on expiration)
    suppress_all: bool,
    /// List of all the rules this comment has started suppressing (must be
    /// removed from the suppressed set on expiration)
    suppressed_rules: Vec<RuleFilter<'static>>,
    /// List of all the rule instances this comment has started suppressing.
    suppressed_instances: Vec<(RuleFilter<'static>, String)>,
    /// Set to `true` when a signal matching this suppression was emitted and
    /// suppressed
    did_suppress_signal: bool,
}

then at flush_matches method, when handling each signal, it try to find a LineSuppression that match the signal by line_index, text_range and suppress filters, if match, the signal is suppressed.

Task 3

add a new suppress filter

struct LineSuppression {
    /// ...
    suppressed_plugins: Vec<String>,
}

for SuppressionKind::Plugin and SuppressionKind::PluginInstance, create LineSuppression with suppressed_plugins field accordingly

then at flush_matches, in the matching process, if any plugin in suppressed_plugins equals to the signal's plugin field, can ignore the signal.

TODO: check what does a signal raised from plugin looks like

[arendjr]

Thanks for the clear write-up!

Task 1

You make a good observation that we'll need a dynamic subcategory. So the static category can remain plugin, which I think already exists. But instead of changing the syntax as you suggest in your choices, I'd try to see if you can "special-case" the plugin case: Just look whether the category starts with "plugin/", and if so, set the category to plugin, and the subcategory to whatever comes after. This way, we don't need a special syntax and can still handle a dynamic subcategory for this use case.

The reason why I'd suggest doing it this way is because I would try to avoid leaking the implementation details of static vs. dynamic categories to the user. If we introduce a special syntax for this, it just adds confusion: Why another syntax? Why can't it work like other categories? We'd have no way to explain the rationale without explaining the implementation.

Task 2

I don't think we'll need PluginInstance just yet, so let's keep it out until there is a need for it. Otherwise your suggestion sounds good 👍

Task 3

Sounds good to me! 🚀

PS.: Maybe @ematipico also has some insights here that I might've missed.

[ematipico]

All categories are prebuild and defined in crates/biome_diagnostics_categories/src/categories.rs. if not included, will cause parse fail.

That's not technically correct. That file contains is there only to create a macro called category! that fails at runtime. So, internally, we have auto completion and the macro fails if we pass a category we don't expect .

Under the hoods, categories can be whatever we want.

Regarding task 3, there's no need for a new suppression kind I think, because plugins aren't going to introduce a new variant of biome-ignore-*, so they will behave as current suppressions.

[wanghaoPolar]

Thanks for helping review! I will fix the errors and finalize details in this doc soon and start the implementation 🤝