Skip to content
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

feat: Fix pattern type mismatches for bindings, enable pattern type mismatch diagnostics again #14732

Merged
merged 1 commit into from May 4, 2023

Conversation

HKalbasi
Copy link
Member

@HKalbasi HKalbasi commented May 3, 2023

Reduces pattern type mismatches on self to 4

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2023
/// It also includes the changes that binding mode makes in the type. For example in
/// `let ref x @ Some(_) = None` the result of `type_of_pat` is `Option<T>` but the result
/// of this function is `&mut Option<T>`
pub fn type_of_binding_in_pat(&self, pat: &ast::IdentPat) -> Option<Type> {
Copy link
Member

@Veykril Veykril May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting thing to keep in mind that effectively pattern and binding are not always equal in a sense

@@ -148,7 +148,6 @@ struct Struct {
field: &'static str,
}
fn foo(s @ Struct { field, .. }: &Struct) {}
//^^^^^^^^^^^^^^^^^^^^^^^^ref
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong to me? s is being bound by reference so the ref mode should still be here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think s here is binding by moving the reference itself, not binding by ref the thing inside of reference. This example makes the difference clear:

let t @ T = &&&T; // type is `&&&T`, binding mode is `move`
let (t @ T,) = &&&(T,); // type is `&T`, binding mode is `ref`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, turns out I completely misunderstood how binding modes worked for @ bindings! I think you are right here (at least judging from testing with == to avoid coercions from messing things up).

That also explains the pattern type mismatches now. Thanks for clarifying

@HKalbasi
Copy link
Member Author

HKalbasi commented May 4, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented May 4, 2023

📌 Commit 36c9d5c has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 4, 2023

⌛ Testing commit 36c9d5c with merge 884835e...

@Veykril
Copy link
Member

Veykril commented May 4, 2023

Also, what do tuples have to do with this PR, isn't this fixing incorrect type inference for bindings?

@bors
Copy link
Collaborator

bors commented May 4, 2023

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 884835e to master...

@bors bors merged commit 884835e into rust-lang:master May 4, 2023
10 checks passed
@Veykril Veykril changed the title Fix pattern type mismatch in tuples feat: Fix pattern type mismatches for bindings, enable pattern type mismatch diagnostics again May 4, 2023
@HKalbasi
Copy link
Member Author

HKalbasi commented May 4, 2023

The main change in this PR is that infer_pat now returns the pattern type after adjustments, so if an adjustment happens inside a field in a tuple pattern, it would affect the result tuple type. But after this change type mismatches increased, since in the let (x, y) = &(2, 3) the type of x pattern was &i32 since it was equal to the binding type, so I separated the binding type and the pattern type. It was possible to avoid making this distinction, for example by special casing bindings in infer_pat or something like that, but I thought this way is more correct, and it showed benefit in some of the missing_match_arms tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants