feat(analyzer): top level suppression #4306

ematipico · 2024-10-15T14:45:12Z

Summary

This PR introduces a new type of suppression: top-level suppression.

TL;DR:

/** biome-ignore lint/style/useConst: reason */

let foo = 1;
let bar = 12;

The top-level suppression works only if:

it's at the beginning of the file
there are two newlines after the comment

This means this comment is seen as a "next line suppression", and it should raise a diagnostic for let bar = 12:

/** biome-ignore lint/style/useConst: reason */
let foo = 1;
let bar = 12;

To recognise them, the parse_suppression_comment now accepts a SyntaxToken. If the range of the comment starts at 0 and there are exactly two newlines after the comment, it means that it's a top-level suppression. The range check is done with the trivia, because we want to make sure to catch the 0 position.

This PR also removes legacy suppression comments.

More changes

The Rule type generates the suppression via R::suppress function. Initially, I wanted to change the type returned to a Vec, but that doesn't play well with our infrastructure because the returned type is mapped exactly to a code action, which means that returning a vector wasn't possible.

I renamed R::suppress to R::inline_suppression, and I created a new function called R::toplevel_suppression. This function leverage the already existing trait SuppressionAction, which now requires to implement a new function called apply_top_level_suppression. The logic of this function is way simpler than that of inline suppression. This function accepts the first token of a root CST, and from there, it's very simple to prepend a new comment trivia.

I also created two new known variants to the action category called OtherActionCategory, where we can add new known LSP client signals. These two signals were added to the LSP client capabilities.

Test Plan

I added various tests for the CLI and the analyzer.

I also updated the LSP tests, which now send a new action for the top-level suppression. I manually tested this with Zed, and I can correctly see the new top-level action pulled and applied via the editor.

codspeed-hq · 2024-10-15T15:28:47Z

CodSpeed Performance Report

Merging #4306 will not alter performance

_{Comparing feat/top-level-suppression (716819c) with next (dc52b46)}

Summary

✅ 101 untouched benchmarks

vohoanglong0107 · 2024-10-16T22:57:19Z

May I ask why we need to enforce top level suppression to be a block comment? Some languages, like GraphQL 😅, only have single line comment.
Implementation wise I don't think it would impact other languages that much, but I think it's worth mentioning in the Changelog, something like "for languages without block comment, single line comment is sufficient"

ematipico · 2024-10-17T06:08:18Z

why we need to enforce top level suppression to be a block comment? Some languages, like GraphQL 😅, only have single line comment.

I wasn't aware of that. I should be able to lift the restriction

ematipico · 2024-10-17T09:42:56Z

@vohoanglong0107 The algorithm has been updated, and the heuristic is different. I updated the PR description to reflect that.

ematipico · 2024-10-18T15:17:43Z

crates/biome_js_analyze/tests/suppression/correctness/noUnusedVariables/ts-module-export.ts

This wasn't needed, it didn't emit any suppressions

ematipico · 2024-10-18T15:18:35Z

crates/biome_analyze/src/diagnostics.rs

        }
    }

-    pub(crate) fn with_tags(mut self, tags: DiagnosticTags) -> Self {
-        self.tags |= tags;
+    pub(crate) fn note(mut self, message: impl Into<String>, range: impl Into<TextRange>) -> Self {


This new API was created to suppose this new diagnostic: https://github.com/biomejs/biome/pull/4306/files#diff-6b7a758a5c6e91dd2722ccc423ad136ff4e811311c0d49b80c659d61cc54e2a6

Neat :)

ematipico · 2024-10-18T15:22:34Z

crates/biome_analyze/src/lib.rs

    /// List of all the rule instances this comment has started suppressing.
-    suppressed_instances: Vec<(RuleFilter<'static>, String)>,
+    suppressed_instances: FxHashMap<String, RuleFilter<'static>>,


I had to swap this, because the primary key is the instance, not the filter.

ematipico · 2024-10-18T15:23:10Z

crates/biome_analyze/src/lib.rs

-
-            (self.emit_signal)(&signal)?;
+        if is_top_level_suppression {
+            self.top_level_suppressions.expand_range(range);


We might encounter multiple top level comments, so we need to expand its initial range until the end.

ematipico · 2024-10-18T15:23:40Z

crates/biome_analyze/src/lib.rs

-    /// A suppression using the legacy syntax to disable a specific rule eg. `// biome-ignore lint(style/useWhile)`
-    MaybeLegacy(&'a str),


Not needed anymore. It was an old rome thing (the TS codebase one)

ematipico · 2024-10-18T15:24:36Z

crates/biome_analyze/src/signals.rs

+    pub fn is_inline_suppression(&self) -> bool {
+        self.category.matches(SUPPRESSION_INLINE_ACTION_CATEGORY)
+    }
+
+    pub fn is_top_level_suppression(&self) -> bool {
+        self.category.matches(SUPPRESSION_TOP_LEVEL_ACTION_CATEGORY)


We will need these two new functions for the upcoming feature from @anthonyshew

arendjr

Great stuff! I think people are going to love this!

I do have my doubts about the requirement to have two newlines following the suppression comment. I understand why you’re doing it, but it’s not intuitive and hard to explain. It feels like a “magic behavior” that you have to know about that will easily bite you if you don’t.

Can’t we have a syntax like biome-ignore-all so that we can lift this requirement?

ematipico · 2024-10-20T07:21:55Z

Great stuff! I think people are going to love this!

I do have my doubts about the requirement to have two newlines following the suppression comment. I understand why you’re doing it, but it’s not intuitive and hard to explain. It feels like a “magic behavior” that you have to know about that will easily bite you if you don’t.

Can’t we have a syntax like biome-ignore-all so that we can lift this requirement?

I put great thoughts into what you suggested. At the beginning, I thought it could work, however I found two downsides that are worth considering.

First, lifting the "2 newlines" requirement could cause issues with the import sorting, where a comment is moved with its relative import statement:

// biome-ignore-all lint/style/useConst: reason
import zod from "zod"
import lodash from "lodash" // added by IDE automatically

If we execute the import sorting, the first import statement will be moved to the end, together with the suppression comment. This will break the suppression system. The "2 newlines" are already understood by the import sorting, which are used as a separator for groups.

Second, we need to consider if this new suppression kind has repercussions on the formatter. If so, how?

So, if we would want to implement a new suppression kind, we would need to consider what we need to change, where and how.

arendjr · 2024-10-20T09:17:24Z

Yeah, the import sorting is a valid concern indeed. We could solve it by adding an exception in the sorter, which is one of the benefits of an integrated solution like Biome. It would be extra effort for us, but I think it may benefit users.

Not sure what impact it would have on the formatter though. At least I can’t really think of something… The formatter should leave comments where they are, so it should remain a top-level suppression, right?

I guess if you wanted to you could let the formatter recognize biome-ignore-all comments and insert two newlines after. But given it wouldn’t be a requirement anymore I also don’t see a really good reason to do so.

ematipico · 2024-10-21T15:15:05Z

OK, I will apply this change in a later PR.

cc @Conaclos, as you're involved in the import sorting, and you should be interested in this conversation.

ematipico marked this pull request as draft October 15, 2024 14:45

github-actions bot added A-Core Area: core A-Linter Area: linter A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS L-JSON Language: JSON and super languages L-HTML Language: HTML L-Grit Language: GritQL labels Oct 15, 2024

ematipico force-pushed the feat/top-level-suppression branch from f78eaf7 to 16fa3ad Compare October 16, 2024 14:20

github-actions bot removed A-Formatter Area: formatter L-HTML Language: HTML L-Grit Language: GritQL labels Oct 16, 2024

ematipico force-pushed the feat/top-level-suppression branch 2 times, most recently from 2de0850 to 7d9cd48 Compare October 16, 2024 15:00

ematipico marked this pull request as ready for review October 16, 2024 15:00

ematipico requested review from a team October 16, 2024 15:00

github-actions bot added the A-CLI Area: CLI label Oct 16, 2024

ematipico force-pushed the feat/top-level-suppression branch from 7d9cd48 to 1a59e00 Compare October 16, 2024 15:24

vohoanglong0107 mentioned this pull request Oct 16, 2024

feat(graphql_analyze): add suppression action for the GraphQL linter #4312

Merged

ematipico force-pushed the feat/top-level-suppression branch from 1a59e00 to fbd3c8c Compare October 17, 2024 09:32

github-actions bot removed the A-Core Area: core label Oct 17, 2024

ematipico force-pushed the feat/top-level-suppression branch from fbd3c8c to 3b3392f Compare October 17, 2024 09:41

vohoanglong0107 approved these changes Oct 17, 2024

View reviewed changes

ematipico marked this pull request as draft October 17, 2024 10:19

ematipico added 3 commits October 18, 2024 15:34

update CSS tests

5ef54b0

update package.json files

3bcd86d

rebase and update graphql suppressions

d1c6e89

ematipico force-pushed the feat/top-level-suppression branch from b945709 to d1c6e89 Compare October 18, 2024 14:39

github-actions bot removed A-Core Area: core A-Project Area: project A-Parser Area: parser A-Formatter Area: formatter L-Grit Language: GritQL labels Oct 18, 2024

format, dah

9d67ec2

ematipico marked this pull request as ready for review October 18, 2024 15:16

ematipico commented Oct 18, 2024

View reviewed changes

arendjr approved these changes Oct 19, 2024

View reviewed changes

code action breaking change

a8be40c

github-actions bot added the A-Project Area: project label Oct 21, 2024

fix issue in the suppression action

716819c

ematipico merged commit d66cfaf into next Oct 21, 2024
11 of 12 checks passed

ematipico deleted the feat/top-level-suppression branch October 21, 2024 15:15

ematipico added a commit that referenced this pull request Oct 23, 2024

feat(analyzer): top level suppression (#4306)

971b008

ematipico added a commit that referenced this pull request Oct 31, 2024

feat(analyzer): top level suppression (#4306)

0a27ee8

ematipico added a commit that referenced this pull request Nov 6, 2024

feat(analyzer): top level suppression (#4306)

c19673a

ematipico added a commit that referenced this pull request Nov 7, 2024

feat(analyzer): top level suppression (#4306)

7db7988

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(analyzer): top level suppression #4306

feat(analyzer): top level suppression #4306

ematipico commented Oct 15, 2024 •

edited

Loading

codspeed-hq bot commented Oct 15, 2024 •

edited

Loading

vohoanglong0107 commented Oct 16, 2024 •

edited

Loading

ematipico commented Oct 17, 2024

ematipico commented Oct 17, 2024

ematipico Oct 18, 2024

ematipico Oct 18, 2024

ematipico Oct 18, 2024

ematipico Oct 18, 2024

ematipico Oct 18, 2024

ematipico Oct 18, 2024

arendjr left a comment

ematipico commented Oct 20, 2024 •

edited

Loading

arendjr commented Oct 20, 2024

ematipico commented Oct 21, 2024

		/// A suppression using the legacy syntax to disable a specific rule eg. `// biome-ignore lint(style/useWhile)`
		MaybeLegacy(&'a str),

feat(analyzer): top level suppression #4306

feat(analyzer): top level suppression #4306

Conversation

ematipico commented Oct 15, 2024 • edited Loading

Summary

TL;DR:

More changes

Test Plan

codspeed-hq bot commented Oct 15, 2024 • edited Loading

CodSpeed Performance Report

Merging #4306 will not alter performance

Summary

vohoanglong0107 commented Oct 16, 2024 • edited Loading

ematipico commented Oct 17, 2024

ematipico commented Oct 17, 2024

ematipico Oct 18, 2024

Choose a reason for hiding this comment

ematipico Oct 18, 2024

Choose a reason for hiding this comment

ematipico Oct 18, 2024

Choose a reason for hiding this comment

ematipico Oct 18, 2024

Choose a reason for hiding this comment

ematipico Oct 18, 2024

Choose a reason for hiding this comment

ematipico Oct 18, 2024

Choose a reason for hiding this comment

arendjr left a comment

Choose a reason for hiding this comment

ematipico commented Oct 20, 2024 • edited Loading

arendjr commented Oct 20, 2024

ematipico commented Oct 21, 2024

ematipico commented Oct 15, 2024 •

edited

Loading

codspeed-hq bot commented Oct 15, 2024 •

edited

Loading

vohoanglong0107 commented Oct 16, 2024 •

edited

Loading

ematipico commented Oct 20, 2024 •

edited

Loading