Show env normalization differences under two solvers by adwinwhite · Pull Request #148939 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation43 Commits1 Checks12 Files changed

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

[ Show hidden characters]({{ revealButtonHref }})

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

Some changes occurred to the core trait solver

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

This comment has been minimized.

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.

This comment has been minimized.

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

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

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.

This comment has been minimized.

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

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>,

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.

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.

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.

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

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?

Choose a reason for hiding this comment

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

yes

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.

This comment has been minimized.

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

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.

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)

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

🚧 Experiment pr-148939 is now running

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

🎉 Experiment pr-148939 is completed!
📊 655 regressed and 3 fixed (739678 total)
📊 1901 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

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

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

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

🚧 Experiment pr-148939-1 is now running

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

🎉 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

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

🚧 Experiment pr-148939-2 is now running

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

🎉 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

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

@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.

No problem. I'll look into them.

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

Labels

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)