-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[WIP] Resolve: refactor define
into define_local
and define_extern
#143884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[WIP] Resolve: refactor define
into define_local
and define_extern
#143884
Conversation
@@ -477,7 +492,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { | |||
if self.is_accessible_from(binding.vis, scope) { | |||
let imported_binding = self.import(binding, *import); | |||
let key = BindingKey { ident, ..key }; | |||
let _ = self.try_define( | |||
// FIXME: local or external? | |||
let _ = self.try_define_local( |
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.
This is in update_resolutions
, I don't know if this is an equivalent change.
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.
You could add asserts to (try_)define_(local,extern)
making sure that the passed parent
, res
and vis
are actually local or external, respectively.
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 don't know how I should check if vis
is local or not.
@@ -1078,7 +1078,7 @@ pub struct Resolver<'ra, 'tcx> { | |||
extern_module_map: RefCell<FxIndexMap<DefId, Module<'ra>>>, | |||
binding_parent_modules: FxHashMap<NameBinding<'ra>, Module<'ra>>, | |||
|
|||
underscore_disambiguator: u32, | |||
underscore_disambiguator: Cell<u32>, |
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.
This is correct, right?
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.
No.
The local disambiguator shouldn't be used for external underscore names.
Such names should either never appear in external module children (but it seems like we cannot do that, unfortunately), or we should assign disambiguators to them before writing them to metadata.
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.
Apparently the current resolver logic relies on all disambiguators being different for all local and loaded external names.
I'll think what to do with this.
☔ The latest upstream changes (presumably #143888) made this pull request unmergeable. Please resolve the merge conflicts. |
Conflicts must be resolved in Petrochenkov's PRs, I think. I can then rebase off that. |
) { | ||
let binding = self.arenas.new_res_binding(res, vis.to_def_id(), span, expn_id); | ||
let key = self.new_disambiguated_key(ident, ns); | ||
self.check_reserved_macro_name(key.ident, binding.res()); |
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.
This is not needed for external items.
Follow up on #143550 and part of #gsoc > Project: Parallel Macro Expansion.
Split up
define
intodefine_local
anddefine_extern
. Refactor usages ofdefine
into either one where it's "correct" (idk if everything is correct atm). Big part of this is thatresolution
can now take a&Resolver
instead of a mutable one.Needed the module changes of #143550. Review will probably be easier when viewing my first commit instead of all the changes.
r? @petrochenkov