-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Switch next solver to use a specific associated type for trait def id #145377
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
a370dc8
to
a854097
Compare
This comment has been minimized.
This comment has been minimized.
a854097
to
e854390
Compare
☔ The latest upstream changes (presumably #144991) made this pull request unmergeable. Please resolve the merge conflicts. |
e854390
to
9d377f8
Compare
This PR was rebased onto a different master commit! Check out the changes with our |
☔ The latest upstream changes (presumably #145600) made this pull request unmergeable. Please resolve the merge conflicts. |
sorry for not replying to this PR up until now 😅 I am somewhat torn about this and avoided forming a final opinion up until now. I think this is generally useful and I am in favor of this change. I do feel like ideally this should not limited to the solver and we actually have some I feel like the main issue is to actually use these IDs everywhere 😅 I don't know what exactly I want and changing stuff here results in a lot of churn/merge conflicts so yeah, want to take some time to properly sort out my thoughts here, am generally in favor, but don't have the time to do that rn |
@lcnr That's exactly why I opened this PR: so you can see (and I can be sure) that it's not, in fact, a big change. Doing it throughout the compiler might be interesting but it definitely will be a big change, and I don't want to do that. I also didn't create this PR to have better type safety within the solver (although this is nice), but to help rust-analyzer. |
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.
Can you rename TraitSolverLangItem
to SolverLangItem
?
TraitSolverTrait
is hard to read.
Also, rename uses of trait_def_id
to just trait_id
maybe?
then r=me. While I'd like to maybe extend that to rustc, I agree that it doesn't make the trait solver harder to read and it's useful for r-a
9d377f8
to
43692ad
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
@lcnr Did what you requested, except renaming all |
The compiler just puts `DefId` in there, but rust-analyzer uses different types for each kind of item.
43692ad
to
38bd808
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
That must be a spurious failure. |
The compiler just puts
DefId
in there, but rust-analyzer uses different types for each kind of item.See the Zulip discussion. In short, it will be a tremendous help to r-a to use specific associated types, while for the solver and the compiler it's a small change. So I ported
TraitId
, as a proof of concept and it's also likely the most impactful.r? types