check the recursion limit when finding a struct's tail

[SNCPlay42]

fixes #79437

This does a delay_span_bug (via ty_error_with_message) rather than emit a new error message, under the assumption that there will be an error elsewhere (even if the type isn't infinitely recursive, just deeper than the recursion limit, this appears to be the case).

[rust-highfive]

r? @eddyb

(rust-highfive has picked a reviewer for you, use r? to override)

[meithecatte]

I don't know much about rustc internals, but nevertheless, I don't think this is the right approach. I would expect that making all code work with cyclic data structures, even when such cycles can only be present in ill-formed code, is not an attainable goal. As I've mentioned in #79437 (comment), I believe that when the error about recursive types is emitted in the first place, the offending field should be replaced with a TyKind::Error.

[oli-obk]

I believe that when the error about recursive types is emitted in the first place, the offending field should be replaced with a TyKind::Error.

that would be ideal, but I don't see how that can be done in the present setup. The typeck code that actually performs the check that detects the recursion happens without yielding any results but diagnostic messages:

rust/compiler/rustc_typeck/src/check/check.rs

Line 365 in 760430e

check_representable(tcx, span, def_id);

It would require a nontrivial amount of refactoring to implement that, so I think we should first merge a fix, but keep open an issue about solving this properly and use that issue to plan how to actually do that.

[oli-obk]

r? @oli-obk

[oli-obk]

Instead of having an iteration limit, we could also fill in an FxHashSet<Ty<'tcx>> to see if we've encountered a type before. If we have, we can abort directly. If the set grows beyond the recursion limit, we abort due to the recursion limit.

We'll need to benchmark that change though

[oli-obk]

Instead of having an iteration limit, we could also fill in an FxHashSet<Ty<'tcx>> to see if we've encountered a type before. If we have, we can abort directly. If the set grows beyond the recursion limit, we abort due to the recursion limit.

What do you think about this? Do prefer if we merge the PR as it is (it's totally fine as it is) and move this to an issue, or do you want to have a look at that in this PR?

[SNCPlay42]

Wouldn't that only improve performance in the case we're going to error? If we ever stop early because we found a type we'd seen before in the hash set, we've seen a recursive type and compilation will ultimately fail. It doesn't seem like it would be worth the performance cost of building the set in the happy path.

(oops, fumbled the close button)

[oli-obk]

It doesn't seem like it would be worth the performance cost of building the set in the happy path.

Yea, that's what I was worried about as well with my comment about benchmarking above

@bors r+

[bors]

📌 Commit 98fc02d has been approved by oli-obk

[bors]

⌛ Testing commit 98fc02d with merge cc29f8ddc616e9245378d720729339a9f22df7b7...

[bors]

💔 Test failed - checks-actions

[jonas-schievink]

@bors retry

[bors]

⌛ Testing commit 98fc02d with merge 5bb68c3...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 5bb68c3 to master...