Move checking placeholder types in return types to typeck

[Zoxc]

This moves checking placeholder types in return types from the fn_sig query to typeck. typeck computes the return value of the body which is used for error suggestions. This is done to prevent a query cycle between fn_sig and typeck.

Currently fn_sig is marked with cycle_delay_bug to deal with this cycle, but I'm not sure if it's sufficient as we need the query we resume to have a Value impl, which may not be fn_sig.

Functions such as fn foo() -> _ will now return as fn foo() -> <error> from fn_sig instead of using the return type of their bodies. This can hide some later errors, but that seems like a minor downside.

This is also a step towards removing query cycle recovery.

[rustbot]

HIR ty lowering was modified

cc @fmease

[rustbot]

r? @dingxiangfei2009

rustbot has assigned @dingxiangfei2009.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 15 candidates

[dingxiangfei2009]

Thank you for contribution! 🙇

@bors r+ rollup=always

[rust-bors]

📌 Commit 8ddc609 has been approved by dingxiangfei2009

It is now in the queue for this repository.

[fmease]

Currently fn_sig is marked with cycle_delay_bug to deal with this cycle, but I'm not sure if it's sufficient as we need the query we resume to have a Value impl, which may not be fn_sig.

I'm struggling to understand why you didn't remove cycle_delay_bug from query fn_sig then (plus the Value impl for PolyFnSig)? Is some other place still relying on fn_sig delaying cycle errors as bugs?

[fmease]

To be frank, I'm not super stoked about adding a hyper-specific field to ItemCtxt plus an "anonymous" Boolean parameter to various functions. This solution feels blunt(?) and slightly hacky.

I haven't had the time to think about alternative solutions yet if there are any. I believe it should be possible to localize this code a lot more even if that meant ~reverting parts of PR #125819 (before which this recovery was 'more local' IIRC).

(personally speaking I'm generally unhappy about this super invasive & complex recovery logic for an 'incredibly niche' user error; it's already caused me headaches in the past)

[Zoxc]

We could possibly split out placeholder errors into a separate HIR visitor, then just skip visiting return types as appropriate.

[nnethercote]

(personally speaking I'm generally unhappy about this super invasive & complex recovery logic for an 'incredibly niche' user error; it's already caused me headaches in the past)

I think we occasionally go too far in our quest for high quality error messages. If an obscure error message is causing problems for perf or code complexity I think removing it is totally reasonable.

Just to clarify: which user error is the incredibly niche one?

[fmease]

Just to clarify: which user error is the incredibly niche one?

So I don't know how often beginner or average users encounter this error or how helpful they find the suggestion, I'm talking about code like const C = 1; (no type annotation), static S: _ = 1; (_ in item signatures), fn f() -> _ { 0 } but also static A: [&str; _] = [];.

I get that they can be quite useful, so maybe I've exaggerated. Still, personally speaking I just never use _ like this to obtain the right type from the compiler, so I'm a bit biased.

It's just that I know the implementation is quite unwieldy and hairy. Most crucially calling typeck instead of type_of on items that usually mandate a type signature is prone to query cycles (e.g, I most recently had to fight them in RUST-143029) which can be quite hindering when developing new features.

[Zoxc]

We could possibly split out placeholder errors into a separate HIR visitor, then just skip visiting return types as appropriate.

ItemCtxt had a few more users then I expected, so this might not be a good idea as it could be easy to skip the separate visitor.

[fmease]

Sorry, I'm gonna unapprove this for now due to #153243 (comment) (question) and #153243 (comment) (semi-actionable concern)

@bors r-

[Zoxc]

I'm struggling to understand why you didn't remove cycle_delay_bug from query fn_sig then (plus the Value impl for PolyFnSig)? Is some other place still relying on fn_sig delaying cycle errors as bugs?

It unhides unrelated cycle errors, so I kept it out here to keep the PR smaller.

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

[Zoxc]

@fmease Can this get a re-evaluation with the extra parameter now removed?

[rust-bors]

☔ The latest upstream changes (presumably #154916) made this pull request unmergeable. Please resolve the merge conflicts.