add `TypingMode::Borrowck` #138785

lcnr · 2025-03-21T13:19:00Z

Shares the first commit with #138499, doesn't really matter which PR to land first 😊 😁

Introduces TypingMode::Borrowck which unlike TypingMode::Analysis, uses the hidden type computed by HIR typeck as the initial value of opaques instead of an unconstrained infer var. This is a part of rust-lang/types-team#129.

Using this new TypingMode is unfortunately a breaking change for now, see tests/ui/impl-trait/non-defining-uses/as-projection-term.rs. Using an inference variable as the initial value results in non-defining uses in the defining scope. We therefore only enable it if with -Znext-solver=globally or -Ztyping-mode-borrowck

To do that the PR contains the following changes:

TypeckResults::concrete_opaque_type are already mapped to the definition of the opaque type
- writeback now checks that the non-lifetime parameters of the opaque are universal
- for this, fn check_opaque_type_parameter_valid is moved from rustc_borrowck to rustc_trait_selection
we add a new query type_of_opaque_hir_typeck which, using the same visitors as MIR typeck, attempts to merge the hidden types from HIR typeck from all defining scopes
- done by adding a DefiningScopeKind flag to toggle between using borrowck and HIR typeck
- the visitors stop checking that the MIR type matches the HIR type. This is trivial as the HIR type are now used as the initial hidden types of the opaque. This check is useful as a safeguard when not using TypingMode::Borrowck, but adding it to the new structure is annoying and it's not soundness critical, so I intend to not add it back.
add a TypingMode::Borrowck which behaves just like TypingMode::Analysis except when normalizing opaque types
- it uses type_of_opaque_hir_typeck(opaque) as the initial value after replacing its regions with new inference vars
- it uses structural lookup in the new solver

fixes #112201, fixes #132335, fixes #137751

r? @compiler-errors @oli-obk

add `TypingMode::Borrowck` Still not quite ready Based on rust-lang#138492 and rust-lang#138719 r? `@compiler-errors` `@oli-obk`

increment depth of nested obligations properly fixes the root cause of rust-lang#109268. While we didn't get hangs here before, I ended up encountering its root cause again with rust-lang#138785. r? types

lcnr · 2025-04-01T21:41:42Z

@rustbot ready

rustbot · 2025-04-01T21:41:46Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

Some changes occurred in exhaustiveness checking

cc @Nadrieril

lcnr · 2025-04-01T21:53:46Z

tests/ui/impl-trait/issue-55872.rs

        //~^ ERROR type parameter `T` is part of concrete type but not used in parameter list for the `impl Trait` type alias
+        || ()


unfortunate side-effect from HIR typeck eagerly replacing the return type with an inference var. This will be partially fixed when removing this eager normalization call. The new solver does point to the closure instead.

However, as we're still normalizing away opaques to infer vars during generalization, there may still be span regressions where we point at the place introducing the opaque instead of the place which actually constrains it. This feels unavoidable without major changes to the way we track spans for opaques/in the type system in general.

OK, yeah that seems like the major fallout of this PR (along w/ the bad spans for mismatched hidden types).

I guess most of these will not be a problem w/ RPIT.

(along w/ the bad spans for mismatched hidden types).

which happens for the same reason :3 In MIR tpyeck, we equate each return statement with the return type and constrain the opaque there.

In HIR typeck, we replace the opaque with an infer var at the span of the return type, so that's the span used for its defining use.

tests/ui/impl-trait/issue-99073-2.rs

lcnr · 2025-04-01T21:59:31Z

@rust-timer build d7cf428

Rollup merge of rust-lang#139022 - lcnr:incr-obligation-depth, r=oli-obk increment depth of nested obligations properly fixes the root cause of rust-lang#109268. While we didn't get hangs here before, I ended up encountering its root cause again with rust-lang#138785. r? types

…errors small opaque type/borrowck cleanup pulled out of rust-lang#138785

Rollup merge of rust-lang#139191 - lcnr:interner-opaques, r=compiler-errors small opaque type/borrowck cleanup pulled out of rust-lang#138785

lcnr · 2025-04-02T09:35:20Z

@bors try @rust-timer queue

add `TypingMode::Borrowck` First two commits are from rust-lang#139191. Introduces `TypingMode::Borrowck` which unlike `TypingMode::Analysis`, uses the hidden type computed by HIR typeck as the initial value of opaques instead of an unconstrained infer var. This is a part of rust-lang/types-team#129. Using this new `TypingMode` is unfortunately a breaking change for now, see tests/ui/impl-trait/non-defining-uses/as-projection-term.rs. Using an inference variable as the initial value results in non-defining uses in the defining scope. We therefore only enable it if with `-Znext-solver=globally` or `-Ztyping-mode-borrowck` To do that the PR contains the following changes: - `TypeckResults::concrete_opaque_type` are already mapped to the definition of the opaque type - writeback now checks that the non-lifetime parameters of the opaque are universal - for this, `fn check_opaque_type_parameter_valid` is moved from `rustc_borrowck` to `rustc_trait_selection` - we add a new `query type_of_opaque_hir_typeck` which, using the same visitors as MIR typeck, attempts to merge the hidden types from HIR typeck from all defining scopes - done by adding a `DefiningScopeKind` flag to toggle between using borrowck and HIR typeck - the visitors stop checking that the MIR type matches the HIR type. This is trivial as the HIR type are now used as the initial hidden types of the opaque. This check is useful as a safeguard when not using `TypingMode::Borrowck`, but adding it to the new structure is annoying and it's not soundness critical, so I intend to not add it back. - add a `TypingMode::Borrowck` which behaves just like `TypingMode::Analysis` except when normalizing opaque types - it uses `type_of_opaque_hir_typeck(opaque)` as the initial value after replacing its regions with new inference vars - it uses structural lookup in the new solver fixes rust-lang#112201, fixes rust-lang#132335, fixes rust-lang#137751 r? `@compiler-errors` `@oli-obk`

bors · 2025-04-02T09:36:33Z

⌛ Trying commit 6e4f81e with merge 423064c...

bors · 2025-04-02T11:42:20Z

☀️ Try build successful - checks-actions
Build commit: 423064c (423064ca6a6975340d4e353b5271613f585eda43)

rust-timer · 2025-04-02T13:05:11Z

Finished benchmarking commit (423064c): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.4%, 0.6%]	4
Regressions ❌ (secondary)	0.4%	[0.2%, 0.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	0.5%	[0.4%, 0.6%]	4

Max RSS (memory usage)

Results (secondary -3.9%)

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

Cycles

Results (secondary 0.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)	1.8%	[1.5%, 2.6%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.5%	[-3.5%, -3.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 775.08s -> 776.357s (0.16%)
Artifact size: 365.94 MiB -> 365.90 MiB (-0.01%)

lcnr · 2025-04-02T13:33:05Z

slightly worse perf, not sure why. Might be due to another variant in the TypingMode enum and the additional type_of_opaque_hir_typeck query? 🤔 this impact is acceptable to me, so I'd like to merge this as is

compiler-errors · 2025-04-03T00:25:59Z

tests/ui/generic-associated-types/issue-87258_a.stderr

+LL |     fn foo<'a>() -> Self::FooFuture<'a> {
+   |        ^^^
+   |
+   = note: consider removing `#[define_opaque]` or adding an empty `#[define_opaque()]`


This note will need tweaking

Yeah, that's existing though and the suggestiono to add an empty #[define_opaque()] is applicable for ATPIT afaik? Or at least it should be

Yes, the "consider removing" part should be dependent on the existence of the attr, tho.

compiler-errors · 2025-04-03T00:27:01Z

tests/ui/impl-trait/issue-55872.rs

        //~^ ERROR type parameter `T` is part of concrete type but not used in parameter list for the `impl Trait` type alias
+        || ()


OK, yeah that seems like the major fallout of this PR (along w/ the bad spans for mismatched hidden types).

I guess most of these will not be a problem w/ RPIT.

compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs

we already collect opaque types from nested items during `mir_borrowck` of the root, checking that they are consistent this way.

oli-obk

makes all sense to me

oli-obk · 2025-04-03T09:55:13Z

compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs

            } else {
-                hir_opaque_ty = Some(concrete_type);
+                let hir_ty = tcx.type_of_opaque_hir_typeck(def_id).instantiate_identity();


When do we go down this route? If borrowck was tainted, we don't get here. If borrowck didn't find the opaque constraint because it was only in typeck, that's allowed for RPIT, but shouldn't we have gotten the HIR type in borrowck now that all the HIR types are seeding the borrowck table?

Well, this is reachable if neither -Ztyping-mode-borrowck or -Znext-solver=globally are enabled, right?

We're not seeding the borrowck table anymore, we only seed the default value when encountering an opaque. If the only uses of the opaque are in dead code, we still don't have any opaque in the opaque type storage in the old solver. cc #112417

I think the new solver may actually never reach this as we should try to eagerly normalize the fn sig which defines the opaque? But for now having only uses in dead code does reach this

rustbot assigned compiler-errors Mar 21, 2025

rustbot added A-tidy S-waiting-on-review T-bootstrap T-compiler T-libs WG-trait-system-refactor labels Mar 21, 2025