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

When suggesting to import an item, also suggest changing the path if appropriate #96353

Merged
merged 2 commits into from
May 4, 2022

Conversation

estebank
Copy link
Contributor

When we don't find an item we search all of them for an appropriate
import and suggest useing it. This is sometimes done for expressions
that have paths with more than one segment. We now also suggest changing
that path to work with the use.

Fix #95413

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 23, 2022
@rust-highfive
Copy link
Collaborator

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2022
@est31
Copy link
Member

est31 commented Apr 24, 2022

I think the ... notation is not easy to parse, given all the other punctuation that's present. What about "Consider importing this item / You can then refer to it directly"?

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Not sure if I have a strong opinion on the "... then" notation as opposed to more explicit phrasing, since we have examples of other suggestions split up by ....

Feel free to refine it if you want, or r=me if not.

@est31
Copy link
Member

est31 commented Apr 25, 2022

@compiler-errors your comment has made me look for existing examples in the compiler. There were a bunch.

error: multiple patterns overlap on their endpoints
  --> $DIR/overlapping_range_endpoints.rs:16:22
   |
LL |     m!(0u8, 30..=40, 20..=30);
   |             -------  ^^^^^^^ ... with this range
   |             |
   |             this range overlaps on `30_u8`...
   |
   = note: you likely meant to write mutually exclusive ranges
error[E0746]: return type cannot have an unboxed trait object
  --> $DIR/point-to-type-err-cause-on-impl-trait-return.rs:77:13
   |
LL | fn pug() -> dyn std::fmt::Display {
   |             ^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = note: for information on trait objects, see <https://doc.rust-lang.org/book/ch17-02-trait-objects.html#using-trait-objects-that-allow-for-values-of-different-types>
   = note: if all the returned values were of the same type you could use `impl std::fmt::Display` as the return type
   = note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
   = note: you can create a new `enum` with a variant for each returned type
help: return a boxed trait object instead
   |
LL | fn pug() -> Box<dyn std::fmt::Display> {
   |             ++++                     +
help: ... and box this value
   |
LL |         0 => Box::new(0i32),
   |              +++++++++    +
help: ... and box this value
   |
LL |         1 => Box::new(1u32),
   |              +++++++++    +
help: ... and box this value
   |
LL |         _ => Box::new(2u32),
   |              +++++++++    +
error[E0499]: cannot borrow `*heap` as mutable more than once at a time
  --> $DIR/issue-85581.rs:9:22
   |
LL |     match heap.peek_mut() {
   |           ---------------
   |           |
   |           first mutable borrow occurs here
   |           a temporary with access to the first borrow is created here ...
LL |         Some(_) => { heap.pop(); },
   |                      ^^^^^^^^^^ second mutable borrow occurs here
...
LL | }
   | - ... and the first borrow might be used here, when that temporary is dropped and runs the destructor for type `Option<PeekMut<'_, i32>>`

They read more clearly to me than

help: consider importing this function...
   |
   |
LL | use hi_str;
LL | use hi_str;
   |
   |
help: ...and refer to it directly
   |
LL -     println!("{}", circular_modules_main::hi_str());
LL +     println!("{}", hi_str());
   | 

Maybe it's because of the suggested diff that's being printed, without the ^^^^ underlining?

@compiler-errors
Copy link
Member

yeah, I think the verbose suggestion renders a bit strange, since unlike an addition it doesn't just have ++++ signifying where the edit happens...

@est31
Copy link
Member

est31 commented Apr 25, 2022

(Note that I don't feel strongly about it, this PR is an improvement even in the current state)

@estebank
Copy link
Contributor Author

I'm ok with changing the wording

@bors

This comment was marked as resolved.

…appropriate

When we don't find an item we search all of them for an appropriate
import and suggest `use`ing it. This is sometimes done for expressions
that have paths with more than one segment. We now also suggest changing
that path to work with the `use`.

Fix rust-lang#95413
@estebank
Copy link
Contributor Author

estebank commented May 3, 2022

Rebased and changed the wording to avoid the ...s.

Re the rendered output being weird, is because of the specialized removal "diff" output. It looks clearer with colors (but I'm biased):

Screen Shot 2022-05-02 at 7 07 30 PM

@estebank
Copy link
Contributor Author

estebank commented May 3, 2022

Merging as per

Feel free to refine it if you want, or r=me if not.

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented May 3, 2022

📌 Commit 4934a9e has been approved by compiler-errors

@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 May 3, 2022
@bors
Copy link
Contributor

bors commented May 3, 2022

⌛ Testing commit 4934a9e with merge 8c727a798c153d9239d2378e139f9a5028efdc24...

@bors
Copy link
Contributor

bors commented May 3, 2022

💔 Test failed - checks-actions

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

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@compiler-errors
Copy link
Member

@bors retry transient error on apple runner it seems

@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 May 3, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 3, 2022
…rors

When suggesting to import an item, also suggest changing the path if appropriate

When we don't find an item we search all of them for an appropriate
import and suggest `use`ing it. This is sometimes done for expressions
that have paths with more than one segment. We now also suggest changing
that path to work with the `use`.

Fix rust-lang#95413
@bors
Copy link
Contributor

bors commented May 4, 2022

⌛ Testing commit 4934a9e with merge 9add632...

@bors
Copy link
Contributor

bors commented May 4, 2022

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 9add632 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 4, 2022
@bors bors merged commit 9add632 into rust-lang:master May 4, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 4, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9add632): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: no relevant changes found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 2 0 0 0 2
mean2 0.5% N/A N/A N/A 0.5%
max 0.8% N/A N/A N/A 0.8%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

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.

Bug with associated type discovery
8 participants