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.

Sign up for GitHub

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

Jump to bottom

Continue work on associated const equality #93285

Merged
merged 4 commits into from
Feb 2, 2022
Merged

Continue work on associated const equality #93285

merged 4 commits into from
Feb 2, 2022

Conversation

JulianKnodt
Copy link
Contributor

@JulianKnodt JulianKnodt commented Jan 25, 2022 •
edited
Loading

This actually implements some more complex logic for assigning associated consts to values.
Inside of projection candidates, it now defers to a separate function for either consts or
types. To reduce amount of code, projections are now generic over T, where T is either a Type or
a Const. I can add some comments back later, but this was the fastest way to implement it.

It also now finds the correct type of consts in type_of.

The current main TODO is finding the const of the def id for the LeafDef.

Right now it works if the function isn't called, but once you use the trait impl with the bound it fails inside projection.
I was hoping to get some help in getting the &'tcx ty::Const<'tcx>, in addition to a bunch of other todo!()s which I think may not be hit.

r? @oli-obk

Updates #92827

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 25, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 25, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jan 25, 2022

☔ The latest upstream changes (presumably #93095) made this pull request unmergeable. Please resolve the merge conflicts.

@JulianKnodt
Copy link
Contributor Author

Hm, after naively adding in Term instead of being generic over Ty and Const, I feel pretty strongly that it makes a lot of sense for now to special case the few branches where const is hit. If you feel pretty strongly that it should be a term, I can still do that, but I do think it would be a better idea to have it not be an enum.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jan 27, 2022

☔ The latest upstream changes (presumably #93352) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk
oli-obk requested changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Yea, I like this a lot more than the previous version, thanks for trying it out!

You accidentally included a cargo submodule update.

compiler/rustc_trait_selection/src/traits/project.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/project.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/project.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/project.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/project.rs Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/project.rs Outdated Show resolved Hide resolved
compiler/rustc_traits/src/normalize_projection_ty.rs Show resolved Hide resolved
src/test/ui/associated-consts/assoc-const.rs Show resolved Hide resolved
compiler/rustc_infer/src/infer/at.rs Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Jan 27, 2022

Hm, after naively adding in Term instead of being generic over Ty and Const, I feel pretty strongly that it makes a lot of sense for now to special case the few branches where const is hit. If you feel pretty strongly that it should be a term, I can still do that, but I do think it would be a better idea to have it not be an enum.

Hmm... can you point me to the source of examples that you think are unreachable for constants (or is it just those that now have .ty().unwrap() added in this PR?

@JulianKnodt
Copy link
Contributor Author

It's mostly the points where there are ty().unwrap()s. I also feel it's both safer in terms of previous code still working, and also more efficient since it won't have to do pattern matching and require extra storage in the cache. I do realize it is significant code bloat tho, so I can avoid it, it might just be that my usage of golang has been getting to my brain

@JulianKnodt JulianKnodt force-pushed the const_eq_2 branch 3 times, most recently from 8bda326 to 2d7af2d Compare January 27, 2022 14:40
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Feb 1, 2022

📌 Commit 78fb74a has been approved by JulianKnodt

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2022
@JulianKnodt
Copy link
Contributor Author

@bors r+
r? @oli-obk

@bors
Copy link
Contributor

bors commented Feb 1, 2022

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Feb 1, 2022

📌 Commit 78fb74a has been approved by JulianKnodt

@JulianKnodt
Copy link
Contributor Author

I'm not sure if that's how it's done but I hope it's fine?

@mati865
Copy link
Contributor

mati865 commented Feb 1, 2022 •
edited
Loading

@JulianKnodt it's @bors r=oli-obk

@bors
Copy link
Contributor

bors commented Feb 1, 2022

@mati865: 🔑 Insufficient privileges: Not in reviewers

@oli-obk
Copy link
Contributor

oli-obk commented Feb 1, 2022

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 1, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Feb 1, 2022

@bors r+

Sorry about the lack of instructions.

@bors
Copy link
Contributor

bors commented Feb 1, 2022

📌 Commit 78fb74a has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2022
@JulianKnodt
Copy link
Contributor Author

my bad, I should've asked before just doing

@bors
Copy link
Contributor

bors commented Feb 1, 2022

⌛ Testing commit 78fb74a with merge 1ea4851...

@bors
Copy link
Contributor

bors commented Feb 2, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 1ea4851 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 2, 2022
@bors bors merged commit 1ea4851 into rust-lang:master Feb 2, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 2, 2022
This was referenced Feb 2, 2022
Closed
Merged
Merged
@JulianKnodt JulianKnodt deleted the const_eq_2 branch February 2, 2022 02:41
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1ea4851): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@JulianKnodt JulianKnodt mentioned this pull request Feb 8, 2022
Open
8 tasks
@Badel2 Badel2 mentioned this pull request Feb 9, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk requested changes

Assignees

@oli-obk oli-obk

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Associated const equality (ACE)
Status: Done
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
8 participants
@JulianKnodt @rust-log-analyzer @bors @oli-obk @rust-timer @mati865 @rust-highfive @rustbot