Move checking placeholder types in return types to typeck - #153243
Conversation
|
HIR ty lowering was modified cc @fmease |
This comment was marked as outdated.
This comment was marked as outdated.
|
Thank you for contribution! 🙇 @bors r+ rollup=always |
I'm struggling to understand why you didn't remove |
| tcx: TyCtxt<'tcx>, | ||
| item_def_id: LocalDefId, | ||
| tainted_by_errors: Cell<Option<ErrorGuaranteed>>, | ||
| suppress_placeholder_errors: Cell<bool>, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
We could possibly split out placeholder errors into a separate HIR visitor, then just skip visiting return types as appropriate.
There was a problem hiding this comment.
(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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
To be frank, I'm not super stoked about adding a hyper-specific field to
ItemCtxt
agreed on this, even though it seems like the latter half seems to be "fixed". What does this PR look like without this?
There was a problem hiding this comment.
in reply to #153243 (comment)
Without this flag we emit generic error diagnostics per _ in return types plus one diagnostic that actually contains the specialized suggestion with multiple primary highlights.
Using diagnostic stashing + downgrading to delayed bugs I was able go get rid of this flag while preserving the desired behavior. I'll push that later to this PR.
|
Sorry, I'm gonna unapprove this for now due to #153243 (comment) (question) and #153243 (comment) (semi-actionable concern) @bors r- |
It unhides unrelated cycle errors, so I kept it out here to keep the PR smaller. |
This comment has been minimized.
This comment has been minimized.
8ddc609 to
3424ede
Compare
This comment has been minimized.
This comment has been minimized.
|
@fmease Can this get a re-evaluation with the extra parameter now removed? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
da8d686 to
9f17ce4
Compare
|
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. |
This comment has been minimized.
This comment has been minimized.
| ); | ||
| } else if let Some(sugg) = rustc_hir_analysis::suggest_impl_trait( | ||
| &tcx.infer_ctxt().build(ty::TypingMode::non_body_analysis()), | ||
| tcx.param_env(def_id), |
There was a problem hiding this comment.
Note to myself: We could avoid the query call by accepting the param_env from the caller. Furthermore, we might be able to reuse the InferCtxt from the caller's FnCtxt, although that one is "typeck_for_body", not "non_body_analysis" & maybe the InferCtxt is bricked after writeback?
| tcx: TyCtxt<'tcx>, | ||
| item_def_id: LocalDefId, | ||
| tainted_by_errors: Cell<Option<ErrorGuaranteed>>, | ||
| suppress_placeholder_errors: Cell<bool>, |
There was a problem hiding this comment.
in reply to #153243 (comment)
Without this flag we emit generic error diagnostics per _ in return types plus one diagnostic that actually contains the specialized suggestion with multiple primary highlights.
Using diagnostic stashing + downgrading to delayed bugs I was able go get rid of this flag while preserving the desired behavior. I'll push that later to this PR.
f883e4e to
4f8d202
Compare
Instead of manually keeping track of whether to suppress "bad placeholder" diagnostics via flag `ItemCtxt.suppress_placeholder_errors` just stash the relevant diagnostics beforehand & steal them later on to delay them as bugs.
|
Since I've now contributed substantial changes to this PR in form of 3 commits, I'd like another reviewer to take a look at all the changes. r? @nnethercote maybe? feel free to reassign |
View all comments
This moves checking placeholder types in return types from the
fn_sigquery totypeck.typeckcomputes the return value of the body which is used for error suggestions. This is done to prevent a query cycle betweenfn_sigandtypeck.Currently
fn_sigis marked withcycle_delay_bugto deal with this cycle, but I'm not sure if it's sufficient as we need the query we resume to have aValueimpl, which may not befn_sig.Functions such as
fn foo() -> _will now return asfn foo() -> <error>fromfn_siginstead 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.