Skip to content
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

Remove last uses of gensyms #64623

Merged
merged 4 commits into from
Oct 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/librustc/ty/print/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1483,7 +1483,7 @@ impl<F: fmt::Write> FmtPrinter<'_, 'tcx, F> {
}

// Replace any anonymous late-bound regions with named
// variants, using gensym'd identifiers, so that we can
// variants, using new unique identifiers, so that we can
// clearly differentiate between named and unnamed regions in
// the output. We'll probably want to tweak this over time to
// decide just how much information to give.
Expand Down
16 changes: 8 additions & 8 deletions src/librustc_resolve/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ impl<'a> Resolver<'a> {
where T: ToNameBinding<'a>,
{
let binding = def.to_name_binding(self.arenas);
if let Err(old_binding) = self.try_define(parent, ident, ns, binding) {
let key = self.new_key(ident, ns);
if let Err(old_binding) = self.try_define(parent, key, binding) {
self.report_conflict(parent, ident, ns, old_binding, &binding);
}
}
Expand Down Expand Up @@ -349,9 +350,12 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {

self.r.indeterminate_imports.push(directive);
match directive.subclass {
// Don't add unresolved underscore imports to modules
SingleImport { target: Ident { name: kw::Underscore, .. }, .. } => {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this special case is removed? Some diagnostics regress?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, we just won't be able to remove it in resolve_imports.rs:831.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, the disambiguator is not available in that context.

SingleImport { target, type_ns_only, .. } => {
self.r.per_ns(|this, ns| if !type_ns_only || ns == TypeNS {
let mut resolution = this.resolution(current_module, target, ns).borrow_mut();
let key = this.new_key(target, ns);
let mut resolution = this.resolution(current_module, key).borrow_mut();
resolution.add_single_import(directive);
});
}
Expand Down Expand Up @@ -407,7 +411,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
};
match use_tree.kind {
ast::UseTreeKind::Simple(rename, ..) => {
let mut ident = use_tree.ident().gensym_if_underscore();
let mut ident = use_tree.ident();
let mut module_path = prefix;
let mut source = module_path.pop().unwrap();
let mut type_ns_only = false;
Expand Down Expand Up @@ -585,7 +589,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
let parent_scope = &self.parent_scope;
let parent = parent_scope.module;
let expansion = parent_scope.expansion;
let ident = item.ident.gensym_if_underscore();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'd expect every _ name to get SyntaxContext from a fresh macro expansion with def-site in empty_module, like it was done for other gensyms in #63919.

Thus we'll reuse the same common mechanism, and when hygiene data becomes encodable we'll be able to properly encode _s in metadata as well, thus resolving the "FIXME: We shouldn't create the gensym here" below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glob imports operate on the actual hygiene data: https://github.com/rust-lang/rust/blob/da01d31ff97feb199036359c364aedc809711123/src/librustc_resolve/resolve_imports.rs#L563

I'll see if there's a way to do this without increasing the complexity of reverse_glob_adjust

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, the difference is that _s bring traits into scope for the purpose of type-based resolution, and names from opaque macros don't.

This difference is orthogonal to globs, so perhaps it's collection of traits in scope that needs to be adjusted, not reverse_glob_adjust.

(It's somewhat unfortunate that the difference exists and the features cannot be fully unified, but in both cases the behavior looks entirely reasonable and appropriate.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm specifically concerned about preserving the behavior of the following:

mod x {
    use Trait as _;
}
use x::*;
fn f() {
    ().method_on_trait(); 
    // ^This resolves if the whole snippet comes from a single macro expansion
    // It generally doesn't resolve otherwise
}

I think I have a working solution for this, which I'll push once I'm happy.

let ident = item.ident;
let sp = item.span;
let vis = self.resolve_visibility(&item.vis);

Expand Down Expand Up @@ -850,10 +854,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
fn build_reduced_graph_for_external_crate_res(&mut self, child: Export<NodeId>) {
let parent = self.parent_scope.module;
let Export { ident, res, vis, span } = child;
// FIXME: We shouldn't create the gensym here, it should come from metadata,
// but metadata cannot encode gensyms currently, so we create it here.
// This is only a guess, two equivalent idents may incorrectly get different gensyms here.
let ident = ident.gensym_if_underscore();
let expansion = ExpnId::root(); // FIXME(jseyfried) intercrate hygiene
// Record primary definitions.
match res {
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_resolve/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,11 @@ impl<'a> Resolver<'a> {
names: &mut Vec<TypoSuggestion>,
filter_fn: &impl Fn(Res) -> bool,
) {
for (&(ident, _), resolution) in self.resolutions(module).borrow().iter() {
for (key, resolution) in self.resolutions(module).borrow().iter() {
if let Some(binding) = resolution.borrow().binding {
let res = binding.res();
if filter_fn(res) {
names.push(TypoSuggestion::from_res(ident.name, res));
names.push(TypoSuggestion::from_res(key.ident.name, res));
}
}
}
Expand Down Expand Up @@ -849,7 +849,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
}

let resolutions = self.r.resolutions(crate_module).borrow();
let resolution = resolutions.get(&(ident, MacroNS))?;
let resolution = resolutions.get(&self.r.new_key(ident, MacroNS))?;
let binding = resolution.borrow().binding()?;
if let Res::Def(DefKind::Macro(MacroKind::Bang), _) = binding.res() {
let module_name = crate_module.kind.name().unwrap();
Expand Down
38 changes: 33 additions & 5 deletions src/librustc_resolve/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,22 @@ impl ModuleKind {
}
}

type Resolutions<'a> = RefCell<FxIndexMap<(Ident, Namespace), &'a RefCell<NameResolution<'a>>>>;
/// A key that identifies a binding in a given `Module`.
///
/// Multiple bindings in the same module can have the same key (in a valid
/// program) if all but one of them come from glob imports.
#[derive(Copy, Clone, PartialEq, Eq, Hash)]
struct BindingKey {
/// The identifier for the binding, aways the `modern` version of the
/// identifier.
ident: Ident,
ns: Namespace,
/// 0 if ident is not `_`, otherwise a value that's unique to the specific
/// `_` in the expanded AST that introduced this binding.
disambiguator: u32,
}

type Resolutions<'a> = RefCell<FxIndexMap<BindingKey, &'a RefCell<NameResolution<'a>>>>;

/// One node in the tree of modules.
pub struct ModuleData<'a> {
Expand Down Expand Up @@ -491,8 +506,8 @@ impl<'a> ModuleData<'a> {
fn for_each_child<R, F>(&'a self, resolver: &mut R, mut f: F)
where R: AsMut<Resolver<'a>>, F: FnMut(&mut R, Ident, Namespace, &'a NameBinding<'a>)
{
for (&(ident, ns), name_resolution) in resolver.as_mut().resolutions(self).borrow().iter() {
name_resolution.borrow().binding.map(|binding| f(resolver, ident, ns, binding));
for (key, name_resolution) in resolver.as_mut().resolutions(self).borrow().iter() {
name_resolution.borrow().binding.map(|binding| f(resolver, key.ident, key.ns, binding));
}
}

Expand Down Expand Up @@ -879,6 +894,7 @@ pub struct Resolver<'a> {
module_map: FxHashMap<DefId, Module<'a>>,
extern_module_map: FxHashMap<(DefId, bool /* MacrosOnly? */), Module<'a>>,
binding_parent_modules: FxHashMap<PtrKey<'a, NameBinding<'a>>, Module<'a>>,
underscore_disambiguator: u32,

/// Maps glob imports to the names of items actually imported.
pub glob_map: GlobMap,
Expand Down Expand Up @@ -1156,6 +1172,7 @@ impl<'a> Resolver<'a> {
label_res_map: Default::default(),
export_map: FxHashMap::default(),
trait_map: Default::default(),
underscore_disambiguator: 0,
empty_module,
module_map,
block_map: Default::default(),
Expand Down Expand Up @@ -1280,6 +1297,17 @@ impl<'a> Resolver<'a> {
self.arenas.alloc_module(module)
}

fn new_key(&mut self, ident: Ident, ns: Namespace) -> BindingKey {
let ident = ident.modern();
let disambiguator = if ident.name == kw::Underscore {
self.underscore_disambiguator += 1;
self.underscore_disambiguator
} else {
0
};
BindingKey { ident, ns, disambiguator }
}

fn resolutions(&mut self, module: Module<'a>) -> &'a Resolutions<'a> {
if module.populate_on_access.get() {
module.populate_on_access.set(false);
Expand All @@ -1288,9 +1316,9 @@ impl<'a> Resolver<'a> {
&module.lazy_resolutions
}

fn resolution(&mut self, module: Module<'a>, ident: Ident, ns: Namespace)
fn resolution(&mut self, module: Module<'a>, key: BindingKey)
-> &'a RefCell<NameResolution<'a>> {
*self.resolutions(module).borrow_mut().entry((ident.modern(), ns))
*self.resolutions(module).borrow_mut().entry(key)
.or_insert_with(|| self.arenas.alloc_name_resolution())
}

Expand Down
Loading