Reconstify `Add` #120381

fee1-dead · 2024-01-26T15:30:32Z

r? project-const-traits

I'm not happy with the ui test changes (or failures because I did not bless them and include the diffs in this PR). There is at least some bugs I need to look and try fix:

A third duplicated diagnostic when a consumer crate that does not have effects enabled has a trait selection error for an upstream const_trait trait. See tests/ui/ufcs/ufcs-qpath-self-mismatch.rs.
For some reason, making Add a const trait would stop us from suggesting T: Add when we try to add two Ts without that bound. See tests/ui/suggestions/issue-97677.rs

fee1-dead · 2024-01-27T16:16:51Z

the first item seems to be reverting a previous const_trait_impl change: 191d3b7#diff-bcbca6666787ee0e6f033b0ba775250deb6d0cfb58d2f2195e11635f2e00ad11

fee1-dead · 2024-01-27T16:24:12Z

making as ready for review, see the latest commits

fee1-dead · 2024-01-29T09:42:28Z

.. forgot to rename the title

compiler-errors · 2024-01-29T18:39:28Z

tests/ui/ufcs/ufcs-qpath-self-mismatch.stderr

@@ -11,16 +11,29 @@ LL |     <i32 as Add<u32>>::add(1, 2);
             <&'a i32 as Add<i32>>
             <&i32 as Add<&i32>>

+error[E0277]: cannot add `u32` to `i32`


It's fine that this error message is duplicated imo.

compiler-errors · 2024-01-29T18:40:23Z

tests/ui/suggestions/invalid-bin-op.stderr

@@ -16,6 +16,10 @@ help: consider annotating `S<T>` with `#[derive(PartialEq)]`
 LL + #[derive(PartialEq)]
 LL | struct S<T>(T);
   |
+help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
+   |
+LL | pub fn foo<T>(s: S<T>, t: S<T>) where S<T>: PartialEq {


Why is this being suggested now?

It was removed in a const-traits PR: d464dd0#diff-a4f9511c26556adec6fac6f4073f446a12e9c9c883b0fd5ae672956caa5836c9 in #118661. This has been suggested before PartialEq gained the effect param, and adding the effect param in that commit made PartialEq not suggestable (which this PR makes it suggestable again)

compiler-errors · 2024-01-29T18:40:50Z

r=me on the changes, though I am curious why that new suggestion is being suggested. Can we suppress it?

fee1-dead · 2024-02-01T13:59:27Z

I don't know why that suggestion exists, but I don't think it is related to const traits (per my latest comment).

@rustbot ready

compiler-errors · 2024-02-01T15:09:45Z

cool

@bors r+

bors · 2024-02-01T15:09:48Z

📌 Commit 2e1e574 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-02-01T15:09:48Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

…r=compiler-errors Reconstify `Add` r? project-const-traits I'm not happy with the ui test changes (or failures because I did not bless them and include the diffs in this PR). There is at least some bugs I need to look and try fix: 1. A third duplicated diagnostic when a consumer crate that does not have `effects` enabled has a trait selection error for an upstream const_trait trait. See tests/ui/ufcs/ufcs-qpath-self-mismatch.rs. 2. For some reason, making `Add` a const trait would stop us from suggesting `T: Add` when we try to add two `T`s without that bound. See tests/ui/suggestions/issue-97677.rs

matthiaskrgr · 2024-02-03T21:02:22Z

@bors r-
seems to be failing in #120622 (comment) ?

can this have some perf impact, I wonder if we should rollup iffy or never.

it works when a non-const context that does not enable effects calls into a const effects-enabled trait. We'd simply suggest the non-const trait bound in this case consistent to its fallback.

these ui changes are closer to what was there before const_trait_impl changes.

fee1-dead · 2024-02-04T03:36:44Z

I have blessed the test. Don't think this could have a large perf impact, but it might because of adding a generic param though.

@bors rollup=iffy

fee1-dead · 2024-02-07T15:20:07Z

Blessing is a minor change, don't think it needs to be reviewed again

@bors r=compiler-errors

bors · 2024-02-07T15:20:10Z

📌 Commit 74dbf3a has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-02-08T00:04:17Z

⌛ Testing commit 74dbf3a with merge 6894f43...

bors · 2024-02-08T02:07:11Z

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

rust-timer · 2024-02-08T04:30:07Z

Finished benchmarking commit (6894f43): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -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.4%, 0.5%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.4%, 0.5%]	2

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

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

Binary size

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

Bootstrap: 662.945s -> 663.461s (0.08%)
Artifact size: 308.24 MiB -> 308.30 MiB (0.02%)

rustbot assigned compiler-errors Jan 26, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 26, 2024

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the reconstify-add branch from 8284875 to 2e1e574 Compare January 27, 2024 16:23

fee1-dead marked this pull request as ready for review January 27, 2024 16:23

fee1-dead changed the title ~~[WIP] Reconstify Add~~ Reconstify Add Jan 29, 2024

compiler-errors approved these changes Jan 29, 2024

View reviewed changes

compiler-errors reviewed Jan 29, 2024

View reviewed changes

compiler-errors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2024

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

fee1-dead mentioned this pull request Feb 3, 2024

Tracking Issue for removing impl const and ~const in the standard library #110395

Closed

4 tasks

matthiaskrgr mentioned this pull request Feb 3, 2024

Rollup of 7 pull requests #120622

Closed

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

Reconstify Add

c9192be

fee1-dead added 2 commits February 4, 2024 11:30

make effect infer variables suggestable in diagnostics

96108c5

it works when a non-const context that does not enable effects calls into a const effects-enabled trait. We'd simply suggest the non-const trait bound in this case consistent to its fallback.

fix ui tests

74dbf3a

these ui changes are closer to what was there before const_trait_impl changes.

fee1-dead force-pushed the reconstify-add branch from 2e1e574 to 74dbf3a Compare February 4, 2024 03:34

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

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

bors merged commit 6894f43 into rust-lang:master Feb 8, 2024

rustbot added this to the 1.78.0 milestone Feb 8, 2024

fee1-dead deleted the reconstify-add branch February 14, 2024 12:22

matthiaskrgr mentioned this pull request Feb 24, 2024

effects: add: ConstContext::Maybe must have host effect param #121536

Closed

GrigorenkoPV mentioned this pull request Jun 30, 2024

[const_closures, const_mut_refs, effects] thread 'rustc' panicked at rustc_middle/src/ty/util.rs:945:22: ConstContext::Maybe must have host effect param #125866

Closed

Reconstify Add #120381

Reconstify Add #120381

Uh oh!

Conversation

fee1-dead commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

fee1-dead commented Jan 27, 2024

Uh oh!

fee1-dead commented Jan 27, 2024

Uh oh!

fee1-dead commented Jan 29, 2024

Uh oh!

compiler-errors Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

compiler-errors Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

fee1-dead Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Jan 29, 2024

Uh oh!

fee1-dead commented Feb 1, 2024

Uh oh!

compiler-errors commented Feb 1, 2024

Uh oh!

bors commented Feb 1, 2024

Uh oh!

bors commented Feb 1, 2024

Uh oh!

matthiaskrgr commented Feb 3, 2024

Uh oh!

fee1-dead commented Feb 4, 2024

Uh oh!

fee1-dead commented Feb 7, 2024

Uh oh!

bors commented Feb 7, 2024

Uh oh!

bors commented Feb 8, 2024

Uh oh!

bors commented Feb 8, 2024

Uh oh!

rust-timer commented Feb 8, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Uh oh!

Reconstify `Add` #120381

Reconstify `Add` #120381

fee1-dead commented Jan 26, 2024 •

edited

Loading