feat(rome_js_semantic): simple globals resolution #3354
Conversation
✅ Deploy Preview for rometools canceled.
|
xunilrj
changed the title
| @@ -1041,7 +1109,7 @@ mod test { | |||
| FileId::zero(), | |||
| SourceType::js_module(), | |||
| ); | |||
| let model = semantic_model(&r.tree()); | |||
| let model = semantic_model(&r.tree(), SemanticModelOptions::default()); | |||
There was a problem hiding this comment.
Shouldn't we fill the globals with the ones that we have inside globals/mod.rs? At least with the ones we use inside noUndeclaredVariables.
And maybe, while we are at it, we can change the rule to use the new API, so we know it works 100%
There was a problem hiding this comment.
Shouldn't we fill the globals with the ones that we have inside globals/mod.rs?
Yes, but it should be done at crates\rome_js_analyze\src\lib.rs:76 where you have info from the file being analyzed and can choose the globals.
| #[derive(Default)] | ||
| /// Extra options for the [SemanticModel] creation. | ||
| pub struct SemanticModelOptions { | ||
| /// All the allowed globals names |
There was a problem hiding this comment.
| /// All the allowed globals names | |
| /// All the allowed global names |
| /// Extra options for the [SemanticModel] creation. | ||
| pub struct SemanticModelOptions { | ||
| /// All the allowed globals names | ||
| pub globals: HashSet<String>, |
There was a problem hiding this comment.
How does HashSet compare in performance against using a pre-sorted Vec of strings and a binary search ?
There was a problem hiding this comment.
I prefer to merge this like it is, and we can bench in a real case later. I did a similar test while ago tough: #2975
Summary
This PR implements a simple global resolution for the semantic model.
Test Plan