-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: Type inference for pattern matching #20020
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
Rust: Type inference for pattern matching #20020
Conversation
97cf8fe to
78a6968
Compare
| class AccessPosition = DeclarationPosition; | ||
|
|
||
| class Access extends StructPat { | ||
| Type getTypeArgument(TypeArgumentPosition apos, TypePath path) { none() } |
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.
Why is this none()?
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.
Since #19847, getTypeArgument is only used for type arguments supplied to functions, not to their defining type, i.e. it is used for foo::<i32>(...) but not for Bar::<i32>::foo(...).
Since explicit type arguments in pattern matching corresponds to the latter, it is none() here, and instead handled in getInferredType below. I'll add some tests for this.
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.
Adding test, of course, revealed some corner cases that were not handled, I'm gonna do that on this PR as well.
| } | ||
|
|
||
| let my_tuple_struct = MyTupleStruct(42, false); | ||
| if let MyTupleStruct(value1, value2) = my_tuple_struct { |
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.
There are a bunch of other pattern types, lets add some tests for those too. No need to fully implement them all, having some MISSING annotations is fine.
Let's also add some more tests with:
- some more deeply nested patterns
- use of
refkeyword:let Some(ref x) = ... - a
uniontype
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 idea with nested patterns and ref; we don't handle union types yet, so I'll not add those 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.
I just asked co-pilot to generate sample test code that covers all the cases for Pat from the ungrammar. This is the result hvitved#8 .
I had to drop the sample code about "const block patterns" because that feature has been removed from Rust.
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.
Could we perhaps add those tests follow-up?
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.
Sure, just wanted to let you know, so you wouldn't write them manually.
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.
Thanks :-)
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.
Very nice! Are you planning to add pattern matching on tuple types as well? Or is that best left as a follow-up?
78a6968 to
29dc76c
Compare
I was thinking we could do that once we even support tuple types. |
29dc76c to
4ab2977
Compare
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.
LGTM
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.
LGTM as well.
I also looked at the DCA run, it shows a pretty substantial improvement on missing call targets, and some new results for rust/access-after-lifetime-ended (probably as we're identifying more sinks correctly). 👍
This PR implements type inference support for pattern matching. For example, we are now able to infer that
xhas typei32inWe also fix an existing bug where we would not distinguish
let x = ...fromlet ref x = ..., as well as handling type arguments supplied to variants, e.g.Option::Some::<i32>(Default::default)(#19847 follow-up).Commit-by-commit review is encouraged.
DCA looks great; call resolution rate up by 2.5 percentage points, no significant performance regressions.