-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Misc HIR typeck type mismatch tweaks #112116
Misc HIR typeck type mismatch tweaks #112116
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
ce910f8
to
89032b4
Compare
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.
Can we add a check for the following?
let _: Result<&usize, _> = &Ok(42);
error[E0308]: mismatched types | ||
--> $DIR/dont-suggest-cyclic-constraint.rs:7:35 | ||
| | ||
LL | debug_assert_eq!(iter.next(), Some(value)); | ||
| ^^^^^^^^^^^ expected `Option<<I as Iterator>::Item>`, found `Option<&<I as Iterator>::Item>` | ||
| | ||
= note: expected enum `Option<<I as Iterator>::Item>` | ||
found enum `Option<&<I as Iterator>::Item>` |
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.
The expectation in the ticket was for this to suggest .as_ref()
, right?
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.
Yes, which is why I didn't close it just yet.
| ------------ ^^^^ expected `Option<&String>`, found `&Option<String>` | ||
| | |
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.
Sad to lose these.
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'll see if I can add it back
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.
Wait, I'm not actually sure what you're talking about when looking at this now 😅
For the &Some
case, we still give the right suggestion below:
help: try using `.as_ref()` to convert `&Option<String>` to `Option<&String>`
|
LL - takes_option(&res);
LL + takes_option(res.as_ref());
And for the &None
case, we no longer suggest None.as_ref()
, in favor of just suggesting removing the &
which is far more accurate.
7a667ec
to
6615862
Compare
@compiler-errors the tests look good, thanks for the work! I'll review the code shortly... One question though, does the code support something like this: let _: Option<&usize> = Some(1); I'd expect rustc to suggest |
@WaffleLapkin: I don't think it does. I debated adding support for autoref-ish suggestions, but that case specifically has a higher likelihood than the others to lead to later borrowck errors compared to just deref-related suggestions because it may require taking a reference to a temporary or something. |
Oops, correction: it does suggest let y = Some(1);
let _: Option<&usize> = y; Which says:
Otherwise the compiler prefers to suggest |
help: try using `.as_ref()` to convert `&Result<{integer}, _>` to `Result<&usize, _>` | ||
| | ||
LL - let _: Result<&usize, _> = &Ok(42); | ||
LL + let _: Result<&usize, _> = Ok(42).as_ref(); |
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.
Despite the concerns of this not being idiomatic, I'm more than ok with landing with a suggestion like this.
} else if let ty::Adt(adt, _) = found_ty_inner.peel_refs().kind() | ||
&& Some(adt.did()) == self.tcx.lang_items().string() | ||
&& peeled.is_str() |
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.
Why this checks for string specifically? (and not for example for a type with as_ref
with an appropriate signature or something like that)
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.
Because it's complicated, 🤷
I didn't implement this specific suggestion, just moved it around, and I don't really want to make it particularly more complicated 😅
err.span_suggestion_verbose( | ||
expr.span.shrink_to_hi(), | ||
fluent::hir_typeck_convert_to_str, | ||
".map(|x| x.as_str())", |
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.
String::as_str
without the closure, maybe?
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 think this applies with any number of peeled refs, so this won't work:
fn f(x: Option<&&&&String>) {
let _: Option<&str> = x;
}
r=me with(out) nits |
6615862
to
a918822
Compare
@bors r=WaffleLapkin |
☀️ Test successful - checks-actions |
Finished benchmarking commit (9c843d9): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 649.387s -> 649.729s (0.05%) |
@rustbot label: +perf-regression-triaged |
Also I don't think I changed anything that isn't on the error path. |
These are all intended to improve #112104, but I couldn't get it to actually suggest adding
as_ref
to the LHS of the equality expr without some hacks that I may play around with some more.Each commit's title should explain what it's doing except for perhaps the last one, which addresses the bogus suggestion on #112104 itself.