Skip to content

Suggest .as_ref() when appropriate for Option and Result #57158

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

Merged
merged 2 commits into from
Dec 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
67 changes: 67 additions & 0 deletions src/librustc/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -956,6 +956,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
diag.span_label(span, message);
}
}
self.suggest_as_ref_where_appropriate(span, &exp_found, diag);
}

diag.note_expected_found(&"type", expected, found);
Expand All @@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

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 ...

&self,
span: Span,
exp_found: &ty::error::ExpectedFound<Ty<'tcx>>,
diag: &mut DiagnosticBuilder<'tcx>,
) {
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);
Copy link
Member

Choose a reason for hiding this comment

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

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.)

if exp_def == &found_def {
let opt_msg = "you can convert from `&Option<T>` to `Option<&T>` using \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.)

`.as_ref()`";
let result_msg = "you can convert from `&Result<T, E>` to \
`Result<&T, &E>` using `.as_ref()`";
let have_as_ref = &[
("std::option::Option", opt_msg),
("core::option::Option", opt_msg),
("std::result::Result", result_msg),
("core::result::Result", result_msg),
];
if let Some(msg) = have_as_ref.iter()
.filter_map(|(path, msg)| if &path_str == path {
Some(msg)
} else {
None
}).next()
{
let mut show_suggestion = true;
for (exp_ty, found_ty) in exp_substs.types().zip(found_substs.types()) {
match exp_ty.sty {
TyKind::Ref(_, exp_ty, _) => {
match (&exp_ty.sty, &found_ty.sty) {
(_, TyKind::Param(_)) |
(_, TyKind::Infer(_)) |
(TyKind::Param(_), _) |
(TyKind::Infer(_), _) => {}
_ if ty::TyS::same_type(exp_ty, found_ty) => {}
_ => show_suggestion = false,
};
}
TyKind::Param(_) | TyKind::Infer(_) => {}
_ => show_suggestion = false,
}
}
if let (Ok(snippet), true) = (
self.tcx.sess.source_map().span_to_snippet(span),
show_suggestion,
) {
diag.span_suggestion_with_applicability(
span,
msg,
format!("{}.as_ref()", snippet),
Applicability::MachineApplicable,
);
}
}
}
}
}
_ => {}
}
}

pub fn report_and_explain_type_error(
&self,
trace: TypeTrace<'tcx>,
Expand Down
28 changes: 14 additions & 14 deletions src/librustc/ty/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,19 @@ impl<'a, 'tcx> ty::TyS<'tcx> {
tcx.needs_drop_raw(param_env.and(self))
}

pub fn same_type(a: Ty<'tcx>, b: Ty<'tcx>) -> bool {
match (&a.sty, &b.sty) {
(&Adt(did_a, substs_a), &Adt(did_b, substs_b)) => {
if did_a != did_b {
return false;
}

substs_a.types().zip(substs_b.types()).all(|(a, b)| Self::same_type(a, b))
}
_ => a == b,
}
}

/// Check whether a type is representable. This means it cannot contain unboxed
/// structural recursion. This check is needed for structs and enums.
pub fn is_representable(&'tcx self,
Expand Down Expand Up @@ -730,19 +743,6 @@ impl<'a, 'tcx> ty::TyS<'tcx> {
}
}

fn same_type<'tcx>(a: Ty<'tcx>, b: Ty<'tcx>) -> bool {
match (&a.sty, &b.sty) {
(&Adt(did_a, substs_a), &Adt(did_b, substs_b)) => {
if did_a != did_b {
return false;
}

substs_a.types().zip(substs_b.types()).all(|(a, b)| same_type(a, b))
}
_ => a == b,
}
}

// Does the type `ty` directly (without indirection through a pointer)
// contain any types on stack `seen`?
fn is_type_structurally_recursive<'a, 'tcx>(
Expand Down Expand Up @@ -807,7 +807,7 @@ impl<'a, 'tcx> ty::TyS<'tcx> {
// struct Foo { Option<Option<Foo>> }

for &seen_type in iter {
if same_type(ty, seen_type) {
if ty::TyS::same_type(ty, seen_type) {
debug!("ContainsRecursive: {:?} contains {:?}",
seen_type,
ty);
Expand Down
47 changes: 0 additions & 47 deletions src/test/ui/as-ref.stderr

This file was deleted.

10 changes: 10 additions & 0 deletions src/test/ui/as-ref.rs → src/test/ui/suggestions/as-ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,14 @@ fn main() {
//~^ ERROR mismatched types [E0308]
opt.and_then(|arg| Ok(takes_ref(arg)));
//~^ ERROR mismatched types [E0308]
let x: &Option<usize> = &Some(3);
let y: Option<&usize> = x;
//~^ ERROR mismatched types [E0308]
let x: &Result<usize, usize> = &Ok(3);
let y: Result<&usize, &usize> = x;
//~^ ERROR mismatched types [E0308]
// note: do not suggest because of `E: usize`
let x: &Result<usize, usize> = &Ok(3);
let y: Result<&usize, usize> = x;
//~^ ERROR mismatched types [E0308]
}
81 changes: 81 additions & 0 deletions src/test/ui/suggestions/as-ref.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
error[E0308]: mismatched types
--> $DIR/as-ref.rs:6:27
|
LL | opt.map(|arg| takes_ref(arg));
| - ^^^ expected &Foo, found struct `Foo`
| |
| help: consider using `as_ref` instead: `as_ref().`
Copy link
Member

Choose a reason for hiding this comment

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

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.

|
= note: expected type `&Foo`
found type `Foo`

error[E0308]: mismatched types
--> $DIR/as-ref.rs:8:37
|
LL | opt.and_then(|arg| Some(takes_ref(arg)));
| - ^^^ expected &Foo, found struct `Foo`
| |
| help: consider using `as_ref` instead: `as_ref().`
|
= note: expected type `&Foo`
found type `Foo`

error[E0308]: mismatched types
--> $DIR/as-ref.rs:11:27
|
LL | opt.map(|arg| takes_ref(arg));
| - ^^^ expected &Foo, found struct `Foo`
| |
| help: consider using `as_ref` instead: `as_ref().`
|
= note: expected type `&Foo`
found type `Foo`

error[E0308]: mismatched types
--> $DIR/as-ref.rs:13:35
|
LL | opt.and_then(|arg| Ok(takes_ref(arg)));
| - ^^^ expected &Foo, found struct `Foo`
| |
| help: consider using `as_ref` instead: `as_ref().`
|
= note: expected type `&Foo`
found type `Foo`

error[E0308]: mismatched types
--> $DIR/as-ref.rs:16:27
|
LL | let y: Option<&usize> = x;
| ^
| |
| expected enum `std::option::Option`, found reference
| help: you can convert from `&Option<T>` to `Option<&T>` using `.as_ref()`: `x.as_ref()`
|
= note: expected type `std::option::Option<&usize>`
found type `&std::option::Option<usize>`

error[E0308]: mismatched types
--> $DIR/as-ref.rs:19:35
|
LL | let y: Result<&usize, &usize> = x;
| ^ expected enum `std::result::Result`, found reference
|
= note: expected type `std::result::Result<&usize, &usize>`
found type `&std::result::Result<usize, usize>`
help: you can convert from `&Result<T, E>` to `Result<&T, &E>` using `.as_ref()`
|
LL | let y: Result<&usize, &usize> = x.as_ref();
| ^^^^^^^^^^

error[E0308]: mismatched types
--> $DIR/as-ref.rs:23:34
|
LL | let y: Result<&usize, usize> = x;
| ^ expected enum `std::result::Result`, found reference
|
= note: expected type `std::result::Result<&usize, usize>`
found type `&std::result::Result<usize, usize>`

error: aborting due to 7 previous errors

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