Skip to content

Move abstract const to middle #99000

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
Merged
merged 2 commits into from
Jul 14, 2022
Merged

Move abstract const to middle #99000

merged 2 commits into from
Jul 14, 2022

Conversation

@JulianKnodt
Copy link
Contributor

@JulianKnodt JulianKnodt commented Jul 7, 2022 •
edited
Loading

Moves AbstractConst (and all associated methods) to rustc middle for use in rustc_infer.
This allows for const resolution in infer to use abstract consts to walk consts and check if
they are resolvable.

This attempts to resolve the issue where Foo<{ concrete const }, generic T> is incorrectly marked as conflicting, and is independent from the other issue where nested abstract consts must be resolved.

r? @lcnr

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 7, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2022

Some changes occurred in const_evaluatable.rs

cc @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 7, 2022
@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch 4 times, most recently from 88d4c71 to b189045 Compare July 7, 2022 06:13
@rust-log-analyzer

This comment has been minimized.

Sign in to view
lcnr
lcnr reviewed Jul 7, 2022
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

please move some more stuff, then r=me

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch 3 times, most recently from a2847a2 to 916a91d Compare July 8, 2022 07:33
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch from 916a91d to a6c1672 Compare July 8, 2022 08:38
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Jul 8, 2022

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

@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch from a6c1672 to 22b7e63 Compare July 9, 2022 09:35
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch from 22b7e63 to 90887d4 Compare July 9, 2022 09:48
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch from 90887d4 to 5e93508 Compare July 9, 2022 18:49
@bors
Copy link
Collaborator

bors commented Jul 9, 2022

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

@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch from 5e93508 to 3d1228d Compare July 9, 2022 20:49
@BoxyUwU
Copy link
Member

BoxyUwU commented Jul 11, 2022

doesnt seem like this PR has any changes to rustc_infer to solve the bug this PR is talking about.

lcnr
lcnr reviewed Jul 11, 2022
View reviewed changes
compiler/rustc_middle/src/ty/abstract_const.rs Outdated
Comment on lines 117 to 137
Copy link
Contributor

@lcnr lcnr Jul 11, 2022

Choose a reason for hiding this comment

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

it feels weird to have this be a query which is provided in rustc_trait_selectioneven though it's fully defined in rustc_middle.

I guess we actually want to keep try_unify_abstract_const and the ConstUnifyCtxt in rustc_trait_selection for now?

Copy link
Member

@BoxyUwU BoxyUwU Jul 11, 2022

Choose a reason for hiding this comment

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

i feel like this should be in rustc_trait_selection still since it seems reasonable for try_unify to want to be able to unify inference vars eventaully- although "eventually" we probably want to remove try_unify_abstract_consts so I dont know how important this is

Copy link
Contributor

@lcnr lcnr Jul 14, 2022

Choose a reason for hiding this comment

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

still relevant

@JulianKnodt
Copy link
Contributor Author

JulianKnodt commented Jul 12, 2022

@BoxyUwU oops I initially made this PR to fix the bug, but turns out I would have to move this to rustc_middle::ty, and doing both seemed to mix two different concerns

@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch from 3d1228d to e612e26 Compare July 12, 2022 02:21
@rust-log-analyzer

This comment has been minimized.

Sign in to view
lcnr
lcnr reviewed Jul 12, 2022
View reviewed changes
lcnr
lcnr reviewed Jul 12, 2022
View reviewed changes
lcnr
lcnr reviewed Jul 12, 2022
View reviewed changes
klensy
klensy reviewed Jul 12, 2022
View reviewed changes
@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch 2 times, most recently from f9d9386 to de618fb Compare July 13, 2022 04:32
@JulianKnodt
Copy link
Contributor Author

JulianKnodt commented Jul 13, 2022 •
edited
Loading

It seems that it works just by attempting the identity substs without actually walking the abstract const,
so I removed the HasTyError variant from FailureKind (although it might make sense to have it in there otherwise).

lcnr
lcnr reviewed Jul 13, 2022
View reviewed changes
@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch from de618fb to c8fd0fe Compare July 13, 2022 17:25
lcnr
lcnr reviewed Jul 14, 2022
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

there's still a final nit, and then r=me ❤️ thanks a lot

compiler/rustc_middle/src/ty/abstract_const.rs Outdated
Comment on lines 117 to 137
Copy link
Contributor

@lcnr lcnr Jul 14, 2022

Choose a reason for hiding this comment

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

still relevant

@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch from c8fd0fe to 297e23a Compare July 14, 2022 06:45
@lcnr
Copy link
Contributor

lcnr commented Jul 14, 2022

@bors delegate+

@bors
Copy link
Collaborator

bors commented Jul 14, 2022

✌️ @JulianKnodt can now approve this pull request

@JulianKnodt JulianKnodt force-pushed the allow_resolve_no_substs branch from 297e23a to 20fb8ab Compare July 14, 2022 09:01
@lcnr
Copy link
Contributor

lcnr commented Jul 14, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 14, 2022

📌 Commit 20fb8ab has been approved by lcnr

It is now in the queue for this repository.

@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 Jul 14, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Jul 14, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2022
@bors
Auto merge of rust-lang#99242 - Dylan-DPC:rollup-34bqdh8, r=Dylan-DPC
74621c7 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#98072 (Add provider API to error trait)
 - rust-lang#98580 (Emit warning when named arguments are used positionally in format)
 - rust-lang#99000 (Move abstract const to middle)
 - rust-lang#99192 (Fix spans for asm diagnostics)
 - rust-lang#99222 (Better error message for generic_const_exprs inference failure)
 - rust-lang#99236 (solaris: unbreak build on native platform)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ecae3d7 into rust-lang:master Jul 14, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 14, 2022
@JulianKnodt JulianKnodt deleted the allow_resolve_no_substs branch July 14, 2022 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BoxyUwU BoxyUwU BoxyUwU left review comments

@lcnr lcnr lcnr left review comments

+1 more reviewer

@klensy klensy klensy left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lcnr lcnr

Labels

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

None yet

Milestone

1.64.0

Development

Successfully merging this pull request may close these issues.

8 participants

@JulianKnodt @rustbot @rust-log-analyzer @bors @BoxyUwU @lcnr @klensy @rust-highfive