Auto merge of #56392 - petrochenkov:regensym, r=oli-obk

Delay gensym creation for "underscore items" (`use foo as _`/`const _`) until name resolution

So they cannot be cloned by macros. See #56303 for the discussion.

Mostly fix cross-crate use of underscore items by inverting the "gensyms are lost in metadata" bug as described in #56303 (comment).
Fix unused import warnings for single-segment imports (first commit) and `use crate_name as _` imports (as specified in #56303 (comment)).
Prohibit accidentally implemented `static _: TYPE = EXPR;` (cc #55983).
Add more tests for `use foo as _` imports.

Author: bors

src/librustc_resolve/build_reduced_graph.rs

@@ -157,7 +157,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
         };
         match use_tree.kind {
             ast::UseTreeKind::Simple(rename, ..) => {
-                let mut ident = use_tree.ident();
+                let mut ident = use_tree.ident().gensym_if_underscore();
                 let mut module_path = prefix;
                 let mut source = module_path.pop().unwrap();
                 let mut type_ns_only = false;
@@ -230,13 +230,18 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                 }
 
                 let subclass = SingleImport {
-                    target: ident,
                     source: source.ident,
-                    result: PerNS {
+                    target: ident,
+                    source_bindings: PerNS {
                         type_ns: Cell::new(Err(Undetermined)),
                         value_ns: Cell::new(Err(Undetermined)),
                         macro_ns: Cell::new(Err(Undetermined)),
                     },
+                    target_bindings: PerNS {
+                        type_ns: Cell::new(None),
+                        value_ns: Cell::new(None),
+                        macro_ns: Cell::new(None),
+                    },
                     type_ns_only,
                 };
                 self.add_import_directive(
@@ -329,7 +334,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
     fn build_reduced_graph_for_item(&mut self, item: &Item, parent_scope: ParentScope<'a>) {
         let parent = parent_scope.module;
         let expansion = parent_scope.expansion;
-        let ident = item.ident;
+        let ident = item.ident.gensym_if_underscore();
         let sp = item.span;
         let vis = self.resolve_visibility(&item.vis);
 
@@ -623,7 +628,11 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
 
     /// Builds the reduced graph for a single item in an external crate.
     fn build_reduced_graph_for_external_crate_def(&mut self, parent: Module<'a>, child: Export) {
-        let Export { ident, def, vis, span, .. } = child;
+        let Export { ident, def, 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 def_id = def.def_id();
         let expansion = Mark::root(); // FIXME(jseyfried) intercrate hygiene
         match def {

src/librustc_resolve/lib.rs

@@ -1519,8 +1519,12 @@ pub struct Resolver<'a, 'b: 'a> {
     /// The current self item if inside an ADT (used for better errors).
     current_self_item: Option<NodeId>,
 
-    /// FIXME: Refactor things so that this is passed through arguments and not resolver.
+    /// FIXME: Refactor things so that these fields are passed through arguments and not resolver.
+    /// We are resolving a last import segment during import validation.
     last_import_segment: bool,
+    /// This binding should be ignored during in-module resolution, so that we don't get
+    /// "self-confirming" import resolutions during import validation.
+    blacklisted_binding: Option<&'a NameBinding<'a>>,
 
     /// The idents for the primitive types.
     primitive_type_table: PrimitiveTypeTable,
@@ -1871,6 +1875,7 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
             current_self_type: None,
             current_self_item: None,
             last_import_segment: false,
+            blacklisted_binding: None,
 
             primitive_type_table: PrimitiveTypeTable::new(),
 

src/librustc_resolve/macros.rs

@@ -977,12 +977,14 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                 let what = self.binding_description(binding, ident,
                                                     flags.contains(Flags::MISC_FROM_PRELUDE));
                 let note_msg = format!("this import refers to {what}", what = what);
-                if binding.span.is_dummy() {
+                let label_span = if binding.span.is_dummy() {
                     err.note(&note_msg);
+                    ident.span
                 } else {
                     err.span_note(binding.span, &note_msg);
-                    err.span_label(binding.span, "not an extern crate passed with `--extern`");
-                }
+                    binding.span
+                };
+                err.span_label(label_span, "not an extern crate passed with `--extern`");
                 err.emit();
             }
 

src/librustc_resolve/resolve_imports.rs

@@ -42,9 +42,15 @@ use std::{mem, ptr};
 #[derive(Clone, Debug)]
 pub enum ImportDirectiveSubclass<'a> {
     SingleImport {
-        target: Ident,
+        /// `source` in `use prefix::source as target`.
         source: Ident,
-        result: PerNS<Cell<Result<&'a NameBinding<'a>, Determinacy>>>,
+        /// `target` in `use prefix::source as target`.
+        target: Ident,
+        /// Bindings to which `source` refers to.
+        source_bindings: PerNS<Cell<Result<&'a NameBinding<'a>, Determinacy>>>,
+        /// Bindings introduced by `target`.
+        target_bindings: PerNS<Cell<Option<&'a NameBinding<'a>>>>,
+        /// `true` for `...::{self [as target]}` imports, `false` otherwise.
         type_ns_only: bool,
     },
     GlobImport {
@@ -227,6 +233,11 @@ impl<'a, 'crateloader> Resolver<'a, 'crateloader> {
         }
 
         let check_usable = |this: &mut Self, binding: &'a NameBinding<'a>| {
+            if let Some(blacklisted_binding) = this.blacklisted_binding {
+                if ptr::eq(binding, blacklisted_binding) {
+                    return Err((Determined, Weak::No));
+                }
+            }
             // `extern crate` are always usable for backwards compatibility, see issue #37020,
             // remove this together with `PUB_USE_OF_PRIVATE_EXTERN_CRATE`.
             let usable = this.is_accessible(binding.vis) || binding.is_extern_crate();
@@ -642,10 +653,10 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
             if let Some((span, err, note)) = self.finalize_import(import) {
                 errors = true;
 
-                if let SingleImport { source, ref result, .. } = import.subclass {
+                if let SingleImport { source, ref source_bindings, .. } = import.subclass {
                     if source.name == "self" {
                         // Silence `unresolved import` error if E0429 is already emitted
-                        if let Err(Determined) = result.value_ns.get() {
+                        if let Err(Determined) = source_bindings.value_ns.get() {
                             continue;
                         }
                     }
@@ -765,9 +776,11 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
         };
 
         directive.imported_module.set(Some(module));
-        let (source, target, result, type_ns_only) = match directive.subclass {
-            SingleImport { source, target, ref result, type_ns_only } =>
-                (source, target, result, type_ns_only),
+        let (source, target, source_bindings, target_bindings, type_ns_only) =
+                match directive.subclass {
+            SingleImport { source, target, ref source_bindings,
+                           ref target_bindings, type_ns_only } =>
+                (source, target, source_bindings, target_bindings, type_ns_only),
             GlobImport { .. } => {
                 self.resolve_glob_import(directive);
                 return true;
@@ -777,7 +790,7 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
 
         let mut indeterminate = false;
         self.per_ns(|this, ns| if !type_ns_only || ns == TypeNS {
-            if let Err(Undetermined) = result[ns].get() {
+            if let Err(Undetermined) = source_bindings[ns].get() {
                 // For better failure detection, pretend that the import will
                 // not define any names while resolving its module path.
                 let orig_vis = directive.vis.replace(ty::Visibility::Invisible);
@@ -786,13 +799,13 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
                 );
                 directive.vis.set(orig_vis);
 
-                result[ns].set(binding);
+                source_bindings[ns].set(binding);
             } else {
                 return
             };
 
             let parent = directive.parent_scope.module;
-            match result[ns].get() {
+            match source_bindings[ns].get() {
                 Err(Undetermined) => indeterminate = true,
                 Err(Determined) => {
                     this.update_resolution(parent, target, ns, |_, resolution| {
@@ -810,6 +823,7 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
                 }
                 Ok(binding) => {
                     let imported_binding = this.import(binding, directive);
+                    target_bindings[ns].set(Some(imported_binding));
                     let conflict = this.try_define(parent, target, ns, imported_binding);
                     if let Err(old_binding) = conflict {
                         this.report_conflict(parent, target, ns, imported_binding, old_binding);
@@ -879,8 +893,11 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
             PathResult::Indeterminate | PathResult::NonModule(..) => unreachable!(),
         };
 
-        let (ident, result, type_ns_only) = match directive.subclass {
-            SingleImport { source, ref result, type_ns_only, .. } => (source, result, type_ns_only),
+        let (ident, target, source_bindings, target_bindings, type_ns_only) =
+                match directive.subclass {
+            SingleImport { source, target, ref source_bindings,
+                           ref target_bindings, type_ns_only } =>
+                (source, target, source_bindings, target_bindings, type_ns_only),
             GlobImport { is_prelude, ref max_vis } => {
                 if directive.module_path.len() <= 1 {
                     // HACK(eddyb) `lint_if_path_starts_with_module` needs at least
@@ -919,20 +936,28 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
         let mut all_ns_err = true;
         self.per_ns(|this, ns| if !type_ns_only || ns == TypeNS {
             let orig_vis = directive.vis.replace(ty::Visibility::Invisible);
+            let orig_blacklisted_binding =
+                mem::replace(&mut this.blacklisted_binding, target_bindings[ns].get());
             let orig_last_import_segment = mem::replace(&mut this.last_import_segment, true);
             let binding = this.resolve_ident_in_module(
                 module, ident, ns, Some(&directive.parent_scope), true, directive.span
             );
             this.last_import_segment = orig_last_import_segment;
+            this.blacklisted_binding = orig_blacklisted_binding;
             directive.vis.set(orig_vis);
 
             match binding {
                 Ok(binding) => {
                     // Consistency checks, analogous to `finalize_current_module_macro_resolutions`.
-                    let initial_def = result[ns].get().map(|initial_binding| {
+                    let initial_def = source_bindings[ns].get().map(|initial_binding| {
                         all_ns_err = false;
-                        this.record_use(ident, ns, initial_binding,
-                                        directive.module_path.is_empty());
+                        if let Some(target_binding) = target_bindings[ns].get() {
+                            if target.name == "_" &&
+                               initial_binding.is_extern_crate() && !initial_binding.is_import() {
+                                this.record_use(ident, ns, target_binding,
+                                                directive.module_path.is_empty());
+                            }
+                        }
                         initial_binding.def_ignoring_ambiguity()
                     });
                     let def = binding.def_ignoring_ambiguity();
@@ -1034,7 +1059,7 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
         let mut reexport_error = None;
         let mut any_successful_reexport = false;
         self.per_ns(|this, ns| {
-            if let Ok(binding) = result[ns].get() {
+            if let Ok(binding) = source_bindings[ns].get() {
                 let vis = directive.vis.get();
                 if !binding.pseudo_vis().is_at_least(vis, &*this) {
                     reexport_error = Some((ns, binding));
@@ -1078,7 +1103,7 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
             let mut full_path = directive.module_path.clone();
             full_path.push(Segment::from_ident(ident));
             self.per_ns(|this, ns| {
-                if let Ok(binding) = result[ns].get() {
+                if let Ok(binding) = source_bindings[ns].get() {
                     this.lint_if_path_starts_with_module(
                         directive.crate_lint(),
                         &full_path,
@@ -1092,7 +1117,7 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
         // Record what this import resolves to for later uses in documentation,
         // this may resolve to either a value or a type, but for documentation
         // purposes it's good enough to just favor one over the other.
-        self.per_ns(|this, ns| if let Some(binding) = result[ns].get().ok() {
+        self.per_ns(|this, ns| if let Some(binding) = source_bindings[ns].get().ok() {
             let mut def = binding.def();
             if let Def::Macro(def_id, _) = def {
                 // `DefId`s from the "built-in macro crate" should not leak from resolve because

src/libsyntax/parse/parser.rs

@@ -2017,6 +2017,17 @@ impl<'a> Parser<'a> {
         }
     }
 
+    fn parse_ident_or_underscore(&mut self) -> PResult<'a, ast::Ident> {
+        match self.token {
+            token::Ident(ident, false) if ident.name == keywords::Underscore.name() => {
+                let span = self.span;
+                self.bump();
+                Ok(Ident::new(ident.name, span))
+            }
+            _ => self.parse_ident(),
+        }
+    }
+
     /// Parses qualified path.
     /// Assumes that the leading `<` has been parsed already.
     ///
@@ -6434,13 +6445,7 @@ impl<'a> Parser<'a> {
     }
 
     fn parse_item_const(&mut self, m: Option<Mutability>) -> PResult<'a, ItemInfo> {
-        let id = match self.token {
-                token::Ident(ident, false) if ident.name == keywords::Underscore.name() => {
-                    self.bump(); // `_`
-                    ident.gensym()
-                    },
-                _ => self.parse_ident()?,
-            };
+        let id = if m.is_none() { self.parse_ident_or_underscore() } else { self.parse_ident() }?;
         self.expect(&token::Colon)?;
         let ty = self.parse_ty()?;
         self.expect(&token::Eq)?;
@@ -7725,13 +7730,7 @@ impl<'a> Parser<'a> {
 
     fn parse_rename(&mut self) -> PResult<'a, Option<Ident>> {
         if self.eat_keyword(keywords::As) {
-            match self.token {
-                token::Ident(ident, false) if ident.name == keywords::Underscore.name() => {
-                    self.bump(); // `_`
-                    Ok(Some(ident.gensym()))
-                }
-                _ => self.parse_ident().map(Some),
-            }
+            self.parse_ident_or_underscore().map(Some)
         } else {
             Ok(None)
         }

src/libsyntax_pos/symbol.rs

@@ -80,6 +80,10 @@ impl Ident {
         Ident::new(self.name.gensymed(), self.span)
     }
 
+    pub fn gensym_if_underscore(self) -> Ident {
+        if self.name == keywords::Underscore.name() { self.gensym() } else { self }
+    }
+
     pub fn as_str(self) -> LocalInternedString {
         self.name.as_str()
     }
@@ -473,7 +477,7 @@ impl Ident {
     // We see this identifier in a normal identifier position, like variable name or a type.
     // How was it written originally? Did it use the raw form? Let's try to guess.
     pub fn is_raw_guess(self) -> bool {
-        self.name != keywords::Invalid.name() &&
+        self.name != keywords::Invalid.name() && self.name != keywords::Underscore.name() &&
         self.is_reserved() && !self.is_path_segment_keyword()
     }
 }

src/test/ui/feature-gates/feature-gate-uniform-paths.stderr

@@ -25,11 +25,7 @@ LL | use inline; //~ ERROR imports can only refer to extern crate names
    |     ^^^^^^ not an extern crate passed with `--extern`
    |
    = help: add #![feature(uniform_paths)] to the crate attributes to enable
-note: this import refers to the built-in attribute imported here
-  --> $DIR/feature-gate-uniform-paths.rs:21:5
-   |
-LL | use inline; //~ ERROR imports can only refer to extern crate names
-   |     ^^^^^^
+   = note: this import refers to a built-in attribute
 
 error[E0658]: imports can only refer to extern crate names passed with `--extern` on stable channel (see issue #53130)
   --> $DIR/feature-gate-uniform-paths.rs:23:5
@@ -38,11 +34,7 @@ LL | use Vec; //~ ERROR imports can only refer to extern crate names
    |     ^^^ not an extern crate passed with `--extern`
    |
    = help: add #![feature(uniform_paths)] to the crate attributes to enable
-note: this import refers to the struct imported here
-  --> $DIR/feature-gate-uniform-paths.rs:23:5
-   |
-LL | use Vec; //~ ERROR imports can only refer to extern crate names
-   |     ^^^
+   = note: this import refers to a struct from prelude
 
 error[E0658]: imports can only refer to extern crate names passed with `--extern` on stable channel (see issue #53130)
   --> $DIR/feature-gate-uniform-paths.rs:25:5
@@ -51,11 +43,7 @@ LL | use vec; //~ ERROR imports can only refer to extern crate names
    |     ^^^ not an extern crate passed with `--extern`
    |
    = help: add #![feature(uniform_paths)] to the crate attributes to enable
-note: this import refers to the macro imported here
-  --> $DIR/feature-gate-uniform-paths.rs:25:5
-   |
-LL | use vec; //~ ERROR imports can only refer to extern crate names
-   |     ^^^
+   = note: this import refers to a macro from prelude
 
 error: aborting due to 4 previous errors
 

src/test/ui/underscore_const_names_feature_gate.rs renamed to src/test/ui/feature-gates/underscore_const_names_feature_gate.rs

@@ -9,6 +9,5 @@
 // except according to those terms.
 
 const _: () = (); //~ ERROR is unstable
-static _: () = (); //~ ERROR is unstable
 
 fn main() {}

src/test/ui/underscore_const_names_feature_gate.stderr renamed to src/test/ui/feature-gates/underscore_const_names_feature_gate.stderr

@@ -6,14 +6,6 @@ LL | const _: () = (); //~ ERROR is unstable
    |
    = help: add #![feature(underscore_const_names)] to the crate attributes to enable
 
-error[E0658]: naming constants with `_` is unstable (see issue #54912)
-  --> $DIR/underscore_const_names_feature_gate.rs:12:1
-   |
-LL | static _: () = (); //~ ERROR is unstable
-   | ^^^^^^^^^^^^^^^^^^
-   |
-   = help: add #![feature(underscore_const_names)] to the crate attributes to enable
-
-error: aborting due to 2 previous errors
+error: aborting due to previous error
 
 For more information about this error, try `rustc --explain E0658`.

src/test/ui/parser/underscore_static.rs

@@ -0,0 +1,3 @@
+// compile-flags: -Z parse-only
+
+static _: () = (); //~ ERROR expected identifier, found reserved identifier `_`

src/test/ui/parser/underscore_static.stderr

@@ -0,0 +1,8 @@
+error: expected identifier, found reserved identifier `_`
+  --> $DIR/underscore_static.rs:3:8
+   |
+LL | static _: () = (); //~ ERROR expected identifier, found reserved identifier `_`
+   |        ^ expected identifier, found reserved identifier
+
+error: aborting due to previous error
+