-
Notifications
You must be signed in to change notification settings - Fork 662
Conversation
Deploying with
|
Latest commit: |
c058442
|
Status: | ✅ Deploy successful! |
Preview URL: | https://3f779f77.tools-8rn.pages.dev |
Branch Preview URL: | https://feature-semantic-events-hois.tools-8rn.pages.dev |
/// Generated for: | ||
/// - All reference identifiers | ||
Read { | ||
range: TextRange, | ||
declaration_at: Option<TextRange>, | ||
}, | ||
|
||
/// Signifies that a new scope was started | ||
HoistedRead { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we document this?
/// Signifies that a new scope was started | ||
HoistedRead { | ||
range: TextRange, | ||
declaration_at: TextRange, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
declared_at
?
/// Generated for: | ||
/// - Blocks | ||
/// - Function body | ||
ScopeStarted { range: TextRange }, | ||
|
||
/// Signifies that a new scope was ended | ||
/// Tracks when a scope ends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"when" => "where"?
/// Tracks when a reference do no have any matching | ||
/// binding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracks where a reference is read, but it doesn't have any matching binding
?
Would it be possible to add a concrete example? Is this a valid example?
var _window = window;
var _this = this;
declared: Vec<ScopeDeclaration>, | ||
/// All bindings declared inside this scope | ||
bindings: Vec<Binding>, | ||
/// Reference that do not have a matching declaration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some mistake in the wording of this doc
.position(|scope| !scope.allows_decl_hoisting); | ||
|
||
match idx { | ||
None => unreachable!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a small explanation of why it's not reachable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Some(()) | ||
} | ||
|
||
fn solve_pending_references( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we document this method please? Mainly to explain the cases where we use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is seems that this function is used only inside a case where we already know that the current binding is a var. Do you plan to use it somewhere else in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It will be used for everything that can be hoisted, like functions.
for scope in scopes { | ||
if let Some(references) = scope.references.remove(&name) { | ||
for reference in references { | ||
self.stash.push_back(SemanticEvent::HoistedRead { | ||
range: reference.range, | ||
declaration_at: node.text_range(), | ||
}) | ||
} | ||
} | ||
|
||
if !scope.allows_decl_hoisting { | ||
break; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly are we doing here? Would it be possible add some snippet/example in the documentation? I don't really understand what "solving a pending reference" means in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this function and moved this code to the pop_scope
. Hopefully comments there are enough.
shadowed: vec![], | ||
allows_decl_hoisting, | ||
}); | ||
} | ||
|
||
fn pop_scope(&mut self, range: TextRange) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe some documentation/comments would help to understand the logic behind pop_scope
. Probably because I find really hard to grasp the whole logic and why we are doing that. I think that maybe some snippet can help to explain why/what we are doing:
- pushing events into the back of the stack
- based on what we decide the type of event
- the promotion of references
ok_hoisting_redeclaration_before_use, r#"var a/*#A1*/ = 1; | ||
function f() { | ||
var a/*#A2*/ = 10; | ||
console.log(a/*READ A2*/); | ||
} | ||
f();"#, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's worth adding a case where we have also console.log(a/*READ A1*/);
outside of the function f
This PR is the part that matches declarations and scopes of the main PR (#2489).
For more details: #2488
This PR implements hoisting.
We decided that the event extraction will not generate diagnostics. This will be a task to whoever read these events.
This PR also contains renaming some functions that did not enter in the last PR.