generic_const_args: allow paths to non type consts#155341
generic_const_args: allow paths to non type consts#155341khyperia wants to merge 1 commit intorust-lang:mainfrom
Conversation
|
changes to the core type system cc @lcnr Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred in engine.rs, potentially modifying the public API of cc @lcnr Some changes occurred in cc @BoxyUwU HIR ty lowering was modified cc @fmease changes to the core type system cc @lcnr |
There was a problem hiding this comment.
cool ✨ two big picture things:
- you've changed
evaluate_constto take an unevaluated const instead of aty::Const. iirc we talked about doing this to make IACs work but since we've dropped support for them can those changes be reverted? - if it would be kinda chill for you to do can you make the change of adding fields to
AliasTermKind's variants be in a separate commit so it's easier to review just the "meaningful" stuff? If it's non-trivial/difficult it's fine to keep it as is 🤔
cd8a2ae to
53e96d1
Compare
yeah I guess, it just makes me sad to revert, the change seems nice :c :P - I've stashed the work so can maybe done sometime in the future
done! edit: ack, the split wasn't fully clean, there's a swap from edit: ok, pushed, moving the change to the other commit
I think the |
53e96d1 to
bc46b88
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I'm not super fussy about the is_type_const field I guess. It probably doesn't really change much about how much effort it'll be for one of you or waffle to fix things up between this and #155392.
I think this PR basically just needs some more tests at this point. I'm thinking off the top of my head.
- generic uses of free consts, e.g.
FREE<N>so it can't just be immediately evaluated - equality rules of non-type consts, so for both free and associated consts we should have tests that:
ALIAS1andALIAS2are equal if they evalaute to the same value (and aren't if they don't).ALIAS<N>is only equal to itself and not other aliases with the same body
then it should be good to go
|
oh also can you link to the tracking issue for gca in the PR description |
bc46b88 to
1103290
Compare
|
This PR was rebased onto a different main 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. |
| // Perhaps we could split EvaluateConstErr::HasGenericsOrInfers into HasGenerics | ||
| // and HasInfers or something, and make evaluate_const_and_instantiate change | ||
| // its behavior based on that, rather than it checking `has_non_region_infer`. | ||
| let target_args = ecx.resolve_vars_if_possible(target_args); |
There was a problem hiding this comment.
@BoxyUwU this is annoying and gross (see HACK comment), resolve_vars_if_possible is called twice on target_args. I could maybe clean this up in a follow-up PR? Or I could gut/refactor in this PR. I would slightly prefer doing it in a follow-up, but, let me know!
There was a problem hiding this comment.
we probably should split EvaluateConstErr in two eventually yeah.
Can you move this resolve_vars_if_possible into evaluate_const_and_instantiate_normalizes_to_term. Right now it kind of detracts from understanding the high level logic of how normalization works
ce2009d to
556ad2f
Compare
|
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
do we have a test which would expose the "normalizing free alias to an infer var resulting in MIR typeck ICEs"? would be nice to add that revisioned on both old/new solver so we can show new solver working and if we allow GCA with old solver we know it also works
| // - the fresh vars are then used as target_args | ||
| // we need the actual args to run const eval, so we need to actually do the `eq` | ||
| // and figure out the args, so, call try_evaluate_added_goals | ||
| ecx.try_evaluate_added_goals()?; |
There was a problem hiding this comment.
do you have a link to a test that requires this?
I'm wondering if it make sense to move this into evaluate_const_and_instantiate_normalizes_to_term. I don't think evaluating free consts would need this but I expect IACs will wind up needing it too. And generally it just feels kind of more principled to do so 🤔/would make the code nicer to read I think.
There was a problem hiding this comment.
I could have sworn this was necessary in an earlier version of the code, but I can't for the life of me get a repro where it's necessary now. Perhaps it was only necessary with IACs, but not with projected.
I'm unable to find a repro with this line commented out:
but that line already does what the line in my PR is attempting to do, so I think it's fine to delete this line from my PR.
| return; | ||
| } | ||
|
|
||
| // Require the new solver with GCA, because the old solver does not implement GCA correctly. |
There was a problem hiding this comment.
| // Require the new solver with GCA, because the old solver does not implement GCA correctly. | |
| // Require the new solver with GCA, because the old solver can't implement GCA correctly | |
| // as it does not support normalization obligations for free and inherent consts. |
Yes,
what do you mean by that? like, add a |
yeah, that way it'll change from that to incorrectly passing or something if we allow the feature in the old solver |
View all comments
tracking issue: #151972
Non type consts should be usable in the type system in
feature(generic_const_args). These are directly plugged into the constant evaluator, unlike type consts, which are attempted to be reasoned about by the type system.Inherent associated constants are not supported at this time, due to complications around how generic arguments are represented for them (it's currently a mess). The mess is being cleaned up (e.g. #154758), so instead of trying to hack support in before the refactoring is done, let's just wait to be able to implement it more cleanly.
r? @BoxyUwU