feat(analyzer): assists

[ematipico]

Summary

Part of #3178

This PR implements the CLI part of the assists. The assists are considered another tool of Biome, so they deserve a particular spot inside Biome. That's how first assist will look like:

{
	"assists": {
		"enabled": true,
		"actions": {
			"source": {
				"useSortedKeys": "on" // or "off" if we want to turn it off
			}
		}
	}
}

Important

The rule organizeImports is an assist, but we consider it a special one because it has its own configuration, which means it is excluded from the other rules.

As for rule groups, we follow the same distinction that the LSP uses:

• source: actions to apply to the whole document/file, they will be part of fixAll action
• the rest of the groups that we will have (e.g. refactor, inline, extract, etc.) won't be applied by the fixAll action

The assists rules aren't meant to have any kind of options, so they just have a "on" or "off" flag to enabled them or disable them.

Here's the list of technical changes that this PR does:

• Update generate_configuration and generate_analyzer to generate the new configuration for assists, which differs from the linter rules.
• Update the Biome configuration to add assists and overrides.assists.
• Update the CLI commands ci and check to accept --assists-enabled flag.
• Update the diagnostic categories with a new one called assists
• Create a new process_file/assists.rs file, responsible to pull the fixes of RuleCategories::Action and apply them. If --write mode, it updates the file, otherwise it emits a diagnostic. It uses the existing CLI infrastructure, so there's nothing new added.
• Update the pull_actionsmethod to accept skip and only, which will be relevant in the future. range is now a Option<TextRange>.
• Update the fix_file method to accept a RuleCategories, which will become relevant in the future.
• changes .gitattributes to include graphql crates, and make it more maintainable.
• Fix xtask/rules_check, where the GraphQL files weren't checked. They are now checked, and I fixed a case where an example wasn't emitting a diagnostic.
• Refactor the whole analyzer infrastructure inside file_handlers/mod.rs. There was still a LOT of repeated code. So now we have a type called AnalyzerVisitorBuilder that builds the visitors, and the rest is done inside a function called finish(). When we add a new language, we just need to update this method, and the compiler will do the rest, forcing the developer to apply the traits for the new language. The ActionVisitor is already read to accept skip and only, but they aren't used yet. They will become useful in the future in case we want to provide functionality to apply single assists.
• The macro declare_refactor_rule is renamed to declare_source_rule, which is inline with the semantics of source

Test Plan

I added new tests for the new useSortedKeys rule.

What's missing

I plan to follow up with new PRs to address the following

• Update the internal contribution guide to take these changes into account.
• Resurrect an old rule called flipBinaryExpression, to show how non-source rules will work inside the LSP.
• Update the changelog
• Update the website, and prepare a new section for the assists

[github-actions]

Parser conformance results on

js/262

Test result	main count	This PR count	Difference
Total	48452	48452	0
Passed	47255	47255	0
Failed	1197	1197	0
Panics	0	0	0
Coverage	97.53%	97.53%	0.00%

jsx/babel

Test result	main count	This PR count	Difference
Total	40	40	0
Passed	37	37	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.50%	92.50%	0.00%

symbols/microsoft

Test result	main count	This PR count	Difference
Total	6551	6551	0
Passed	2199	2199	0
Failed	4352	4352	0
Panics	0	0	0
Coverage	33.57%	33.57%	0.00%

ts/babel

Test result	main count	This PR count	Difference
Total	670	670	0
Passed	598	598	0
Failed	72	72	0
Panics	0	0	0
Coverage	89.25%	89.25%	0.00%

ts/microsoft

Test result	main count	This PR count	Difference
Total	18333	18333	0
Passed	14026	14026	0
Failed	4307	4307	0
Panics	0	0	0
Coverage	76.51%	76.51%	0.00%

[codspeed-hq]

CodSpeed Performance Report

Merging #3399 will not alter performance

_{Comparing feat/config-assists (7a76baa) with main (98deae6)}

Summary

✅ 104 untouched benchmarks

[Conaclos]

I wonder why we use this kind of assertion when we have snapshots?

[Conaclos]

The configuration and the used concepts seem very close to the linter. It will certainly be impacted by #689. In this case, I wonder if we should decide on #689 before shipping assits to users?

Personally, I could remove the recommended/all options at group level as their combination is known to cause complexity. We had to make several PR on several months to get it right. We have no so much assists rules for now. Thus, it is fine to omit them. I could also omit all at rules level, keeping only recommended.

Because it is close to the linter configuration, we get the same questions: how handle multi-languages? For instance useSortedKeys is for now a JSON rule. However, it is clear that this could also be a rule for JS objects? I wonder again if we should not move the rules in the language-specific configuration:

{
  "json": {
    "assists": {
      "rules": {
        "source": {
          "useSortedKeys": "on"
        }
      }
    }
  },
  "javascript": {
    "assists": {
      "rules": {
        "source": {
          "useSortedClasses": "on"
        }
      }
    }
  },
}

source: actions to apply to the whole document/file, they will be part of fixAll action

I am a bit worried about tightening up a group (source) to fix-all actions.
I already though about users requesting a configuration to allow a rule from another group to be applied on fix-on-save...

he rest of the groups that we will have (e.g. refactor, inline, extract, etc.) won't be applied by the fixAll action

Could you give us some examples for each group?

The assists rules aren't meant to have any kind of options, so they just have a "on" or "off" flag to enabled them or disable them.

However, can we expect users to request options for some of them?
I think we should not be surprised if one day we need to add an option to a rule.

Also, you choose "on"/"off" instead of true/false, I guess it is for extensibility?

Do we agree that an assist rule should be available on a command palette in an editor even if it is turned off in the configuration?

I wonder if we should rename rules into actions?

The rule organizeImports is an assist, but we consider it a special one because it has its own configuration, which means it is excluded from the other rules.

Do you think we could move organizeImports to aasists (I guess useSortedImports rule) in Biome 2.0? Maybe it is a good opportunity of implementing #3177 there? This could allow a smooth transition from organizeImports to useSortedImports that will allow custom orders. This also allows us to fix the naming ambiguity (organizer -> sorter).

[ematipico]

The configuration and the used concepts seem very close to the linter. It will certainly be impacted by #689. In this case, I wonder if we should decide on #689 before shipping assits to users?

I'd prefer to ship now, and change later. We haven't decided how the configuration is going to change, and how. It could take a lot of time. I would feel demoralised if I can't ship a feature only because we haven't take a decision 😞

Personally, I could remove the recommended/all options at group level as their combination is known to cause complexity. We had to make several PR on several months to get it right. We have no so much assists rules for now. Thus, it is fine to omit them. I could also omit all at rules level, keeping only recommended.

What do you think about removing all and recommended also at the top level? By default all assists are off, and users will opt-in the ones they want.

I wonder again if we should not move the rules in the language-specific configuration

I think it's a good thing that rules work across languages. Our documentation will be refactored so it can show which languages a rule is for. That's why I added the language metadata. Configuration-wise, we shouldn't change things.

I am a bit worried about tightening up a group (source) to fix-all actions.

But that's the meaning of source from the LSP: Source code actions apply to the entire file.

Could you give us some examples for each group?

I already did: refactor, inline, extract. Those groups will be mapped from codeActionKind), so we don't need to re-invent the wheel, and it's already part of a documented spec.

However, can we expect users to request options for some of them? I think we should not be surprised if one day we need to add an option to a rule.

That's possible, but options feel more like something for lint rules.

Also, you choose "on"/"off" instead of true/false, I guess it is for extensibility?

That, and because we already use "off" in the lint rule options.

Do we agree that an assist rule should be available on a command palette in an editor even if it is turned off in the configuration?

Yes, but only for assists that aren't source. source assists are triggered via source.fixAll.biome, so if a user didn't want them via configuration, we should not provide them. Instead, other assists would be available regardless, I think.

I wonder if we should rename rules into actions?

I think so, yes! Good suggestion

Do you think we could move organizeImports to aasists (I guess useSortedImports rule) in Biome 2.0? Maybe it is a good opportunity of implementing #3177 there? This could allow a smooth transition from organizeImports to useSortedImports that will allow custom orders. This also allows us to fix the naming ambiguity (organizer -> sorter).

I think it's a separate discussion. Also, since when there's an ambiguity?

[Conaclos]

What do you think about removing all and recommended also at the top level? By default all assists are off, and users will opt-in the ones they want.

I am ok with that also :)
We can still add recommended/all later.

I think it's a good thing that rules work across languages. Our documentation will be refactored so it can show which languages a rule is for. That's why I added the language metadata. Configuration-wise, we shouldn't change things.

I am still skeptical about this. Hopefully we will reach some consensus by Biome 2.0.

That, and because we already use "off" in the lint rule options.

Good point!

Also, since when there's an ambiguity?

It is not really an ambiguity, but rather an inconsistency.

[Conaclos]

Yes, but only for assists that aren't source. source assists are triggered via source.fixAll.biome, so if a user didn't want them via configuration, we should not provide them. Instead, other assists would be available regardless, I think.

I see some possible usage: for instance I want to sort quickly the keys of a JavaScript object without requiring me to sort the keys of every object in the file.

[ematipico]

It is not really an ambiguity, but rather an inconsistency.

I believe I miss some context here, because I don't know what you're talking about 😅

I see some possible usage: for instance I want to sort quickly the keys of a JavaScript object without requiring me to sort the keys of every object in the file.

I can see the use case. IDEs should be able to provide code actions based on text ranges. I will tackle that part in a later PR