`rustc_infer` cleanups #131226

nnethercote · 2024-10-04T07:04:35Z

Various small improvements I found while reading over this code.

r? @lcnr

rustbot · 2024-10-04T07:04:44Z

changes to the core type system

cc @compiler-errors, @lcnr

changes to the core type system

cc @compiler-errors, @lcnr

lcnr · 2024-10-04T11:23:52Z

compiler/rustc_infer/src/infer/opaque_types/mod.rs

@@ -377,7 +377,7 @@ impl<'tcx> InferCtxt<'tcx> {
 ///
 /// We ignore any type parameters because impl trait values are assumed to
 /// capture all the in-scope type parameters.
-pub struct ConstrainOpaqueTypeRegionVisitor<'tcx, OP: FnMut(ty::Region<'tcx>)> {
+struct ConstrainOpaqueTypeRegionVisitor<'tcx, OP: FnMut(ty::Region<'tcx>)> {
    pub tcx: TyCtxt<'tcx>,


now that the type is private, it's fields can be as well. Also... interesting that we don't have a lint for this case

The unreachable_pub lint was deliberately changed to not warn on this case in #126040, because it was deemed annoying. Specifically:

This PR restrict the unreachable_pub lint by not linting on pub fields of pub(restricted) structs and unions. This is done because that can quickly clutter the code for an uncertain value, in particular since the "real" visibility is defined by the parent (the struct it-self).

The implementation does more than that description indicates -- it also doesn't lint on pub fields of non-pub structs and unions. Perhaps it should?

lcnr

some nits, then r=me

compiler/rustc_infer/src/infer/resolve.rs

bors · 2024-10-05T01:46:22Z

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

It's simpler, for this tiny module.

Three of the modules don't need to be `pub`, and then `warn(unreachable_pub)` identifies a bunch more things that also shouldn't be `pub`, plus a couple of things that are unused.

It helps people reading the code understand how widely things are used.

It's no longer used meaningfully. This also means `DiagCtxtHandle::err_count_excluding_lint_errs` can be removed.

`FixupError` is isomorphic with `TyOrConstInferVar`, so this commit changes it to just be a wrapper around `TyOrConstInferVar`. Also, move the `Display` impl for `FixupError` next to `FixupError`.

It has a single use.

Inline and remove `next_const_var_id`, `next_int_var_id`, `next_float_var_id`, all of which have a single call site.

Matches involving `GenericArgKind` pairs typically use a single `_` for the impossible case. This commit shortens two verbose matches in this way.

nnethercote · 2024-10-06T23:00:27Z

I addressed the comments and rebased.

@bors r=lcnr

bors · 2024-10-06T23:00:29Z

📌 Commit 3fdcde7 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-10-07T03:22:07Z

⌛ Testing commit 3fdcde7 with merge 8841a3d...

bors · 2024-10-07T05:59:59Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 8841a3d to master...

rust-timer · 2024-10-07T08:34:28Z

Finished benchmarking commit (8841a3d): 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.4%, 1.0%]	8
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.2%]	40
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.2%]	15
All ❌✅ (primary)	-0.3%	[-0.5%, -0.2%]	40

Max RSS (memory usage)

Results (primary 4.5%)

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

Cycles

Results (secondary -6.8%)

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)	-6.8%	[-8.9%, -4.2%]	5
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 773.837s -> 775.129s (0.17%)
Artifact size: 329.55 MiB -> 329.51 MiB (-0.01%)

nnethercote · 2024-10-07T21:28:04Z

Huh, those perf results look like they could be genuine improvements? Not sure how that would happen, this commit is the only one I can imagine might have a perf impact, and even then it would be surprising.

lcnr · 2024-10-08T04:30:29Z

compiler/rustc_middle/src/ty/relate.rs

-            (ty::GenericArgKind::Const(unpacked), x) => {
-                bug!("impossible case reached: can't relate: {:?} with {:?}", unpacked, x)
-            }
+            _ => bug!("impossible case reached: can't relate: {a:?} with {b:?}"),


this may also cause the improvement. I would expect that this is some incredibly hot code and likely these paths couldn't get deduplicated because of line information?

…ups, r=lcnr More `rustc_infer` cleanups A sequel to rust-lang#131226. r? `@lcnr`

Rollup merge of rust-lang#131348 - nnethercote:rustc_infer-more-cleanups, r=lcnr More `rustc_infer` cleanups A sequel to rust-lang#131226. r? `@lcnr`

rylev · 2024-10-09T09:59:06Z

Perf improvements outweigh the regressions so marking this as triaged

@rustbot label: +perf-regression-triaged

rustbot assigned lcnr Oct 4, 2024

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 Oct 4, 2024

lcnr reviewed Oct 4, 2024

View reviewed changes

compiler/rustc_infer/src/infer/resolve.rs Show resolved Hide resolved

nnethercote added 12 commits October 7, 2024 09:50

Remove unused features.

cfeee7b

Rename errors/mod.rs as errors.rs.

7a9bbd0

It's simpler, for this tiny module.

Reduce visibilities.

5486d72

Three of the modules don't need to be `pub`, and then `warn(unreachable_pub)` identifies a bunch more things that also shouldn't be `pub`, plus a couple of things that are unused.

Remove unused UnitResult type.

5c015ee

Reduce visibilities some more.

2d2755f

It helps people reading the code understand how widely things are used.

Remove InferCtxt::err_count_on_creation.

25d1ef1

It's no longer used meaningfully. This also means `DiagCtxtHandle::err_count_excluding_lint_errs` can be removed.

Fix FIXME comment on FixupError.

0ca4784

`FixupError` is isomorphic with `TyOrConstInferVar`, so this commit changes it to just be a wrapper around `TyOrConstInferVar`. Also, move the `Display` impl for `FixupError` next to `FixupError`.

Inline and remove InferCtxtBuilder::with_defining_opaque_types.

deeb0c5

It has a single use.

Streamline next_*_var* methods.

3386530

Inline and remove `next_const_var_id`, `next_int_var_id`, `next_float_var_id`, all of which have a single call site.

Simplify two matches.

e800967

Matches involving `GenericArgKind` pairs typically use a single `_` for the impossible case. This commit shortens two verbose matches in this way.

Move a use statement so it's with the other use statements.

afbff05

Remove out-of-date comment.

3fdcde7

nnethercote force-pushed the rustc_infer-cleanups branch from ebd7a1e to 3fdcde7 Compare October 6, 2024 22:59

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 7, 2024

bors merged commit 8841a3d into rust-lang:master Oct 7, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Oct 7, 2024

nnethercote mentioned this pull request Oct 7, 2024

More rustc_infer cleanups #131348

Merged

This was referenced Oct 7, 2024

Introduce SolverRelating type relation to the new solver #131263

Merged

Remove CombineFields #131343

Merged

rustbot added the perf-regression Performance regression. label Oct 7, 2024

nnethercote deleted the rustc_infer-cleanups branch October 7, 2024 21:26

lcnr reviewed Oct 8, 2024

View reviewed changes

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 8, 2024

Rollup merge of rust-lang#131348 - nnethercote:rustc_infer-more-clean…

7cb47b5

…ups, r=lcnr More `rustc_infer` cleanups A sequel to rust-lang#131226. r? `@lcnr`

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 8, 2024

Unrolled build for rust-lang#131348

4373263

Rollup merge of rust-lang#131348 - nnethercote:rustc_infer-more-cleanups, r=lcnr More `rustc_infer` cleanups A sequel to rust-lang#131226. r? `@lcnr`

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rustc_infer` cleanups #131226

`rustc_infer` cleanups #131226

nnethercote commented Oct 4, 2024

rustbot commented Oct 4, 2024

lcnr Oct 4, 2024

nnethercote Oct 7, 2024

lcnr left a comment

bors commented Oct 5, 2024

nnethercote commented Oct 6, 2024

bors commented Oct 6, 2024

bors commented Oct 7, 2024

bors commented Oct 7, 2024

rust-timer commented Oct 7, 2024

nnethercote commented Oct 7, 2024

lcnr Oct 8, 2024

rylev commented Oct 9, 2024

rustc_infer cleanups #131226

rustc_infer cleanups #131226

Conversation

nnethercote commented Oct 4, 2024

rustbot commented Oct 4, 2024

lcnr Oct 4, 2024

Choose a reason for hiding this comment

nnethercote Oct 7, 2024

Choose a reason for hiding this comment

lcnr left a comment

Choose a reason for hiding this comment

bors commented Oct 5, 2024

nnethercote commented Oct 6, 2024

bors commented Oct 6, 2024

bors commented Oct 7, 2024

bors commented Oct 7, 2024

rust-timer commented Oct 7, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

nnethercote commented Oct 7, 2024

lcnr Oct 8, 2024

Choose a reason for hiding this comment

rylev commented Oct 9, 2024

`rustc_infer` cleanups #131226

`rustc_infer` cleanups #131226