From e35fbb31e7ecda8d73fd29158252c9c42eb9339c Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sat, 7 Sep 2019 21:22:36 +0100 Subject: [PATCH 1/4] Add more tests for underscore imports --- src/test/ui/underscore-imports/hygiene-2.rs | 33 ++++++++++++++ src/test/ui/underscore-imports/hygiene.rs | 40 +++++++++++++++++ src/test/ui/underscore-imports/hygiene.stderr | 27 +++++++++++ .../ui/underscore-imports/macro-expanded.rs | 45 +++++++++++++++++++ 4 files changed, 145 insertions(+) create mode 100644 src/test/ui/underscore-imports/hygiene-2.rs create mode 100644 src/test/ui/underscore-imports/hygiene.rs create mode 100644 src/test/ui/underscore-imports/hygiene.stderr create mode 100644 src/test/ui/underscore-imports/macro-expanded.rs diff --git a/src/test/ui/underscore-imports/hygiene-2.rs b/src/test/ui/underscore-imports/hygiene-2.rs new file mode 100644 index 0000000000000..bea61eae6b51a --- /dev/null +++ b/src/test/ui/underscore-imports/hygiene-2.rs @@ -0,0 +1,33 @@ +// Make sure that underscore imports with different contexts can exist in the +// same scope. + +// check-pass + +#![feature(decl_macro)] + +mod x { + pub use std::ops::Deref as _; +} + +macro n() { + pub use crate::x::*; +} + +#[macro_export] +macro_rules! p { + () => { pub use crate::x::*; } +} + +macro m($y:ident) { + mod $y { + crate::n!(); // Reexport of `Deref` should not be imported in `main` + crate::p!(); // Reexport of `Deref` should be imported into `main` + } +} + +m!(y); + +fn main() { + use crate::y::*; + (&()).deref(); +} diff --git a/src/test/ui/underscore-imports/hygiene.rs b/src/test/ui/underscore-imports/hygiene.rs new file mode 100644 index 0000000000000..a254f6eaa5980 --- /dev/null +++ b/src/test/ui/underscore-imports/hygiene.rs @@ -0,0 +1,40 @@ +// Make sure that underscore imports have the same hygiene considerations as +// other imports. + +#![feature(decl_macro)] + +mod x { + pub use std::ops::Deref as _; +} + + +macro glob_import() { + pub use crate::x::*; +} + +macro underscore_import() { + use std::ops::DerefMut as _; +} + +mod y { + crate::glob_import!(); + crate::underscore_import!(); +} + +macro create_module($y:ident) { + mod $y { + crate::glob_import!(); + crate::underscore_import!(); + } +} + +create_module!(z); + +fn main() { + use crate::y::*; + use crate::z::*; + glob_import!(); + underscore_import!(); + (&()).deref(); //~ ERROR no method named `deref` + (&mut ()).deref_mut(); //~ ERROR no method named `deref_mut` +} diff --git a/src/test/ui/underscore-imports/hygiene.stderr b/src/test/ui/underscore-imports/hygiene.stderr new file mode 100644 index 0000000000000..75797b0698d69 --- /dev/null +++ b/src/test/ui/underscore-imports/hygiene.stderr @@ -0,0 +1,27 @@ +error[E0599]: no method named `deref` found for type `&()` in the current scope + --> $DIR/hygiene.rs:38:11 + | +LL | (&()).deref(); + | ^^^^^ method not found in `&()` + | + = help: items from traits can only be used if the trait is in scope +help: the following trait is implemented but not in scope, perhaps add a `use` for it: + | +LL | use std::ops::Deref; + | + +error[E0599]: no method named `deref_mut` found for type `&mut ()` in the current scope + --> $DIR/hygiene.rs:39:15 + | +LL | (&mut ()).deref_mut(); + | ^^^^^^^^^ method not found in `&mut ()` + | + = help: items from traits can only be used if the trait is in scope +help: the following trait is implemented but not in scope, perhaps add a `use` for it: + | +LL | use std::ops::DerefMut; + | + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0599`. diff --git a/src/test/ui/underscore-imports/macro-expanded.rs b/src/test/ui/underscore-imports/macro-expanded.rs new file mode 100644 index 0000000000000..43f527bc9a408 --- /dev/null +++ b/src/test/ui/underscore-imports/macro-expanded.rs @@ -0,0 +1,45 @@ +// Check that macro expanded underscore imports behave as expected + +// check-pass + +#![feature(decl_macro, rustc_attrs)] + +mod x { + pub use std::ops::Not as _; +} + +macro m() { + mod w { + mod y { + pub use std::ops::Deref as _; + } + use crate::x::*; + use self::y::*; + use std::ops::DerefMut as _; + fn f() { + false.not(); + (&()).deref(); + (&mut ()).deref_mut(); + } + } +} + +#[rustc_macro_transparency = "transparent"] +macro n() { + mod z { + pub use std::ops::Deref as _; + } + use crate::x::*; + use crate::z::*; + use std::ops::DerefMut as _; + fn f() { + false.not(); + (&()).deref(); + (&mut ()).deref_mut(); + } +} + +m!(); +n!(); + +fn main() {} From c646cda1b2e7ec9108609250c2982c06b3e80289 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Fri, 27 Sep 2019 20:30:07 +0100 Subject: [PATCH 2/4] Don't use `gensym_if_underscore` to resolve `_` bindings Instead generate unique `SyntaxContexts` --- src/librustc/ich/impls_syntax.rs | 1 + src/librustc/lint/mod.rs | 4 ++- src/librustc_resolve/build_reduced_graph.rs | 12 ++++----- src/librustc_resolve/resolve_imports.rs | 10 ++++--- src/libsyntax_pos/hygiene.rs | 30 ++++++++++++++++++--- src/libsyntax_pos/lib.rs | 1 + src/libsyntax_pos/symbol.rs | 12 ++++++++- 7 files changed, 56 insertions(+), 14 deletions(-) diff --git a/src/librustc/ich/impls_syntax.rs b/src/librustc/ich/impls_syntax.rs index bdcf9e42ac2a8..7d98ebd88f174 100644 --- a/src/librustc/ich/impls_syntax.rs +++ b/src/librustc/ich/impls_syntax.rs @@ -373,6 +373,7 @@ impl_stable_hash_for!(enum ::syntax_pos::hygiene::ExpnKind { Root, Macro(kind, descr), AstPass(kind), + Underscore, Desugaring(kind) }); diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs index 5b490b701267d..1453bb5ce8519 100644 --- a/src/librustc/lint/mod.rs +++ b/src/librustc/lint/mod.rs @@ -871,7 +871,9 @@ pub fn provide(providers: &mut Providers<'_>) { pub fn in_external_macro(sess: &Session, span: Span) -> bool { let expn_data = span.ctxt().outer_expn_data(); match expn_data.kind { - ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop) => false, + ExpnKind::Root + | ExpnKind::Underscore + | ExpnKind::Desugaring(DesugaringKind::ForLoop) => false, ExpnKind::AstPass(_) | ExpnKind::Desugaring(_) => true, // well, it's "external" ExpnKind::Macro(MacroKind::Bang, _) => { if expn_data.def_site.is_dummy() { diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index f76aa95dd2cc8..2ede6c5ecf808 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -406,7 +406,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().unique_if_underscore(); let mut module_path = prefix; let mut source = module_path.pop().unwrap(); let mut type_ns_only = false; @@ -584,7 +584,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(); + let ident = item.ident.unique_if_underscore(); let sp = item.span; let vis = self.resolve_visibility(&item.vis); @@ -849,10 +849,10 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { fn build_reduced_graph_for_external_crate_res(&mut self, child: Export) { 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(); + // FIXME: We shouldn't create the SyntaxContext here, it should come from metadata, + // but metadata doesn't encode hygiene information currently, so we create it here. + // This is only a guess, two equivalent idents will get different SyntaxContexts here. + let ident = ident.unique_if_underscore(); let expansion = ExpnId::root(); // FIXME(jseyfried) intercrate hygiene // Record primary definitions. match res { diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 360343169bc3d..03cf5706dfdb8 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -1340,7 +1340,7 @@ impl<'a, 'b> ImportResolver<'a, 'b> { // hygiene. // FIXME: Implement actual cross-crate hygiene. let is_good_import = binding.is_import() && !binding.is_ambiguity() - && !ident.span.modern().from_expansion(); + && (!ident.span.from_expansion() || ident.name == kw::Underscore); if is_good_import || binding.is_macro_def() { let res = binding.res(); if res != Res::Err { @@ -1350,8 +1350,8 @@ impl<'a, 'b> ImportResolver<'a, 'b> { } } reexports.push(Export { - ident: ident.modern(), - res: res, + ident, + res, span: binding.span, vis: binding.vis, }); @@ -1418,6 +1418,10 @@ impl<'a, 'b> ImportResolver<'a, 'b> { } if reexports.len() > 0 { + // Type pretty printing will prefer reexports that appear earlier + // in this map, so sort them to ensure that diagnostic output is + // stable. + reexports.sort_by_cached_key(|export| export.ident.as_str()); if let Some(def_id) = module.def_id() { self.r.export_map.insert(def_id, reexports); } diff --git a/src/libsyntax_pos/hygiene.rs b/src/libsyntax_pos/hygiene.rs index e28d93267579a..bc7a6643d58ff 100644 --- a/src/libsyntax_pos/hygiene.rs +++ b/src/libsyntax_pos/hygiene.rs @@ -217,8 +217,14 @@ impl HygieneData { fn adjust(&self, ctxt: &mut SyntaxContext, expn_id: ExpnId) -> Option { let mut scope = None; - while !self.is_descendant_of(expn_id, self.outer_expn(*ctxt)) { - scope = Some(self.remove_mark(ctxt).0); + let mut outer = self.outer_expn(*ctxt); + while !self.is_descendant_of(expn_id, outer) { + if let ExpnData { kind: ExpnKind::Underscore, call_site, .. } = *self.expn_data(outer) { + *ctxt = call_site.ctxt(); + } else { + scope = Some(self.remove_mark(ctxt).0); + } + outer = self.outer_expn(*ctxt); } scope } @@ -420,6 +426,21 @@ impl SyntaxContext { HygieneData::with(|data| data.marks(self)) } + crate fn unique(base_span: Span) -> Self { + HygieneData::with(|data| { + // We store the original span as the call-site so that we can + // recover it. We call `modern` here to save us calling it when we + // access `call_site`. + let modern = base_span.with_ctxt(data.modern(base_span.ctxt())); + let expn_id = data.fresh_expn(Some(ExpnData::default( + ExpnKind::Underscore, + modern, + data.expn_data(data.outer_expn(modern.ctxt())).edition, + ))); + data.apply_mark(SyntaxContext::root(), expn_id, Transparency::Opaque) + }) + } + /// Adjust this context for resolution in a scope created by the given expansion. /// For example, consider the following three resolutions of `f`: /// @@ -508,7 +529,7 @@ impl SyntaxContext { pub fn reverse_glob_adjust(&mut self, expn_id: ExpnId, glob_span: Span) -> Option> { HygieneData::with(|data| { - if data.adjust(self, expn_id).is_some() { + if data.adjust(&mut {*self}, expn_id).is_some() { return None; } @@ -674,6 +695,8 @@ pub enum ExpnKind { Macro(MacroKind, Symbol), /// Transform done by the compiler on the AST. AstPass(AstPass), + /// Generated by name resolution to give underscores unique identifiers. + Underscore, /// Desugaring done by the compiler during HIR lowering. Desugaring(DesugaringKind) } @@ -684,6 +707,7 @@ impl ExpnKind { ExpnKind::Root => kw::PathRoot, ExpnKind::Macro(_, descr) => descr, ExpnKind::AstPass(kind) => Symbol::intern(kind.descr()), + ExpnKind::Underscore => kw::Underscore, ExpnKind::Desugaring(kind) => Symbol::intern(kind.descr()), } } diff --git a/src/libsyntax_pos/lib.rs b/src/libsyntax_pos/lib.rs index 674f17de618e9..592cdf3f947bc 100644 --- a/src/libsyntax_pos/lib.rs +++ b/src/libsyntax_pos/lib.rs @@ -443,6 +443,7 @@ impl Span { ExpnKind::Root => break, ExpnKind::Desugaring(..) => ("desugaring of ", ""), ExpnKind::AstPass(..) => ("", ""), + ExpnKind::Underscore => ("", ""), ExpnKind::Macro(macro_kind, _) => match macro_kind { MacroKind::Bang => ("", "!"), MacroKind::Attr => ("#[", "]"), diff --git a/src/libsyntax_pos/symbol.rs b/src/libsyntax_pos/symbol.rs index 44a34070deccd..5c8fb3ecf3336 100644 --- a/src/libsyntax_pos/symbol.rs +++ b/src/libsyntax_pos/symbol.rs @@ -15,7 +15,7 @@ use std::fmt; use std::hash::{Hash, Hasher}; use std::str; -use crate::{Span, DUMMY_SP, GLOBALS}; +use crate::{Span, SyntaxContext, DUMMY_SP, GLOBALS}; #[cfg(test)] mod tests; @@ -814,6 +814,16 @@ impl Ident { } } + /// If the name of this ident is "_", then return a new unique identifier. + /// Otherwise returns `self` unmodified + #[inline] + pub fn unique_if_underscore(mut self) -> Ident { + if self.name == kw::Underscore { + self.span = self.span.with_ctxt(SyntaxContext::unique(self.span)); + } + self + } + /// Convert the name to a `LocalInternedString`. This is a slowish /// operation because it requires locking the symbol interner. pub fn as_str(self) -> LocalInternedString { From 7efb40f60d1f4a9519b1f6d3c8d5b49d37d176f1 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sat, 31 Aug 2019 16:40:20 +0100 Subject: [PATCH 3/4] Remove gensyms --- src/libsyntax_pos/symbol.rs | 90 ++++--------------------------- src/libsyntax_pos/symbol/tests.rs | 7 --- 2 files changed, 9 insertions(+), 88 deletions(-) diff --git a/src/libsyntax_pos/symbol.rs b/src/libsyntax_pos/symbol.rs index 5c8fb3ecf3336..876aa20041094 100644 --- a/src/libsyntax_pos/symbol.rs +++ b/src/libsyntax_pos/symbol.rs @@ -803,17 +803,6 @@ impl Ident { Ident::new(self.name, self.span.modern_and_legacy()) } - /// Transforms an underscore identifier into one with the same name, but - /// gensymed. Leaves non-underscore identifiers unchanged. - pub fn gensym_if_underscore(self) -> Ident { - if self.name == kw::Underscore { - let name = with_interner(|interner| interner.gensymed(self.name)); - Ident::new(name, self.span) - } else { - self - } - } - /// If the name of this ident is "_", then return a new unique identifier. /// Otherwise returns `self` unmodified #[inline] @@ -830,8 +819,7 @@ impl Ident { self.name.as_str() } - /// Convert the name to an `InternedString`. This is a slowish operation - /// because it requires locking the symbol interner. + /// Convert the name to an `InternedString`. pub fn as_interned_str(self) -> InternedString { self.name.as_interned_str() } @@ -886,26 +874,9 @@ impl UseSpecializedDecodable for Ident { } } -/// A symbol is an interned or gensymed string. A gensym is a symbol that is -/// never equal to any other symbol. -/// -/// Conceptually, a gensym can be thought of as a normal symbol with an -/// invisible unique suffix. Gensyms are useful when creating new identifiers -/// that must not match any existing identifiers, e.g. during macro expansion -/// and syntax desugaring. Because gensyms should always be identifiers, all -/// gensym operations are on `Ident` rather than `Symbol`. (Indeed, in the -/// future the gensym-ness may be moved from `Symbol` to hygiene data.) +/// An interned string. /// -/// Examples: -/// ``` -/// assert_eq!(Ident::from_str("_"), Ident::from_str("_")) -/// assert_ne!(Ident::from_str("_").gensym_if_underscore(), Ident::from_str("_")) -/// assert_ne!( -/// Ident::from_str("_").gensym_if_underscore(), -/// Ident::from_str("_").gensym_if_underscore(), -/// ) -/// ``` -/// Internally, a symbol is implemented as an index, and all operations +/// Internally, a `Symbol` is implemented as an index, and all operations /// (including hashing, equality, and ordering) operate on that index. The use /// of `newtype_index!` means that `Option` only takes up 4 bytes, /// because `newtype_index!` reserves the last 256 values for tagging purposes. @@ -956,12 +927,9 @@ impl Symbol { }) } - /// Convert to an `InternedString`. This is a slowish operation because it - /// requires locking the symbol interner. + /// Convert to an `InternedString`. pub fn as_interned_str(self) -> InternedString { - with_interner(|interner| InternedString { - symbol: interner.interned(self) - }) + InternedString { symbol: self } } pub fn as_u32(self) -> u32 { @@ -971,12 +939,7 @@ impl Symbol { impl fmt::Debug for Symbol { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let is_gensymed = with_interner(|interner| interner.is_gensymed(*self)); - if is_gensymed { - write!(f, "{}({:?})", self, self.0) - } else { - write!(f, "{}", self) - } + fmt::Display::fmt(self, f) } } @@ -999,15 +962,11 @@ impl Decodable for Symbol { } // The `&'static str`s in this type actually point into the arena. -// -// Note that normal symbols are indexed upward from 0, and gensyms are indexed -// downward from SymbolIndex::MAX_AS_U32. #[derive(Default)] pub struct Interner { arena: DroplessArena, names: FxHashMap<&'static str, Symbol>, strings: Vec<&'static str>, - gensyms: Vec, } impl Interner { @@ -1040,34 +999,10 @@ impl Interner { self.names.insert(string, name); name } - - fn interned(&self, symbol: Symbol) -> Symbol { - if (symbol.0.as_usize()) < self.strings.len() { - symbol - } else { - self.gensyms[(SymbolIndex::MAX_AS_U32 - symbol.0.as_u32()) as usize] - } - } - - fn gensymed(&mut self, symbol: Symbol) -> Symbol { - self.gensyms.push(symbol); - Symbol::new(SymbolIndex::MAX_AS_U32 - self.gensyms.len() as u32 + 1) - } - - fn is_gensymed(&mut self, symbol: Symbol) -> bool { - symbol.0.as_usize() >= self.strings.len() - } - // Get the symbol as a string. `Symbol::as_str()` should be used in // preference to this function. pub fn get(&self, symbol: Symbol) -> &str { - match self.strings.get(symbol.0.as_usize()) { - Some(string) => string, - None => { - let symbol = self.gensyms[(SymbolIndex::MAX_AS_U32 - symbol.0.as_u32()) as usize]; - self.strings[symbol.0.as_usize()] - } - } + self.strings[symbol.0.as_usize()] } } @@ -1252,19 +1187,12 @@ impl fmt::Display for LocalInternedString { } } -/// An alternative to `Symbol` that is focused on string contents. It has two -/// main differences to `Symbol`. +/// An alternative to `Symbol` that is focused on string contents. /// -/// First, its implementations of `Hash`, `PartialOrd` and `Ord` work with the +/// Its implementations of `Hash`, `PartialOrd` and `Ord` work with the /// string chars rather than the symbol integer. This is useful when hash /// stability is required across compile sessions, or a guaranteed sort /// ordering is required. -/// -/// Second, gensym-ness is irrelevant. E.g.: -/// ``` -/// assert_ne!(Symbol::gensym("x"), Symbol::gensym("x")) -/// assert_eq!(Symbol::gensym("x").as_interned_str(), Symbol::gensym("x").as_interned_str()) -/// ``` #[derive(Clone, Copy, PartialEq, Eq)] pub struct InternedString { symbol: Symbol, diff --git a/src/libsyntax_pos/symbol/tests.rs b/src/libsyntax_pos/symbol/tests.rs index 1b91c9bb845a4..f74b9a0cd1d1d 100644 --- a/src/libsyntax_pos/symbol/tests.rs +++ b/src/libsyntax_pos/symbol/tests.rs @@ -14,13 +14,6 @@ fn interner_tests() { assert_eq!(i.intern("cat"), Symbol::new(1)); // dog is still at zero assert_eq!(i.intern("dog"), Symbol::new(0)); - let z = i.intern("zebra"); - assert_eq!(i.gensymed(z), Symbol::new(SymbolIndex::MAX_AS_U32)); - // gensym of same string gets new number: - assert_eq!(i.gensymed(z), Symbol::new(SymbolIndex::MAX_AS_U32 - 1)); - // gensym of *existing* string gets new number: - let d = i.intern("dog"); - assert_eq!(i.gensymed(d), Symbol::new(SymbolIndex::MAX_AS_U32 - 2)); } #[test] From 0e1c0b5a8d5a3aa975872e6931bce25c1a769a9d Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sat, 31 Aug 2019 16:41:13 +0100 Subject: [PATCH 4/4] Remove some mentions of gensyms --- src/librustc/ty/print/pretty.rs | 2 +- src/libsyntax/ext/proc_macro_server.rs | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustc/ty/print/pretty.rs b/src/librustc/ty/print/pretty.rs index 0adb75626975f..02ff5dd2dfe4d 100644 --- a/src/librustc/ty/print/pretty.rs +++ b/src/librustc/ty/print/pretty.rs @@ -1480,7 +1480,7 @@ impl 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. diff --git a/src/libsyntax/ext/proc_macro_server.rs b/src/libsyntax/ext/proc_macro_server.rs index dfec9ee28809a..0c6918bafb2dd 100644 --- a/src/libsyntax/ext/proc_macro_server.rs +++ b/src/libsyntax/ext/proc_macro_server.rs @@ -332,8 +332,7 @@ impl Ident { if !Self::is_valid(&string) { panic!("`{:?}` is not a valid identifier", string) } - // Get rid of gensyms to conservatively check rawness on the string contents only. - if is_raw && !sym.as_interned_str().as_symbol().can_be_raw() { + if is_raw && !sym.can_be_raw() { panic!("`{}` cannot be a raw identifier", string); } Ident { sym, is_raw, span }