Skip to content

Commit

Permalink
Auto merge of #65043 - Aaron1011:fix/reexport-determinism, r=petroche…
Browse files Browse the repository at this point in the history
…nkov

Make re-export collection deterministic

Fixes #65036

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
  • Loading branch information
bors committed Oct 6, 2019
2 parents 5a8fb7c + add0a42 commit 0358617
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 29 deletions.
2 changes: 1 addition & 1 deletion src/librustc_resolve/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_resolve/late/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -760,7 +760,7 @@ impl<'a> LateResolutionVisitor<'a, '_> {
fn collect_enum_variants(&mut self, def_id: DefId) -> Option<Vec<Path>> {
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));
Expand Down
14 changes: 2 additions & 12 deletions src/librustc_resolve/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,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};
Expand Down Expand Up @@ -430,7 +431,7 @@ impl ModuleKind {
}
}

type Resolutions<'a> = RefCell<FxHashMap<(Ident, Namespace), &'a RefCell<NameResolution<'a>>>>;
type Resolutions<'a> = RefCell<FxIndexMap<(Ident, Namespace), &'a RefCell<NameResolution<'a>>>>;

/// One node in the tree of modules.
pub struct ModuleData<'a> {
Expand Down Expand Up @@ -495,17 +496,6 @@ impl<'a> ModuleData<'a> {
}
}

fn for_each_child_stable<R, F>(&'a self, resolver: &mut R, mut f: F)
where R: AsMut<Resolver<'a>>, F: FnMut(&mut R, Ident, Namespace, &'a NameBinding<'a>)
{
let resolutions = resolver.as_mut().resolutions(self).borrow();
let mut resolutions = resolutions.iter().collect::<Vec<_>>();
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<Res> {
match self.kind {
ModuleKind::Def(kind, def_id, _) => Some(Res::Def(kind, def_id)),
Expand Down
26 changes: 13 additions & 13 deletions src/test/ui/did_you_mean/issue-43871-enum-instead-of-variant.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-35675.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 0358617

Please sign in to comment.