Skip to content

Normalize before checking trivial bounds#159118

Open
sjwang05 wants to merge 1 commit into
rust-lang:mainfrom
sjwang05:issue-154145
Open

Normalize before checking trivial bounds#159118
sjwang05 wants to merge 1 commit into
rust-lang:mainfrom
sjwang05:issue-154145

Conversation

@sjwang05

@sjwang05 sjwang05 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 11, 2026
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 11, 2026
@sjwang05

sjwang05 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

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:

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

#[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?

@rust-log-analyzer

This comment has been minimized.

@lcnr

lcnr commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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().

@rust-log-analyzer

This comment has been minimized.

@rustbot

rustbot commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

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 rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Jul 15, 2026
@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot

rustbot commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

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.

@rust-log-analyzer

This comment has been minimized.

@rust-bors

This comment has been minimized.

@rustbot

rustbot commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

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

lcnr commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

@bors try @rust-timer queue

let's do a crater run 😁

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 21, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Jul 21, 2026
Normalize before checking trivial bounds
@rust-bors

rust-bors Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

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%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 21, 2026
@rust-bors

rust-bors Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

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

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

Projects

None yet

5 participants