Check hidden types in dead code #102700

oli-obk · 2022-10-05T09:32:38Z

best reviewed commit by commit

compiler-errors · 2022-10-05T17:54:50Z

compiler/rustc_borrowck/src/region_infer/opaque_types.rs

        match self.map.get(&r.into()).map(|k| k.unpack()) {
            Some(GenericArgKind::Lifetime(r1)) => r1,
            Some(u) => panic!("region mapped to unexpected kind: {:?}", u),
            None if self.do_not_error => self.tcx.lifetimes.re_static,
-            None if generics.parent.is_some() => {
-                if let Some(hidden_ty) = self.hidden_ty.take() {


Why did this code exist? What did it achieve?

Oh, well it was dead code anyways. I should read the commit title.

bors · 2022-10-07T18:29:18Z

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

compiler-errors · 2022-10-13T18:51:50Z

I literally wrote up a comment that had bors r+ but it's on a computer that I don't have access to for a day or two.

Anyways this code makes sense to me and thanks for splitting it up into easily reviewable commits.

@bors r+ rollup=never (just in case we want to bisect to this PR)

bors · 2022-10-13T18:51:51Z

📌 Commit 3c8b46c has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2022-10-13T22:39:09Z

⌛ Testing commit 3c8b46c with merge 60bd3f9...

bors · 2022-10-14T01:27:55Z

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

rust-timer · 2022-10-14T04:04:37Z

Finished benchmarking commit (60bd3f9): comparison URL.

Overall result: ❌✅ regressions and improvements - 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
Regressions ❌ (secondary)	1.2%	[1.0%, 1.3%]	4
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	13
Improvements ✅ (secondary)	-0.6%	[-0.7%, -0.5%]	4
All ❌✅ (primary)	-0.3%	[-0.4%, -0.2%]	13

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.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-2.7%	[-3.2%, -2.2%]	14
All ❌✅ (primary)	-2.2%	[-2.2%, -2.2%]	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)	2.3%	[1.1%, 3.8%]	4
Regressions ❌ (secondary)	2.7%	[1.7%, 3.6%]	12
Improvements ✅ (primary)	-2.3%	[-2.5%, -2.0%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[-2.5%, 3.8%]	6

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

oli-obk · 2022-10-14T07:38:48Z

This is a small improvement for everything but the ctfe stress test. The stress test regressed in actual CTFE (though it also added a few additional query calls to specializes). The only reason for the longer CTFE time that I can see is that typeck's result now contains more data potentially causing more hashing/comparisons in the impl Trait heavy part of the stress test. Either way, I think we should ignore it considering this is now actually checking the right thing, so being more expensive is somewhat expected.

@rustbot label: +perf-regression-triaged

Check hidden types in dead code fixes rust-lang#99490 r? `@compiler-errors` best reviewed commit by commit

…ck-bad, r=oli-obk Restrict recursive opaque type check We have a recursive opaque check in writeback to avoid inferring the hidden of an opaque type to be itself: https://github.com/rust-lang/rust/blob/33a2c2487ac5d9927830ea4c1844335c6b9f77db/compiler/rustc_hir_typeck/src/writeback.rs#L556-L575 Issue rust-lang#113619 treats `make_option2` as not defining the TAIT `TestImpl` since it is inferred to have the definition `TestImpl := B<TestImpl>`, which fails this check. This regressed in rust-lang#102700 (5d15beb), I think due to the refactoring that made us record the hidden types of TAITs during writeback. However, nothing actually seems to go bad if we relax this recursion checker to only check for directly recursive definitions. This PR fixes rust-lang#113619 by changing this recursive check from being a visitor to just checking that the hidden type is exactly the same as the opaque being inferred. Alternatively, we may be able to fix rust-lang#113619 by restricting this recursion check only to RPITs/async fns. It seems to only be possible to use misuse the recursion check to cause ICEs for TAITs (though I didn't try too hard to create a bad RPIT example... may be possible, actually.) r? `@oli-obk` -- Fixes rust-lang#113314

rust-highfive assigned compiler-errors Oct 5, 2022

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

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

compiler-errors reviewed Oct 5, 2022

View reviewed changes

oli-obk added 7 commits October 7, 2022 19:36

Unconditionally encode hidden types in typeck results

5d15beb

Remove some dead code

d643ae1

Move ReverseMapper logic onto OpaqueHiddenType

9eb69e8

Remap hidden types from typeck before storing them in the TypeckResult

70d39ab

Check hidden types in dead code

f85d3a7

Add a regression test

e07f36c

typeck result is now affected by opaque type spans

3c8b46c

oli-obk force-pushed the 0xDEAD_TAIT branch from 8a8318b to 3c8b46c Compare October 7, 2022 19:48

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 Oct 13, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 14, 2022

bors merged commit 60bd3f9 into rust-lang:master Oct 14, 2022

rustbot added this to the 1.66.0 milestone Oct 14, 2022

rustbot added the perf-regression Performance regression. label Oct 14, 2022

oli-obk deleted the 0xDEAD_TAIT branch October 14, 2022 07:33

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 14, 2022

compiler-errors mentioned this pull request Oct 16, 2022

Cannot resolve TAIT when used with impl/dyn Trait inside RPIT #101750

Closed

skyzh mentioned this pull request Oct 23, 2022

large number of obligation produced in async code causing compiler stuck #103423

Closed

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023

Auto merge of rust-lang#102700 - oli-obk:0xDEAD_TAIT, r=compiler-errors

5d5bb36

Check hidden types in dead code fixes rust-lang#99490 r? `@compiler-errors` best reviewed commit by commit

compiler-errors mentioned this pull request Jul 13, 2023

Restrict recursive opaque type check #113636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check hidden types in dead code #102700

Check hidden types in dead code #102700

oli-obk commented Oct 5, 2022

compiler-errors Oct 5, 2022

compiler-errors Oct 5, 2022

bors commented Oct 7, 2022

compiler-errors commented Oct 13, 2022

bors commented Oct 13, 2022

bors commented Oct 13, 2022

bors commented Oct 14, 2022

rust-timer commented Oct 14, 2022

oli-obk commented Oct 14, 2022

Check hidden types in dead code #102700

Check hidden types in dead code #102700

Conversation

oli-obk commented Oct 5, 2022

compiler-errors Oct 5, 2022

Choose a reason for hiding this comment

compiler-errors Oct 5, 2022

Choose a reason for hiding this comment

bors commented Oct 7, 2022

compiler-errors commented Oct 13, 2022

bors commented Oct 13, 2022

bors commented Oct 13, 2022

bors commented Oct 14, 2022

rust-timer commented Oct 14, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

oli-obk commented Oct 14, 2022