Move checking placeholder types in return types to typeck#153243
Move checking placeholder types in return types to typeck#153243Zoxc wants to merge 3 commits intorust-lang:mainfrom
typeck#153243Conversation
|
HIR ty lowering was modified cc @fmease |
rustbot
added
S-waiting-on-review
|
rustbot has assigned @dingxiangfei2009. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
|
||
| #[instrument(level = "debug", skip(tcx), ret)] | ||
| fn fn_sig(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::EarlyBinder<'_, ty::PolyFnSig<'_>> { | ||
| /// We'll emit an error about the suggestable infer ty in `typeck` query if this is true. |
There was a problem hiding this comment.
Sorry, what is true here by this?
|
|
||
| _ => None, | ||
| }; | ||
| sig.and_then(|sig| sig.decl.output.is_suggestable_infer_ty()) |
There was a problem hiding this comment.
Looks like we care only about if suggestable infer type exists downstream. Would you like to switch it to return just a boolean? Or do you have a plan for this function?
Nevermind, I did not search hard enough.
|
Thank you for contribution! 🙇 @bors r+ rollup=always |
rust-bors
bot
added
S-waiting-on-bors
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.
|
Sorry, I'm gonna unapprove this for now due to #153243 (comment) (question) and #153243 (comment) (semi-actionable concern) @bors r- |
rust-bors
bot
added
S-waiting-on-author
It unhides unrelated cycle errors, so I kept it out here to keep the PR smaller. |
5 participants
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.