Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix bad placeholder type error on certain consts and statics #77431

Closed
wants to merge 1 commit into from
Closed

Fix bad placeholder type error on certain consts and statics #77431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions compiler/rustc_typeck/src/collect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,14 @@ fn convert_item(tcx: TyCtxt<'_>, item_id: hir::HirId) {
if let hir::ItemKind::Fn(..) = it.kind {
tcx.ensure().fn_sig(def_id);
}
// Account for the `_` placeholder.
// We only check on `static` or `const` to avoid too many duplicated errors
// (see https://github.com/rust-lang/rust/issues/77428).
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);
Copy link
Contributor

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.

Copy link
Member Author

@varkor varkor Oct 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, let me try…

Copy link
Member Author

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.

}
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/test/ui/error-codes/E0121.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
fn foo() -> _ { 5 } //~ ERROR E0121

static BAR: _ = "test"; //~ ERROR E0121
//~^ ERROR E0121
Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Contributor

@estebank estebank Oct 3, 2020 •
edited
Loading

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

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=80460e273fc377baa6546198c301a8ba

Copy link
Member Author

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?


fn main() {
}
const FOO: dyn Fn() -> _ = ""; //~ ERROR E0121

static BOO: dyn Fn() -> _ = ""; //~ ERROR E0121

fn main() {}
20 changes: 19 additions & 1 deletion src/test/ui/error-codes/E0121.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,24 @@ LL | static BAR: _ = "test";
| not allowed in type signatures
| help: replace `_` with the correct type: `&str`

error: aborting due to 2 previous errors
error[E0121]: the type placeholder `_` is not allowed within types on item signatures
--> $DIR/E0121.rs:3:13
|
LL | static BAR: _ = "test";
| ^ not allowed in type signatures

error[E0121]: the type placeholder `_` is not allowed within types on item signatures
--> $DIR/E0121.rs:6:24
|
LL | const FOO: dyn Fn() -> _ = "";
| ^ not allowed in type signatures

error[E0121]: the type placeholder `_` is not allowed within types on item signatures
--> $DIR/E0121.rs:8:25
|
LL | static BOO: dyn Fn() -> _ = "";
| ^ not allowed in type signatures

error: aborting due to 5 previous errors

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