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: type inference for generic associated types #13494

Merged
merged 5 commits into from
Oct 27, 2022
Merged

Conversation

lowr
Copy link
Contributor

@lowr lowr commented Oct 27, 2022

This PR implements type inference for generic associated types. Basically, this PR lowers generic arguments for associated types in valid places and creates Substitutions for them.

I focused on the inference for correct Rust programs, so there are cases where we accidentally manage to infer things that are actually invalid (which would then be reported by flycheck so I deem them non-fatal). See the following tests and FIXME notes on them: gats_with_dyn, gats_with_impl_trait.

The added tests are rather arbitrary. Let me know if there are cases I'm missing or I should add.

Closes #9673

@lowr
Copy link
Contributor Author

lowr commented Oct 27, 2022

I've run analysis-stats on self and std, confirmed it doesn't crash and the numbers don't change.

Copy link
Member

@flodiebold flodiebold left a comment

Choose a reason for hiding this comment

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

LGTM, surprisingly few changes. Maybe we can simplify the lowering code in the future.

@bors d+

let a = *p;
}
"#,
expect![[r#"
Copy link
Member

Choose a reason for hiding this comment

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

in general, I want to get rid of the expect tests and move to check etc., because they're more flexible and it's a bit clearer what we're actually testing (vs. what's just incidental code)

@flodiebold
Copy link
Member

@bors d+

@flodiebold
Copy link
Member

Forgot that old bors doesn't understand d+ 😅

Anyway, it's probably fine to just merge anyway.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 27, 2022

📌 Commit 5fc18ad has been approved by flodiebold

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 27, 2022

⌛ Testing commit 5fc18ad with merge c6e1e17...

@lowr
Copy link
Contributor Author

lowr commented Oct 27, 2022

I think bors doesn't have d+ but delegate+? I can replace the expect test with appropriate one in a few hours (a follow-up PR would also suffice though).

@bors
Copy link
Contributor

bors commented Oct 27, 2022

☀️ Test successful - checks-actions
Approved by: flodiebold
Pushing c6e1e17 to master...

@bors bors merged commit c6e1e17 into rust-lang:master Oct 27, 2022
@flodiebold
Copy link
Member

Yes, it's delegate+. I didn't think it was really necessary to hold up the PR on that though 😄

@lowr
Copy link
Contributor Author

lowr commented Oct 27, 2022

Ahh I see. Thanks so much for your quick review by the way, I'm really relieved to get this merged before GATs come to stable!

bors added a commit that referenced this pull request Oct 29, 2022
Clean up tests and add documentation for GATs related stuff

This is a follow-up PR for #13494.

- addresses #13494 (comment)
- documents the ordering constraint on `Binders` and `Substitution` (which is not really follow-up for the previous PR, but it was introduced to support GATs and I strongly feel it's worth it)
@lnicola
Copy link
Member

lnicola commented Oct 31, 2022

image

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 6, 2022
Auto merge of #13505 - lowr:feat/gats, r=flodiebold

Clean up tests and add documentation for GATs related stuff

This is a follow-up PR for #13494.

- addresses rust-lang/rust-analyzer#13494 (comment)
- documents the ordering constraint on `Binders` and `Substitution` (which is
  not really follow-up for the previous PR, but it was introduced to support
  GATs and I strongly feel it's worth it)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GAT support
4 participants