Lazy normalization of constants (Reprise) #71973
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lcnr
force-pushed
the
lazy-norm
branch
4 times, most recently
from
aac9109
to
7cd013c
Compare
lcnr
commented
May 7, 2020
|
I'll try to take a look at this again soon. |
|
I will also be attempting to do another review tomorrow! Sorry, Thursday is my most meeting-full day. |
varkor
reviewed
May 8, 2020
| @@ -671,6 +671,10 @@ impl<'tcx> TypeRelatingDelegate<'tcx> for QueryTypeRelatingDelegate<'_, 'tcx> { | |||
| }); | |||
| } | |||
|
|
|||
| fn const_equate(&mut self, _a: &'tcx Const<'tcx>, _b: &'tcx Const<'tcx>) { | |||
| unimplemented!() | |||
There was a problem hiding this comment.
Is this intended for a follow up? Do we know what's blocking this?
There was a problem hiding this comment.
This was is from @Skinny121, will have to look into this.
There was a problem hiding this comment.
This seems like it is unreachable rn. QueryTypeRelatingDelegate is only used with TypeRelating. TypeRelating should never use const_equate afaict.
Replaced it with a span_bug 🤔 We can probably ignore this for now.
edit: It can actually be reached. I would still keep the span_bug here to see what would actually use this code path.
|
Reverted the changes moving |
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 8, 2020
Add const-generics test Taken from rust-lang#71973 as this apparently already compiles. r? @varkor
lcnr
commented
May 8, 2020
| // FIXME(lazy_normalization_consts) We currenctly don't detect lifetimes within substs | ||
| // which would violate this check. Even though the particular substitution is not used | ||
| // within the const, this should still be fixed. | ||
| return false; |
There was a problem hiding this comment.
I updated this FIXME but am not completely certain this is actually correct.
Is this statement correct? Especially the "Even though the particular substitution is not used within the const" part?
There was a problem hiding this comment.
I sort of have no idea what this FIXME is trying to say, yeah. Why don't we do this? I guess it's because the check would basically always fail for any constant, even though a lot of the constants are like {22}. In other words, we really have to normalize the constant (evaluate it) to do this check.
|
For reference, here are the lazy normalization notes I prepared that were background for the original PR. |
|
Although they don't include all the updates, in that I remember having some thoughts about how we can just "not equate" for bound regions to side-step for now the leak-check question, but that doesn't appear to be in the notes. Might've been on Zulip. |
nikomatsakis
reviewed
May 8, 2020
| // Do not walk substitutions of unevaluated consts, as they contain `Self`, even | ||
| // though the const expression doesn't necessary use it. Currently type variables | ||
| // inside array length expressions are forbidden, so they can't break the above | ||
| // rules. |
There was a problem hiding this comment.
If I'm not mistaken, the code that's needed here is very similar to the code for associated type projections above, and could probably be factored out fairly easily? I think it's fine to leave for later, though I agree that before landing this PR, we should make a tracking issue and add these FIXMEs to it to help us keep sight of them. Ideally, I'd like to see tests that show the problem, too
|
We currently get a stack overflow in a test due to incorrect handling of rust/src/librustc_typeck/collect/type_of.rs Line 273 in 7b80539 I think that I can quickly fix this myself edit: isn't as simple as I though, there may be a more difficult underlying problem here |
|
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 |
crlf0710
added
A-lazy-normalization
nikomatsakis
approved these changes
May 18, 2020
|
@bors r+ I don't have a strong opinion about how many feature gates we have, but splitting into a "pseudo-feature-gate" seems like a reasonable compromise to me. |
|
📌 Commit 9da8a5b has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 18, 2020
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
This was referenced May 18, 2020
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 18, 2020
Rollup of 5 pull requests Successful merges: - rust-lang#71599 (Support coercion between (FnDef | Closure) and (FnDef | Closure)) - rust-lang#71973 (Lazy normalization of constants (Reprise)) - rust-lang#72283 (Drop Elaboration Elaboration) - rust-lang#72290 (Add newer rust versions to linker-plugin-lto.md) - rust-lang#72318 (Add help text for remote-test-client) Failed merges: r? @ghost
Merged
This was referenced Jun 30, 2020
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 4, 2020
…arkor add `lazy_normalization_consts` feature gate In rust-lang#71973 I underestimated the amount of code which is influenced by lazy normalization of consts and decided against having a separate feature flag for this. Looking a bit more into this, the following issues are already working with lazy norm in its current state rust-lang#47814 rust-lang#57739 rust-lang#73980 I therefore think it is worth it to enable lazy norm separately. Note that `#![feature(const_generics)]` still automatically activates this feature, so using `#![feature(const_generics, lazy_normalization_consts)]` is redundant. r? @varkor @nikomatsakis
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 5, 2020
…arkor add `lazy_normalization_consts` feature gate In rust-lang#71973 I underestimated the amount of code which is influenced by lazy normalization of consts and decided against having a separate feature flag for this. Looking a bit more into this, the following issues are already working with lazy norm in its current state rust-lang#47814 rust-lang#57739 rust-lang#73980 I therefore think it is worth it to enable lazy norm separately. Note that `#![feature(const_generics)]` still automatically activates this feature, so using `#![feature(const_generics, lazy_normalization_consts)]` is redundant. r? @varkor @nikomatsakis
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 5, 2020
…arkor add `lazy_normalization_consts` feature gate In rust-lang#71973 I underestimated the amount of code which is influenced by lazy normalization of consts and decided against having a separate feature flag for this. Looking a bit more into this, the following issues are already working with lazy norm in its current state rust-lang#47814 rust-lang#57739 rust-lang#73980 I therefore think it is worth it to enable lazy norm separately. Note that `#![feature(const_generics)]` still automatically activates this feature, so using `#![feature(const_generics, lazy_normalization_consts)]` is redundant. r? @varkor @nikomatsakis
16 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Continuation of #67890 by @Skinny121.
Initial implementation of #60471 for constants.
Perform normalization/evaluation of constants lazily, which is known as lazy normalization. Lazy normalization is only enabled when using
#![feature(const_generics)], 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 #71922, fixes #71986