Normalize before checking trivial bounds

[sjwang05]

View all comments

Based on #140313
zulip discussion: #t-types > working on issue #154145

Currently, if a trivial bound is placed on an assoc type, and the type contains generics, check_false_global_bounds improperly tries to prove the predicate globally, since normalization, which would reveal the generics, happens only after the is_global() check. This PR moves normalization occur before the globality check. Additionally, we skip only the outermost HRTB with pred.kind().bound_vars().is_empty() rather than checking !pred.has_type_flags(TypeFlags::HAS_BINDER_VARS).

As a consequence of normalizing, this PR emits errors for code like this, which currently compiles:

fn foo<T>()
where
    T: Trait<Assoc = String>,
    <T as Trait>::Assoc: Copy,
{}

fixes #154145
fixes #140309

r? lcnr

[sjwang05]

Seems like CI is failing because of this proc macro in salsa: https://github.com/salsa-rs/salsa/blob/5e239195eee4e54440b84593396ec1c8ca37146c/components/salsa-macro-rules/src/setup_tracked_fn.rs#L495-L497
which expands to the trivial bound for<'trivial_bounds> salsa::plumbing::function::NoopEviction: salsa::plumbing::function::HasCapacity when lru isn't passed as one of the args to the macro, since NoopEviction doesn't impl HasCapacity, which happens here in rust-analyzer:

rust/src/tools/rust-analyzer/crates/base-db/src/editioned_file_id.rs

Line 58 in 624f761

#[salsa::tracked(returns(as_deref))]

rust/src/tools/rust-analyzer/crates/base-db/src/lib.rs

Line 382 in 624f761

#[salsa::tracked(returns(deref))]

So I guess it's a hacky version of "trivial_bounds but for stable" to make set_lru_capacity uncallable when the lru's size isn't provided?

[lcnr]

Additionally, rather than skipping HRTBs with !pred.has_type_flags(TypeFlags::HAS_BINDER_VARS), we instead call enter_forall so we can properly instantiate and deal with them.

I think not checking trivial bounds if they are higher ranked where-clauses is intended. We want to give users a way to opt-out of this check and this is something that users rely on.

I want us to continue to ignore for<'a> SomeType: Bound where-bounds, but to start checking Wrapper<for<'a> fn(&' ())>: Bound where-bounds.

[sjwang05]

Ah got it, I looked through #48214 and couldn't find much discussion as to whether this should be allowed or not, so thanks for clarifying. I updated the tests and changed it back to bound_vars().is_empty().

[rustbot]

This PR modifies tests/ui/issues/. If this PR is adding new tests to tests/ui/issues/,
please refrain from doing so, and instead add it to more descriptive subdirectories.

[rustbot]

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

[rustbot]

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.

[lcnr]

@bors try @rust-timer queue

let's do a crater run 😁

[rust-bors]

☀️ Try build successful (CI)
Build commit: ac56ae8 (ac56ae821c3737dd227af5395216be69b814e0ca)
Base parent: 5a8cd23 (5a8cd237d4fad99d862aed51fab8cb5345c97013)

[rust-timer]

Finished benchmarking commit (ac56ae8): comparison URL.

Overall result: ❌ regressions - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@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)	0.3%	[0.2%, 0.4%]	7
Regressions ❌ (secondary)	0.9%	[0.6%, 1.4%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.4%]	7

Max RSS (memory usage)

Results (secondary 4.9%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.9%	[3.7%, 6.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 2.3%)

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

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 487.476s -> 490.061s (0.53%)
Artifact size: 387.96 MiB -> 387.92 MiB (-0.01%)

[rust-bors]

☔ The latest upstream changes (presumably #154989) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[lcnr]

@craterbot check

[craterbot]

👌 Experiment pr-159118 created and queued.
🤖 Automatically detected try build ac56ae8
🔍 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]

🚧 Experiment pr-159118 is now running

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

[craterbot]

🎉 Experiment pr-159118 is completed!
📊 191 regressed and 0 fixed (1053871 total)
📊 5991 spurious results on the retry-regressed-list.txt, consider a retry¹ 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-159118/retry-regressed-list.txt ↩

[sjwang05]

So of the dependency tree roots that caused other crates to transitively fail:

• alloy-rpc-client didn't compile from 0.1.1 through 0.9.2, but 1.0.0 onward through current compiles. This was the cause of 181 of the failures. There are also a few forks of pre-1.0.0 code that fail to compile.
• beet_core failed to compile in 0.0.6 and fixed in 0.0.7, current is also fine
• bevy_flurx stopped compiling in 0.12.0, 0.13.0 up through current is fine.

wyre-0.2.16 seems to be spurious, since cargo failed to resolve its bytes dependency.

Of the ones whose latest still don't compile:

• wgsl_linker and wgsl-analyzer both need just a #[derive(Default)] on one struct, since its trivial bound currently comes from a parser generator, and both those structs are empty
• overflowing_int needs a for<'trivial>-esque binder on 4 bounds
• mzPeak and advent-utils need a similar binder on one bound each

For the last three crates, the bounds were false and the functions were never callable in the first place, so adding the higher-ranked binder preserves existing behavior while letting them compile again, which I verified using the branch's stage1.

[lcnr]

please instead add feature(trivial_bounds) and keep this as check-pass. Otherwise this test doesn't test what it's intended to anymore

View changes since the review

[lcnr]

similar to the one review, if there are any test which test the behavior on compilation success, please try to instead keep them as check-pass by enabling feature(trivial_bounds)

View changes since this review

[lcnr]

one option would be to only check pred.is_is_global() && normalize_pred && normalized_pred.is_global() which would allow <T as Normalize>::WillBeUsize: Trait to keep compiling.

Doing this would minimize the breakage and perf impact.

One other option would be to change this function to not be on the wf-check but instead use a separate ObligationCtxt at which point we should then be able to emit the trait errors as future compat warnings instead.

Thinking about this, I generally feel kind of :/ about breaking anything here. I feel like trivial bounds should be a deny-by-default lint and we should just stabilize feature(trivial_bounds) 🤔

not sure what to ask/expect of you here. Would you be interested in looking into the existing trivial_bounds tracking issue and related work/discussions to try and summarize what is blocking there currently?

View changes since the review

[sjwang05]

I looked into this a little bit. The tracking issue itself says stabilization is blocked on next-solver, and I also went through the open issues myself. Of the F-trivial_bounds tagged issues that are still open:

• trivial_bounds lint lints on non-global associated type constraints #140311 is a bad diagnostics issue that reproduces on both old and next solver
• Impossible bounds cause a linker error in a dylib. #128682 is a weird linker error that also reproduces on both
• ICE: "Missing value for constant, but no error reported?" with unresolvabe const due to trivial bounds #135617 ICEs on old solver and emits E0282 on next solver
• ICE with #[derive(rkyv::Archive)] on cyclic enum #123238 seems to no longer ICE on either solver (hooray!)

and of the issues tagged with A-impossible_bounds that still reproduce:

• [ICE]: Operand evaluation failure during slice pattern matching with Sized constraint #153076, [ICE]: ICE with multiple Drop impls with impossible bound and Self: Drop #153947, ICE from specializing Drop impl with impossible bound #150387 ICE on both solvers
• Function with impossible-to-satisfy higher-ranked bound triggers error when a closure is present #89409 is a weird diagnostic that still reproduces on both solvers
• assoc types in binders can pass wf check but error when trying to normalize via impl #161405 is an unsoundness whose fix depends on assumptions-on-binders
• min_specialization is unsound due to trivially-false lifetime bound #149257 is an unsoundness that still reproduces on old solver, but not on next-solver. It's also related to min_specialization, so I feel like this is more of a min_specialization thing than a blocker on stabilization.