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.

Sign up for GitHub

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

Fix suggestion for removing &mut from &mut macro!(). #85939

Merged
merged 2 commits into from
Jun 5, 2021
Merged

Fix suggestion for removing &mut from &mut macro!(). #85939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4524f8
Fix suggestion for removing &mut from &mut macro!().
m-ou-se Jun 2, 2021
ecebb66
Add test for removing &mut for &mut format!().
m-ou-se Jun 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions compiler/rustc_typeck/src/check/demand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use rustc_span::Span;
use super::method::probe;

use std::fmt;
use std::iter;

impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
pub fn emit_coerce_suggestions(
Expand Down Expand Up @@ -577,12 +578,19 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// We have `&T`, check if what was expected was `T`. If so,
// we may want to suggest removing a `&`.
if sm.is_imported(expr.span) {
if let Ok(src) = sm.span_to_snippet(sp) {
if let Some(src) = src.strip_prefix('&') {
// Go through the spans from which this span was expanded,
// and find the one that's pointing inside `sp`.
//
// E.g. for `&format!("")`, where we want the span to the
// `format!()` invocation instead of its expansion.
if let Some(call_span) =
iter::successors(Some(expr.span), |s| s.parent()).find(|&s| sp.contains(s))
Copy link
Member Author

@m-ou-se m-ou-se Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@estebank What do you think about adding this as a function on Span (e.g. span.find_parent_within(span) or something)?

I have a feeling more diagnostics can/should use this, but I haven't written enough diagnostics yet to know if that's true.

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 with your assessment and we should have something like that (bikeshed notwithstanding).

{
if let Ok(code) = sm.span_to_snippet(call_span) {
return Some((
sp,
"consider removing the borrow",
src.to_string(),
code,
Comment on lines -585 to +593
Copy link
Member Author

Choose a reason for hiding this comment

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

In another diagnostic, I emitted (potentially multiple) suggestions for removing the parts that need to be removed, instead of a single suggestion that contains a copy of (part of) the original source. Is there a guideline for which to prefer?

Basically:
A. "replace &mut (format!("")) by format!("")
B. "remove &mut ( and remove )"

Most diagnostics seem to use A, which surprises me a bit. The suggestion for A can get quite large for large expressions.

Copy link
Contributor

@estebank estebank Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The later didn't exist until "recently" (2019?), so there's a historical component. There's also a bug that needs to be fixed on rustc's side to allow rustfix apply multipart suggestions, which it can't today.

Even with that caveat, I push for multipart suggestions in all new diagnostics when possible/necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

There's also a bug that needs to be fixed on rustc's side to allow rustfix apply multipart suggestions, which it can't today.

That should be fixed by rust-lang/rustfix#195

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! That should also allow us to add a // run-rustfix to a bunch of tests that don't have it today because of that 😄

Applicability::MachineApplicable,
));
}
Expand Down
4 changes: 4 additions & 0 deletions src/test/ui/suggestions/format-borrow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,8 @@ fn main() {
//~^ ERROR mismatched types
let b: String = &format!("b");
//~^ ERROR mismatched types
let c: String = &mut format!("c");
//~^ ERROR mismatched types
let d: String = &mut (format!("d"));
//~^ ERROR mismatched types
}
22 changes: 21 additions & 1 deletion src/test/ui/suggestions/format-borrow.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,26 @@ LL | let b: String = &format!("b");
| | help: consider removing the borrow: `format!("b")`
| expected due to this

error: aborting due to 2 previous errors
error[E0308]: mismatched types
--> $DIR/format-borrow.rs:6:21
|
LL | let c: String = &mut format!("c");
| ------ ^^^^^^^^^^^^^^^^^
| | |
| | expected struct `String`, found `&mut String`
| | help: consider removing the borrow: `format!("c")`
| expected due to this

error[E0308]: mismatched types
--> $DIR/format-borrow.rs:8:21
|
LL | let d: String = &mut (format!("d"));
| ------ ^^^^^^^^^^^^^^^^^^^
| | |
| | expected struct `String`, found `&mut String`
| | help: consider removing the borrow: `format!("d")`
| expected due to this

error: aborting due to 4 previous errors

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