Track implicit `Sized` obligations in type params #98816

estebank · 2022-07-02T18:16:51Z

When we evaluate ty::GenericPredicates we introduce the implicit
Sized predicate of type params, but we do so with only the Predicate
its Span as context, we don't have an Obligation or
ObligationCauseCode we could influence. To try and carry this
information through, we add a new field to ty::GenericPredicates that
tracks both which predicates come from a type param and whether that
param has any bounds already (to use in suggestions).

We also suggest adding a ?Sized bound if appropriate on E0599.

Address part of #98539.

rust-highfive · 2022-07-02T18:16:54Z

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

estebank · 2022-07-02T18:17:29Z

@bors try @rust-timer queue

rust-timer · 2022-07-02T18:17:30Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-02T18:17:36Z

⌛ Trying commit 35c8de37a381d04518bc264b7ce77540e60248e2 with merge ad41a769c55008b2c294b5a07fecd82cb7dcba8c...

estebank · 2022-07-02T18:17:40Z

r? @oli-obk

bors · 2022-07-02T19:52:23Z

💔 Test failed - checks-actions

estebank · 2022-07-02T20:33:41Z

@bors try @rust-timer queue

rust-timer · 2022-07-02T20:33:42Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-02T20:33:48Z

⌛ Trying commit 8d080c6627d392ebb85974416bf97d00c5931105 with merge 4b74390cde4e98d80560abd60516653939311c30...

bors · 2022-07-02T22:00:04Z

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

rust-timer · 2022-07-02T22:00:06Z

Queued 4b74390cde4e98d80560abd60516653939311c30 with parent 750d6f8, future comparison URL.

rust-timer · 2022-07-03T03:36:25Z

Finished benchmarking commit (4b74390cde4e98d80560abd60516653939311c30): comparison url.

Instruction count

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	0.6%	2.1%	58
Regressions 😿 (secondary)	1.1%	3.5%	28
Improvements 🎉 (primary)	-0.2%	-0.2%	3
Improvements 🎉 (secondary)	-0.4%	-0.6%	8
All 😿🎉 (primary)	0.6%	2.1%	61

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	1.9%	6.4%	11
Regressions 😿 (secondary)	2.4%	3.4%	5
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.2%	-2.2%	1
All 😿🎉 (primary)	1.9%	6.4%	11

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	2.8%	2.8%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.3%	-3.3%	1
All 😿🎉 (primary)	2.8%	2.8%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

compiler/rustc_typeck/src/check/method/suggest.rs

oli-obk · 2022-07-04T11:49:16Z

The keccak regression is essentially just

<ObligationForest<PendingPredicateObligation>>::process_obligations::<FulfillProcessor, Outcome<PendingPredicateObligation, FulfillmentErrorCode>>

I'm guessing this is a caching issue now, as we can't ignore the new information like we could if it were part of the obligation cause. A hacky way forward would be to remove (canonicalize?) this information when invoking queries. But my root concern (adding such information to the predicate when it isn't actually used to make language decisions) still stands, and I don't have an obviously good solution for the problem here.

One thing we could try is to add a magical trait ExplicitlySized: Sized that we add in addition to explicit Sized bounds. When diagnostics are trying to add Sized bounds, we can check for ExplicitlySized and avoid suggesting Sized in that case.

Hmm... one thing I don't know if we talked about: have you attempted looking at the HIR to figure out if there's a Sized bound already at the time of diagnostic reporting?

estebank · 2022-07-06T20:55:07Z

There we go hir hiking :)

oli-obk · 2022-07-07T07:30:56Z

compiler/rustc_typeck/src/check/method/suggest.rs

-                    ty::Adt(def, _) if def.did().is_local() => {
-                        tcx.def_ident_span(def.did()).map(|span| span)
-                    }
+                    ty::Adt(def, _) => Some(tcx.def_span(def.did())),


this causes a lot of ui test noise and I don't think any of it is actually an improvement?

I can revert.

It still seems to be there?

The only part of the change that is still there is the removal of the unnecessary map call, the match guard is now present.

you also changed def_ident_span to def_span, which now points to the entire item, which is what I meant originally 😆 I didn't even notice the match guard.

I changed it to be consistent with other places where we use def_span so that multiple span labels pointing at the same thing don't stagger. After a recent change def_span points only at the signature and not the whole item, so it is suitable to use now. I didn't move all to be def_ident_span because that would break the derive suggestions (I'm planning on reducing the span in the future, but not in this PR).

This is an example of what I'm saying:

error[E0599]: the method `get` exists for struct `Victim<'_, Self>`, but its trait bounds were not satisfied --> $DIR/impl-derived-implicit-sized-bound.rs:31:19 | LL | struct Victim<'a, T: Perpetrator + ?Sized> | ------------------------------------------ | | | method `get` not found for this struct | doesn't satisfy `Victim<'_, Self>: VictimTrait`

Without the change the label for "method get not found for this struct" would point at the ident, making the visual presentation a bit less... clean.

compiler/rustc_trait_selection/src/traits/wf.rs

Suggest adding a `?Sized` bound if appropriate on E0599 by inspecting the HIR Generics. (Fix rust-lang#98539)

…`MiscObligation`

oli-obk · 2022-07-08T15:53:51Z

@bors r+

bors · 2022-07-08T15:53:53Z

📌 Commit 385c793 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-07-08T23:17:46Z

⌛ Testing commit 385c793 with merge 47575bb...

bors · 2022-07-09T01:58:20Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 47575bb to master...

rust-timer · 2022-07-09T03:25:56Z

Finished benchmarking commit (47575bb): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	1.7%	2.0%	4
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	1.2%	2.2%	2
Regressions 😿 (secondary)	4.5%	4.5%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	1.2%	2.2%	2

Cycles

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	2.2%	2.3%	2
Regressions 😿 (secondary)	3.4%	4.4%	4
Improvements 🎉 (primary)	-3.0%	-3.0%	1
Improvements 🎉 (secondary)	-3.0%	-3.3%	2
All 😿🎉 (primary)	0.5%	-3.0%	3

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

oli-obk · 2022-07-09T10:25:42Z

@rustbot label:perf-regression-triaged

Just noise, #98961 had the same perf diff, just as an improvement

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 2, 2022

rust-highfive assigned petrochenkov Jul 2, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 2, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 2, 2022

rust-highfive assigned oli-obk and unassigned petrochenkov Jul 2, 2022

This comment has been minimized.

Sign in to view

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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2022

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 3, 2022

kadiwa4 reviewed Jul 3, 2022

View reviewed changes

compiler/rustc_typeck/src/check/method/suggest.rs Outdated Show resolved Hide resolved

estebank force-pushed the implicit-sized branch from 96ce528 to 52753fc Compare July 6, 2022 20:54

This comment was marked as resolved.

Sign in to view

estebank force-pushed the implicit-sized branch from 52753fc to f62ebf4 Compare July 7, 2022 00:22

This comment was marked as outdated.

Sign in to view

estebank force-pushed the implicit-sized branch from f62ebf4 to 7353aac Compare July 7, 2022 03:57

oli-obk reviewed Jul 7, 2022

View reviewed changes

estebank added 3 commits July 7, 2022 11:24

Add test for rust-lang#98539

d5642ac

Track implicit Sized obligations in type params

af10a45

Suggest adding a `?Sized` bound if appropriate on E0599 by inspecting the HIR Generics. (Fix rust-lang#98539)

Track WellFormed obligations with an appropriate cause, instead of …

385c793

…`MiscObligation`

estebank force-pushed the implicit-sized branch from 7353aac to 385c793 Compare July 7, 2022 19:15

estebank removed the perf-regression Performance regression. label Jul 8, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 9, 2022

bors merged commit 47575bb into rust-lang:master Jul 9, 2022

rustbot added this to the 1.64.0 milestone Jul 9, 2022

This was referenced Jul 9, 2022

Add TyAlias into rustc_type_ir TyKind enum #97974

Closed

stop using FnCtxt outside of hir typeck #99015

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track implicit `Sized` obligations in type params #98816

Track implicit `Sized` obligations in type params #98816

estebank commented Jul 2, 2022

rust-highfive commented Jul 2, 2022

estebank commented Jul 2, 2022

rust-timer commented Jul 2, 2022

bors commented Jul 2, 2022

estebank commented Jul 2, 2022

This comment has been minimized.

bors commented Jul 2, 2022

This comment has been minimized.

estebank commented Jul 2, 2022

rust-timer commented Jul 2, 2022

bors commented Jul 2, 2022

This comment has been minimized.

bors commented Jul 2, 2022

rust-timer commented Jul 2, 2022

rust-timer commented Jul 3, 2022

oli-obk commented Jul 4, 2022

estebank commented Jul 6, 2022

This comment was marked as resolved.

This comment was marked as outdated.

oli-obk Jul 7, 2022

estebank Jul 7, 2022

estebank Jul 7, 2022

oli-obk Jul 7, 2022

estebank Jul 8, 2022

oli-obk Jul 8, 2022

estebank Jul 8, 2022

estebank Jul 8, 2022

oli-obk commented Jul 8, 2022

bors commented Jul 8, 2022

bors commented Jul 8, 2022

bors commented Jul 9, 2022

rust-timer commented Jul 9, 2022

oli-obk commented Jul 9, 2022

Track implicit Sized obligations in type params #98816

Track implicit Sized obligations in type params #98816

Conversation

estebank commented Jul 2, 2022

rust-highfive commented Jul 2, 2022

estebank commented Jul 2, 2022

rust-timer commented Jul 2, 2022

bors commented Jul 2, 2022

estebank commented Jul 2, 2022

This comment has been minimized.

bors commented Jul 2, 2022

This comment has been minimized.

estebank commented Jul 2, 2022

rust-timer commented Jul 2, 2022

bors commented Jul 2, 2022

This comment has been minimized.

bors commented Jul 2, 2022

rust-timer commented Jul 2, 2022

rust-timer commented Jul 3, 2022

Footnotes

oli-obk commented Jul 4, 2022

estebank commented Jul 6, 2022

This comment was marked as resolved.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jul 8, 2022

bors commented Jul 8, 2022

bors commented Jul 8, 2022

bors commented Jul 9, 2022

rust-timer commented Jul 9, 2022

Footnotes

oli-obk commented Jul 9, 2022

Track implicit `Sized` obligations in type params #98816

Track implicit `Sized` obligations in type params #98816