From dd403e4ab0ae25a27c35a803eb229416625b8e55 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 3 Oct 2019 00:31:21 -0400 Subject: [PATCH 1/2] Make re-export collection deterministic Previously, we were using an `FxHashMap` to collect module re-exports. However, re-exports end up getting serialized into crate metadata, which means that metadata generation was non-deterministic. This resulted in spurious error messages changes (e.g. PR #64906) due to pretty-printing implicitly depending on the order of re-exports when computing the proper path to show to the user. See #65042 for a long-term strategy to detect this kind of issue --- src/librustc_resolve/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index e7292b52ab3e8..2995ce2daefa2 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -58,6 +58,7 @@ use std::{cmp, fmt, iter, ptr}; use std::collections::BTreeSet; use rustc_data_structures::ptr_key::PtrKey; use rustc_data_structures::sync::Lrc; +use rustc_data_structures::fx::FxIndexMap; use diagnostics::{Suggestion, ImportSuggestion}; use diagnostics::{find_span_of_binding_until_next_binding, extend_span_to_previous_binding}; @@ -431,7 +432,7 @@ impl ModuleKind { } } -type Resolutions<'a> = RefCell>>>; +type Resolutions<'a> = RefCell>>>; /// One node in the tree of modules. pub struct ModuleData<'a> { From add0a42034b4e3d565ae5934e355bce9b88d15a1 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 5 Oct 2019 16:02:34 -0400 Subject: [PATCH 2/2] Remove `for_each_child_stable` Now that `Resolutions` has a deterministic iteration order, it's no longer necessary to sort its entries before iterating over them --- src/librustc_resolve/diagnostics.rs | 2 +- src/librustc_resolve/late/diagnostics.rs | 4 +-- src/librustc_resolve/lib.rs | 11 -------- ...issue-43871-enum-instead-of-variant.stderr | 26 +++++++++---------- src/test/ui/issues/issue-35675.stderr | 2 +- 5 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/librustc_resolve/diagnostics.rs b/src/librustc_resolve/diagnostics.rs index 7f819486f5bd3..5c85650718c1e 100644 --- a/src/librustc_resolve/diagnostics.rs +++ b/src/librustc_resolve/diagnostics.rs @@ -527,7 +527,7 @@ impl<'a> Resolver<'a> { in_module_is_extern)) = worklist.pop() { // We have to visit module children in deterministic order to avoid // instabilities in reported imports (#43552). - in_module.for_each_child_stable(self, |this, ident, ns, name_binding| { + in_module.for_each_child(self, |this, ident, ns, name_binding| { // avoid imports entirely if name_binding.is_import() && !name_binding.is_extern_crate() { return; } // avoid non-importable candidates as well diff --git a/src/librustc_resolve/late/diagnostics.rs b/src/librustc_resolve/late/diagnostics.rs index d3bf82b66ad1c..1016989ca6e38 100644 --- a/src/librustc_resolve/late/diagnostics.rs +++ b/src/librustc_resolve/late/diagnostics.rs @@ -728,7 +728,7 @@ impl<'a> LateResolutionVisitor<'a, '_> { // abort if the module is already found if result.is_some() { break; } - in_module.for_each_child_stable(self.r, |_, ident, _, name_binding| { + in_module.for_each_child(self.r, |_, ident, _, name_binding| { // abort if the module is already found or if name_binding is private external if result.is_some() || !name_binding.vis.is_visible_locally() { return @@ -760,7 +760,7 @@ impl<'a> LateResolutionVisitor<'a, '_> { fn collect_enum_variants(&mut self, def_id: DefId) -> Option> { self.find_module(def_id).map(|(enum_module, enum_import_suggestion)| { let mut variants = Vec::new(); - enum_module.for_each_child_stable(self.r, |_, ident, _, name_binding| { + enum_module.for_each_child(self.r, |_, ident, _, name_binding| { if let Res::Def(DefKind::Variant, _) = name_binding.res() { let mut segms = enum_import_suggestion.path.segments.clone(); segms.push(ast::PathSegment::from_ident(ident)); diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 2995ce2daefa2..eaa4850f65c0a 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -497,17 +497,6 @@ impl<'a> ModuleData<'a> { } } - fn for_each_child_stable(&'a self, resolver: &mut R, mut f: F) - where R: AsMut>, F: FnMut(&mut R, Ident, Namespace, &'a NameBinding<'a>) - { - let resolutions = resolver.as_mut().resolutions(self).borrow(); - let mut resolutions = resolutions.iter().collect::>(); - resolutions.sort_by_cached_key(|&(&(ident, ns), _)| (ident.as_str(), ns)); - for &(&(ident, ns), &resolution) in resolutions.iter() { - resolution.borrow().binding.map(|binding| f(resolver, ident, ns, binding)); - } - } - fn res(&self) -> Option { match self.kind { ModuleKind::Def(kind, def_id, _) => Some(Res::Def(kind, def_id)), diff --git a/src/test/ui/did_you_mean/issue-43871-enum-instead-of-variant.stderr b/src/test/ui/did_you_mean/issue-43871-enum-instead-of-variant.stderr index d02f30152d687..d8826d4072a9d 100644 --- a/src/test/ui/did_you_mean/issue-43871-enum-instead-of-variant.stderr +++ b/src/test/ui/did_you_mean/issue-43871-enum-instead-of-variant.stderr @@ -5,10 +5,10 @@ LL | let x = Option(1); | ^^^^^^ help: try using one of the enum's variants | -LL | let x = std::prelude::v1::Option::None(1); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -LL | let x = std::prelude::v1::Option::Some(1); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | let x = std::option::Option::None(1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | let x = std::option::Option::Some(1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0532]: expected tuple struct/variant, found enum `Option` --> $DIR/issue-43871-enum-instead-of-variant.rs:21:12 @@ -17,10 +17,10 @@ LL | if let Option(_) = x { | ^^^^^^ help: try using one of the enum's variants | -LL | if let std::prelude::v1::Option::None(_) = x { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -LL | if let std::prelude::v1::Option::Some(_) = x { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | if let std::option::Option::None(_) = x { + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | if let std::option::Option::Some(_) = x { + | ^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0532]: expected tuple struct/variant, found enum `Example` --> $DIR/issue-43871-enum-instead-of-variant.rs:27:12 @@ -47,14 +47,14 @@ LL | let z = ManyVariants(); | ^^^^^^^^^^^^ help: try using one of the enum's variants | -LL | let z = ManyVariants::Eight(); +LL | let z = ManyVariants::One(); + | ^^^^^^^^^^^^^^^^^ +LL | let z = ManyVariants::Two(); + | ^^^^^^^^^^^^^^^^^ +LL | let z = ManyVariants::Three(); | ^^^^^^^^^^^^^^^^^^^ -LL | let z = ManyVariants::Five(); - | ^^^^^^^^^^^^^^^^^^ LL | let z = ManyVariants::Four(); | ^^^^^^^^^^^^^^^^^^ -LL | let z = ManyVariants::Nine(); - | ^^^^^^^^^^^^^^^^^^ and 6 other candidates error: aborting due to 5 previous errors diff --git a/src/test/ui/issues/issue-35675.stderr b/src/test/ui/issues/issue-35675.stderr index 28555a15afae1..856d6506f2a9f 100644 --- a/src/test/ui/issues/issue-35675.stderr +++ b/src/test/ui/issues/issue-35675.stderr @@ -63,7 +63,7 @@ LL | fn qux() -> Some { | ^^^^ | | | not a type - | help: try using the variant's enum: `Option` + | help: try using the variant's enum: `std::option::Option` error: aborting due to 7 previous errors