[WIP] Resolver: introduce SmartResolver for speculative and finalize resolutions.
#144912
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-author
LorrensP-2158466
force-pushed
the
smart-resolver
branch
from
4d8cc9f to
3ab8e2f
Compare
LorrensP-2158466
commented
Aug 4, 2025
| None, | ||
| None, | ||
| ) else { | ||
| let Ok((Some(ext), _)) = SmartResolver::Finalize(this) |
There was a problem hiding this comment.
There are a lot of constructions like this, should we introduce some helpers in Resolver?:
fn early(&self) -> SmartResolver;
fn late(&mut self) -> SmartResolver;
LorrensP-2158466
commented
Aug 4, 2025
Comment on lines
+3254
to
+3263
| impl<'r, 'ra, 'tcx> SmartResolver<'r, 'ra, 'tcx> { | ||
| pub(crate) fn lint_if_path_starts_with_module( | ||
| mut self, | ||
| finalize: Option<Finalize>, | ||
| path: &[Segment], | ||
| second_binding: Option<NameBinding<'_>>, | ||
| ) { | ||
| let Some(Finalize { node_id, root_span, .. }) = finalize else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
Should this take an Option<Finalize>? Maybe we should handle this at the call site so this function can stay inside of the Resolver.
LorrensP-2158466
commented
Aug 4, 2025
Comment on lines
+43
to
+51
| pub(crate) trait ScopeVisitor<'ra, 'tcx> | ||
| where | ||
| Self: AsRef<Resolver<'ra, 'tcx>> + Sized, | ||
| { | ||
| /// A generic scope visitor. | ||
| /// Visits scopes in order to resolve some identifier in them or perform other actions. | ||
| /// If the callback returns `Some` result, we stop visiting scopes and return it. | ||
| pub(crate) fn visit_scopes<T>( | ||
| &mut self, | ||
| fn visit_scopes<T>( | ||
| mut self, |
There was a problem hiding this comment.
In the first commit I have this function duplicated for SmartResolver. In the second commit I have combined them in a trait, is this allowed?
LorrensP-2158466
commented
Aug 4, 2025
Comment on lines
+1483
to
-1471
| fn record_segment_res<'r, 'ra, 'tcx>( | ||
| mut this: SmartResolver<'r, 'ra, 'tcx>, | ||
| finalize: Option<Finalize>, | ||
| res: Res, | ||
| id: Option<NodeId>, | ||
| ) { | ||
| if finalize.is_some() | ||
| && let Some(id) = id | ||
| && !this.partial_res_map.contains_key(&id) | ||
| { | ||
| assert!(id != ast::DUMMY_NODE_ID, "Trying to resolve dummy id"); | ||
| this.res_mut().record_partial_res(id, PartialRes::new(res)); | ||
| } | ||
| } | ||
|
|
||
| for (segment_idx, &Segment { ident, id, .. }) in path.iter().enumerate() { | ||
| debug!("resolve_path ident {} {:?} {:?}", segment_idx, ident, id); | ||
| let record_segment_res = |this: &mut Self, res| { | ||
| if finalize.is_some() | ||
| && let Some(id) = id | ||
| && !this.partial_res_map.contains_key(&id) | ||
| { | ||
| assert!(id != ast::DUMMY_NODE_ID, "Trying to resolve dummy id"); | ||
| this.record_partial_res(id, PartialRes::new(res)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This was the only way i could circumvent borrow check errors. If this change is unwanted I can find out if the closure is still possible.
LorrensP-2158466
commented
Aug 4, 2025
There was a problem hiding this comment.
Every use of the SmartResolver in this file is SmartResolver::Finalize, is this correct?
LorrensP-2158466
commented
Aug 4, 2025
Comment on lines
+2388
to
+2391
| enum SmartResolver<'r, 'ra, 'tcx> { | ||
| Speculative(&'r Resolver<'ra, 'tcx>), | ||
| Finalize(&'r mut Resolver<'ra, 'tcx>), | ||
| } |
There was a problem hiding this comment.
Names are still debatable.
LorrensP-2158466
commented
Aug 4, 2025
Comment on lines
+1122
to
+1124
| if trace { | ||
| let kind = kind.expect("macro kind must be specified if tracing is enabled"); | ||
| self.res_mut().multi_segment_macro_resolutions.push(( |
There was a problem hiding this comment.
Relevant part of the trace and force question in #gsoc > Project: Parallel Macro Expansion @ 💬.
Edit: CI also fails here
LorrensP-2158466
commented
Aug 4, 2025
Comment on lines
+1151
to
+1153
| if trace { | ||
| let kind = kind.expect("macro kind must be specified if tracing is enabled"); | ||
| self.res_mut().single_segment_macro_resolutions.push(( |
There was a problem hiding this comment.
Relevant part of the trace and force question in #gsoc > Project: Parallel Macro Expansion @ 💬.
Edit: CI also fails here
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a
SmartResolver, which knows when it's in speculative or finalize mode and if it's in finalize, it can give out a mutable reference to the actual Resolver:I have moved functions of the resolver to the smarter one, where I thought this was necessary, so if something looks out of place, it probably is.
I'll update this description when the pr is almost done, for now I'll post comments on parts of code I think deserve more attention, it's a pretty big diff...
r? petrochenkov