-Znext-solver Remove the forced ambiguity hack from search graph - #149904
Merged
Conversation
rustbot
added
S-waiting-on-review
Contributor
rustbot
added
S-blocked
lcnr
added
the
T-types
Contributor
|
This PR removes a performance optimization which seems like it is no longer necessary, as we no longer have any affected tests. This performance regression was originally added during the stabilization of The optimization does incorrectly force goals to be ambiguous, breaking In this case we do cause hangs, we can then find a different solution and potentially temporarily revert this change again. I did not immediately see any concerning regressions in the crater run with @rfcbot fcp merge types |
Collaborator
|
Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rust-rfcbot
added
proposed-final-comment-period
Collaborator
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rust-rfcbot
added
finished-final-comment-period
Collaborator
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. |
Contributor
|
@bors r+ rollup=never |
Contributor
|
📋 This PR cannot be approved because it currently has the following label: |
lcnr
removed
the
S-blocked
Contributor
|
@bors r+ |
Contributor
rust-bors
Bot
added
the
S-waiting-on-bors
jdonszelmann
added a commit
to jdonszelmann/rust
that referenced
this pull request
Feb 17, 2026
…lcnr `-Znext-solver` Remove the forced ambiguity hack from search graph As discussed in rust-lang/trait-system-refactor-initiative#257 r? lcnr
rust-timer
added a commit
that referenced
this pull request
Feb 17, 2026
Rollup merge of #149904 - ShoyuVanilla:ns-remove-sg-hack, r=lcnr `-Znext-solver` Remove the forced ambiguity hack from search graph As discussed in rust-lang/trait-system-refactor-initiative#257 r? lcnr
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Apr 2, 2026
Revert "`-Znext-solver` Remove the forced ambiguity hack from search graph" This PR reverts rust-lang#149904 as discussed and requested in https://rust-lang.zulipchat.com/#narrow/channel/326866-t-types.2Fnominated/topic/.23153910.3A.20Significant.20compilation.20time.20regression.20starting.20i.E2.80.A6. We would like a bit of time to investigate without pressure how rust-lang#149904 caused the compile time regression in rust-lang#153910, and the PR is in beta and release is approaching. When that is better understood, and we e.g. have a fix or decide to eat it, we'd want to re-land the original work. cc @ShoyuVanilla @lcnr
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Apr 2, 2026
Revert "`-Znext-solver` Remove the forced ambiguity hack from search graph" This PR reverts rust-lang#149904 as discussed and requested in https://rust-lang.zulipchat.com/#narrow/channel/326866-t-types.2Fnominated/topic/.23153910.3A.20Significant.20compilation.20time.20regression.20starting.20i.E2.80.A6. We would like a bit of time to investigate without pressure how rust-lang#149904 caused the compile time regression in rust-lang#153910, and the PR is in beta and release is approaching. When that is better understood, and we e.g. have a fix or decide to eat it, we'd want to re-land the original work. cc @ShoyuVanilla @lcnr
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Apr 2, 2026
Revert "`-Znext-solver` Remove the forced ambiguity hack from search graph" This PR reverts rust-lang#149904 as discussed and requested in https://rust-lang.zulipchat.com/#narrow/channel/326866-t-types.2Fnominated/topic/.23153910.3A.20Significant.20compilation.20time.20regression.20starting.20i.E2.80.A6. We would like a bit of time to investigate without pressure how rust-lang#149904 caused the compile time regression in rust-lang#153910, and the PR is in beta and release is approaching. When that is better understood, and we e.g. have a fix or decide to eat it, we'd want to re-land the original work. cc @ShoyuVanilla @lcnr
rust-timer
added a commit
that referenced
this pull request
Apr 2, 2026
Rollup merge of #154712 - lqd:revert-149904, r=jieyouxu Revert "`-Znext-solver` Remove the forced ambiguity hack from search graph" This PR reverts #149904 as discussed and requested in https://rust-lang.zulipchat.com/#narrow/channel/326866-t-types.2Fnominated/topic/.23153910.3A.20Significant.20compilation.20time.20regression.20starting.20i.E2.80.A6. We would like a bit of time to investigate without pressure how #149904 caused the compile time regression in #153910, and the PR is in beta and release is approaching. When that is better understood, and we e.g. have a fix or decide to eat it, we'd want to re-land the original work. cc @ShoyuVanilla @lcnr
github-actions Bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
Apr 3, 2026
Revert "`-Znext-solver` Remove the forced ambiguity hack from search graph" This PR reverts rust-lang/rust#149904 as discussed and requested in https://rust-lang.zulipchat.com/#narrow/channel/326866-t-types.2Fnominated/topic/.23153910.3A.20Significant.20compilation.20time.20regression.20starting.20i.E2.80.A6. We would like a bit of time to investigate without pressure how rust-lang/rust#149904 caused the compile time regression in rust-lang/rust#153910, and the PR is in beta and release is approaching. When that is better understood, and we e.g. have a fix or decide to eat it, we'd want to re-land the original work. cc @ShoyuVanilla @lcnr
This was referenced Apr 20, 2026
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jul 24, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jul 24, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jul 24, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jul 25, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jul 25, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jul 25, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Jul 25, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Jul 25, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Jul 25, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Jul 25, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jul 26, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jul 26, 2026
…=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
rust-timer
added a commit
that referenced
this pull request
Jul 26, 2026
Rollup merge of #155914 - lcnr:search-graph-bail-on-ambig, r=BoxyUwU when bailing on ambiguity, don't force other results to ambig A smaller version of #149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to #158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
pull Bot
pushed a commit
to xtqqczze/rust-lang-miri
that referenced
this pull request
Jul 27, 2026
when bailing on ambiguity, don't force other results to ambig A smaller version of rust-lang/rust#149904 which doesn't cause the perf issue in bevy, or at least in the minimization While not necessarily necessary for rust-lang/trait-system-refactor-initiative#257 due to rust-lang/rust#158643. This does fix the underlying issues there. This change does affect `binius_field`, the remaining issues are tracked in rust-lang/trait-system-refactor-initiative#274. The core idea is that for - A - B - A (cycle with initial result) - B <--- We normally run A until we reach a fixpoint. We don't do so in case trying to do so encounters overflow or the result of `A` ends up ambiguous to avoid performance issues. We want to generally keep the provisional cache entries which depend on A around. Evaluating these goals depends on the current provisional result of A. This means it is fine to keep them around if A reached a fixpoint. If we bail without reaching a fixpoint, the provisional result used while computing B differs from the final result of A. This means the entry for B is not valid and we'd need to discard it. Discarding all nested provisional cache entries when not reaching a fixpoint causes perf issues. In case the final result of A is ambiguous, then this PR keeps nested cache entries which are ambiguous and don't have any inference constraints around. That is sound, as having more goals be ambiguous is never an issue, the trait solver can always say ":shrug: idk". It also shouldn't cause any incorrect ambiguity errors issues, as changing the provisional result of A to be ambiguity should not change B to go from being ambiguous to something else. The current implementation mutated the result of all nested provisional cache entries to be ambiguous, which resulted in incorrect ambiguity errors in rust-lang/trait-system-refactor-initiative#257. This PR differs from that by dropping goals whose result isn't already ambiguous. That's means we don't keep quite as many cache entries around, which is potentially worse for perf, but from what I can tell this doens't cause issues in practice. r? BoxyUwU
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
5 participants
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.
As discussed in rust-lang/trait-system-refactor-initiative#257
r? lcnr