Make `can_eq` process obligations (almost) everywhere #127172

compiler-errors · 2024-06-30T15:52:57Z

Move can_eq to an extension trait on InferCtxt in rustc_trait_selection, and change it so that it processes obligations. This should strengthen it to be more accurate in some cases, but is most important for the new trait solver which delays relating aliases to AliasRelate goals. Without this, we always basically just return true when passing aliases to can_eq, which can lead to weird errors, for example #127149.

I'm not actually certain if we should have can_eq be called on the good path. In cases where we need can_eq, we probably should just be using a regular probe.

Fixes #127149

r? lcnr

compiler-errors · 2024-06-30T15:53:25Z

@bors try @rust-timer queue

bors · 2024-06-30T15:54:36Z

⌛ Trying commit c09d9dc with merge 65614b9...

…re, r=<try> Make `can_eq` process obligations (almost) everywhere r? lcnr

bors · 2024-06-30T17:36:42Z

☀️ Try build successful - checks-actions
Build commit: 65614b9 (65614b93fa8459085e779581efbb85a823143288)

rust-timer · 2024-06-30T18:57:34Z

Finished benchmarking commit (65614b9): comparison URL.

Overall result: ❌ regressions - 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 is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

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

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: 696.099s -> 696.897s (0.11%)
Artifact size: 324.69 MiB -> 324.61 MiB (-0.02%)

rustbot · 2024-07-01T00:07:48Z

HIR ty lowering was modified

cc @fmease

lcnr

can_eq_shallow only exists for error reporting in rustc_infer, does it? Looking at it for just a few minutes... we should move all of that code into rustc_trait_selection, shouldn't we?

I don't mind doing that after this PR and modifying the fixme, but we already have some weird hacks in the TypeErrCtxt which feel like they should be unnecessary 🤔

lcnr · 2024-07-02T16:07:01Z

compiler/rustc_hir_analysis/src/check/wfcheck.rs

+            };
+            ocx.select_where_possible().is_empty()
+        })
+    };


does wfcx.eq ever fail when can_eq_self returns true? I feel like it can#t and all this code should just use commit_if_ok 🤔 could be a followup

Ok, I just did it in this PR. You should probably read it and convince yourself it's correct, since I did kinda simplify a lot of the control flow which was redundant.

lcnr

alright, after my comment in wfcheck, r=me

lcnr · 2024-07-02T22:00:03Z

compiler/rustc_hir_analysis/src/check/wfcheck.rs


-    // Without `feature(arbitrary_self_types)`, we require that `receiver_ty` implements `Receiver`.
-    if !arbitrary_self_types_enabled
-        && !receiver_is_implemented(wfcx, receiver_trait_def_id, cause.clone(), receiver_ty)


I am very confused by the previous control flow here:

if auto-deref steps returns None and a type still implemented the receiver trait, it was fine? That feels undesirable. receiver_is_implemented could also be converted to a commit_if_ok 🤔 not sure how that impacts diagnostics though

r=me with or without that change

If autoderef returns None then it went into the else branch of the if-let and returned false.

It shouldn't affect diagnostics, since we're still registering the receiver trait, though I don't think it does much since it's a single root obligation

compiler-errors · 2024-07-02T22:04:28Z

Don't think it's worth changing the receiver check rn, since it's just a single trait bound so it doesn't really have the same benefit as doing eq in a commit_if_ok.

@bors r=lcnr

bors · 2024-07-02T22:04:30Z

📌 Commit 3239713 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-07-03T11:44:10Z

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

bors · 2024-07-05T13:52:44Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout full-can_eq-everywhere (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self full-can_eq-everywhere --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

CONFLICT (modify/delete): tests/ui/rfcs/rfc-2632-const-trait-impl/issue-100222.yy.stderr deleted in heads/homu-tmp and modified in HEAD. Version HEAD of tests/ui/rfcs/rfc-2632-const-trait-impl/issue-100222.yy.stderr left in tree.
CONFLICT (modify/delete): tests/ui/rfcs/rfc-2632-const-trait-impl/issue-100222.yn.stderr deleted in heads/homu-tmp and modified in HEAD. Version HEAD of tests/ui/rfcs/rfc-2632-const-trait-impl/issue-100222.yn.stderr left in tree.
CONFLICT (modify/delete): tests/ui/rfcs/rfc-2632-const-trait-impl/issue-100222.ny.stderr deleted in heads/homu-tmp and modified in HEAD. Version HEAD of tests/ui/rfcs/rfc-2632-const-trait-impl/issue-100222.ny.stderr left in tree.
CONFLICT (modify/delete): tests/ui/rfcs/rfc-2632-const-trait-impl/issue-100222.nn.stderr deleted in heads/homu-tmp and modified in HEAD. Version HEAD of tests/ui/rfcs/rfc-2632-const-trait-impl/issue-100222.nn.stderr left in tree.
Auto-merging compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs
Auto-merging compiler/rustc_infer/src/infer/error_reporting/note_and_explain.rs
Auto-merging compiler/rustc_hir_typeck/src/pat.rs
Auto-merging compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
Automatic merge failed; fix conflicts and then commit the result.

lcnr · 2024-07-07T08:27:30Z

@bors r+ rollup=never

bors · 2024-07-07T08:27:32Z

📌 Commit 465e7d5 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-07-07T23:03:52Z

⌛ Testing commit 465e7d5 with merge 89aefb9...

bors · 2024-07-08T01:18:25Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 89aefb9 to master...

rust-timer · 2024-07-08T02:34:34Z

Finished benchmarking commit (89aefb9): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.2%, 0.6%]	25
Regressions ❌ (secondary)	0.8%	[0.7%, 0.9%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.6%]	25

Max RSS (memory usage)

Results (primary 2.1%)

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)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

Cycles

Results (primary -2.6%)

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)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.6%	[-2.6%, -2.6%]	1

Binary size

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

Bootstrap: 699.951s -> 699.119s (-0.12%)
Artifact size: 328.35 MiB -> 328.36 MiB (0.00%)

…, r=<try> Revert "Rework receiver_is_valid" This reverts commit 465e7d5. Testing perf: rust-lang#127172 (comment) r? `@ghost`

Kobzol · 2024-07-09T19:49:12Z

Small regressions across various benchmarks. Not marking as triaged yet, although it seems like #127473 could claw some of the perf. back.

rustbot assigned lcnr Jun 30, 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Jun 30, 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 Jun 30, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 30, 2024

Auto merge of rust-lang#127172 - compiler-errors:full-can_eq-everywhe…

65614b9

…re, r=<try> Make `can_eq` process obligations (almost) everywhere r? lcnr

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 Jun 30, 2024

compiler-errors marked this pull request as ready for review July 1, 2024 00:07

lcnr reviewed Jul 2, 2024

View reviewed changes

compiler-errors force-pushed the full-can_eq-everywhere branch from c09d9dc to 3239713 Compare July 2, 2024 21:22

lcnr reviewed Jul 2, 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 Jul 2, 2024

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 3, 2024

compiler-errors added 2 commits July 5, 2024 11:59

Process alias-relate obligations when proving receiver_is_valid

fdde66a

Actually just make can_eq process obligations (almost) everywhere

fb8d5f1

Rework receiver_is_valid

465e7d5

compiler-errors force-pushed the full-can_eq-everywhere branch from 3239713 to 465e7d5 Compare July 5, 2024 16:00

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 7, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 8, 2024

bors merged commit 89aefb9 into rust-lang:master Jul 8, 2024
7 checks passed

rustbot added this to the 1.81.0 milestone Jul 8, 2024

bors mentioned this pull request Jul 8, 2024

WIP: eliminate DefineOpaqueTypes by using Yes across the compiler #127034

Closed

rustbot added the perf-regression Performance regression. label Jul 8, 2024

compiler-errors mentioned this pull request Jul 8, 2024

Avoid doing redundant work in receiver_is_valid #127473

Closed

compiler-errors mentioned this pull request Jul 8, 2024

Work tracking issue for moving rustc_infer error reporting into rustc_trait_selection #127492

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `can_eq` process obligations (almost) everywhere #127172

Make `can_eq` process obligations (almost) everywhere #127172

compiler-errors commented Jun 30, 2024 •

edited

Loading

compiler-errors commented Jun 30, 2024

This comment has been minimized.

bors commented Jun 30, 2024

bors commented Jun 30, 2024

This comment has been minimized.

rust-timer commented Jun 30, 2024

rustbot commented Jul 1, 2024

lcnr left a comment

lcnr Jul 2, 2024

compiler-errors Jul 2, 2024

lcnr left a comment

lcnr Jul 2, 2024

compiler-errors Jul 2, 2024

compiler-errors commented Jul 2, 2024

bors commented Jul 2, 2024

bors commented Jul 3, 2024

bors commented Jul 5, 2024

lcnr commented Jul 7, 2024

bors commented Jul 7, 2024

bors commented Jul 7, 2024

bors commented Jul 8, 2024

rust-timer commented Jul 8, 2024

Kobzol commented Jul 9, 2024

Make can_eq process obligations (almost) everywhere #127172

Make can_eq process obligations (almost) everywhere #127172

Conversation

compiler-errors commented Jun 30, 2024 • edited Loading

compiler-errors commented Jun 30, 2024

This comment has been minimized.

bors commented Jun 30, 2024

bors commented Jun 30, 2024

This comment has been minimized.

rust-timer commented Jun 30, 2024

Overall result: ❌ regressions - no action needed

rustbot commented Jul 1, 2024

lcnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Jul 2, 2024

bors commented Jul 2, 2024

bors commented Jul 3, 2024

bors commented Jul 5, 2024

lcnr commented Jul 7, 2024

bors commented Jul 7, 2024

bors commented Jul 7, 2024

bors commented Jul 8, 2024

rust-timer commented Jul 8, 2024

Overall result: ❌ regressions - ACTION NEEDED

Kobzol commented Jul 9, 2024

Make `can_eq` process obligations (almost) everywhere #127172

Make `can_eq` process obligations (almost) everywhere #127172

compiler-errors commented Jun 30, 2024 •

edited

Loading