feat(linter): domains and deps #4713

ematipico · 2024-12-09T10:42:32Z

Summary

As part of #4662, this PR introduced two new features in our linter

`domains`

Closes #4701
Closes #4712

Domains are concepts shared by different rules. As opposed to groups:

They are precise concepts such as testing, runtime, frameworks, etc.
They aren't part of the diagnostic category of a rule
There's no default domain, because there isn't a clear definition of what a "default domain" means, in Biome. I would argue that the default domain is the recommended rules of Biome.
They don't change the severity of the rules. Domains enable or disable rules.

A RuleDomain can specify (for now) manifest dependencies and analyzer globals, via two functions:

RuleDomain::manifest_dependencies
RuleDomain::globals

These (and future functions) must be const functions, so they can be used when generating the automated documentation, and provide the relevant information for a domain/rule.

Dependencies are a list of tuples. Each tuple contains the dependency's name and the range of versions available from. For example, it doesn't make sense to enable the react rules if the project uses a react version without hooks.

Important

Based on the discussion we just had in this PR, here's the business logic of activation of the rule with domains:

A rule that is recommended and has domains, it's not enabled via recommended: true. It's only enabled when the relative domain is "recommended" or "all"`.
A rule that is recommended and doesn't have domains, is enabled via recommneded: true.
A rule that is not recommended and has domains, is enabled when the relative domain is "all".

Unfortunately, the package.json detection is limited to Biome capabilities, so the support of monorepos is limited.

Note

This new feature should address @arendjr concerns regarding the removal of useExhasutiveDependencies from the recommended rules

Technical changes

There were some snapshot testing that belonged to the Biome configuration. Initially they were inside biome_service, but when we moved everything to biome_configuration, we forgot to move them too. I moved them in this PR.
The new logic for dependencies and domains has been added to the LinterVisitor type, which is in charge to select the enabled rules by looking that workspace settings (configuration) and the metadata registry.
The previous PR refactor: get rule severity from metadata severity #4687 introduced the rule severity; I did change the codegen to create a function that returns the severity of the rule. I realised this isn't needed because we can assign the correct severity of the rule when we call R::diagnostic() inside the analyzer. Way less code :)
I created a new type called ProcessLint to reduce the duplicated code in the lint functions called by the different languages.
I updated some tests that had incorrect syntax.
The rules from depedencies and domains can only be enabled is only isn't present, or it's empty. This is a business requirement because --only wins over everything.

Test Plan

I added various tests to ensure the new features work as expected.

codspeed-hq · 2024-12-09T11:23:20Z

CodSpeed Performance Report

Merging #4713 will not alter performance

_{Comparing feat/rule-domain (21553f9) with next (21ef4aa)}

Summary

✅ 97 untouched benchmarks

ematipico · 2024-12-09T12:12:04Z

crates/biome_service/src/file_handlers/mod.rs

-        if let Some(only) = self.only {
-            for selector in only {
-                if RuleFilter::from(selector).match_group::<G>() {
-                    G::record_rules(self)
-                }
-            }
-        }
-
-        if let Some(skip) = self.skip {
-            for selector in skip {
-                if RuleFilter::from(selector).match_group::<G>() {
-                    G::record_rules(self)
-                }
-            }
-        }


This was repeated logic, which I moved inside push_rule

Conaclos · 2024-12-09T18:39:40Z

Some suggestions/discussions:

Because we are likely to have a domain for each dependency, should we enable the associated domain when we met a given dependency?
For example, the react dependency could enable the react domain.

Regarding domain value: instead of true/false I could use "all"/"none". This allows us to introduce an extra value: "recommended".
If we introduce a default domain, we could so replace the recommended flag by setting the default domain to "recommended".

arendjr

Really nice! I love this! I especially like this gives us the ability to automatically pick up on the enabling of rules, just by adding a dependency 👍

One thing I wonder about: It feels like dependencies and domains have quite a bit of overlap in intention, whereas the way they work they are actually orthogonal to one another. I haven't yet made up my mind whether this is useful or confusing.

For instance, if I have the react dependency, but biome.json contains { "domains": { "react": false } }, will React be enabled or disabled? The dependencies suggests it should be enabled, but the domain was disabled. So which should it be?

I think I would've expected a slightly different variation maybe:

Rules belong to one or more domains
Domains can be auto-enabled through dependencies
In the future, maybe we find other ways to auto-enable a domain, such as the engines field, or the presence of tsconfig.json, deno.json.
Maybe domains can also be used to define presets for:
- Globals (process, __dirname, jest, etc..)
- Assumed imports that are always available (like the example of vscode for VS Code plugins)

Of course, that does require a separate solution to decide whether a rule within a domain is recommended or not. But I think we can do this using a small config change:

{
  "linter": {
    "domains": {
      "test": "off", // or `false`,
      "react": "all", // or `true`, enables all rules in the React domain
      "node": "recommended" // enabled recommended rules in the Node.js domain (this is the default if the rule's prerequisites are met)
    }
  }
}

None of this is blocking, just thinking out loud on how we could make it even better!

.changeset/introduce_the_domains_linter_feature.md

arendjr · 2024-12-09T18:20:03Z

.changeset/introduce_the_domains_linter_feature.md

+{
+  "linter": {
+    "domains": {
+      "test": false, // rules around testing are disabled


It would be really neat if our biome init script could make a best effort to detect what kind of folder structure and/or file name pattern is in use for tests. And based on that it could automatically populate the overrides. For example:

{ "overrides": [{ "include": ["**/*.test.js"], "linter": { "domains": { "test": true } } }] }

We can go even further, and make this part of the RuleDomain feature. A RuleDomain can specify a Matcher, and enable the rules if it path matches a certain glob. This logic is applied the same way as dependencies are applied, which means that if the user tinker with configuration, it's not applied anymore

CONTRIBUTING.md

crates/biome_analyze/CONTRIBUTING.md

ematipico · 2024-12-09T20:40:33Z

This allows us to introduce an extra value: "recommended".

And how do we define a rule that belongs to a domain and it's recommended? Another metadata field? Open to suggestions.

One thing I wonder about: It feels like dependencies and domains have quite a bit of overlap in intention, whereas the way they work they are actually orthogonal to one another. I haven't yet made up my mind whether this is useful or confusing.

Even though this is true, I think they shouldn't be correlated, at all. It would be a logic complex. domains are good for a possible global config, and for projects that don't need a package.json, while dependencies it's a nice opt-in.

For instance, if I have the react dependency, but biome.json contains { "domains": { "react": false } }, will React be enabled or disabled? The dependencies suggests it should be enabled, but the domain was disabled. So which should it be?

That's a valid question. We can decide on the relative issue or here. IMHO our configuration always wins, regardless of external factors (manifest, other config files e.g. deno or TypeScript). This will be documented, so people know the order of priority.

Conaclos · 2024-12-09T21:43:56Z

And how do we define a rule that belongs to a domain and it's recommended? Another metadata field? Open to suggestions.

We could reuse the recommended metadata and change its semantic: if recommended is true, then the rule is recommended for the domains it belongs to. If the rule has no domains, then it is recommended in the sense we have now (enabled by linter.recommended). With this in place, I could remove all <group>.recommended.

We could even remove linter.recommended if we add the concept of default domain (domains.default = "recommended" could equivalent to linter.recommeded = true).

ematipico · 2024-12-09T22:00:02Z

Is there a strong rationale to change the semantics of settings that we already have? What advantages brings to the users? And to us? What are we simplifying?

You convinced me when we talked about the relationship between a rule being recommended and the severity, and we found a good rational.

In this case I can't seem to find a rational to provide to our users and contributors.

If the rule has no domains, then it is recommended in the sense we have now (enabled by linter.recommended)

I fear this logic isn't too evident:

it's not intuitive when you write the rule and when you test it
it might not be intuitive for users

arendjr · 2024-12-09T22:00:49Z

Hehe, I hadn’t seen @Conaclos ‘s original response while typing mine, but it sounds like we’re quite aligned :)

We could reuse the recommended metadata and change its semantic

Agreed, this makes most sense to me as well. After all, the old semantic doesn’t make sense anymore if a rule also has a domain. Something that’s unconditionally recommended should by definition not be limited to a specific domain.

We could even remove linter.recommended if we add the concept of default domain (domains.default = "recommended" could equivalent to linter.recommeded = true).

This is just a stretch too far for me. A default domain still feels meaningless to me as it goes against what domains are. It’s more the absence of a domain, really.

Even though this is true, I think they shouldn't be correlated, at all. It would be a logic complex. domains are good for a possible global config, and for projects that don't need a package.json, while dependencies it's a nice opt-in.

Why would it be complex? The way I would imagine “domain dependencies” to work would have the same semantics as what you’re proposing, a nice opt-in that gives great defaults if someone has a package.json while still allowing manual configuration.

But in terms of config resolution, I think it would be less complex than what you’re proposing, because there can no longer be a conflict between the domain rules and the dependencies rules, as they’ve become a single thing.

This will be documented, so people know the order of priority.

Even documented, people will regularly ask about this because they have a habit of only reading documentation until after things bite them :) I would rather avoid this potential conflict between business rules alltogether.

arendjr · 2024-12-09T22:08:23Z

One more thing that is not very intuitive about having dependencies and domains be completely separated is that a rule can declare that react is one of its dependencies without declaring itself part of the react domain. But what good does that do? I wouldn’t consider that a valid use case and it’s another thing rule authors (and reviewers) need to watch out for.

And what about a rule that’s part of the react domain but shouldn’t be recommended? Doesn’t it too depend on React? But it cannot declare this, because it would become automatically enabled. The semantics seem only more confusing by treating them separate.

ematipico · 2024-12-10T03:36:11Z

And what about a rule that’s part of the react domain but shouldn’t be recommended

Not sure where the recommendation is coming from, because it isn't part of the current PR, let's take a step back.

I think it would be less complex than what you’re proposing, because there can no longer be a conflict between the domain rules and the dependencies rules, as they’ve become a single thing.

With the current setup proposed in this PR, you can have a react project, with the default biome configuration (no domains), and you'll get diagnostics from the react rules. This should address the initial concern of yours regarding not recommending the react rules anymore.

However, it's still not clear what you're proposing with a relationship between domains and dependencies. But as long as it addresses your initial concern, it's fine by me.

arendjr · 2024-12-10T07:23:01Z

Yeah, so what I’m suggesting is that rules can be associated to one or more domains, but they are not directly associated with dependencies anymore.

For useExhaustiveDependencies, we could say:

declare_lint_rule! {
    /// Documentation
    pub(crate) UseExhaustiveDependencies {
        version: "next",
        name: "useExhaustiveDependencies",
        language: "js",
        recommended: true,
        domains: &[RuleDomain::React],
    }
}

Note the combination of recommended: true and domains: &[RuleDomain::React]. This means the rule is recommended on the condition that the domain is active.

So how do we activate the domain? Either through explicit configuration:

{
  "linter": {
    "domains": {
      "react": "recommended" // enables only the recommended rules within the domain, whereas "all"/true would enable them all
    }
  }
}

But explicit configuration wouldn’t be necessary if you have the react dependency in your package.json. That part of the functionality I would move onto the domains. So domains can specify their dependencies, and unless the config overrides this, they are set to"off"/false if the dependencies are not met, and to recommended if they are met.

ematipico · 2024-12-10T08:52:55Z

That part of the functionality I would move onto the domains. So domains can specify their dependencies, and unless the config overrides this, they are set to"off"/false if the dependencies are not met, and to recommended if they are met.

I'm not a big fan of this part, because it requires more maintenance for us in case the business logic of a domain changes, and this can't be documented inside the page of the rule in an automated way. Hopefully these new domains won't change that often.

ematipico · 2024-12-10T09:06:09Z

ok, I might have an 💡

Conaclos · 2024-12-10T09:34:44Z

I am on the same line as @arendjr.

Is there a strong rationale to change the semantics of settings that we already have? What advantages brings to the users? And to us? What are we simplifying?

We expect the number of rules belonging to a domain to grow.
Similarly to the current rules we have, some rules of a domain could be pedantic, stylistic, ...
We would like to disable by default its rules when we enable a domain.
This is the same concept as recommended, but tailored to domains.

Maybe you are seeing the thing on a different perspective: assimilating domains to presets? react could in fact be react-recommended and so on? I wonder if this could not be a simpler system. recommended could be one preset. I have to review the revamping linter config discussion. If I remember correctly, I proposed domains for a different purpose. I think you are more leaning towards presets?

ematipico · 2024-12-10T09:46:43Z

Maybe you are seeing the thing on a different perspective: assimilating domains to presets?

No, I am more focused on creating a narrative that justifies the change in direction, that's all. We need to present these changes to our users and contributors, and we need to be good at explaining them, especially why. If we can't come up with a good explanation, I think the changes aren't worth exploring.

Conaclos · 2024-12-10T12:08:45Z

What is making domains different from presets?

arendjr · 2024-12-10T12:26:14Z

What is making domains different from presets?

I’m not sure what the preset proposal looked like, but the first thing that comes to mind is: the name. The word “domain” implies a subset of the whole.

Compare the word domain to cities within a country. If someone doesn’t live in a city, they don’t live in a “default city”, they live in no city. With domain it’s the same, if something is not part of a domain, they’re not part of a “default domain” either.

Presets are not like that, because the word carries different expectations. A default preset wouldn’t sound weird to me, whereas a default domain does. Then again, a preset does tend to imply that you pick one and don’t mix them, so I think the word domain works better here.

Co-authored-by: Arend van Beelen jr. <[email protected]>

ematipico · 2024-12-11T14:55:40Z

@arendjr and @Conaclos , please review the PR again, I updated the PR description too

ematipico · 2024-12-11T14:57:33Z

crates/biome_analyze/src/rule.rs

@@ -484,6 +566,7 @@ macro_rules! declare_syntax_rule {
                version: $version,
                name: $name,
                language: $language,
+                severity: biome_diagnostics::Severity::Error,


Syntax rules are errors by definition

Conaclos

This is a nice piece ot work :)

I left some suggestions.

By the way, what makes you change your mind about domain dependencies and recommended for domains?

.changeset/introduce_the_domains_linter_feature.md

crates/biome_analyze/src/rule.rs

crates/biome_configuration/src/analyzer/linter/mod.rs

crates/biome_project/src/node_js_project/package_json.rs

arendjr

❤️

.changeset/introduce_the_domains_linter_feature.md

crates/biome_analyze/CONTRIBUTING.md

crates/biome_analyze/src/rule.rs

arendjr · 2024-12-11T21:15:57Z

crates/biome_analyze/src/rule.rs

+                &("vitest", ">=1.0.0"),
+            ],
+            RuleDomain::Solid => &[&("solid", ">=1.0.0")],
+            RuleDomain::Next => &[&("react", ">=16.0.0"), &("next", ">=14.0.0")],


So merely having React will also enable the Next domain. Is that intended?

It's the other way around, having the next domain will enable Next.js rules and react rules, by scanning the dependencies

In that case I don’t think I understand how this works? If that’s the intention, shouldn’t we add next to the dependencies for the React domain? Instead of adding react to the dependencies of the Next domain?

Good thinking, I don't think it will work. I was trying to be smart having one domain depending on another, but I believe it's going to get things more complex.

However, even adding the next dependency to the react domain won't work either. I'll change that part of the logic later

crates/biome_analyze/src/rule.rs

Co-authored-by: Victorien Elvinger <[email protected]> Co-authored-by: Arend van Beelen jr. <[email protected]>

ematipico · 2024-12-12T09:48:30Z

By the way, what makes you change your mind about domain dependencies and recommended for domains?

You made a good point about the number of rules growing and finding a way to better group them. In the end, we didn't change the semantics of the recommendations for the users; it only has a different business logic for us.

ematipico added 2 commits December 9, 2024 09:11

feat(linter): domains and deps

e8af677

update docs

76c6056

github-actions bot added A-CLI Area: CLI A-Core Area: core A-Project Area: project A-Linter Area: linter A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages labels Dec 9, 2024

clippy

5e75740

ematipico requested review from a team December 9, 2024 10:43

This was linked to issues Dec 9, 2024

Introduce domains #4701

Closed

Introduce dependencies #4712

Closed

ematipico commented Dec 9, 2024

View reviewed changes

ematipico added 2 commits December 9, 2024 13:51

codegen

13070f0

clippy

783fcbe

arendjr approved these changes Dec 9, 2024

View reviewed changes

apply suggestions

a621d13

ematipico force-pushed the feat/rule-domain branch from a74dd93 to a621d13 Compare December 10, 2024 16:34

ematipico and others added 2 commits December 10, 2024 16:36

Apply suggestions from code review

09e4d8b

Co-authored-by: Arend van Beelen jr. <[email protected]>

update docs

955db83

ematipico marked this pull request as draft December 10, 2024 16:53

more refactor to extract globals

08b0e5d

ematipico force-pushed the feat/rule-domain branch from ea8bb33 to 08b0e5d Compare December 11, 2024 10:30

github-actions bot added the A-Parser Area: parser label Dec 11, 2024

ematipico added 3 commits December 11, 2024 14:10

correctly handle dependency range

d0c116c

update docs

540758a

fix regression

673f9c0

ematipico requested a review from arendjr December 11, 2024 14:55

remove changeset

6050e60

ematipico commented Dec 11, 2024

View reviewed changes

ematipico marked this pull request as ready for review December 11, 2024 14:58

ematipico force-pushed the feat/rule-domain branch from c74314d to 61a8947 Compare December 11, 2024 15:44

linting

db55449

ematipico force-pushed the feat/rule-domain branch from 61a8947 to db55449 Compare December 11, 2024 15:50

Conaclos reviewed Dec 11, 2024

View reviewed changes

Conaclos approved these changes Dec 11, 2024

View reviewed changes

arendjr approved these changes Dec 11, 2024

View reviewed changes

ematipico and others added 3 commits December 12, 2024 09:40

Apply suggestions from code review

fffcc89

Co-authored-by: Victorien Elvinger <[email protected]> Co-authored-by: Arend van Beelen jr. <[email protected]>

fix compiling issue

8740a4c

apply suggestion

21553f9

ematipico merged commit 0a9d85a into next Dec 12, 2024
11 checks passed

ematipico deleted the feat/rule-domain branch December 12, 2024 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(linter): domains and deps #4713

feat(linter): domains and deps #4713

ematipico commented Dec 9, 2024 •

edited

Loading

codspeed-hq bot commented Dec 9, 2024 •

edited

Loading

ematipico Dec 9, 2024

Conaclos commented Dec 9, 2024 •

edited

Loading

arendjr left a comment •

edited

Loading

arendjr Dec 9, 2024

ematipico Dec 11, 2024

ematipico commented Dec 9, 2024

Conaclos commented Dec 9, 2024

ematipico commented Dec 9, 2024 •

edited

Loading

arendjr commented Dec 9, 2024 •

edited

Loading

arendjr commented Dec 9, 2024

ematipico commented Dec 10, 2024

arendjr commented Dec 10, 2024

ematipico commented Dec 10, 2024 •

edited

Loading

ematipico commented Dec 10, 2024

Conaclos commented Dec 10, 2024

ematipico commented Dec 10, 2024

Conaclos commented Dec 10, 2024

arendjr commented Dec 10, 2024 •

edited

Loading

ematipico commented Dec 11, 2024

ematipico Dec 11, 2024 •

edited

Loading

Conaclos left a comment

arendjr left a comment

arendjr Dec 11, 2024

ematipico Dec 12, 2024

arendjr Dec 12, 2024

ematipico Dec 12, 2024

ematipico commented Dec 12, 2024

feat(linter): domains and deps #4713

feat(linter): domains and deps #4713

Conversation

ematipico commented Dec 9, 2024 • edited Loading

Summary

domains

Technical changes

Test Plan

codspeed-hq bot commented Dec 9, 2024 • edited Loading

Merging #4713 will not alter performance

Summary

Choose a reason for hiding this comment

Conaclos commented Dec 9, 2024 • edited Loading

arendjr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Dec 9, 2024

Conaclos commented Dec 9, 2024

ematipico commented Dec 9, 2024 • edited Loading

arendjr commented Dec 9, 2024 • edited Loading

arendjr commented Dec 9, 2024

ematipico commented Dec 10, 2024

arendjr commented Dec 10, 2024

ematipico commented Dec 10, 2024 • edited Loading

ematipico commented Dec 10, 2024

Conaclos commented Dec 10, 2024

ematipico commented Dec 10, 2024

Conaclos commented Dec 10, 2024

arendjr commented Dec 10, 2024 • edited Loading

ematipico commented Dec 11, 2024

ematipico Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Conaclos left a comment

Choose a reason for hiding this comment

arendjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Dec 12, 2024

ematipico commented Dec 9, 2024 •

edited

Loading

`domains`

codspeed-hq bot commented Dec 9, 2024 •

edited

Loading

Conaclos commented Dec 9, 2024 •

edited

Loading

arendjr left a comment •

edited

Loading

ematipico commented Dec 9, 2024 •

edited

Loading

arendjr commented Dec 9, 2024 •

edited

Loading

ematipico commented Dec 10, 2024 •

edited

Loading

arendjr commented Dec 10, 2024 •

edited

Loading

ematipico Dec 11, 2024 •

edited

Loading