Suggest .as_ref() when appropriate for Option and Result
#57158
Conversation
|
r? @zackmdavis (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
(hope to catch up on reviews Saturday the twenty-ninth) |
|
I know I missed this sorely when I was at the beginning of my Rust journey. Many thanks for working on this! |
estebank
force-pushed
the
as-ref
branch
2 times, most recently
from
a945167 to
1c31447
Compare
This comment has been minimized.
This comment has been minimized.
| let mut show_suggestion = true; | ||
| for (exp_ty, found_ty) in exp_substs.types().zip(found_substs.types()) { | ||
| if let TyKind::Ref(_, exp_ty, _) = exp_ty.sty { | ||
| match (&exp_ty.sty, &found_ty.sty) { |
There was a problem hiding this comment.
I would like to replace this entire match with FnCtxt::can_coerce, but first I have to find a way to get ahold of a FnCtxt here :-|
@petrochenkov when you have the time, could you point me in the right direction for this? I've noticed that we have same_tys and same_type which don't do exactly the same thing, but are pretty similar...
| LL | opt.map(|arg| takes_ref(arg)); | ||
| | - ^^^ expected &Foo, found struct `Foo` | ||
| | | | ||
| | help: consider using `as_ref` instead: `as_ref().` |
There was a problem hiding this comment.
as_ref(). (trailing dot!) as the suggestion text faked me out for a moment; our insertion suggestions can look kind of awkward sometimes. (But making the span cover opt.map and suggesting opt.as_ref().map is ugly in its own way.)
In any case, the code that sets this was actually introduced in #51100, not here, so it doesn't affect the outcome of this review.
| if let TyKind::Adt(found_def, found_substs) = found_ty.sty { | ||
| let path_str = format!("{:?}", exp_def); | ||
| if exp_def == &found_def { | ||
| let opt_msg = "you can convert from `&Option<T>` to `Option<&T>` using \ |
There was a problem hiding this comment.
| let opt_msg = "you can convert from `&Option<T>` to `Option<&T>` using \ | |
| let opt_msg = "`&Option<T>` can be converted to `Option<&T>` using \ |
I lean against the second person in diagnostic messages. (But that's my opinion; we haven't codifed that anywhere.)
| match (&exp_found.expected.sty, &exp_found.found.sty) { | ||
| (TyKind::Adt(exp_def, exp_substs), TyKind::Ref(_, found_ty, _)) => { | ||
| if let TyKind::Adt(found_def, found_substs) = found_ty.sty { | ||
| let path_str = format!("{:?}", exp_def); |
There was a problem hiding this comment.
Relying on debug output for functionality seems like poor form? (On the grounds that it's meant for debugging convenience and should be subject to change rather than guaranteed stable, even if it happens to be probably-stable in this case.)
| @@ -972,6 +973,72 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |||
| self.note_error_origin(diag, &cause); | |||
| } | |||
|
|
|||
| /// When encountering a case where `.as_ref()` on a `Result` or `Option` would be appropriate, | |||
| /// suggest it. | |||
| fn suggest_as_ref_where_appropriate( | |||
There was a problem hiding this comment.
I like _where_appropriate as part of a function name, but I don't think I've seen it used anywhere else, and I know we have a lot of these "conditionally set a diagnostic" functions ...
|
@bors r+ |
|
📌 Commit 0ecc128 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 0ecc128 with merge 499a5af507151593ba0de1f9bc566147e179392a... |
This comment has been minimized.
This comment has been minimized.
bors
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
@bors retry |
bors
added
S-waiting-on-bors
Suggest `.as_ref()` when appropriate for `Option` and `Result` Fix #55198.
|
☀️ Test successful - status-appveyor, status-travis |
Fix #55198.