some trait system cleanups #120463

lcnr · 2024-01-29T08:26:40Z

Always eagerly replace projections with infer vars if normalization is ambig. Unsure why we previously didn't do so, wasn't able to find an explanation in #90887. This adds some complexity to the trait system and is afaict unnecessary.

The second commit simplifies pred_known_to_hold_modulo_regions, afaict the optional fulfill isn't necessary anymore.

r? types cc @jackh726

lcnr · 2024-01-29T08:26:50Z

@bors try @rust-timer queue

bors · 2024-01-29T08:27:59Z

⌛ Trying commit 9ab257b with merge 8e50202...

…<try> never normalize without eager inference replacement unsure why we previously didn't do so, wasn't able to find an explanation in rust-lang#90887. This adds some complexity to the trait system and is afaict unnecessary, r? types cc `@jackh726`

compiler-errors · 2024-01-29T19:02:00Z

@bors try @rust-timer queue

…<try> some trait system cleanups Always eagerly replace projections with infer vars if normalization is ambig. Unsure why we previously didn't do so, wasn't able to find an explanation in rust-lang#90887. This adds some complexity to the trait system and is afaict unnecessary. The second commit simplifies `pred_known_to_hold_modulo_regions`, afaict the optional `fulfill` isn't necessary anymore. r? types cc `@jackh726`

bors · 2024-01-29T19:03:10Z

⌛ Trying commit 2b3867b with merge 1e74e30...

compiler-errors · 2024-01-29T19:04:19Z

r=me after perf is clean.

I also cannot remember why + don't believe it's meaningful to make this distinction when lazily normalizing.

bors · 2024-01-29T20:32:11Z

☀️ Try build successful - checks-actions
Build commit: 1e74e30 (1e74e30961d4f19fa2e064a923aa2ee5d79f0d85)

rust-timer · 2024-01-29T21:46:42Z

Finished benchmarking commit (1e74e30): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.1%	[2.0%, 2.2%]	2
Improvements ✅ (primary)	-2.6%	[-2.7%, -2.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.6%	[-2.7%, -2.4%]	2

Cycles

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

Binary size

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

Bootstrap: 660.078s -> 661.755s (0.25%)
Artifact size: 308.02 MiB -> 308.06 MiB (0.01%)

jackh726 · 2024-01-29T22:17:47Z

@bors r=jackh726,compiler-errors

I don't remember why I made the try version. There may have been something specific that I ran into, or it may have just been defensive against adding extra obligations when we can't actually make progress on them.

Obviously, test suite passes and no perf regression - happy to remove it.

bors · 2024-01-29T22:17:50Z

📌 Commit 2b3867b has been approved by jackh726,compiler-errors

It is now in the queue for this repository.

jackh726

Actually, quick point. But the if below the normalize call will always be true now: we won't ever not replace the projection, with either the type or an infer var.

jackh726 · 2024-01-29T22:21:19Z

@bors r-

jackh726 · 2024-01-29T22:21:34Z

r=me with that change

lcnr · 2024-01-30T15:16:21Z

Actually, quick point. But the if below the normalize call will always be true now: we won't ever not replace the projection, with either the type or an infer var.

only in evaluate, because we only do so for global goals. In fulfill the projection could be rigid, e.g. for <T as Trait>::Assoc: Trait.

lcnr · 2024-02-15T13:17:24Z

@rustbot ready

bors · 2024-02-17T14:18:17Z

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

compiler-errors

but whatever, we'll find out the hard way

compiler-errors · 2024-02-19T15:02:59Z

compiler/rustc_trait_selection/src/traits/mod.rs

-        false
-    }
+    let obligation = Obligation::new(infcx.tcx, ObligationCause::dummy(), param_env, trait_ref);
+    infcx.predicate_must_hold_modulo_regions(&obligation)


There's a fair chance this will cause regression bc we're no longer falling back to fulfillment in the local infcx lol

we could do a crater run? think it's fine though, reverting this is straightforward 😀

compiler-errors · 2024-02-19T15:03:33Z

@bors r+ rollup=never (so bisection is easier)

bors · 2024-02-19T15:03:36Z

📌 Commit 0c7672a has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-02-19T15:48:24Z

⌛ Testing commit 0c7672a with merge 0243834...

bors · 2024-02-19T18:03:20Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 0243834 to master...

rust-timer · 2024-02-19T19:27:28Z

Finished benchmarking commit (0243834): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

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

Binary size

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

Bootstrap: 640.759s -> 640.857s (0.02%)
Artifact size: 308.79 MiB -> 308.81 MiB (0.01%)

rustbot assigned jackh726 Jan 29, 2024

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. labels Jan 29, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 29, 2024

lcnr changed the title ~~never normalize without eager inference replacement~~ some trait system cleanups Jan 29, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 29, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2024

jackh726 requested changes Jan 29, 2024

View reviewed changes

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 29, 2024

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 29, 2024

lcnr force-pushed the eager-inference-replacement branch from 2b3867b to f996e18 Compare January 30, 2024 15:15

This comment has been minimized.

Sign in to view

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 10, 2024

lcnr force-pushed the eager-inference-replacement branch from aa847c5 to 0b8770c Compare February 12, 2024 10:57

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 Feb 15, 2024

lcnr added 5 commits February 19, 2024 09:06

never normalize without eager inference replacement

486c7b6

remove pred_known_to_hold_modulo_regions

399a258

normalize_projection_ty is not used with next-solver

df55f56

split project into multiple files

9771fb0

remove outdated comment

0c7672a

lcnr force-pushed the eager-inference-replacement branch from 0b8770c to 0c7672a Compare February 19, 2024 08:17

compiler-errors approved these changes Feb 19, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2024

compiler-errors self-assigned this Feb 19, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 19, 2024

bors merged commit 0243834 into rust-lang:master Feb 19, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Feb 19, 2024

This was referenced Feb 19, 2024

more eagerly instantiate binders #119849

Merged

remove sub_relations from the InferCtxt #119989

Merged

[MCP 723] Rename astconv::AstConv and related items #120926

Merged

lcnr deleted the eager-inference-replacement branch February 19, 2024 19:49

fmease mentioned this pull request Feb 20, 2024

Expand weak alias types before collecting constrained/referenced late bound regions + refactorings #121344

Merged

compiler-errors mentioned this pull request Mar 31, 2024

regression: unexpected unsized tail ICE #123275

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

some trait system cleanups #120463

some trait system cleanups #120463

lcnr commented Jan 29, 2024 •

edited

Loading

lcnr commented Jan 29, 2024

This comment has been minimized.

bors commented Jan 29, 2024

compiler-errors commented Jan 29, 2024

This comment has been minimized.

bors commented Jan 29, 2024

compiler-errors commented Jan 29, 2024

bors commented Jan 29, 2024

This comment has been minimized.

rust-timer commented Jan 29, 2024

jackh726 commented Jan 29, 2024

bors commented Jan 29, 2024

jackh726 left a comment •

edited

Loading

jackh726 commented Jan 29, 2024

jackh726 commented Jan 29, 2024

lcnr commented Jan 30, 2024

This comment has been minimized.

lcnr commented Feb 15, 2024

bors commented Feb 17, 2024

compiler-errors left a comment

compiler-errors Feb 19, 2024

lcnr Feb 19, 2024 •

edited

Loading

compiler-errors commented Feb 19, 2024

bors commented Feb 19, 2024

bors commented Feb 19, 2024

bors commented Feb 19, 2024

rust-timer commented Feb 19, 2024

some trait system cleanups #120463

some trait system cleanups #120463

Conversation

lcnr commented Jan 29, 2024 • edited Loading

lcnr commented Jan 29, 2024

This comment has been minimized.

bors commented Jan 29, 2024

compiler-errors commented Jan 29, 2024

This comment has been minimized.

bors commented Jan 29, 2024

compiler-errors commented Jan 29, 2024

bors commented Jan 29, 2024

This comment has been minimized.

rust-timer commented Jan 29, 2024

Overall result: no relevant changes - no action needed

jackh726 commented Jan 29, 2024

bors commented Jan 29, 2024

jackh726 left a comment • edited Loading

Choose a reason for hiding this comment

jackh726 commented Jan 29, 2024

jackh726 commented Jan 29, 2024

lcnr commented Jan 30, 2024

This comment has been minimized.

lcnr commented Feb 15, 2024

bors commented Feb 17, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

compiler-errors commented Feb 19, 2024

bors commented Feb 19, 2024

bors commented Feb 19, 2024

bors commented Feb 19, 2024

rust-timer commented Feb 19, 2024

Overall result: no relevant changes - no action needed

lcnr commented Jan 29, 2024 •

edited

Loading

jackh726 left a comment •

edited

Loading

lcnr Feb 19, 2024 •

edited

Loading