Skip to content

Conversation

@adwinwhite
Copy link
Contributor

@adwinwhite adwinwhite commented Nov 14, 2025

First step for param_env normalization future compat warning. zulip thread

I didn't put the check directly in param_env query because normalize_param_env_or_error has some adjustments on the predicates: elaboration and outlive predicates manipulation. I'd have to replicate these in param_env to compare normalized predicates properly.
The downside of putting the check inside normalize_param_env_or_error is that it's used in more than param_env query. It's also used in compare_impl_item and several other places. I'm not sure if this is desired.

I didn't bless tests since the hard error will be changed to lint later.
Blessed tests to demonstrate the changes.

And there's one test about const generics I don't know how to fix.
Canonicalizer for the next solver will replace ParamConst with PlaceholderConst, but it still uses the same try_evaluate_const internally and process_obligation doesn't like PlaceholderConst.

r? @lcnr

@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2025

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Nov 14, 2025
@rust-log-analyzer

This comment has been minimized.

@adwinwhite adwinwhite force-pushed the env_normalization_fcw branch from 5cd4303 to 4408f14 Compare November 14, 2025 12:30
@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2025

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rust-log-analyzer

This comment has been minimized.

orig_pred,
) {
Ok(pred_by_next) => {
if pred_by_next == *pred_by_old {
Copy link
Contributor

@lcnr lcnr Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to fully resolve the predicates before comparing them. This is causing the error in std. Also just generally move this further to the end of this function?

I think doing it here isn't ideally, but should be good enough for a crater run

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std compiles fine after resolving predicates normalized by the old solver.
It turns out that stderr changes of some tests are noise. They're restored to the original state.

@rust-log-analyzer

This comment has been minimized.

@adwinwhite
Copy link
Contributor Author

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 16, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

impl<'a, T> Check for T
//~^ ERROR: overflow evaluating the requirement `T: Check`
where
T: Iterate<'a>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should projection alias in param env imply that the trait predicate holds for the next solver?
I add manual trait clause for these three specialization tests to avoid unnecessary lint triggering.

@adwinwhite
Copy link
Contributor Author

If we resolve regions for the next solver, these two tests will trigger ICEs. They're the same tests mentioned in 166.

  • tests/ui/higher-ranked/trait-bounds/issue-102899.rs
  • tests/ui/higher-ranked/trait-bounds/issue-100689.rs

The cause is that we add constraints that contains placeholders to infcx's constraints collector and resolve_regions can't handle that.
We replace bounds with placeholders and restore them after trait solving in deeply_normalize, but the restoration doesn't apply to region constraints added to infcx.

Now the only ui test broken should be the one in PR description. Sorry for not running all tests faithfully before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next solver normalizes free alias to opaque while the old solver doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a diverging between the trait solvers, but instead deeply normalizing with the new solver not checking whether predicates may get normalized.

see

fn fold_predicate(&mut self, p: ty::Predicate<'tcx>) -> ty::Predicate<'tcx> {
if p.allow_normalization() && needs_normalization(self.selcx.infcx, &p) {
p.super_fold_with(self)
} else {
p
}
}
which does not exist in the new solver deeply normalize as we've never deeply normalized predicates before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a try_fold_predicate with this check to the next solver version as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next solver normalizes projection alias to opaque while the old solver doesn't.

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Nov 17, 2025

If we resolve regions for the next solver, these two tests will trigger ICEs. They're the same tests mentioned in 166.

* tests/ui/higher-ranked/trait-bounds/issue-102899.rs

* tests/ui/higher-ranked/trait-bounds/issue-100689.rs

The cause is that we add constraints that contains placeholders to infcx's constraints collector and resolve_regions can't handle that. We replace bounds with placeholders and restore them after trait solving in deeply_normalize, but the restoration doesn't apply to region constraints added to infcx.

Now the only ui test broken should be the one in PR description. Sorry for not running all tests faithfully before.

this should similarly affect the old solver except that it does not register outlives constraints if considering_regions is set to false :<

I feel like we can probably just disable that ICE/assert for now, idk where exactly that happens and didn't look into it before writing this comment

@adwinwhite
Copy link
Contributor Author

adwinwhite commented Nov 18, 2025

this should similarly affect the old solver except that it does not register outlives constraints if considering_regions is set to false :<

yeah, the next solver doesn't seem to check infcx.considering_regions as I ripgrepped.
Should we check it in some region constraint registering function of infcx or the next solver?

I feel like we can probably just disable that ICE/assert for now, idk where exactly that happens and didn't look into it before writing this comment

Got it. I'll ignore the errors of resolve_regions for now.

@adwinwhite
Copy link
Contributor Author

adwinwhite commented Nov 18, 2025

About ICE in this test.

I understand that the generic_const_exprs is not supported by the next solver for now.
The problem is that this test uses generic_const_exprs features in dependency crate so we can't skip our lint before hand.
If we can know whether dependencies use generic_const_exprs and disable out lint, the lint's usefulness is damaged?

Now that we accept we will run into generic const in normalization with the next solver, there're two choices:

  • detect generic param and return the unevaluated const back. (The old normalization does this)
  • return error as the normalization does fail. (My impression is that normalization with the next solver is less forgiving)

@adwinwhite adwinwhite force-pushed the env_normalization_fcw branch from 571ea88 to 578bcc8 Compare November 18, 2025 06:46
@adwinwhite
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 18, 2025
@lcnr
Copy link
Contributor

lcnr commented Nov 18, 2025

@bors try

@lcnr
Copy link
Contributor

lcnr commented Nov 24, 2025

probably still has some genuine hangs which will be part of the spurious regressions. These are also useful to figure out though :>

@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 24, 2025

tbh 1900 spurrious regressions is also just like average flakey crater moment 😔

@craterbot
Copy link
Collaborator

🚧 Experiment pr-148939-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-148939-1 is completed!
📊 638 regressed and 0 fixed (2496 total)
📊 168 spurious results on the retry-regessed-list.txt, consider a retry1 if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

  1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-148939-1/retry-regressed-list.txt

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 26, 2025
@lcnr
Copy link
Contributor

lcnr commented Nov 28, 2025

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-148939-1/retry-regressed-list.txt p=1

let's winnow the spurious crates a bit further.

@craterbot
Copy link
Collaborator

👌 Experiment pr-148939-2 created and queued.
🤖 Automatically detected try build c8085cf
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-148939-2 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-148939-2 is completed!
📊 625 regressed and 0 fixed (806 total)
📊 30 spurious results on the retry-regessed-list.txt, consider a retry1 if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

  1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-148939-2/retry-regressed-list.txt

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 29, 2025
@lcnr
Copy link
Contributor

lcnr commented Dec 2, 2025

@adwinwhite we should look into the spurious results to see if any of them are pass -> hang, if so, that perf issue has to be handled first, before we can do anything else with this lint. Would you be up to look into this yourself?

Looking at the crater results, they are mostly what I expect, though there are some places where the lint triggers without that also causing issues in #133502 when fully enabling the new solver.

@adwinwhite
Copy link
Contributor Author

No problem. I'll look into them.

@lcnr
Copy link
Contributor

lcnr commented Dec 16, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2025
@adwinwhite
Copy link
Contributor Author

Sorry for the delay.

I built all the 15 spurious crates locally and none of them are pass -> hang.

crate result
JoelBeicher.leptos-color-picker.a5eb3225012e8502621e30c5c959f18a50d50b3a lint error
LeadClaymore.test_wasm.f128c33ac61c8948399267ef060d145ee05f69f7 lint error
LeslieM98.untitled-rustgame.f784d0e1bcffa993139c4e0e6b627045c5f2d0a0 lint error
Mirch.mpc.334fcf6fef940af5998c319463f6e5a8da8356d2 pass
TaceoLabs.CoNoir-to-R1CS.53bf34b9e730d9b2289d539524896224e96e3dfd lint error
Yummiii.psw.22eb6f165e75ffba7256e67375517f853b005aca pass
askabdul.libro_commerce.0df383067e390c6621b69ef231d2eb70c3c419e3 lint error
comus3.conways_game_of_life_gpu.57edec91adc93251de67ebcd99d740a32cf78943 pass
ga1agon.fatum.cb9a304a54bcf7e443ce059f80ad8c7d2c6dfaee lint error
parrrate.object-rainbow.066fb0962bf349ba79642f5a04f750bb8b2f09eb lint error
pbusenius.polars-domain-lookup.c2496c78516be9603c27f5a2fbca229db09e7c21 lint error
pgattic.convnum.186c44e77c6e824d3c35a47c96bd345cb3098298 pass
typerian.book-ecommerce.ad152d368f478d26cbf48185fd797102018ec481 lint error
lilguy-0.1.2 lint error
object-rainbow-0.0.0-a.9 lint error

@lcnr
Copy link
Contributor

lcnr commented Dec 17, 2025

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Dec 17, 2025
Show env normalization differences under two solvers
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 17, 2025
@rust-bors
Copy link

rust-bors bot commented Dec 17, 2025

☀️ Try build successful (CI)
Build commit: 30bc2a3 (30bc2a332ed6969dec184333bbdd08e680789eeb, parent: ec6f62244c3a019e2224b779d2b606721cabf8f2)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (30bc2a3): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
3.0% [0.2%, 13.0%] 56
Regressions ❌
(secondary)
0.9% [0.2%, 2.8%] 47
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.5% [-1.3%, -0.1%] 10
All ❌✅ (primary) 3.0% [0.2%, 13.0%] 56

Max RSS (memory usage)

Results (primary 2.3%, secondary -3.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.3% [0.8%, 4.6%] 10
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.3% [-6.5%, -2.0%] 6
All ❌✅ (primary) 2.3% [0.8%, 4.6%] 10

Cycles

Results (primary 5.1%, secondary 3.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
5.1% [2.1%, 11.2%] 19
Regressions ❌
(secondary)
3.3% [2.0%, 6.0%] 10
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 5.1% [2.1%, 11.2%] 19

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 482.848s -> 482.088s (-0.16%)
Artifact size: 390.53 MiB -> 390.46 MiB (-0.02%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants