Deny hashing ty/re/ct inference variables #103220

compiler-errors · 2022-10-19T01:58:00Z

cc @cjgillot and #102695 (comment)
r? @lcnr

best reviewed one commit at a time, mostly because the second commit that fixes ClosureOutlivesRequirement is mostly noise because of losing its <'tcx> lifetime parameter.

compiler-errors · 2022-10-19T01:59:37Z

src/test/ui/async-await/issues/issue-72312.stderr

@@ -1,22 +1,23 @@
 error[E0521]: borrowed data escapes outside of associated function
-  --> $DIR/issue-72312.rs:12:9
+  --> $DIR/issue-72312.rs:12:24


This UI test changes its output because we used to sort ClosureOutlivesRequirements by Option<Ty<'tcx>> but now sort by Location, thus we now use the span of the first location in the body, which corresponds to the innermost call.

I don't think this is necessarily an issue, though. The error is more readable, imo.

lcnr

the same should hold for const infer vars

lcnr · 2022-10-19T07:43:48Z

compiler/rustc_middle/src/ty/context.rs

-                    let stable_hash = if flags.flags.intersects(TypeFlags::HAS_RE_INFER)
+                    let stable_hash = if flags
+                        .flags
+                        .intersects(TypeFlags::HAS_RE_INFER | TypeFlags::HAS_TY_INFER)


Suggested change

.intersects(TypeFlags::HAS_RE_INFER | TypeFlags::HAS_TY_INFER)

.intersects(TypeFlags::HAS_RE_INFER | TypeFlags::HAS_TY_INFER | TypeFlags::HAS_CT_INFER)

lcnr · 2022-10-19T07:46:41Z

once we also forbid const infer vars

r=me

bors · 2022-10-19T14:48:24Z

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

compiler-errors · 2022-10-19T18:02:19Z

@bors r=lcnr rollup=iffy

bors · 2022-10-19T18:02:21Z

📌 Commit 6b0ef9c has been approved by lcnr

It is now in the queue for this repository.

bors · 2022-10-20T05:56:59Z

⌛ Testing commit 6b0ef9c with merge 4b3b731...

bors · 2022-10-20T08:43:17Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 4b3b731 to master...

rust-timer · 2022-10-20T09:59:58Z

Finished benchmarking commit (4b3b731): 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)	0.8%	[0.5%, 1.2%]	8
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	2
Improvements ✅ (secondary)	-0.9%	[-1.0%, -0.5%]	9
All ❌✅ (primary)	-0.2%	[-0.3%, -0.2%]	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)	4.5%	[2.1%, 6.8%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.6%, -2.4%]	2
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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.5%, -1.9%]	3
All ❌✅ (primary)	-	-	0

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

compiler-errors · 2022-10-25T17:23:59Z

I have no idea why this would introduce a regression 🤔

nnethercote · 2022-10-25T20:11:26Z

The perf changes are small and the wins balance out the losses. No need to stress on this one, I think.

@rustbot label: +perf-regression-triaged

…gillot Don't carry MIR location in `ConstraintCategory::CallArgument` It turns out that `ConstraintCategory::CallArgument` cannot just carry a MIR location in it, since we may bubble them up to totally different MIR bodies. So instead, revert the commit a6b5f95, and instead just erase regions from the original `Option<Ty<'tcx>>` that it carried, so that it doesn't ICE with the changes in rust-lang#103220. Best reviewed in parts -- the first is just a revert, and the second is where the meaningful changes happen. Fixes rust-lang#103624

Deny hashing ty/re/ct inference variables cc `@cjgillot` and rust-lang#102695 (comment) r? `@lcnr` best reviewed one commit at a time, mostly because the second commit that fixes `ClosureOutlivesRequirement` is mostly noise because of losing its `<'tcx>` lifetime parameter.

rust-highfive assigned lcnr Oct 19, 2022

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

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

compiler-errors commented Oct 19, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the deny-infers branch from cac1fb7 to 547b8bd Compare October 19, 2022 02:45

lcnr reviewed Oct 19, 2022

View reviewed changes

rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Oct 19, 2022

compiler-errors changed the title ~~Deny hashing type and region inference variables~~ Deny hashing ty/re/ct inference variables Oct 19, 2022

compiler-errors added 4 commits October 19, 2022 17:10

Don't hash non-fresh Ty::Infer or RegionKind::Infer

91af4f5

Make ClosureOutlivesRequirement not rely on an unresolved type

a6b5f95

Don't call own_existential_vtable_entries on unresolved trait ref

1e2eb97

Deny const variables as well

6b0ef9c

compiler-errors force-pushed the deny-infers branch from 06164fa to 6b0ef9c Compare October 19, 2022 17:13

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 19, 2022

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

bors merged commit 4b3b731 into rust-lang:master Oct 20, 2022

rustbot added this to the 1.66.0 milestone Oct 20, 2022

This was referenced Oct 20, 2022

Place bounds on fields in rustc_macros derives. #102422

Closed

rework applying closure requirements in borrowck #102618

Merged

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

camsteffen mentioned this pull request Oct 24, 2022

Make typeck aware of uninhabited types #100288

Closed

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

compiler-errors mentioned this pull request Oct 27, 2022

Don't carry MIR location in ConstraintCategory::CallArgument #103641

Merged

compiler-errors deleted the deny-infers branch November 2, 2022 02:54

compiler-errors mentioned this pull request Jan 31, 2023

Erase regions before doing uninhabited check in borrowck #107532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deny hashing ty/re/ct inference variables #103220

Deny hashing ty/re/ct inference variables #103220

compiler-errors commented Oct 19, 2022 •

edited

Loading

compiler-errors Oct 19, 2022

This comment has been minimized.

lcnr left a comment

lcnr Oct 19, 2022

lcnr commented Oct 19, 2022

bors commented Oct 19, 2022

compiler-errors commented Oct 19, 2022

bors commented Oct 19, 2022

bors commented Oct 20, 2022

bors commented Oct 20, 2022

rust-timer commented Oct 20, 2022

compiler-errors commented Oct 25, 2022

nnethercote commented Oct 25, 2022

	.intersects(TypeFlags::HAS_RE_INFER \| TypeFlags::HAS_TY_INFER)
	.intersects(TypeFlags::HAS_RE_INFER \| TypeFlags::HAS_TY_INFER \| TypeFlags::HAS_CT_INFER)

Deny hashing ty/re/ct inference variables #103220

Deny hashing ty/re/ct inference variables #103220

Conversation

compiler-errors commented Oct 19, 2022 • edited Loading

compiler-errors Oct 19, 2022

Choose a reason for hiding this comment

This comment has been minimized.

lcnr left a comment

Choose a reason for hiding this comment

lcnr Oct 19, 2022

Choose a reason for hiding this comment

lcnr commented Oct 19, 2022

bors commented Oct 19, 2022

compiler-errors commented Oct 19, 2022

bors commented Oct 19, 2022

bors commented Oct 20, 2022

bors commented Oct 20, 2022

rust-timer commented Oct 20, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

compiler-errors commented Oct 25, 2022

nnethercote commented Oct 25, 2022

compiler-errors commented Oct 19, 2022 •

edited

Loading