-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix bad placeholder type error on certain consts and statics #77431
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
r? @estebank |
@@ -1,6 +1,10 @@ | |||
fn foo() -> _ { 5 } //~ ERROR E0121 | |||
|
|||
static BAR: _ = "test"; //~ ERROR E0121 | |||
//~^ ERROR E0121 |
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.
There's a duplicate error now, but as this is a pre-existing issue (#77428), and this fixes an ICE that could reasonably be encountered by users, I thought a quick fix was best.
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.
Actually, this triggers quite a few more additional error messages in other places; it'd be nicer to fix this now.
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.
@estebank: I thought we deduplicated diagnostics? Do you know the reason it's failing in this case?
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.
We don't deduplicate automatically at the error level, we do it on a case by case basis ahead of time. For _
E0121 errors it was a bit ad-hoc (introduced in #69148).
I remember having to tweak the logic a bit to have correct coverage (the self.tcx().sess.delay_span_bug(span, "bad placeholder type");
in one of the ty_infer
impls had triggered a couple of times due to it). I would imagine that we'll need to do something along the lines of #70294 because two different codepaths that evaluate static
s end up calling placeholder_type_error
.
Edit: Oh! I see, this only happens for const
/static
for trait types. This is exactly the same type of duplicated problem I was seeing. I think you can get away by looking at the type we're evaluating...
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 you can get away by looking at the type we're evaluating...
What do you mean by this?
if matches!(it.kind, hir::ItemKind::Static(..) | hir::ItemKind::Const(..)) { | ||
let mut visitor = PlaceholderHirTyCollector::default(); | ||
visitor.visit_item(it); | ||
placeholder_type_error(tcx, None, &[], visitor.0, false); |
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... what happens if you remove this call? I'm pretty sure that using PlaceholderHirTyCollector
it will make the call when encountering the _
as expected.
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.
Oh, let me try…
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.
Unfortunately, it has no effect, and the test continues to ICE.
back to author to fix broken test |
(I will try to get back to this, but there are higher priority issues for now.) |
I'm not going to be able to get back to this soon, so closing for now. |
Fixes #75889.