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

Add consts for AssocConst w/ body & ignore regions in is_satisfied_from_param_env #106965

Closed
wants to merge 2 commits into from

Conversation

JulianKnodt
Copy link
Contributor

Add in the ability to unify associated consts and ignore regions when unifying consts.
Attempting to only ignore regions lead to an ice, so I had to add that the associated const would be unified with it.

Handles one of the minimized test cases in #105821, but not sure if it fixes everything.

Fixes #106423 (but I think it may break the compilation of some crates, as I'm not sure if we were previously permitting that behavior?)

r? @BoxyUwU

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 17, 2023
@JulianKnodt JulianKnodt marked this pull request as ready for review January 17, 2023 04:17
@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2023

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occured in rustc_ty_utils::consts.rs

cc @BoxyUwU

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 24, 2023

I don't think these changes actually fix the problem. There's no infer var in the linked issue, we're just unable to evaluate a ConstKind::Expr. The reason it no longer ICEs with these changes is because you emitted an error which is stopping us from getting to monomorphization which is where the ICE happens.

@JulianKnodt
Copy link
Contributor Author

Yes, but I'm not sure whether this was ever intended for this to compile or not. I went back through a few nightly builds over the past 2 years, and it seems it's intermixed with ICE-ing, compiling, and not compiling. Do we intend to handle AssocConsts as Exprs? If we treat them as Exprs, then since we cannot handle all operations it makes sense that this shouldn't compile. If we do want it to compile, then this should not be what is merged in at the end.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 26, 2023

I am not sure what you mean by treating an assoc const as an Expr. I would expect <T as Trait>::ASSOC to be a ConstKind::Unevaluated that can potentially be normalized to a ConstKind::Expr if the actual value in the impl would be valid (i.e. not hit the errors in thir_abstract_const)

@JulianKnodt
Copy link
Contributor Author

Right, but I was wondering whether we should always treat AssocConsts as Exprs? What I suspect is that we were previously trying to convert them into AbstractConsts (prior to the addition of Expr), and ignoring them because it was not supported. Do AssocConsts support arbitrary expressions, and if so what defines if we try to convert it into an Expr?

Overall, I think I'm just not sure what the intended output behavior is. Should this compile at all?

@bors
Copy link
Contributor

bors commented Feb 22, 2023

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

@pnkfelix
Copy link
Member

pnkfelix commented Mar 2, 2023

I think this is best owned by T-types rater than T-compiler

@rustbot label: +T-types -T-compiler

@rustbot rustbot added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 2, 2023
@KittyBorgX
Copy link
Member

@rustbot author
@JulianKnodt Hello! There are some merge conflicts, can you resolve them?
The reviewers can then leave a review and your PR should be good to go.

Once you sort the conflicts, type in @rustbot review here to change the label to S-waiting-on-review

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2023
@JulianKnodt
Copy link
Contributor Author

I'm probably just going to close this, as I think how I thought this should be resolved is different from what the maintainers expected, and I don't really know how to proceed further. Thanks for the reminder!

@JulianKnodt JulianKnodt closed this May 1, 2023
@JulianKnodt JulianKnodt deleted the concat105821 branch May 4, 2023 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: collection encountered polymorphic constant: UnevaluatedConst
6 participants