Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest importing for partial mod path matching in name resolving #112917

Merged
merged 2 commits into from
Jul 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions compiler/rustc_resolve/src/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3499,7 +3499,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
let report_errors = |this: &mut Self, res: Option<Res>| {
if this.should_report_errs() {
let (err, candidates) =
this.smart_resolve_report_errors(path, path_span, source, res);
this.smart_resolve_report_errors(path, path, path_span, source, res);

let def_id = this.parent_scope.module.nearest_parent_mod();
let instead = res.is_some();
Expand Down Expand Up @@ -3556,8 +3556,13 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
_ => return Some(parent_err),
};

let (mut err, candidates) =
this.smart_resolve_report_errors(prefix_path, path_span, PathSource::Type, None);
let (mut err, candidates) = this.smart_resolve_report_errors(
prefix_path,
path,
path_span,
PathSource::Type,
None,
);

// There are two different error messages user might receive at
// this point:
Expand Down
58 changes: 56 additions & 2 deletions compiler/rustc_resolve/src/late/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::late::{LifetimeBinderKind, LifetimeRes, LifetimeRibKind, LifetimeUseS
use crate::{errors, path_names_to_string};
use crate::{Module, ModuleKind, ModuleOrUniformRoot};
use crate::{PathResult, PathSource, Segment};
use rustc_hir::def::Namespace::{self, *};

use rustc_ast::visit::{FnCtxt, FnKind, LifetimeCtxt};
use rustc_ast::{
Expand All @@ -17,7 +18,6 @@ use rustc_errors::{
MultiSpan,
};
use rustc_hir as hir;
use rustc_hir::def::Namespace::{self, *};
use rustc_hir::def::{self, CtorKind, CtorOf, DefKind};
use rustc_hir::def_id::{DefId, CRATE_DEF_ID};
use rustc_hir::PrimTy;
Expand Down Expand Up @@ -221,10 +221,14 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
let suggestion = if self.current_trait_ref.is_none()
&& let Some((fn_kind, _)) = self.diagnostic_metadata.current_function
&& let Some(FnCtxt::Assoc(_)) = fn_kind.ctxt()
&& let FnKind::Fn(_, _, sig, ..) = fn_kind
&& let Some(items) = self.diagnostic_metadata.current_impl_items
&& let Some(item) = items.iter().find(|i| {
if let AssocItemKind::Fn(..) | AssocItemKind::Const(..) = &i.kind
&& i.ident.name == item_str.name
// don't suggest if the item is in Fn signature arguments
// issue #112590
&& !sig.span.contains(item_span)
{
debug!(?item_str.name);
return true
Expand Down Expand Up @@ -318,11 +322,56 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
}
}

/// Try to suggest for a module path that cannot be resolved.
/// Such as `fmt::Debug` where `fmt` is not resolved without importing,
/// here we search with `lookup_import_candidates` for a module named `fmt`
/// with `TypeNS` as namespace.
///
/// We need a separate function here because we won't suggest for a path with single segment
/// and we won't change `SourcePath` api `is_expected` to match `Type` with `DefKind::Mod`
pub(crate) fn smart_resolve_partial_mod_path_errors(
&mut self,
prefix_path: &[Segment],
path: &[Segment],
) -> Vec<ImportSuggestion> {
let next_seg = if path.len() >= prefix_path.len() + 1 && prefix_path.len() == 1 {
path.get(prefix_path.len())
} else {
None
};
if let Some(segment) = prefix_path.last() &&
let Some(next_seg) = next_seg {
let candidates = self.r.lookup_import_candidates(
segment.ident,
Namespace::TypeNS,
&self.parent_scope,
&|res: Res| matches!(res, Res::Def(DefKind::Mod, _)),
);
// double check next seg is valid
candidates
.into_iter()
.filter(|candidate| {
if let Some(def_id) = candidate.did &&
let Some(module) = self.r.get_module(def_id) {
self.r.resolutions(module).borrow().iter().any(|(key, _r)| {
key.ident.name == next_seg.ident.name
})
} else {
false
}
})
.collect::<Vec<_>>()
} else {
Vec::new()
}
}

/// Handles error reporting for `smart_resolve_path_fragment` function.
/// Creates base error and amends it with one short label and possibly some longer helps/notes.
pub(crate) fn smart_resolve_report_errors(
&mut self,
path: &[Segment],
full_path: &[Segment],
span: Span,
source: PathSource<'_>,
res: Option<Res>,
Expand Down Expand Up @@ -364,7 +413,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
}

let (found, candidates) =
self.try_lookup_name_relaxed(&mut err, source, path, span, res, &base_error);
self.try_lookup_name_relaxed(&mut err, source, path, full_path, span, res, &base_error);
if found {
return (err, candidates);
}
Expand Down Expand Up @@ -470,6 +519,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
err: &mut Diagnostic,
source: PathSource<'_>,
path: &[Segment],
full_path: &[Segment],
span: Span,
res: Option<Res>,
base_error: &BaseError,
Expand Down Expand Up @@ -639,6 +689,10 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
}
}

if candidates.is_empty() {
candidates = self.smart_resolve_partial_mod_path_errors(path, full_path);
}

return (false, candidates);
}

Expand Down
32 changes: 24 additions & 8 deletions tests/ui/macros/builtin-prelude-no-accidents.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,37 @@ error[E0433]: failed to resolve: use of undeclared crate or module `env`
|
LL | env::current_dir;
| ^^^ use of undeclared crate or module `env`

error[E0433]: failed to resolve: use of undeclared crate or module `vec`
--> $DIR/builtin-prelude-no-accidents.rs:7:14
|
LL | type B = vec::Vec<u8>;
| ^^^
| |
| use of undeclared crate or module `vec`
| help: a struct with a similar name exists (notice the capitalization): `Vec`
help: consider importing this module
|
LL + use std::env;
|

error[E0433]: failed to resolve: use of undeclared crate or module `panic`
--> $DIR/builtin-prelude-no-accidents.rs:6:14
|
LL | type A = panic::PanicInfo;
| ^^^^^ use of undeclared crate or module `panic`
|
help: consider importing this module
|
LL + use std::panic;
|

error[E0433]: failed to resolve: use of undeclared crate or module `vec`
--> $DIR/builtin-prelude-no-accidents.rs:7:14
|
LL | type B = vec::Vec<u8>;
| ^^^ use of undeclared crate or module `vec`
|
help: a struct with a similar name exists
|
LL | type B = Vec::Vec<u8>;
| ~~~
Comment on lines +29 to +32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion isn't great, since it doesn't take into account the fact that the path may have additional segments that don't make sense after the suggestion is applied.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is not introduced by this PR, but from here:

"{} {} with a similar name exists",

The output is:

RUST_BACKTRACE=1 rustc +dev2 --edition 2021 tests/ui/macros/builtin-prelude-no-accidents.rs
error[E0433]: failed to resolve: use of undeclared crate or module `vec`
 --> tests/ui/macros/builtin-prelude-no-accidents.rs:7:14
  |
7 |     type B = vec::Vec<u8>; //~ ERROR use of undeclared crate or module `vec`
  |              ^^^
  |              |
  |              use of undeclared crate or module `vec`
  |              help: a struct with a similar name exists (notice the capitalization): `Vec`

error: aborting due to previous error

I guess the diagnostic emitter did some formatting stuff, if there are multiple diagnostics in the same place, it will display in list items?

We might fix this suggestion in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is pre-existing and can be done separately. It can be dealt by making smart_resolve_report_errors pass in the whole path (and a len: usize arg to signal the path segment that failed) and doing multiple try_lookup_name_relaxed and try to select the right candidates. I would assume that the last path segment is always the most critical to bias towards when searching for the right candidate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the only thing left would be to check in the above suggestion if the suggested type has an associated item corresponding to the next path segment (in order to avoid suggesting things like Vec::Vec when we know that Vec doesn't have anything named Vec within it.

I think we can do that as a separate PR.

help: consider importing this module
|
LL + use std::vec;
|

error: aborting due to 3 previous errors

Expand Down
13 changes: 13 additions & 0 deletions tests/ui/resolve/export-fully-qualified-2018.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// edition:2018

// In this test baz isn't resolved when called as foo.baz even though
// it's called from inside foo. This is somewhat surprising and may
// want to change eventually.

mod foo {
pub fn bar() { foo::baz(); } //~ ERROR failed to resolve: use of undeclared crate or module `foo`

fn baz() { }
}

fn main() { }
14 changes: 14 additions & 0 deletions tests/ui/resolve/export-fully-qualified-2018.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error[E0433]: failed to resolve: use of undeclared crate or module `foo`
--> $DIR/export-fully-qualified-2018.rs:8:20
|
LL | pub fn bar() { foo::baz(); }
| ^^^ use of undeclared crate or module `foo`
|
help: consider importing this module
|
LL + use crate::foo;
|

error: aborting due to previous error

For more information about this error, try `rustc --explain E0433`.
2 changes: 2 additions & 0 deletions tests/ui/resolve/export-fully-qualified.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// edition:2015

// In this test baz isn't resolved when called as foo.baz even though
// it's called from inside foo. This is somewhat surprising and may
// want to change eventually.
Expand Down
7 changes: 6 additions & 1 deletion tests/ui/resolve/export-fully-qualified.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
error[E0433]: failed to resolve: use of undeclared crate or module `foo`
--> $DIR/export-fully-qualified.rs:6:20
--> $DIR/export-fully-qualified.rs:8:20
|
LL | pub fn bar() { foo::baz(); }
| ^^^ use of undeclared crate or module `foo`
|
help: consider importing this module
|
LL + use foo;
Comment on lines +7 to +9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't that useful either 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference between 2015 edition and later edition,

With this PR, the output for 2021 edition is:

rustc +dev2 --edition 2021 tests/ui/resolve/export-fully-qualified.rs
error[E0432]: unresolved import `foo`
 --> tests/ui/resolve/export-fully-qualified.rs:7:13
  |
7 |         use foo;
  |             ^^^ no external crate `foo`
  |
help: consider importing this module instead
  |
7 |         use crate::foo;
  |  

the testing framework is running without 2021, so it's suggesting to write this:

mod foo {
    pub fn bar() {
        use foo;
        foo::baz();
    } //~ ERROR failed to resolve: use of undeclared crate or module `foo`

    fn baz() { }
}

This is valid code for 2015 edition. If we compile with a new edition we will suggest using crate:

~/rust ❯❯❯ rustc +dev2 --edition 2018 tests/ui/resolve/export-fully-qualified.rs
error[E0432]: unresolved import `foo`
 --> tests/ui/resolve/export-fully-qualified.rs:7:13
  |
7 |         use foo;
  |             ^^^ no external crate `foo`
  |
help: consider importing this module instead
  |
7 |         use crate::foo;
  |             ~~~~~~~~~~

error: aborting due to previous error

Maybe it's related to this issue #112924

Anyway, this suggestion seems right for different versions.

Or it's better to suggest writing like this?

crate::foo::baz();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have two tests, one for 2015 and one for 2018, so that we can ensure that the suggestions keep being reasonable for both :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, added another test tests/ui/resolve/export-fully-qualified-2018.rs.

|

error: aborting due to previous error

Expand Down
5 changes: 5 additions & 0 deletions tests/ui/suggestions/crate-or-module-typo.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ error[E0433]: failed to resolve: use of undeclared crate or module `bar`
|
LL | pub fn bar() { bar::baz(); }
| ^^^ use of undeclared crate or module `bar`
|
help: consider importing this module
|
LL + use crate::bar;
|

error: aborting due to 4 previous errors

Expand Down
10 changes: 10 additions & 0 deletions tests/ui/suggestions/issue-112590-suggest-import.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
pub struct S;

impl fmt::Debug for S { //~ ERROR failed to resolve: use of undeclared crate or module `fmt`
fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result { //~ ERROR failed to resolve: use of undeclared crate or module `fmt`
//~^ ERROR failed to resolve: use of undeclared crate or module `fmt`
Ok(())
}
}

fn main() { }
36 changes: 36 additions & 0 deletions tests/ui/suggestions/issue-112590-suggest-import.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
error[E0433]: failed to resolve: use of undeclared crate or module `fmt`
--> $DIR/issue-112590-suggest-import.rs:3:6
|
LL | impl fmt::Debug for S {
| ^^^ use of undeclared crate or module `fmt`
|
help: consider importing this module
|
LL + use std::fmt;
|

error[E0433]: failed to resolve: use of undeclared crate or module `fmt`
--> $DIR/issue-112590-suggest-import.rs:4:28
|
LL | fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result {
| ^^^ use of undeclared crate or module `fmt`
|
help: consider importing this module
|
LL + use std::fmt;
|

error[E0433]: failed to resolve: use of undeclared crate or module `fmt`
--> $DIR/issue-112590-suggest-import.rs:4:51
|
LL | fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result {
| ^^^ use of undeclared crate or module `fmt`
|
help: consider importing this module
|
LL + use std::fmt;
|

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0433`.