use semantic equality for const param type equality assertion #107940

BoxyUwU · 2023-02-11T23:06:39Z

See added test for what caused this ICE

The current in assertion in relate.rs is rather inadequate when keeping in mind future expansions to const generics:

it will ICE when there are infer vars in a projection in a const param ty
it will spurriously return false when either ty has infer vars because of using == instead of infcx.at(..).eq
i am also unsure if it would be possible with adt_const_params to craft a situation where the const param type is not wf causing normalize_erasing_regions to bug! when we would have emitted a diagnostic.

This impl feels pretty Not Great to me although i am not sure what a better idea would be.

We have to have the logic behind a query because neither relate.rs or combine.rs have access to trait solving machinery (without evaluating nested obligations this assert will become far less useful under lazy norm, which consts are already doing)
relate.rs does not have access to canonicalization machinery which is necessary in order to have types potentially containing infer vars in query arguments.

We could possible add a method to TypeRelation to do this assertion rather than a query but to avoid implementing the same logic over and over we'd probably end up with the logic in a free function somewhere in rustc_trait_selection anyway so I don't think that would be much better.

We could also just remove this assertion, it should not actually be necessary for it to be present. It has caught some bugs in the past though so if possible I would like to keep it.

r? @compiler-errors

BoxyUwU · 2023-02-11T23:11:28Z

@bors try @rust-timer queue

bors · 2023-02-11T23:11:37Z

⌛ Trying commit a85b010 with merge a790aca386d508c3046fdca16a1968508f06510b...

compiler-errors

couple of nits

compiler/rustc_trait_selection/src/traits/misc.rs

bors · 2023-02-12T01:51:55Z

☀️ Try build successful - checks-actions
Build commit: a790aca386d508c3046fdca16a1968508f06510b (a790aca386d508c3046fdca16a1968508f06510b)

rust-timer · 2023-02-12T11:08:34Z

Finished benchmarking commit (a790aca386d508c3046fdca16a1968508f06510b): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.5%	[0.3%, 0.6%]	3
Regressions ❌ (secondary)	4.0%	[4.0%, 4.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	5
All ❌✅ (primary)	0.5%	[0.3%, 0.6%]	3

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)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	4.3%	[1.9%, 7.7%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Cycles

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

BoxyUwU · 2023-02-12T11:30:58Z

wtf??? that benchmark doesnt even relate any consts... theres only like THREE calls to super_combine_consts in that benchmark .-.

@bors try @rust-timer queue

bors · 2023-02-12T11:31:06Z

⌛ Trying commit 0c3a53bc3201f0f0a646fe49ea896f2441c687da with merge 17492353baad22ea6bad6e49df924147f25b83ad...

bors · 2023-02-12T13:43:33Z

☀️ Try build successful - checks-actions
Build commit: 17492353baad22ea6bad6e49df924147f25b83ad (17492353baad22ea6bad6e49df924147f25b83ad)

rust-timer · 2023-02-12T15:01:38Z

Finished benchmarking commit (17492353baad22ea6bad6e49df924147f25b83ad): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.4%	[0.3%, 0.6%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-5.1%, -0.6%]	5
All ❌✅ (primary)	0.4%	[0.3%, 0.6%]	6

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

Cycles

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

compiler-errors · 2023-02-13T00:07:08Z

@bors r+

bors · 2023-02-13T00:07:10Z

📌 Commit 57ad73a has been approved by compiler-errors

It is now in the queue for this repository.

compiler-errors · 2023-02-13T00:09:07Z

There are no calls to that new query in the regressions below -- I'm pretty sure it's still noise..

bors · 2023-02-15T05:18:01Z

⌛ Testing commit 57ad73a with merge 068161e...

bors · 2023-02-15T08:48:37Z

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

rust-timer · 2023-02-15T10:08:36Z

Finished benchmarking commit (068161e): 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.5%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.5%]	5

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)	1.5%	[1.5%, 1.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.8%, -1.2%]	9
All ❌✅ (primary)	-	-	0

Cycles

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

Mark-Simulacrum · 2023-02-21T14:07:43Z

Relatively small significance threshold in a single benchmark -- likely a real regression, but not worth intense scrutiny. Marking as triaged.

Skip over all the nightlies from 2023-02-06 until 2023-02-15 as those ICE when trying to build kani with: ``` error: internal compiler error: cannot relate constants (Const { ty: fn() -> usize {std::mem::size_of::<[T; N]>}, kind: Value(Branch([])) }, Const { ty: fn() -> usize {std::mem::size_of::<[T; _]>}, kind: Value(Branch([])) }) of different types: fn() -> usize {std::mem::size_of::<[T; N]>} != fn() -> usize {std::mem::size_of::<[T; _]>} ``` This issue was reported upstream as rust-lang/rust#107898, and fixed in rust-lang/rust#107940, which isn't part of any of the above nightlies. Doing this multi-day update also requires addressing: Remove some superfluous type parameters from layout.rs rust-lang/rust#107163 Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 s/eval_usize/eval_target_usize/ for clarity rust-lang/rust#108029 Co-authored-by: Qinheping Hu <[email protected]>

make relate's const ty assertion use semantic equality

a85b010

rustbot assigned compiler-errors Feb 11, 2023

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 Feb 11, 2023

BoxyUwU added A-const-generics Area: const generics (parameters and arguments) and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2023

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 Feb 11, 2023

BoxyUwU added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2023

compiler-errors reviewed Feb 11, 2023

View reviewed changes

compiler/rustc_trait_selection/src/traits/misc.rs Outdated Show resolved Hide resolved

compiler/rustc_trait_selection/src/traits/misc.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 12, 2023

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 Feb 12, 2023

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 Feb 12, 2023

rename query and use NoSolution

57ad73a

BoxyUwU force-pushed the const_ty_assertion_use_semantic_equality branch from 0c3a53b to 57ad73a Compare February 12, 2023 19:32

compiler-errors approved these changes Feb 13, 2023

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

compiler-errors mentioned this pull request Feb 13, 2023

Make InferCtxt::can_eq and InferCtxt::can_sub return booleans #108010

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 15, 2023

bors merged commit 068161e into rust-lang:master Feb 15, 2023

rustbot added this to the 1.69.0 milestone Feb 15, 2023

Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Feb 21, 2023

tautschnig mentioned this pull request Apr 15, 2023

Update toolchain to 2023-02-16 model-checking/kani#2379

Merged

3 tasks

tautschnig mentioned this pull request Apr 15, 2023

Update toolchain to 2023-02-16 tautschnig/kani#16

Merged

4 tasks

use semantic equality for const param type equality assertion #107940

use semantic equality for const param type equality assertion #107940

Uh oh!

Conversation

BoxyUwU commented Feb 11, 2023

Uh oh!

BoxyUwU commented Feb 11, 2023

Uh oh!

This comment has been minimized.

bors commented Feb 11, 2023

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bors commented Feb 12, 2023

Uh oh!

This comment has been minimized.

rust-timer commented Feb 12, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Uh oh!

BoxyUwU commented Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

bors commented Feb 12, 2023

Uh oh!

This comment has been minimized.

bors commented Feb 12, 2023

Uh oh!

This comment has been minimized.

rust-timer commented Feb 12, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Uh oh!

compiler-errors commented Feb 13, 2023

Uh oh!

bors commented Feb 13, 2023

Uh oh!

compiler-errors commented Feb 13, 2023

Uh oh!

bors commented Feb 15, 2023

Uh oh!

bors commented Feb 15, 2023

Uh oh!

rust-timer commented Feb 15, 2023

Overall result: ❌ regressions - ACTION NEEDED

Uh oh!

Mark-Simulacrum commented Feb 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

BoxyUwU commented Feb 12, 2023 •

edited

Loading