From 29e2376ac7f8719930a14f256a84fbcd65e10163 Mon Sep 17 00:00:00 2001 From: David Wood Date: Thu, 20 Sep 2018 17:15:52 +0200 Subject: [PATCH 1/4] Add suggestions for unresolved imports. This commit adds suggestions for unresolved imports in the cases where there could be a missing `crate::`, `super::`, `self::` or a missing external crate name before an import. --- src/librustc_resolve/error_reporting.rs | 162 ++++++++++++++++++ src/librustc_resolve/lib.rs | 2 +- src/librustc_resolve/resolve_imports.rs | 18 +- src/test/ui/resolve_self_super_hint.rs | 6 +- src/test/ui/resolve_self_super_hint.stderr | 6 +- src/test/ui/rust-2018/auxiliary/baz.rs | 15 ++ src/test/ui/rust-2018/issue-54006.stderr | 2 +- .../ui/rust-2018/local-path-suggestions.rs | 31 ++++ .../rust-2018/local-path-suggestions.stderr | 21 +++ 9 files changed, 244 insertions(+), 19 deletions(-) create mode 100644 src/librustc_resolve/error_reporting.rs create mode 100644 src/test/ui/rust-2018/auxiliary/baz.rs create mode 100644 src/test/ui/rust-2018/local-path-suggestions.rs create mode 100644 src/test/ui/rust-2018/local-path-suggestions.stderr diff --git a/src/librustc_resolve/error_reporting.rs b/src/librustc_resolve/error_reporting.rs new file mode 100644 index 0000000000000..3ca0a11c1bb56 --- /dev/null +++ b/src/librustc_resolve/error_reporting.rs @@ -0,0 +1,162 @@ +// Copyright 2012-2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use {CrateLint, PathResult}; + +use syntax::ast::Ident; +use syntax::symbol::keywords; +use syntax_pos::Span; + +use resolve_imports::ImportResolver; + +impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> { + /// Add suggestions for a path that cannot be resolved. + pub(crate) fn make_path_suggestion( + &mut self, + span: Span, + path: Vec + ) -> Option> { + debug!("make_path_suggestion: span={:?} path={:?}", span, path); + // If we don't have a path to suggest changes to, then return. + if path.is_empty() { + return None; + } + + // Check whether a ident is a path segment that is not root. + let is_special = |ident: Ident| ident.is_path_segment_keyword() && + ident.name != keywords::CrateRoot.name(); + + match (path.get(0), path.get(1)) { + // Make suggestions that require at least two non-special path segments. + (Some(fst), Some(snd)) if !is_special(*fst) && !is_special(*snd) => { + debug!("make_path_suggestion: fst={:?} snd={:?}", fst, snd); + + self.make_missing_self_suggestion(span, path.clone()) + .or_else(|| self.make_missing_crate_suggestion(span, path.clone())) + .or_else(|| self.make_missing_super_suggestion(span, path.clone())) + .or_else(|| self.make_external_crate_suggestion(span, path.clone())) + }, + _ => None, + } + } + + /// Suggest a missing `self::` if that resolves to an correct module. + /// + /// ``` + /// | + /// LL | use foo::Bar; + /// | ^^^ Did you mean `self::foo`? + /// ``` + fn make_missing_self_suggestion( + &mut self, + span: Span, + mut path: Vec + ) -> Option> { + // Replace first ident with `self` and check if that is valid. + path[0].name = keywords::SelfValue.name(); + let result = self.resolve_path(None, &path, None, false, span, CrateLint::No); + debug!("make_missing_self_suggestion: path={:?} result={:?}", path, result); + if let PathResult::Module(..) = result { + Some(path) + } else { + None + } + } + + /// Suggest a missing `crate::` if that resolves to an correct module. + /// + /// ``` + /// | + /// LL | use foo::Bar; + /// | ^^^ Did you mean `crate::foo`? + /// ``` + fn make_missing_crate_suggestion( + &mut self, + span: Span, + mut path: Vec + ) -> Option> { + // Replace first ident with `crate` and check if that is valid. + path[0].name = keywords::Crate.name(); + let result = self.resolve_path(None, &path, None, false, span, CrateLint::No); + debug!("make_missing_crate_suggestion: path={:?} result={:?}", path, result); + if let PathResult::Module(..) = result { + Some(path) + } else { + None + } + } + + /// Suggest a missing `super::` if that resolves to an correct module. + /// + /// ``` + /// | + /// LL | use foo::Bar; + /// | ^^^ Did you mean `super::foo`? + /// ``` + fn make_missing_super_suggestion( + &mut self, + span: Span, + mut path: Vec + ) -> Option> { + // Replace first ident with `crate` and check if that is valid. + path[0].name = keywords::Super.name(); + let result = self.resolve_path(None, &path, None, false, span, CrateLint::No); + debug!("make_missing_super_suggestion: path={:?} result={:?}", path, result); + if let PathResult::Module(..) = result { + Some(path) + } else { + None + } + } + + /// Suggest a missing external crate name if that resolves to an correct module. + /// + /// ``` + /// | + /// LL | use foobar::Baz; + /// | ^^^ Did you mean `baz::foobar`? + /// ``` + /// + /// Used when importing a submodule of an external crate but missing that crate's + /// name as the first part of path. + fn make_external_crate_suggestion( + &mut self, + span: Span, + mut path: Vec + ) -> Option> { + // Need to clone else we can't call `resolve_path` without a borrow error. + let external_crate_names = self.resolver.session.extern_prelude.clone(); + + // Insert a new path segment that we can replace. + let new_path_segment = path[0].clone(); + path.insert(1, new_path_segment); + + for name in &external_crate_names { + // Don't suggest meta as it will error in `resolve_path`. + if name.as_str() == "meta" { + continue; + } + + // Replace the first after root (a placeholder we inserted) with a crate name + // and check if that is valid. + path[1].name = *name; + let result = self.resolve_path(None, &path, None, false, span, CrateLint::No); + debug!("make_external_crate_suggestion: name={:?} path={:?} result={:?}", + name, path, result); + if let PathResult::Module(..) = result { + return Some(path) + } + } + + // Remove our placeholder segment. + path.remove(1); + None + } +} diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index b4e7f3a8b746c..35241e2b1556a 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -85,7 +85,7 @@ use macros::{InvocationData, LegacyBinding, ParentScope}; // NB: This module needs to be declared first so diagnostics are // registered before they are used. mod diagnostics; - +mod error_reporting; mod macros; mod check_unused; mod build_reduced_graph; diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index e689e6d70fdf4..bc93b43c6ae79 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -957,17 +957,13 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> { return None; } PathResult::Failed(span, msg, true) => { - let (mut self_path, mut self_result) = (module_path.clone(), None); - let is_special = |ident: Ident| ident.is_path_segment_keyword() && - ident.name != keywords::CrateRoot.name(); - if !self_path.is_empty() && !is_special(self_path[0]) && - !(self_path.len() > 1 && is_special(self_path[1])) { - self_path[0].name = keywords::SelfValue.name(); - self_result = Some(self.resolve_path(None, &self_path, None, false, - span, CrateLint::No)); - } - return if let Some(PathResult::Module(..)) = self_result { - Some((span, format!("Did you mean `{}`?", names_to_string(&self_path[..])))) + return if let Some(suggested_path) = self.make_path_suggestion( + span, module_path.clone() + ) { + Some(( + span, + format!("Did you mean `{}`?", names_to_string(&suggested_path[..])) + )) } else { Some((span, msg)) }; diff --git a/src/test/ui/resolve_self_super_hint.rs b/src/test/ui/resolve_self_super_hint.rs index c5dd367c0ab88..a30e73cf02d12 100644 --- a/src/test/ui/resolve_self_super_hint.rs +++ b/src/test/ui/resolve_self_super_hint.rs @@ -19,15 +19,15 @@ mod a { mod b { use alloc::HashMap; //~^ ERROR unresolved import `alloc` [E0432] - //~| Did you mean `a::alloc`? + //~| Did you mean `super::alloc`? mod c { use alloc::HashMap; //~^ ERROR unresolved import `alloc` [E0432] - //~| Did you mean `a::alloc`? + //~| Did you mean `std::alloc`? mod d { use alloc::HashMap; //~^ ERROR unresolved import `alloc` [E0432] - //~| Did you mean `a::alloc`? + //~| Did you mean `std::alloc`? } } } diff --git a/src/test/ui/resolve_self_super_hint.stderr b/src/test/ui/resolve_self_super_hint.stderr index 40b2a4bf96842..b58a23724e413 100644 --- a/src/test/ui/resolve_self_super_hint.stderr +++ b/src/test/ui/resolve_self_super_hint.stderr @@ -8,19 +8,19 @@ error[E0432]: unresolved import `alloc` --> $DIR/resolve_self_super_hint.rs:20:13 | LL | use alloc::HashMap; - | ^^^^^ Did you mean `a::alloc`? + | ^^^^^ Did you mean `super::alloc`? error[E0432]: unresolved import `alloc` --> $DIR/resolve_self_super_hint.rs:24:17 | LL | use alloc::HashMap; - | ^^^^^ Did you mean `a::alloc`? + | ^^^^^ Did you mean `std::alloc`? error[E0432]: unresolved import `alloc` --> $DIR/resolve_self_super_hint.rs:28:21 | LL | use alloc::HashMap; - | ^^^^^ Did you mean `a::alloc`? + | ^^^^^ Did you mean `std::alloc`? error: aborting due to 4 previous errors diff --git a/src/test/ui/rust-2018/auxiliary/baz.rs b/src/test/ui/rust-2018/auxiliary/baz.rs new file mode 100644 index 0000000000000..4ee9c051c6568 --- /dev/null +++ b/src/test/ui/rust-2018/auxiliary/baz.rs @@ -0,0 +1,15 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// This file is used as part of the local-path-suggestions.rs test. + +pub mod foobar { + pub struct Baz; +} diff --git a/src/test/ui/rust-2018/issue-54006.stderr b/src/test/ui/rust-2018/issue-54006.stderr index 1183dc9794a22..37bf19e61f8d8 100644 --- a/src/test/ui/rust-2018/issue-54006.stderr +++ b/src/test/ui/rust-2018/issue-54006.stderr @@ -2,7 +2,7 @@ error[E0432]: unresolved import `alloc` --> $DIR/issue-54006.rs:16:5 | LL | use alloc::vec; - | ^^^^^ Could not find `alloc` in `{{root}}` + | ^^^^^ Did you mean `std::alloc`? error: cannot determine resolution for the macro `vec` --> $DIR/issue-54006.rs:20:18 diff --git a/src/test/ui/rust-2018/local-path-suggestions.rs b/src/test/ui/rust-2018/local-path-suggestions.rs new file mode 100644 index 0000000000000..840e6103ec67d --- /dev/null +++ b/src/test/ui/rust-2018/local-path-suggestions.rs @@ -0,0 +1,31 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// aux-build:baz.rs +// compile-flags:--extern baz +// edition:2018 + +mod foo { + type Bar = u32; +} + +mod baz { + use foo::Bar; + + fn baz() { + let x: Bar = 22; + } +} + +use foo::Bar; + +use foobar::Baz; + +fn main() { } diff --git a/src/test/ui/rust-2018/local-path-suggestions.stderr b/src/test/ui/rust-2018/local-path-suggestions.stderr new file mode 100644 index 0000000000000..38a5d412183cc --- /dev/null +++ b/src/test/ui/rust-2018/local-path-suggestions.stderr @@ -0,0 +1,21 @@ +error[E0432]: unresolved import `foo` + --> $DIR/local-path-suggestions.rs:20:9 + | +LL | use foo::Bar; + | ^^^ Did you mean `crate::foo`? + +error[E0432]: unresolved import `foo` + --> $DIR/local-path-suggestions.rs:27:5 + | +LL | use foo::Bar; + | ^^^ Did you mean `self::foo`? + +error[E0432]: unresolved import `foobar` + --> $DIR/local-path-suggestions.rs:29:5 + | +LL | use foobar::Baz; + | ^^^^^^ Did you mean `baz::foobar`? + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0432`. From c832579fb7cd809d506a7d6e63bf50b36435563d Mon Sep 17 00:00:00 2001 From: David Wood Date: Fri, 28 Sep 2018 11:30:55 +0200 Subject: [PATCH 2/4] Removed hardcoded crate. Previously, `meta` crate was hardcoded as attempting to resolve a path with it would ICE. Now, we attempt to load each extern crate first so that resolving a path involving that crate doesn't error. --- src/librustc_resolve/error_reporting.rs | 28 ++++++++++++------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/librustc_resolve/error_reporting.rs b/src/librustc_resolve/error_reporting.rs index 3ca0a11c1bb56..7a66750d700ba 100644 --- a/src/librustc_resolve/error_reporting.rs +++ b/src/librustc_resolve/error_reporting.rs @@ -121,7 +121,7 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> { /// ``` /// | /// LL | use foobar::Baz; - /// | ^^^ Did you mean `baz::foobar`? + /// | ^^^^^^ Did you mean `baz::foobar`? /// ``` /// /// Used when importing a submodule of an external crate but missing that crate's @@ -139,19 +139,19 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> { path.insert(1, new_path_segment); for name in &external_crate_names { - // Don't suggest meta as it will error in `resolve_path`. - if name.as_str() == "meta" { - continue; - } - - // Replace the first after root (a placeholder we inserted) with a crate name - // and check if that is valid. - path[1].name = *name; - let result = self.resolve_path(None, &path, None, false, span, CrateLint::No); - debug!("make_external_crate_suggestion: name={:?} path={:?} result={:?}", - name, path, result); - if let PathResult::Module(..) = result { - return Some(path) + let ident = Ident::with_empty_ctxt(*name); + // Calling `maybe_process_path_extern` ensures that we're only running `resolve_path` + // on a crate name that won't ICE. + if let Some(_) = self.crate_loader.maybe_process_path_extern(*name, ident.span) { + // Replace the first after root (a placeholder we inserted) with a crate name + // and check if that is valid. + path[1].name = *name; + let result = self.resolve_path(None, &path, None, false, span, CrateLint::No); + debug!("make_external_crate_suggestion: name={:?} path={:?} result={:?}", + name, path, result); + if let PathResult::Module(..) = result { + return Some(path) + } } } From 9d408e0511aeb02ae46c692b2432886372f71c37 Mon Sep 17 00:00:00 2001 From: David Wood Date: Fri, 28 Sep 2018 11:32:04 +0200 Subject: [PATCH 3/4] Update tests to demonstrate 2015 behaviour. Adds a test to demonstrate behaviour of suggestions in the 2015 edition. --- .../rust-2018/local-path-suggestions-2015.rs | 36 +++++++++++++++++++ .../local-path-suggestions-2015.stderr | 9 +++++ ...ions.rs => local-path-suggestions-2018.rs} | 2 +- ...err => local-path-suggestions-2018.stderr} | 6 ++-- 4 files changed, 49 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/rust-2018/local-path-suggestions-2015.rs create mode 100644 src/test/ui/rust-2018/local-path-suggestions-2015.stderr rename src/test/ui/rust-2018/{local-path-suggestions.rs => local-path-suggestions-2018.rs} (96%) rename src/test/ui/rust-2018/{local-path-suggestions.stderr => local-path-suggestions-2018.stderr} (75%) diff --git a/src/test/ui/rust-2018/local-path-suggestions-2015.rs b/src/test/ui/rust-2018/local-path-suggestions-2015.rs new file mode 100644 index 0000000000000..c691d2948229c --- /dev/null +++ b/src/test/ui/rust-2018/local-path-suggestions-2015.rs @@ -0,0 +1,36 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// aux-build:baz.rs +// compile-flags:--extern baz +// edition:2015 + +// This test exists to demonstrate the behaviour of the import suggestions +// from the `local-path-suggestions-2018.rs` test when not using the 2018 edition. + +extern crate baz as aux_baz; + +mod foo { + pub type Bar = u32; +} + +mod baz { + use foo::Bar; + + fn baz() { + let x: Bar = 22; + } +} + +use foo::Bar; + +use foobar::Baz; + +fn main() { } diff --git a/src/test/ui/rust-2018/local-path-suggestions-2015.stderr b/src/test/ui/rust-2018/local-path-suggestions-2015.stderr new file mode 100644 index 0000000000000..d7580507ce425 --- /dev/null +++ b/src/test/ui/rust-2018/local-path-suggestions-2015.stderr @@ -0,0 +1,9 @@ +error[E0432]: unresolved import `foobar` + --> $DIR/local-path-suggestions-2015.rs:34:5 + | +LL | use foobar::Baz; + | ^^^^^^ Did you mean `aux_baz::foobar`? + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0432`. diff --git a/src/test/ui/rust-2018/local-path-suggestions.rs b/src/test/ui/rust-2018/local-path-suggestions-2018.rs similarity index 96% rename from src/test/ui/rust-2018/local-path-suggestions.rs rename to src/test/ui/rust-2018/local-path-suggestions-2018.rs index 840e6103ec67d..147dae401f6ac 100644 --- a/src/test/ui/rust-2018/local-path-suggestions.rs +++ b/src/test/ui/rust-2018/local-path-suggestions-2018.rs @@ -13,7 +13,7 @@ // edition:2018 mod foo { - type Bar = u32; + pub type Bar = u32; } mod baz { diff --git a/src/test/ui/rust-2018/local-path-suggestions.stderr b/src/test/ui/rust-2018/local-path-suggestions-2018.stderr similarity index 75% rename from src/test/ui/rust-2018/local-path-suggestions.stderr rename to src/test/ui/rust-2018/local-path-suggestions-2018.stderr index 38a5d412183cc..97bf748881f29 100644 --- a/src/test/ui/rust-2018/local-path-suggestions.stderr +++ b/src/test/ui/rust-2018/local-path-suggestions-2018.stderr @@ -1,17 +1,17 @@ error[E0432]: unresolved import `foo` - --> $DIR/local-path-suggestions.rs:20:9 + --> $DIR/local-path-suggestions-2018.rs:20:9 | LL | use foo::Bar; | ^^^ Did you mean `crate::foo`? error[E0432]: unresolved import `foo` - --> $DIR/local-path-suggestions.rs:27:5 + --> $DIR/local-path-suggestions-2018.rs:27:5 | LL | use foo::Bar; | ^^^ Did you mean `self::foo`? error[E0432]: unresolved import `foobar` - --> $DIR/local-path-suggestions.rs:29:5 + --> $DIR/local-path-suggestions-2018.rs:29:5 | LL | use foobar::Baz; | ^^^^^^ Did you mean `baz::foobar`? From 5872d3eacd61113c8c241444b2d2403aaec2fbfd Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 3 Oct 2018 15:20:20 +0200 Subject: [PATCH 4/4] Deterministic external crate suggestion. This commit ensures that the external crate suggestion is deterministic by using a `BTreeMap` rather than a `FxHashMap`. This is particularly useful as `std` and `core` will often contain the same items and therefore the suggestion would previously suggest either for any given error - in this case, the suggestion will always prefer `std` now. --- src/librustc_resolve/error_reporting.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/librustc_resolve/error_reporting.rs b/src/librustc_resolve/error_reporting.rs index 7a66750d700ba..b9194fdfc15d7 100644 --- a/src/librustc_resolve/error_reporting.rs +++ b/src/librustc_resolve/error_reporting.rs @@ -10,8 +10,10 @@ use {CrateLint, PathResult}; +use std::collections::BTreeSet; + use syntax::ast::Ident; -use syntax::symbol::keywords; +use syntax::symbol::{keywords, Symbol}; use syntax_pos::Span; use resolve_imports::ImportResolver; @@ -131,14 +133,19 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> { span: Span, mut path: Vec ) -> Option> { - // Need to clone else we can't call `resolve_path` without a borrow error. - let external_crate_names = self.resolver.session.extern_prelude.clone(); + // Need to clone else we can't call `resolve_path` without a borrow error. We also store + // into a `BTreeMap` so we can get consistent ordering (and therefore the same diagnostic) + // each time. + let external_crate_names: BTreeSet = self.resolver.session.extern_prelude + .clone().drain().collect(); // Insert a new path segment that we can replace. let new_path_segment = path[0].clone(); path.insert(1, new_path_segment); - for name in &external_crate_names { + // Iterate in reverse so that we start with crates at the end of the alphabet. This means + // that we'll always get `std` before `core`. + for name in external_crate_names.iter().rev() { let ident = Ident::with_empty_ctxt(*name); // Calling `maybe_process_path_extern` ensures that we're only running `resolve_path` // on a crate name that won't ICE.