feat(rome_service): auto generate configuration for rules

[ematipico]

Summary

Closes #2887

This PR implements a way to auto generate the configuration of the rome.json of the rules using the metadata defined in each rule.

The configuration, in this PR, is now shaped in this way:

{
  "linter": {
    "enabled": true,
    "globals": ["$"],
    "rules": {
        "js": {
            "noDeadCode": "off",
            "useSimplifiedLogicExpression": "warn"
        }
    }
  }
}

There's a new field called globals. We will use this field to store global references to the root scope of the semantic model cc @xunilrj

There's a new field called rules which will contain the rules. The rules divided by groups. This all auto generate from the metadata, so it will change accordingly cc @leops .

Each rule is defined by a RuleConfiguration, where we can:

• turn off a rule
• make a rule to emit warnings
• make a rule to emit errors

A rule is not serialized if it's configured to emit diagnostics, because it's the default. This could change if we will use a recommendation system.

Consuming these defaults is out of scope, this will be implemented in other PRs.

As for now, all the rules are enabled and their default is RuleConfiguration::Error, although since they are not consumed yet, it won't make a difference for now.

The idea is to enable certain rules as "recommended", but this is also out of scope and it will be implemented in another PR.

I also fixed an error on the max diagnostics printer, the message was incorrectly emitted every time.

Test Plan

Because the configuration is not consume it yet, there's no practical test. I added for unknown field on the rule, which demonstrates that the configuration is correctly deserialized by serde.

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Pages

Latest commit:	f3ce1bd
Status:	✅  Deploy successful!
Preview URL:	https://78c2fdeb.tools-8rn.pages.dev
Branch Preview URL:	https://feature-lint-rules-configura.tools-8rn.pages.dev

View logs

[github-actions]

Playground for commit d07c33f

[github-actions]

Parser conformance results on ubuntu-latest

js/262

Test result	main count	This PR count	Difference
Total	45878	45878	0
Passed	44938	44938	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	main count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

symbols/microsoft

Test result	main count	This PR count	Difference
Total	5946	5946	0
Passed	388	388	0
Failed	5558	5558	0
Panics	0	0	0
Coverage	6.53%	6.53%	0.00%

ts/babel

Test result	main count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	main count	This PR count	Difference
Total	16257	16257	0
Passed	12393	12393	0
Failed	3864	3864	0
Panics	0	0	0
Coverage	76.23%	76.23%	0.00%

[leops]

Interestingly, this raises the question of whether generating a static struct for the rules configuration is the best solution, compared for instance to deserializing the rules field to a HashMap<String, Box<RawValue>> and letting the analyzer handle the deserialization of the inner data (the main advantage I see to doing that is the ability for the analyzer to emit a custom diagnostic for invalid configurations, eg. the rule "noDeadCode" does not exist, did you mean "js/noDeadCode" ?)

[MichaReiser]

Looks good. I do have a few questions (and I like @leops point on the suggestions).

• Does Rome-classic support changing the warning level of a diagnostic?
• 'error' is likely not the right default for all lint rules, especially rules for which no sound static analysis is possible due to JS's dynamic nature. How do you intend rules that, by default, should emit warnings?

[ematipico]

@leops

Interestingly, this raises the question of whether generating a static struct for the rules configuration is the best solution, compared for instance to deserializing the rules field to a HashMap<String, Box<RawValue>> and letting the analyzer handle the deserialization of the inner data (the main advantage I see to doing that is the ability for the analyzer to emit a custom diagnostic for invalid configurations, eg. the rule "noDeadCode" does not exist, did you mean "js/noDeadCode" ?)

As we didn't discuss this and I kept the configuration from Rome classic. If we want to evaluate another approach, let's do it on another channel.

As this is more an architecture discussion and not about the code, I followed up the discussion here: #2632 (reply in thread)

I would like to evaluate the PR with this approach. We are always in time for changing it.

@MichaReiser

Does Rome-classic support changing the warning level of a diagnostic?

Rome was still behind and it didn't allow to turn off rules, unless using suppression comments. In this regard we are exploring new ground!

'error' is likely not the right default for all lint rules, especially rules for which no sound static analysis is possible due to JS's dynamic nature. How do you intend rules that, by default, should emit warnings?

That's a nice question, and I don't have an answer yet. The thing is, for now all rules emit warnings or errors and we don't have a more finer way of doing that per rule, not ahead of time at least. Each rule emits a diagnostic and we can't know ahead of time which type, unless we execute them.

[MichaReiser]

One more question. How would this configuration format support rule specific configurations

[ematipico]

One more question. How would this configuration format support rule specific configurations

It doesn't support it and it hasn't been taken in consideration. We haven't discussed the possibility to have configuration per rule, and I believe it does go against the rome philosophy where we should have less configuration as possible.

We can revisit the philosophy in a separated discussion. That would be a possible breaking change.

[MichaReiser]

One more question. How would this configuration format support rule specific configurations

It doesn't support it and it hasn't been taken in consideration. We haven't discussed the possibility to have configuration per rule, and I believe it does go against the rome philosophy where we should have less configuration as possible.

We can revisit the philosophy in a separated discussion. That would be a possible breaking change.

I agree on the philosophy of providing good defaults and avoiding any unnecessary options. The philosophy could even be extended to not have support for overriding the lint rule level by arguing that we pick the appropriate defaults.

I think it would be worth spending some time exploring configuration formats that support per-rule options to avoid a breaking change in the very near future OR at least go through the rules with planned and check if we can implement all of them without having per-rule option support.

[leops]

I would like to evaluate the PR with this approach. We are always in time for changing it.

Absolutely, this is just something I thought about while reviewing this and wanted to raise a possible discussion about, but it's not an issue for merging this PR

'error' is likely not the right default for all lint rules, especially rules for which no sound static analysis is possible due to JS's dynamic nature. How do you intend rules that, by default, should emit warnings?

That's a nice question, and I don't have an answer yet. The thing is, for now all rules emit warnings or errors and we don't have a more finer way of doing that per rule, not ahead of time at least. Each rule emits a diagnostic and we can't know ahead of time which type, unless we execute them.

This is something we can revisit in more detail once this configuration actually gets integrated with the analyzer, but it would be possible to make the severity on RuleDiagnostic optional and let it be filled with the severity associated with the RuleConfiguration for that rule by default, unless the rule specified an explicit severity for the diagnostic.

An alternative / complement to this would be to consider the RuleConfiguration enum as setting a "maximum level" at which a certain rule can emit diagnostics:

• if a rule is set to RuleConfiguration::Error and emits a diagnostic with a severity of Error, Warning or Note the diagnostic is emitted as-is
• if a rule is set to RuleConfiguration::Warn and emits a diagnostic with a severity of Error or Warning it gets emitted as a warning in both case, a severity of Note is still emitted as-is
• if a rule is set to RuleConfiguration::Off no diagnostic gets emitted

This way if a rule is not guaranteed to be sound it could emit a warning, and the diagnostic will still be emitted as a warning even if the rule is set to RuleConfiguration::Error (this is potentially very counterintuitive though)

[ematipico]

I agree on the philosophy of providing good defaults and avoiding any unnecessary options. The philosophy could even be extended to not have support for overriding the lint rule level by arguing that we pick the appropriate defaults.

True, this is a good point. This is something we wanted to do time ago: #1529

I think it would be worth spending some time exploring configuration formats that support per-rule options to avoid a breaking change in the very near future OR at least go through the rules with planned and check if we can implement all of them without having per-rule option support.

This is part of #2741 , where we will evaluate rules that require a configuration and discuss them, and see how we can go from there.

I don't mind breaking changes because we are still in 0.*. It allows us to experiment, change things and whatnot without worrying too much. We can start the exploration after we release 0.8.0.

[ematipico]

@leops do you have any idea why the workflow for the codegen is failing? Can't understand why

[MichaReiser]

I don't mind breaking changes because we are still in 0.*. It allows us to experiment, change things and whatnot without worrying too much. We can start the exploration after we release 0.8.0.

That's true but comes at the risk that other things will be more important in the upcoming 0.9 and then 10.0 release and we then missed our chance. That's why I think it would be valuable to invest a couple of minutes to post a few options on how we could support rule-specific options. For example, one option that isn't a breaking change is to define the rule value as string | { level: "warn" | "off", options: {} } or maybe we find a format that avoids the need for a union type (what do linters of other languages use?)

[ematipico]

I don't mind breaking changes because we are still in 0.*. It allows us to experiment, change things and whatnot without worrying too much. We can start the exploration after we release 0.8.0.

That's true but comes at the risk that other things will be more important in the upcoming 0.9 and then 10.0 release and we then missed our chance. That's why I think it would be valuable to invest a couple of minutes to post a few options on how we could support rule-specific options. For example, one option that isn't a breaking change is to define the rule value as string | { level: "warn" | "off", options: {} } or maybe we find a format that avoids the need for a union type (what do linters of other languages use?)

I believe we can use a mixed approach using what clippy is doing, which by using a custom function to deserialize the rules. Clippy doesn't allow to turn off rules at configuration level though, you can only provide options to rules. Clippy uses macros to turn off rules in a global way.

I will spend some time and propose a solution