-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Improve error message for ambiguous numeric types in closure parameters #147577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
r? @davidtwco rustbot has assigned @davidtwco. Use |
err.span_suggestion_verbose( | ||
pat.span.shrink_to_hi(), | ||
"specify the type in the closure argument list", | ||
format!(": &{concrete_type}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also suggests &
for PatKind::Ref(_, Mut)
when it should probably suggest &mut
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't account for nested refs like &&x
(should suggest : &&T
not : &T
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Added a check in dbc7327
(#147577)
(I cannot come up with a mutable test case which is caught by E0689 🤔)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot come up with a mutable test case which is caught by E0689
[&mut 0].into_iter().map(|&mut x| x.pow(0))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it isn't caught by E0689 but other codes like E0282 (I guess other inference errors are stronger here), so the suggestion here wouldn't be showed.
} | ||
// For closure parameters with reference patterns (e.g., |&v|), suggest the type annotation | ||
// on the pattern itself, e.g., |&v: &i32| | ||
(FileName::Real(_), Node::Pat(pat)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(preexisting) Matching on the file name presumably to suppress this suggestion if this comes from a macro expansion (FileName::MacroExpansion
) is wild and simply incorrect, lol. E.g., it means it never suggests anything if the source is STDIN (FileName::Anon
) like for printf '/* ... */' | rustc -
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, but addressing it in this PR feels like out-of-scope, so added FIXME: 632e759
(#147577)
Or do you want me to resolve it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do you want me to resolve it here?
No, a FIXME is fine 👍
if let Node::Pat(binding_pat) = self.tcx.hir_node(hir_id) | ||
&& let hir::PatKind::Binding(..) = binding_pat.kind | ||
&& let Node::Pat(parent_pat) = parent_node | ||
&& matches!(parent_pat.kind, hir::PatKind::Ref(..)) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Food for thought (I don't have time to investigate this): If the unsolved inference variable is not an integer/float one ({integer}
/ {float}
) but a normal one, then need_type_info
/ InferSourceKind
is able to figure out the type annotation for various kinds of patterns.
E.g.,
[].into_iter().filter(|&x| x.pow(0)); // suggests `: &T` on master
[].into_iter().filter(|(x,)| x.pow(0)); // suggests `: &(T,)` on master
So I guess ideally we would be modifying needs_type_info
to properly deal with integer/float ty vars since that seems to have all the infrastructure in place already (unless they really follow a different code path in which case my suggestion would be nonsensical).
eb3ad28
to
632e759
Compare
Closes #147312