-
Notifications
You must be signed in to change notification settings - Fork 13k
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 using deref in patterns #132939
Suggest using deref in patterns #132939
Conversation
@@ -359,15 +369,35 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { | |||
} | |||
} | |||
if let Some(ty::error::ExpectedFound { found, .. }) = exp_found | |||
&& ty.boxed_ty() == Some(found) | |||
&& let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) | |||
&& let Ok(mut snippet) = self.tcx.sess.source_map().span_to_snippet(span) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's preexisting, but this snippet logic is unnecessary. You should be able to just give a suggestion like "*"
that is inserted before the match scrutinee with span.shrink_to_lo()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may also look better if you change this into a "verbose" suggestion with span_suggestion_verbose
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone back on this, because I'm not sure how to override a span in a suggestion using multipart. Please let me know if there's a way to do so.
compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
ca71995
to
774012d
Compare
I haven't gone through the tests yet, because there are still some problems that I haven't figured out yet. The logic for adding references still has to be redone, but that will come with solving the other issues. First, I'm still not happy about the suggestion given above. I may just add a check for the simple case of having a reference, since I think that covers the majority of times it'll appear. A bigger issue is that, when rustfix tries to apply the suggestions, it does so for each part of the match arm. For example, |
This comment has been minimized.
This comment has been minimized.
5fb18a6
to
d2e4358
Compare
The issues I mentioned above have been resolved. |
I can also expand the suggestion to specify which Deref impls are being used to get from type A to B. |
r? rust-lang/diagnostics |
&& ty.boxed_ty() == Some(found) | ||
&& let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) | ||
&& let Ok(mut peeled_snippet) = | ||
self.tcx.sess.source_map().span_to_snippet(origin_expr.peeled_span) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a snippet, you can track a prefix and an optional postfix suggestion. This way you can build the multipart suggestion right when you need it while being able to modify the prefix as you need to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't been able to implement this without rustfix applying the suggestion twice. Box deref suggestion's MachineApplicability could be downgraded, but that seems worse.
Relevant zulip: https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/Rustfix.20applying.20multipart.20suggestion.20twice
compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs
Outdated
Show resolved
Hide resolved
d2e4358
to
ce4b743
Compare
ce4b743
to
831f454
Compare
@rustbot review |
Thanks for the thorough investigation. I think we'll need to adjust rustfix to also consider suggestion spans as overlapping if they touch @bors r+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, clicked review request instead of dismiss on mobile by accident
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#132939 (Suggest using deref in patterns) - rust-lang#133293 (Updates Solaris target information, adds Solaris maintainer) - rust-lang#133392 (Fix ICE when multiple supertrait substitutions need assoc but only one is provided) - rust-lang#133986 (Add documentation for anonymous pipe module) - rust-lang#134022 (Doc: Extend for tuples to be stabilized in 1.85.0) - rust-lang#134259 (Clean up `infer_return_ty_for_fn_sig`) - rust-lang#134264 (Arbitrary self types v2: Weak & NonNull diagnostics) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#132939 - uellenberg:suggest-deref, r=oli-obk Suggest using deref in patterns Fixes rust-lang#132784 This changes the following code: ```rs use std::sync::Arc; fn main() { let mut x = Arc::new(Some(1)); match x { Some(_) => {} None => {} } } ``` to output ```rs error[E0308]: mismatched types --> src/main.rs:5:9 | LL | match x { | - this expression has type `Arc<Option<{integer}>>` ... LL | Some(_) => {} | ^^^^^^^ expected `Arc<Option<{integer}>>`, found `Option<_>` | = note: expected struct `Arc<Option<{integer}>>` found enum `Option<_>` help: consider dereferencing to access the inner value using the Deref trait | LL | match *x { | ~~ ``` instead of ```rs error[E0308]: mismatched types --> src/main.rs:5:9 | 4 | match x { | - this expression has type `Arc<Option<{integer}>>` 5 | Some(_) => {} | ^^^^^^^ expected `Arc<Option<{integer}>>`, found `Option<_>` | = note: expected struct `Arc<Option<{integer}>>` found enum `Option<_>` ``` This makes it more obvious that a Deref is available, and gives a suggestion on how to use it in order to fix the issue at hand.
Fixes #132784
This changes the following code:
to output
instead of
This makes it more obvious that a Deref is available, and gives a suggestion on how to use it in order to fix the issue at hand.