Lazy normalization of constants #67890
Conversation
JohnTitor
added
the
S-waiting-on-review
Centril
added
F-const_generics
src/librustc_session/options.rs
Outdated
| @@ -948,4 +948,6 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, | |||
| (such as entering an empty infinite loop) by inserting llvm.sideeffect"), | |||
| deduplicate_diagnostics: Option<bool> = (None, parse_opt_bool, [UNTRACKED], | |||
| "deduplicate identical diagnostics"), | |||
| lazy_normalization: bool = (false, parse_bool, [UNTRACKED], | |||
| "lazily evaluate constants (experimental)"), | |||
There was a problem hiding this comment.
-Z flags are less convenient to experiment with, e.g. on the playground (to find bugs and whatnot), so why not use a feature gate?
There was a problem hiding this comment.
It might be useful to have both. A -Z flag will allow turning on lazy normalization for an entire crate graph, via RUSTFLAGS
There was a problem hiding this comment.
My thinking was that this doesn't just affect code that uses the const generic feature, so having a -Z flag would be useful to test non-feature gated crates. Though enabling it also with a feature gate would also be good, as I had to partially already do that to get the impls inside the standard library that use const generics to work.
There was a problem hiding this comment.
tidy is complaining that there is no gate test for this feature, how would I write a gate test for this? Detecting when this feature is needed is very hard, as evident by the previously poor diagnostics/ICEs.
Also is there a way to enable a feature via RUSTFLAGS? I don't want to check for both whether the feature is enabled and the debugging option, every time I check if lazy normalization is enabled.
|
☔ The latest upstream changes (presumably #67917) made this pull request unmergeable. Please resolve the merge conflicts. |
JohnCSimon
added
S-waiting-on-author
|
Ping from triage: @Skinny121 can you please address the merge conflicts? |
varkor
added
S-blocked
|
This is blocked on #67717. |
|
Blocked on #68505, now instead. |
BenLewis-Seequent
force-pushed
the
lazy-norm-anon-const
branch
from
87e8a2a
to
52c777d
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #69247) made this pull request unmergeable. Please resolve the merge conflicts. |
|
It looks like this is unblocked now, since #68505 is now merged. Edit: Nevermind me, I see that commits are already happening. I didn't see those. |
|
@rustbot modify labels: -S-blocked +S-waiting-on-author |
rustbot
added
S-waiting-on-author
|
Is this blocked on something new? |
joelpalmer
removed
the
S-waiting-on-author
|
@mati865 There you go, if you need this again PM me on Discord/Zulip. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
FTR here is backup of original commits: master...mati865:lazy-norm-anon-const-bck |
|
FWIW GitHub also remembers it, to some extent, see #67890 (comment).
|
|
@eddyb AFAIK dropped commits are garbage collected after some time. Better safe than sorry. |
|
☔ The latest upstream changes (presumably #69926) made this pull request unmergeable. Please resolve the merge conflicts. |
|
It looks like another rebase is needed and tidy is failing: |
joelpalmer
added
S-waiting-on-author
|
Will try and get this ready again |
|
I managed to fix all problems (at least locally) in https://github.com/lcnr/rust/tree/lazy-norm! Can someone force push my branch onto this PR? The ICE was caused by accidentally removed |
|
This now fixes and tests #61935, which solves
|
|
@nikomatsakis: ping for review (#67890 (comment)). |
| @@ -603,8 +606,8 @@ where | |||
| b = self.infcx.shallow_resolve(b); | |||
| } | |||
|
|
|||
| match b.val { | |||
| ty::ConstKind::Infer(InferConst::Var(_)) if D::forbid_inference_vars() => { | |||
| match (a.val, b.val) { | |||
There was a problem hiding this comment.
Nit: seems like a useful diff?
There was a problem hiding this comment.
Not quite sure what you mean here 🤔 This does not change the actual behavior afaict which is why I removed this change in #71973.
| @@ -408,6 +408,7 @@ symbols! { | |||
| label_break_value, | |||
| lang, | |||
| lang_items, | |||
| lazy_normalization_consts, | |||
There was a problem hiding this comment.
(Do we really want a separate feature for this?)
There was a problem hiding this comment.
We already use const generics in libcore/libstd, so it might be better to not use lazy normalization there for now. I can go ahead and restrict these changes on const_generics instead though.
There was a problem hiding this comment.
We do want two features: lazy normalisation unblocks some issues even without const generics. We probably do always want lazy normalisation enabled when const generics is, though. (But I think this is the current effect anyway?)
There was a problem hiding this comment.
(But I think this is the current effect anyway?)
No, rn lazy normalization is checked separately from const generics, so we need to enable both to use it.
Considering that we still eagerly normalize array lengths, I don't actually know how to use this on stable.
| } | ||
| } | ||
| } | ||
| // FIXME(skinny121) How to report both errors if both produces errors? |
There was a problem hiding this comment.
Hmm so here we report no errors...
There was a problem hiding this comment.
evaluate only returns ProcessResult::Error if const_eval_resolve returns ErrorHandled::Reported afaict. This comment seems outdated, will make this more explicit.
| Err(_) => Ok(EvaluatedToErr), | ||
| } | ||
| } | ||
|
|
||
| ty::Predicate::ConstEquate(c1, c2) => { | ||
| debug!("evaluate_predicate_recursively: equating consts c1={:?} c2={:?}", c1, c2); |
There was a problem hiding this comment.
Hmm we should try to factor a helper to share the code between this and fulfill.rs
There was a problem hiding this comment.
I tried and failed. 😅 the evaluate closures are different enough that my approaches at extracting part of them into a shared function ended up making the code worse :/
| (ty::ConstKind::Unevaluated(..), _) | ||
| if self.tcx.features().lazy_normalization_consts => | ||
| { | ||
| relation.const_equate_obligation(a, b); |
There was a problem hiding this comment.
Until we get the full universe transition happening here, I think we want to check that there are no escaping late-bound regions in these types...oh, hmm, I guess they would be substituted for placeholders already...urgh.
| _ => relate::super_relate_consts(self, c, c), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub trait ConstEquateRelation<'tcx>: TypeRelation<'tcx> { |
There was a problem hiding this comment.
It's not obvious to me at this moment why we have a distinct trait for this
There was a problem hiding this comment.
There are someTypeRelations which don't need fn const_equate_obligation atm, so I ended up adding a bunch of unimplementeds while moving fn const_equate_obligation to TypeRelation.
I slightly prefer keeping the ConstEquateRelation trait for now. Depending on your preference I would drop the last commit of #71973, which merges these two traits. See deaa50e
|
Question: should we close this PR and re-open one from @lcnr's branch, if they're maintaining this? It seems more natural (we can of course add @Skinny121 as co-author on the commits). |
|
Using my branch directly is a lot easier for me at least 😆 I opened #71973 for now. @nikomatsakis not sure how to deal with your already started review here, will try to either fix/answer them here or copy them to my new PR |
|
@lcnr ok, I'll move over to that branch -- I can try to move my comments over |
|
Closing in favor of #71973 |
Lazy normalization of constants (Reprise) Continuation of rust-lang#67890 by @Skinny121. Initial implementation of rust-lang#60471 for constants. Perform normalization/evaluation of constants lazily, which is known as lazy normalization. Lazy normalization is only enabled when using `#![feature(lazy_normalization_consts)]`, by default constants are still evaluated eagerly as there are currently. Lazy normalization of constants is achieved with a new ConstEquate predicate which type inferences uses to delay checking whether constants are equal to each other until later, avoiding cycle errors. Note this doesn't allow the use of generics within repeat count expressions as that is still evaluated during conversion to mir. There are also quite a few other known problems with lazy normalization which will be fixed in future PRs. r? @nikomatsakis fixes rust-lang#71922, fixes rust-lang#71986
Lazy normalization of constants (Reprise) Continuation of rust-lang#67890 by @Skinny121. Initial implementation of rust-lang#60471 for constants. Perform normalization/evaluation of constants lazily, which is known as lazy normalization. Lazy normalization is only enabled when using `#![feature(lazy_normalization_consts)]`, by default constants are still evaluated eagerly as there are currently. Lazy normalization of constants is achieved with a new ConstEquate predicate which type inferences uses to delay checking whether constants are equal to each other until later, avoiding cycle errors. Note this doesn't allow the use of generics within repeat count expressions as that is still evaluated during conversion to mir. There are also quite a few other known problems with lazy normalization which will be fixed in future PRs. r? @nikomatsakis fixes rust-lang#71922, fixes rust-lang#71986
remove duplicate test for rust-lang#61935 Apparently I somehow messed up the issue number in rust-lang#67890 which caused us to add this test twice, both as https://github.com/rust-lang/rust/blob/master/src/test/ui/const-generics/issues/issue-61935.rs and https://github.com/rust-lang/rust/blob/master/src/test/ui/const-generics/lazy-normalization/issue-71922.rs rust-lang#61935 is the actually fixed issue while rust-lang#71922 is still not working, as it depends on lazy norm of repeat expressions
remove duplicate test for rust-lang#61935 Apparently I somehow messed up the issue number in rust-lang#67890 which caused us to add this test twice, both as https://github.com/rust-lang/rust/blob/master/src/test/ui/const-generics/issues/issue-61935.rs and https://github.com/rust-lang/rust/blob/master/src/test/ui/const-generics/lazy-normalization/issue-71922.rs rust-lang#61935 is the actually fixed issue while rust-lang#71922 is still not working, as it depends on lazy norm of repeat expressions
remove duplicate test for rust-lang#61935 Apparently I somehow messed up the issue number in rust-lang#67890 which caused us to add this test twice, both as https://github.com/rust-lang/rust/blob/master/src/test/ui/const-generics/issues/issue-61935.rs and https://github.com/rust-lang/rust/blob/master/src/test/ui/const-generics/lazy-normalization/issue-71922.rs rust-lang#61935 is the actually fixed issue while rust-lang#71922 is still not working, as it depends on lazy norm of repeat expressions
remove duplicate test for rust-lang#61935 Apparently I somehow messed up the issue number in rust-lang#67890 which caused us to add this test twice, both as https://github.com/rust-lang/rust/blob/master/src/test/ui/const-generics/issues/issue-61935.rs and https://github.com/rust-lang/rust/blob/master/src/test/ui/const-generics/lazy-normalization/issue-71922.rs rust-lang#61935 is the actually fixed issue while rust-lang#71922 is still not working, as it depends on lazy norm of repeat expressions
remove duplicate test for rust-lang#61935 Apparently I somehow messed up the issue number in rust-lang#67890 which caused us to add this test twice, both as https://github.com/rust-lang/rust/blob/master/src/test/ui/const-generics/issues/issue-61935.rs and https://github.com/rust-lang/rust/blob/master/src/test/ui/const-generics/lazy-normalization/issue-71922.rs rust-lang#61935 is the actually fixed issue while rust-lang#71922 is still not working, as it depends on lazy norm of repeat expressions
Based on #67717
Initial implementation of #60471 for constants.
Perform normalization/evaluation of constants lazily, which is known as lazy normalization. Lazy normalization is only enabled given the command line flag
-Z lazy-normalization, by default constants are still evaluated eagerly as there are currently.Lazy normalization of constants is achieved with a new
ConstEquatepredicate which type inferences uses to delay checking whether constants are equal to each other until later, avoiding cycle errors.Note this doesn't allow the use of generics within repeat count expressions as that is still evaluated during conversion to mir. There are also quite a few other known problems with lazy normalization which I'm planning to fix in follow up PRs.
r? @nikomatsakis cc @varkor