feat(rome_analyze): accept options inside analyzer #3325
Conversation
✅ Deploy Preview for rometools canceled.
|
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
| @@ -13,6 +14,7 @@ where | |||
| query_result: &'a RuleQueryResult<R>, | |||
| root: &'a RuleRoot<R>, | |||
| services: RuleServiceBag<R>, | |||
| options: &'a AnalyzerOptions, | |||
There was a problem hiding this comment.
Maybe we could store the options in the service bag, it could make the change simpler than having to add it as a parameter or field on all the intermediate functions and data structures
There was a problem hiding this comment.
Adding it to the ServiceBag involves another series of changes around various traits. Inside RuleContext, services has a trait bound to RuleServiceBag.
I think we can do this change, but I would prefer to defer it to later for two reasons:
- I am not comfortable with this kind of change
- options will become a trait itself too - while rules are language agnostic,
globalsare not; so it seems more appropriate to make this change together with this one
crates/rome_analyze/src/lib.rs
Outdated
| /// } | ||
| /// } | ||
| /// ``` | ||
| pub fn rule_options<F: FnOnce(&RuleOptions) -> ToType, ToType>( |
There was a problem hiding this comment.
In practice since the RuleOptions is a thin wrapper around a RawValue the only way to get a value out of it will be through deserialization, so this function could simply require ToType: Deserialize<'de> (with the function borrowing self as &'de self) and remove the mapper argument.
Also for ease of use it would be nice to alias this function on the RuleContext with the rule_name automatically injected, so that most rules could simply write ctx.rule_options::<OptionsType>() and get the appropriate configuration entry back automatically.
There was a problem hiding this comment.
That's the intended final design, but we are not there yet, and that's why I decided to break down the work in multiple tasks:
- using
Deserializerequires havingserdeas a non-optional dependency, which is not a change we can do lightly (it creates errors) - we can't get the rule name yet, there are changes that needs to be implemented
d95dbaf to
b7ef59d
Compare
b7ef59d to
914cf1d
Compare
9e2560e to
c66f830
Compare
Summary
Closes #3324
This PR starts the works needed to implement "options" or "configuration" for the analyzer. The reason why I used the term "option" is because is in line with the formatter terminology.
As highlighted in the umbrella issue, I decided to split the work in different stages to make the implementation of the feature progressive. There are many changes that we have to do, and I felt that needed to be split in multiple stages.
This PR adds a new
AnalyzerOptionsstruct which, for now, will contain two information:globals, which are derived - for now - byConfiguration::javascript::linter::globals. In the future this will be computed based on the languagerules, which is derived from the configuration of the rules - therome.jsonfile essentially - that matchRuleSettings. At the moment the logic that computes the rules is hardcoded. Ideally this will have to be auto generated for each group. I will do such in another PR.rulesis anHashMapmapped like this: "name of the rule" =>Box<RawValue>. In order to useRawValue, I had to addserde_jsonas dependency. I tried to useBox<impl Any>, but unfortunatelyRawValuedoesn't play well withAnybecauseRawValuedoesn't have a size that the compiler can know at compile time.analyzefunction now needs an&optionsas additional parameterTest Plan
The logic of the analyzer slightly changed, but it should only a compile time change and the rest of the existing tests should pass.