reduce needed recursion limit to avoid next-solver FCW - #172
Merged
Conversation
This file contains hidden or 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
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.
Hi there 👋 we're going to be stabilizing the next-generation trait solver soon. You can test it on nightly with
-Znext-solver.Unfortunately,
generic-arraycurrently encounters overflow errors when computingMaxArrayLengthP1when used with the new solver. While we are weakening them to a FCW - future compatibility warning - for the initial stabilization, that lint isn't ideal as it also affects dependencies usinggeneric-array. We currently get ~5000 warnings in some dependencies. I've been spending some time looking into that and this is a way to reduce the recursion limit:vs
The overflow issue is described in rust-lang/rust#159228. The core issue is that the old solver did not track the required depth in its cache. This meant that evaluating something which requires a depth of 100 and then using its cache entry at a depth of 50 didn't fail with an overflow error, even though if the cache entry wasn't present, we'd have needed a total depth of 150. This can break incremental compilation, so we're now properly tracking this with the new implementation.
Using
Shleft<N, M>requires a recursion depth oflog_2(N) + 2Mbecause of this impl https://github.com/paholg/typenum/blob/0db9a0f731981f29266b63586c29fa07e4477b1a/src/uint.rs#L1005.Shleft<N, M>is implemented asShleft<UInt<N, B0>, M - 1>and needs to prove that its self type isUnsignedat every step. This means after recursing forMsteps the final use of thisShlimpl now need to also check that aN << MisUnsigned, which requireslog_2(N) + Msteps as it's linear in the number of bits.I feel like ideally we'd also change
Shlintypenumto not require2Mrecursion steps, but I don't fully know how to do this and there's also the question of the compile time performance impact. That one will also be affected by the new trait solver and its performance and caching behavior is quite different from the existing implementation. Going to open a separate issue intypenumabout this.I would deeply appreciate if you could publish new minor versions of
generic-array. Certainly 1.0 and looking at https://crates.io/crates/generic-array/reverse_dependencies also 0.14. Older versions would be appreciated, but feel less necessary to me.Thank you so much and I am open for any questions you may have :>