Repository navigation
session: Enable next-solver globally for assumptions-on-binders - #160513
Merged
rust-bors[bot] merged 4 commits intoAug 7, 2026
Merged
rust-bors[bot] merged 4 commits into
rust-bors[bot] merged 4 commits into
Conversation
Normalize next_solver after -Z parsing so flag order does not matter and direct readers of next_solver.globally see the effective config.
Cover the flag alone, both orderings with -Znext-solver, override of -Znext-solver=no, and a UI test that no longer passes -Znext-solver.
Collaborator
|
r? @chenyukang rustbot has assigned @chenyukang. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Dnreikronos
marked this pull request as draft
August 4, 2026 15:43
Dnreikronos
force-pushed
the
assumptions_on_binders/enable_next_solver
branch
from
August 4, 2026 15:50
943cc32 to
57722cb
Compare
Dnreikronos
marked this pull request as ready for review
August 4, 2026 17:07
Member
|
lgtm, but it's better r? @lcnr take a look. |
bit-aloo
reviewed
Aug 5, 2026
| @@ -1,4 +1,4 @@ | |||
| //@ compile-flags: -Znext-solver -Zassumptions-on-binders | |||
| //@ compile-flags: -Zassumptions-on-binders | |||
Member
There was a problem hiding this comment.
We can remove the -Znext-solver from other tests in assumptions_on_binders.
Contributor
Author
There was a problem hiding this comment.
Yep, done. Cleared -Znext-solver / -Znext-solver=globally off the other six tests in that folder. They're redundant once -Zassumptions-on-binders turns the next solver on globally.
Comment on lines
+2693
to
+2695
| if unstable_opts.assumptions_on_binders { | ||
| unstable_opts.next_solver = NextSolverConfig { coherence: true, globally: true }; | ||
| } |
Member
There was a problem hiding this comment.
Should we add a warning in case if a user adds -Znext-solver=no and -Zassumptions-on-binders, saying using -Zassumption-on-binders unconditionally enables next solver?
Contributor
Author
There was a problem hiding this comment.
Yep. Overriding an explicit -Znext-solver=no without saying anything felt too sneaky, so I added an early warning instead of a hard error:
-Zassumptions-on-binders unconditionally enables the next trait solver; `-Znext-solver=no` is ignored
We only warn when next_solver.coherence is false, which is the =no case. UI test is next-solver-no-overridden.rs.
-Zassumptions-on-binders already forces the next solver globally, so the explicit -Znext-solver flags in these sibling tests do nothing.
-Zassumptions-on-binders still forces the next solver globally, but an explicit -Znext-solver=no is surprising to discard silently. Emit an early warning in that case, matching the staticlib-hide-internal-symbols precedent for conflicting -Z combos.
Contributor
|
@bors r=lcnr,bit-aloo |
Contributor
Contributor
|
@bors rollup=never |
Member
|
I suppose this should fix #160256 ? |
Member
|
@bors p=6 |
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Aug 6, 2026
…t_solver, r=lcnr,bit-aloo session: Enable next-solver globally for assumptions-on-binders Zulip: https://rust-lang.zulipchat.com/#narrow/channel/618216-t-types.2Fcall-for-participation/topic/assumptions.20on.20binders.3A.20enable.20next-solver.20automatically/with/614548133 `-Zassumptions-on-binders` already needs the next trait solver, but you still had to pass `-Znext-solver` by hand. Easy to forget, and some spots (feature gates) read `next_solver.globally` directly, so just special-casing `next_trait_solver_globally()` wouldn't cut it. After `-Z` parsing, if assumptions-on-binders is on, force `NextSolverConfig { coherence: true, globally: true }`. Same pattern as `-Zretpoline-external-thunk`. Flag order doesn't matter. `-Znext-solver=no` gets overridden too; imo that's the right call since the assumptions code can't run on the old solver. A hard conflict error would also be fine, just more annoying for day-to-day hacking. Covered the flag alone, both orderings, and `=no` in a unit test. Dropped the explicit `-Znext-solver` from one UI test under `assumptions_on_binders`. btw idk if we should strip `-Znext-solver` from the rest of that folder asap or leave the redundancy. irl I'd leave it for now. fyi this also means anything checking `next_solver.globally` sees the effective config. ltm if you'd rather go the conflict-error route instead of overriding `=no`.
Member
|
@bors cancel |
Contributor
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #160513. |
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Aug 6, 2026
…t_solver, r=lcnr,bit-aloo session: Enable next-solver globally for assumptions-on-binders Zulip: https://rust-lang.zulipchat.com/#narrow/channel/618216-t-types.2Fcall-for-participation/topic/assumptions.20on.20binders.3A.20enable.20next-solver.20automatically/with/614548133 `-Zassumptions-on-binders` already needs the next trait solver, but you still had to pass `-Znext-solver` by hand. Easy to forget, and some spots (feature gates) read `next_solver.globally` directly, so just special-casing `next_trait_solver_globally()` wouldn't cut it. After `-Z` parsing, if assumptions-on-binders is on, force `NextSolverConfig { coherence: true, globally: true }`. Same pattern as `-Zretpoline-external-thunk`. Flag order doesn't matter. `-Znext-solver=no` gets overridden too; imo that's the right call since the assumptions code can't run on the old solver. A hard conflict error would also be fine, just more annoying for day-to-day hacking. Covered the flag alone, both orderings, and `=no` in a unit test. Dropped the explicit `-Znext-solver` from one UI test under `assumptions_on_binders`. btw idk if we should strip `-Znext-solver` from the rest of that folder asap or leave the redundancy. irl I'd leave it for now. fyi this also means anything checking `next_solver.globally` sees the effective config. ltm if you'd rather go the conflict-error route instead of overriding `=no`.
Member
|
Looks like this failed to start. @bors yield |
Contributor
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #160513. |
This comment has been minimized.
This comment has been minimized.
Contributor
Contributor
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 84b36a7 (parent) -> 88f7399 (this PR) Test differencesShow 7 test diffsStage 0
Stage 1
Stage 2
Additionally, 2 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 88f7399cb4912680976e1c342ea7661b9dc84940 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Collaborator
|
Finished benchmarking commit (88f7399): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -3.6%, secondary -6.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 17.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 457.798s -> 460.09s (0.50%) |
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.
Zulip: https://rust-lang.zulipchat.com/#narrow/channel/618216-t-types.2Fcall-for-participation/topic/assumptions.20on.20binders.3A.20enable.20next-solver.20automatically/with/614548133
-Zassumptions-on-bindersalready needs the next trait solver, but you still had to pass-Znext-solverby hand. Easy to forget, and some spots (feature gates) readnext_solver.globallydirectly, so just special-casingnext_trait_solver_globally()wouldn't cut it.After
-Zparsing, if assumptions-on-binders is on, forceNextSolverConfig { coherence: true, globally: true }. Same pattern as-Zretpoline-external-thunk. Flag order doesn't matter.-Znext-solver=nogets overridden too; imo that's the right call since the assumptions code can't run on the old solver. A hard conflict error would also be fine, just more annoying for day-to-day hacking.Covered the flag alone, both orderings, and
=noin a unit test. Dropped the explicit-Znext-solverfrom one UI test underassumptions_on_binders.btw idk if we should strip
-Znext-solverfrom the rest of that folder asap or leave the redundancy. irl I'd leave it for now. fyi this also means anything checkingnext_solver.globallysees the effective config. ltm if you'd rather go the conflict-error route instead of overriding=no.