PoC: loadable plugin [DONT MERGE]

[arendjr]

Summary

This PR extends the analyzer to dynamically execute a plugin that is defined in the configuration file. This PR is intentionally implemented without too much attention for detail, so that I can solicit feedback about the direction, before cleaning stuff up.

Open questions:

• This PR adds a plugins section to the linter configuration. This should be extended still to support plugin options, but how do we feel about this otherwise? Discussion can also go here: ☂️ Linter plugin configuration #2458
• Should plugin loading go into a separate crate? That might help us to make sure biome_analyze doesn't have to depend on Grit and OXC resolver directly.
• What about the rest of the analyzer integration? Do you agree with integrating into the Analyzer struct like this?
• How should Grit plugins report their diagnostics? In the RFC I proposed something like this:

or {
    `<$component $attrs />`,
    `<$component $attrs>$...</$component>`
} where {
    $attrs <: some $attr => diagnostic(
      message = "Use explicit boolean values for boolean JSX props.",
      fixer = `$attr={true}`,
      fixerDescription = "Add explicit `true` literal for this attribute",
      category = "quickFix",
      applicability = "always"
    ) where $attr <: r"[\w-]+"
}`

This is not in the PR yet, but I would continue along this path unless someone has a better suggestion :)

[dyc3]

Should plugin loading go into a separate crate?

I think this would be the right way to go. It would make it easy to find for someone looking to contribute to the plugin system.

[codspeed-hq]

CodSpeed Performance Report

Merging #4071 will degrade performances by 6.58%

_{Comparing arendjr:plugin-poc (ccce38f) with main (ecf1f16)}

Summary

❌ 1 regressions
✅ 100 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	arendjr:plugin-poc	Change
❌	big5-added_15586211152145260264.json[uncached]	546.8 µs	585.4 µs	-6.58%

[morgante]

Awesome to see this coming together!

I'm not sure I like using rewrites for the diagnostics:

• While the quick-fix itself is available in the "fixer" section it seems somewhat removed from the code. It makes the eventual rewrite a bit harder to read IMO.
• It entangles the metadata with the code.
• It prevents using plugin patterns with the rest of the Grit ecosystem

I'd lean towards either:

• Putting metadata outside the Grit file entirely, so it's defined in the plugins array. I think this is the easiest to understand, but does reduce the potential for dynamic messages. This is how the Grit CLI does it: https://docs.grit.io/guides/config#patterns
• Just using a function to define the diagnostic, Ex.

or {
    `<$component $attrs />`,
    `<$component $attrs>$...</$component>`
} where {
    $attrs <: some r"[\w-]+" as $attr => `$attr={true}` where
      register_diagnostic($attr,
      message = "Use explicit boolean values for boolean JSX props.",
      fixerDescription = "Add explicit `true` literal for this attribute",
      category = "quickFix",
      applicability = "always"
      )
}

[arendjr]

@morgante Makes sense! I think I’d towards the register_diagnostic() function then.

[jerome-benoit]

Should plugin loading go into a separate crate?

I think this would be the right way to go. It would make it easy to find for someone looking to contribute to the plugin system.

Meaning yet another ~40 binaries dependency with a significant size? Or the plugin DSO is ABI independent?

Guys, the former it's not sustainable. Please consider an ABI agnostic alternative with minimum overhead like wasm.

[dyc3]

@jerome-benoit No, we're just talking about putting the bulk of the logic for the plugin system in another crate, which biome depends on (like we already do for most major modules).

I'd highly recommend reading up on the plugins RFC and related threads. They should give you all the context you need for this PR.

[ematipico]

This PR adds a plugins section to the linter configuration. This should be extended still to support plugin options, but how do we feel about this otherwise? Discussion can also go here: #2458

I would think in the long run, and make the plugins a top level configuration. I don't see any particular reason to start from the linter. We could craft the loading of the manifest this way

// biome.json

{
	"plugins": ["./some-plugin"] // it could be npm package too
}

Then inside the some-plugin/ folder, we could have a biome.manifest:

// biome.manifest
{
	"version": "",
	"rules": ["./path/to/rule.grit"]
}

Should plugin loading go into a separate crate? That might help us to make sure biome_analyze doesn't have to depend on Grit and OXC resolver directly.

Yes, it must be. We should use the same loading logic we use for the extends filed inside the biome.json file. This is currently done in the biome_fs crate.

What about the rest of the analyzer integration? Do you agree with integrating into the Analyzer struct like this?

Yes, it make sense. How would a user define which language a plugin is able to interact with?

How should Grit plugins report their diagnostics? In #1762 I proposed something like this:

I don't know much about grit, but can I user-pass a text range to the diagnostic? A text range isn't always the node that is being queried.

I don't want to be a downer, however I wonder if we could be more strict about the messages of the rules. In Biome we try to enforce the three rule pillars in our rules, and I wonder if we should do the same for the rule plugins too.

[arendjr]

Thanks, I like the idea for plugin manifests. That’ll indeed allow us to create more comprehensive plugins down the line 👍

How would a user define which language a plugin is able to interact with?

Maybe this should also be defined in the manifest then?

can I user-pass a text range to the diagnostic? A text range isn't always the node that is being queried.

In this snippet, the first argument to register_diagnostic would define the range:

or {
    `<$component $attrs />`,
    `<$component $attrs>$...</$component>`
} where {
    $attrs <: some r"[\w-]+" as $attr => `$attr={true}` where
      register_diagnostic($attr,
      message = "Use explicit boolean values for boolean JSX props.",
      fixerDescription = "Add explicit `true` literal for this attribute",
      category = "quickFix",
      applicability = "always"
      )
}

So you’re not explicitly passing ranges, but something that you can match on. But the query allows you to use as many variables as you need, so you retain the freedom to decide on which part to report the diagnostic.

I wonder if we could be more strict about the messages of the rules. In Biome we try to enforce the three rule pillars in our rules, and I wonder if we should do the same for the rule plugins too.

If you want really wanted, we could mandate that every rule plugin defines a rewrite rule, so there is always a code action. Thankfully they’re very easy to declare with Grit, but I’m still skeptical that’s what we should do.

We know there’s situations where even providing an unsafe fix can be daunting, so if we mandate it we may just risk a repeat of the situation with mandatory suppression reasons: people will fill in nonsensical workarounds and blame us for the silly restrictions.

[arendjr]

Real implementation is happening here: #4234