feat(rome_rowan): add the root node as an associated type on the language trait #2689
Conversation
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
💥 Failed to Panic (1):ts/babel
ts/microsoft
|
|
@xunilrj , Is this behavior expected ? |
|
@leops is there a test plan for this PR? Could you please put back that section from the template? |
| fn pull_actions( | ||
| &self, | ||
| params: PullActionsParams, | ||
| ) -> Result<Vec<AnalyzerAction<JsLanguage>>, RomeError>; |
There was a problem hiding this comment.
This doesn't seem correct. JsLanguage should not be inside this generic trait
There was a problem hiding this comment.
This is what I mentioned at the end of the PR description, basically this interface was never language-agnostic in the first place (because the analyzer currently only supports emitting code actions for JavaScript), this change only makes it explicit.
Eventually the Workspace will not directly return a list of AnalyzerAction objects, since those contain actual syntax tree instances it would be complicated to transfer those across an IPC boundary for instance, so the code action will probably get transformed into a simple text diff before being sent to the CLI or language server (which is then completely independent of the language)
| pub fn pull_actions( | ||
| &self, | ||
| range: TextRange, | ||
| ) -> Result<Vec<AnalyzerAction<JsLanguage>>, RomeError> { |
There was a problem hiding this comment.
Same here. Is there a plan to make this language-agnostic?
| pub(crate) struct RuleRegistry { | ||
| rules: Vec<RegistryRule>, | ||
| pub(crate) struct RuleRegistry<L: Language> { | ||
| rules: Vec<RegistryRule<L>>, | ||
| } | ||
|
|
||
| /// Utility macro for implementing the `with_filter` method of [RuleRegistry] |
There was a problem hiding this comment.
The comment seems outdated now. This macro is now language specific, maybe we should call it with a different name?
There was a problem hiding this comment.
The comment is still explaining what the macro is used for, although I agree it doesn't really explain why it works like this. In a future refactor this macro will either get moved to rome_js_analyze along with all the JS lint rules or completely removed and be replaced by the analyzer codegen script, so this is basically intended as temporary helper until the analysis infrastructure is properly split out from the language rules.
I didn't include that section because I didn't really have anything to add there, this change is done entirely at the type system level and doesn't modify any actual "program code" so it's tested by "if it builds, it works" |
leops
force-pushed
the
feature/language-root-type
branch
from
df162e4
to
d2861ae
Compare
Deploying with
|
| Latest commit: |
d2861ae
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e7e592da.tools-8rn.pages.dev |
| Branch Preview URL: | https://feature-language-root-type.tools-8rn.pages.dev |
No. I am trying to understand what is happening. :( |
Summary
This PR is part of the general goal of making the analyzer language-agnostic. It adds a
Rootassociated type on theLanguagetrait inrome_rowanbound to theAstNode<Language = Self>trait (as well asClone,DebugandEqfor convenience) and corresponding to the "document root" node type for this language (eg.JsAnyRootfor JavaScript).This lets most of the analyzer be agnostic over the language it's processing, except at the root level in the
analyzefunction since theRuleRegistryonly has a factory for JS rules at the moment. As an extension of this, theWorkspacealso only supports emitting code actions strongly typed to the JavaScript language for now.