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

Conversation

chenyukang
Copy link
Member

Fixes #112590

@rustbot
Copy link
Collaborator

rustbot commented Jun 22, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 22, 2023
Comment on lines +29 to +32
help: a struct with a similar name exists
|
LL | type B = Vec::Vec<u8>;
| ~~~
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.

Comment on lines +7 to +9
help: consider importing this module
|
LL + use foo;
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.

@chenyukang chenyukang force-pushed the yukang-fix-112590 branch 2 times, most recently from a035b2d to d6293a9 Compare June 23, 2023 23:22
@chenyukang chenyukang force-pushed the yukang-fix-112590 branch 3 times, most recently from 9fd7ef8 to da3913a Compare July 2, 2023 03:06
@estebank
Copy link
Contributor

estebank commented Jul 3, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jul 3, 2023

📌 Commit b26701e has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2023
@bors
Copy link
Contributor

bors commented Jul 4, 2023

⌛ Testing commit b26701e with merge e728b5b...

@bors
Copy link
Contributor

bors commented Jul 4, 2023

☀️ Test successful - checks-actions
Approved by: estebank
Pushing e728b5b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 4, 2023
@bors bors merged commit e728b5b into rust-lang:master Jul 4, 2023
12 checks passed
@rustbot rustbot added this to the 1.72.0 milestone Jul 4, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e728b5b): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.6% [1.6%, 1.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.062s -> 670.726s (0.25%)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 10, 2023
…-positive, r=estebank

Add filter with following segment while lookup typo for path

From the discussion: rust-lang#112917 (comment)

Seems we can not get the assoc items for `Struct`, `Enum` in the resolving phase.
A obvious filter is avoid suggesting the same name with the following segment path.

Use `following_seg` can extend the function `smart_resolve_partial_mod_path_errors` for more scenarios, such as `std::sync_error::atomic::AtomicBool` in test case.

r? `@estebank`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undeclared crate or module error has invalid suggestions when implementing fmt::Debug
7 participants