Abort after representability errors

[nnethercote]

Doing so results in better error messages and makes the code a bit simpler. Details in individual commits.

r? @oli-obk

[rustbot]

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

[nnethercote]

This overlaps with #153028 by @Zoxc. (I wrote this independently a few days ago after writing #153169 and then subsequently discovered the overlap.) That PR changes the representability queries and also adds per-query cycle handlers. Those changes are all combined into a single large commit and the review comments have been mixed, so I thought it worth creating this PR for just the representability changes. They improve error messages, make the code simpler, don't get in the way of removing query cycle handling, but also don't commit to removing query cycle handling (which @zetanumbers has concerns about). I have also introduced the use of ensure_ok() and added check_ prefixes to the queries, which I think are good.

[nnethercote]

I have also introduced the use of ensure_ok()

Turns out this doesn't work for the reason because it requires dropping anon from the queries, which caused an incremental test failure. So I have undone that part, and I am just using let _ = tcx.check_representability(...);.

[zetanumbers]

I like that you haven't got rid of this type, although I would have made it pub struct Representability(()) in order to make its construction a private detail for a query implementation.

[zetanumbers]

Hmm, but we cannot do that as query implementation is in another crate. I guess I would've done new_unchecked unsafe fn instead then if rustc was my project, but that's kinda a far too theoretical concern for an open project.

[petrochenkov]

I have also introduced the use of ensure_ok()

Turns out this doesn't work for the reason because it requires dropping anon from the queries, which caused an incremental test failure. So I have undone that part, and I am just using let _ = tcx.check_representability(...);.

The second commit message still mentions ensure_ok and anon

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[nnethercote]

I have rebased and removed the anon mention from the second commit message.

[oli-obk]

if you want to keep enforcing let _ = in front of the query invocations, then this struct should be #[must_use]. Otherwise just call the query like we call unit queries

[nnethercote]

If I change this:

    let _ = tcx.check_representability(item.owner_id.def_id);

to this:

    tcx.check_representability(item.owner_id.def_id);

I get this warning:

warning: unused return value of `queries::<impl rustc_middle::ty::TyCtxt<'tcx>>::check_representability` that must be used
    --> compiler/rustc_hir_analysis/src/check/wfcheck.rs:1000:5
     |
1000 |     tcx.check_representability(item.owner_id.def_id);
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: `#[warn(unused_must_use)]` (part of `#[warn(unused)]`) on by default
help: use `let _ = ...` to ignore the resulting value
     |
1000 |     let _ = tcx.check_representability(item.owner_id.def_id);
     |     +++++++

I'm not sure where the must_use that triggers this comes from.

I could instead do this:

    tcx.ensure_ok().check_representability(item.owner_id.def_id);

which is probably better, given that ensure_ok can help with incremental perf. Sound ok to you?

[nnethercote]

Oh, I totally goldfished that I tried ensure_ok before.

Anyway, I don't see how to avoid the let _ = things, given that I get the warnings without them.

[oli-obk]

Right. Unfortunate but makes sense for now

[oli-obk]

@bors r+

[rust-bors]

📌 Commit ff3d308 has been approved by oli-obk

It is now in the queue for this repository.