Improve suggestions for closure parameters #97262

kuecks · 2022-05-22T02:03:42Z

Currently, the type inference for closures does not handle _ in the arguments very well. It works specifically for the case of having something like |x: _|, but not if the _ has any indirection like |x: &_| or |x: Vec<_>| etc. Instead, the compiler assumes the issue has to do with the return type annotation, leading to some silly suggestions. The motivating example was this:

fn main() {
    let x = |a: &_| -> i32 { 1 };
}

Which currently gives a silly suggestion:

error[[E0282]](https://doc.rust-lang.org/stable/error-index.html#E0282): type annotations needed for the closure `fn(&_) -> i32`
 --> src/main.rs:2:13
  |
2 |     let x = |a: &_| -> i32 { 1 };
  |             ^^^^^^^^^^^^^^^^^^^^ cannot infer type for closure `[closure@src/main.rs:2:13: 2:33]`
  |
help: give this closure an explicit return type without `_` placeholders
  |
2 |     let x = |a: &_| -> i32 { 1 };
  |                        ~~~

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

(playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=cb2ba62db3f94f09c2f5dc83217e877e).

This diff changes the diagnostic so that it now looks for any unresolved input parameters before and suggest specifying those before defaulting to the return value.

r? @compiler-errors

rust-highfive · 2022-05-22T02:03:45Z

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

Please see the contribution instructions for more information.

kuecks

There is still the old logic which only works for |x: _|. This code is still called in that case and I didn't really know how to unify them without potentially breaking other things (though I didn't try that hard). I'm a huge github noob so idk how to comment on that line... it's line 737 in compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs in the new version, 707 on the old version

compiler-errors · 2022-05-22T02:43:20Z

compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs

+                    for arg in fn_sig.inputs().skip_binder().iter() {
+                        if let ControlFlow::Break((ty, _)) = visitor.visit_ty(*arg) {
+                            let mut inner = self.inner.borrow_mut();
+                            let ty_vars = &inner.type_variables();


Can you inline this variable?

I cannot due to the lifetime of the borrow_mut:

error[E0716]: temporary value dropped while borrowed --> compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs:626:42 | 626 | ... let ty_vars= self.inner.borrow_mut().type_variables(); | ^^^^^^^^^^^^^^^^^^^^^^^ - temporary value is freed at the end of this statement | | | creates a temporary which is freed while still in use ... 629 | ... let var_origin = ty_vars.var_origin(ty_vid); | -------------------------- borrow later used here | = note: consider using a `let` binding to create a longer lived value

sorry, I mean inlining ty_vars -- like let var_origin = self.inner.borrow_mut().type_variables().var_origin(ty_vid)?

compiler-errors

Thanks for the PR, @kuecks.

There is still the old logic which only works for |x: _|.

Do you mean |_|? Otherwise I'm not sure what you're talking about. Anywho, it's fine to keep this separate from that I think.

Do you mind unifying all of your UI tests into one file? This adds quite a few of the same "kind" of check, so I think it's fine to group them.

compiler-errors · 2022-05-22T02:43:59Z

compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs

+                            };
+                            err.span_label(
+                                ty_span,
+                                "consider giving this closure parameter an explicit type without `_` placeholders",


I prefer a more direct phrasing:

Suggested change

"consider giving this closure parameter an explicit type without `_` placeholders",

"give this closure parameter an explicit type instead of `_`",

I think it reads a little oddly when the type is something like Vec<_> since the way you have it written, it sounds like we think the entire closure parameter currently has the type _ (and i copied the wording from the pre-existing return value case). But I don't have super strong feelings either way

makes sense, then can you at least s/consider giving/give?

compiler-errors · 2022-05-22T02:48:29Z

compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs

+                            );
+                            // We don't want to give the other suggestions when the problem is a
+                            // closure input type.
+                            err.span_label(


Do you think we need to point out the span of the closure? I kinda like the rendered suggestion of unspecified-parameter.rs, do you know why it renders differently?

Yeah it renders differently because that's the old code path. I'll see if I can change it so they are uniform, I assume it won't be that hard

kuecks · 2022-05-22T22:21:02Z

Do you mean |_|?

I suppose yes? Both of these forms: |_| { ... } and |x: _| { ... } will execute the old path, but something like |x: &_| { ... } will execute the new path in this PR. That is, the old code only handled the closure parameter type begin exactly _, not an arbitrary type containing unresolved _ inside

I actually can't merge the tests because we only emit diagnostics for one at a time. It prefers the |x: _|/|_| case, then the other cases in first come first serve order:

fn main() {
    let a = |a: &_| -> i32 { 1 };
    let b = |a: _| -> i32 { 1 };
    let c = |a: Vec<_>| -> i32 { 1 };
}

ekuecks@ekuecks-mbp test_diag % rustc +stage1 main.rs
error[E0282]: type annotations needed
 --> main.rs:3:14
  |
3 |     let b = |a: _| -> i32 { 1 };
  |              ^ consider giving this closure parameter a type

error: aborting due to previous error

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

fn main() {
    let a = |a: &_| -> i32 { 1 };
    let c = |a: Vec<_>| -> i32 { 1 };
}

ekuecks@ekuecks-mbp test_diag % rustc +stage1 main.rs
error[E0282]: type annotations needed for the closure `fn(&_) -> i32`
 --> main.rs:2:13
  |
2 |     let a = |a: &_| -> i32 { 1 };
  |             ^^^^^-^^^^^^^^^^^^^^
  |             |    |
  |             |    consider giving this closure parameter an explicit type without `_` placeholders
  |             cannot infer type for closure `[[email protected]:2:13: 2:33]`

error: aborting due to previous error

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

compiler-errors · 2022-05-22T22:31:27Z

I actually can't merge the tests because we only emit diagnostics for one at a time.

That's weird, lol. So some errors are being emitted at a different stage than the others.

Alright, well you should at least be able to unify the new cases you added into one file, right?

kuecks · 2022-05-22T22:42:15Z

Alright, well you should at least be able to unify the new cases you added into one file, right?

No, even the new ones, it only emits one of them (the first one) :(

compiler-errors · 2022-05-22T22:49:25Z

compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs

+                    let mut visitor = UnresolvedTypeFinder::new(self);
+                    for arg in fn_sig.inputs().skip_binder().iter() {
+                        if let ControlFlow::Break((ty, _)) = visitor.visit_ty(*arg) {


Can you use InferCtxt::unresolved_type_vars instead of invoking UnresolvedTypeFinder yourself?

Also, you probably don't need to use type_variables() at all -- you can probably just take the Option<Span> that's returned from unresolved_type_vars and do .unwrap_or(ty_span).

The UnresolvedTypeFinder already tries to look up the var_origin of the ty_vid in question, so you're doing a bit of duplicated work below.

r=me after this

So for some reason, the span is always None when I do this (which is afaict basically the same as what I already have, note that I am currently ignoring the span). I can convert to this and then when it's None fallback to the current implementation?

Presumably there needs to be another case here? https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_infer/infer/resolve.rs.html#139 I think that would probably be a separate PR but lmk if you want me to take a stab at it

Oh, no actually you should do this:

1.) Edit the UnresolvedTypeFinder implementation of TypeVisitor to return the span always, but also get it to return the TypeVariableOriginKind (the logic is here) too. The BreakTy should then be Ty, Span, TypeVariableOriginKind.

2.) Then edit the existing logic that uses UnresolvedTypeFinder to now be conditional (I think it's just used in generator_interior.rs) on that TypeVariableOriginKind being of type TypeVariableOriginKind::TypeParameterDefinition.

Edit: actually, the signature will be (Ty, Option<Span>, TypeVariableOriginKind). We still probably need to unwrap_or on the span to deal with when it's None, but I think you should fall back to the outer span of the closure. You shouldn't need to inspect infcx.inner in your logic.

kuecks · 2022-05-23T04:50:25Z

Obsoleted by #97302

rust-highfive assigned compiler-errors May 22, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 22, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2022

kuecks commented May 22, 2022

View reviewed changes

compiler-errors reviewed May 22, 2022

View reviewed changes

compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2022

compiler-errors reviewed May 22, 2022

View reviewed changes

kuecks force-pushed the closure_parameters branch from eb5ed67 to 7dab378 Compare May 22, 2022 22:50

Improve suggestions for closure parameters

16273eb

kuecks force-pushed the closure_parameters branch from 7dab378 to 16273eb Compare May 23, 2022 02:19

kuecks closed this May 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve suggestions for closure parameters #97262

Improve suggestions for closure parameters #97262

kuecks commented May 22, 2022

rust-highfive commented May 22, 2022

kuecks left a comment

compiler-errors May 22, 2022

kuecks May 22, 2022

compiler-errors May 22, 2022

compiler-errors left a comment •

edited

Loading

compiler-errors May 22, 2022

kuecks May 22, 2022

compiler-errors May 22, 2022

compiler-errors May 22, 2022

kuecks May 22, 2022

kuecks commented May 22, 2022 •

edited

Loading

compiler-errors commented May 22, 2022

kuecks commented May 22, 2022

compiler-errors May 22, 2022 •

edited

Loading

compiler-errors May 22, 2022

kuecks May 23, 2022

kuecks May 23, 2022

compiler-errors May 23, 2022

compiler-errors May 23, 2022 •

edited

Loading

kuecks commented May 23, 2022

	"consider giving this closure parameter an explicit type without `_` placeholders",
	"give this closure parameter an explicit type instead of `_`",

Improve suggestions for closure parameters #97262

Improve suggestions for closure parameters #97262

Conversation

kuecks commented May 22, 2022

rust-highfive commented May 22, 2022

kuecks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuecks commented May 22, 2022 • edited Loading

compiler-errors commented May 22, 2022

kuecks commented May 22, 2022

compiler-errors May 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors May 23, 2022 • edited Loading

Choose a reason for hiding this comment

kuecks commented May 23, 2022

compiler-errors left a comment •

edited

Loading

kuecks commented May 22, 2022 •

edited

Loading

compiler-errors May 22, 2022 •

edited

Loading

compiler-errors May 23, 2022 •

edited

Loading