52985: cause cycle err on inf trait normalization

[tristanburgess]

Issue: #52985

• If an existential type is defined, but no user code infers the
  concrete type behind the existential type, normalization would
  infinitely recurse on this existential type which is only defined in
  terms of itself.
• Instead of raising an inf recurse error, we cause a cycle error to
  help highlight that the issue is that the type is only defined in terms
  of itself.
• Three known potential improvements:
  • If type folding itself was exposed as a query, used by
    normalization and other mechanisms, cases that would cause infinite recursion would
    automatically cause a cycle error.
  • The span for the cycle error should be improved to point to user
    code that fails to allow inference of the concrete type of the existential type,
    assuming that this error occurs because no user code can allow inference the
    concrete type.
  • A mechanism to extend the cycle error with a helpful note would be nice. Currently,
    the error is built and maintained by src/librustc/ty/query/plumbing,
    with no known way to extend the information that the error gets built
    with.

r? @oli-obk

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[oli-obk]

The span for the cycle error should be improved

You could check if the call site of the outer normalize_ty_after_erasing_regions has more span information available. If so, we can add the span there.

If type folding itself was exposed as a query

My first notion was "type folding can't be a query, because it tracks state in the folder". But you might be onto something there. Can you open an issue about it explaining the idea with some links to the relevant code here (also to the definition of the folder in question)?

A mechanism to extend the cycle error with a helpful note would be nice

Again, can you open an issue about this? I think it would nicely fit into the same scheme as the tcx.at(span) system. We could have a tcx.at_note(span, "text") or something.

[tristanburgess]

Pertaining to filing the two issues mentioned, I will do that (maybe be a couple days since I'll be away for a couple days coming up).

Let me just re-post the snippet here from the original issue ticket for clarity.

#![feature(existential_type)]

existential type Foo: Copy;

// make compiler happy about using 'Foo'
fn bar(x: Foo) -> Foo { x }

fn main() {
    
    // make compiler happy about the types??
    let _: Foo = unsafe { std::mem::transmute(0u8) };
}

Pertaining to the first point about the span, the closest span to the normalize_ty_after_erasing_regions query is the one pointing to the transmute call. Would this one be better?

A bit of background, the type normalization as part of this ICE happens as part of computing SizeSkeleton for the src and dst types of transmute (so that their sizes can be compared for a valid transmute() to proceed). Normalization is supposed to figure out the concrete type behind the existential type so that its true size can be determined. So ultimately, the span we saw was for the transmute because that's the expression being worked on that leads to type normalization.

In reference to the above snippet, the span that would point at the function (bar(), which is ultimately reason the concrete type behind Foo can't be inferenced) occurs much earlier than where the ICE occurs. I believe that means that bar()'s definition has already been processed by that time, so I don't know if we'd be able to transfer that span over.

Moreover, removing the use of Foo (and transmute()) in main() compiles fine, which tells me that only defining an existential type in terms of itself either

1. isn't detected or
2. isn't a problem in itself, unless there is code that depends on the existential type being backed by a concrete type (which, to my understanding, is any blackbox user of Foo).

If it's 1), then I think this is real issue, as we only seem to get any error when a piece of code uses Foo as if it had a concrete type defined. My understanding of existential types are that they are hiding concrete types from external clients, thus we would need earlier error checking on specifically things that make/return Foos to validate that a concrete type can be inferred, in order to use the spans of the definers.

If it's 2), then I suspect that the span pointing to the user of Foo, which expects it to have a concrete type behind it (transmute() in this case), may be the best we can get. Perhaps there's additional info we can have the transmute() err (listed below) give out in this special failure case.

./dev/code/rust$ rustc +stage1 ./dev/tests/data/rust_52895_ice.rs 
error[E0512]: transmute called with types of different sizes
  -->  ./dev/tests/data/rust_52895_ice.rs:11:27
   |
11 |     let _: Foo = unsafe { std::mem::transmute(0u8) };
   |                           ^^^^^^^^^^^^^^^^^^^
   |
   = note: source type: u8 (8 bits)
   = note: target type: Foo (this type's size can vary)

error: aborting due to previous error

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

[oli-obk]

I believe that means that bar()'s definition has already been processed by that time, so I don't know if we'd be able to transfer that span over.
Moreover, removing the use of Foo (and transmute()) in main() compiles fine, which tells me that only defining an existential type in terms of itself either
isn't a problem in itself, unless there is code that depends on the existential type being backed by a concrete type (which, to my understanding, is any blackbox user of Foo).

I think that's the big question. It seems fine but useless to support this case.

existential type Foo: std::fmt::Debug;

struct Bar(Foo);

fn foo(f: Foo) -> Bar {
    Bar(f)
}

is not a defining use, while

struct Bar<T>(T);

fn foo(f: Foo) -> Bar<Foo> {
    Bar(f)
}

is a defining use. I think this is the underlying issue.

Maybe we should exclude such things from being allowed to be defining uses. So we'd add an else if defined_in_terms_of_self branch to

rust/src/librustc_typeck/collect.rs

Line 1216 in 866a713

} else {

and report an error in that case. If the user wants to write this function, they can do so outside the defining use scope (so simply one module above the one defining the existential type).

This might be a little too late though. Maybe the typeck_tables_of query should call type_of on all TyAnon it found inside a concrete type of a TyAnon. This can be done by calling type_of on all TyAnon found in definition_ty in

rust/src/librustc_typeck/check/writeback.rs

Line 495 in c7cba3d

let old = self.tables.concrete_existential_types.insert(def_id, definition_ty);

Not sure if we are forbidding real use cases this way, but I think all such cases should be able to work around it by moving the offending function up one level in the module hierarchy

[tristanburgess]

Looking further into

If type folding itself was exposed as a query, used by
normalization and other mechanisms, cases that would cause infinite recursion would
automatically cause a cycle error.

it looks like the infinite recursion defenses in normalization are a special case rather than the norm for uses of TypeFolders, so I think the benefit we would get from it, given the likely effort to implement, would be rather minimal. I think that given the limited use cases, it also makes more sense to have ty.fold_with(folder) than something like tcx.fold_with(ty, folder), as folding is a trait of types themselves. Secondly, I think it's a better error message where the cycle reports that there was a cycle during normalization, than what I think would happen if type folding was a query, where a cycle error would say there was a problem with type folding, which is much more ambiguous.

I think one of the main reasons I came up with the idea was that I was trying to figure out the lines between what is supposed to be a query vs what is supposed to be underlying machinery behind queries, per https://github.com/rust-lang/rust/tree/d767ee11616390d128853a06f5addb619e79213f/src/librustc/ty/query. The lines are still blurry to me, but I think that type folding should remain as is for now given likely effort vs win.

I have filed an issue for

A mechanism to extend the cycle error with a helpful note would be nice. Currently,
the error is built and maintained by src/librustc/ty/query/plumbing,
with no known way to extend the information that the error gets built
with.

#53453

[tristanburgess]

@oli-obk ok to review this current PR as is and I'll open up a new one once I look into the points of our discussion above?

[oli-obk]

@bors r+

Yea, let's address the diagnostics improvements in further separate prs

[bors]

📌 Commit 8895e3b has been approved by oli-obk

[oli-obk]

One improvement might also be to add a span to the parent query, too

[bors]

⌛ Testing commit 8895e3b with merge f28f648...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing f28f648 to master...

[Valinora]

Good perf win for keccak it seems.
Perf Results

[oli-obk]

I'm not sure how this could have caused perf changes... Maybe the removal of the panic caused llvm to optimize better?

[samlh]

That comparison also includes bfc3b20 and 3ac79c7, so that is probably where the wins came from.

[pnkfelix]

I put nomination tags on this because it seems like it may fix the stable-to-stable regression #52701.

But I am only nominating it because I want other members of the T-compiler team to weigh in about whether to backport. (My personal inclination is to not backport this to stable, and probably not to backport beta either.)

[oli-obk]

To the best of my knowledge this was/is just a diagnostics issue. The code would never have been reasonable in the first place. So we regressed a compile-time error to an ICE and fixed it again to be an error.

[oli-obk]

removing the beta nomination as it is already in beta by now

[nikomatsakis]

Why is this nominated for stable backport? Because it is believed to fix #52701? Can anyone verify that?

[pietroalbini]

Removed the stable nomination, it's not worth the risk of a stable backport this late in the release cycle for a diagnostics regression.