Use `TypingMode` throughout the compiler instead of `ParamEnv` #132460

lcnr · 2024-11-01T14:34:57Z

Hopefully the biggest single PR as part of rust-lang/types-team#128.

`infcx.typing_env` while defining opaque types

I don't know how'll be able to correctly handle opaque types when using something taking a TypingEnv while defining opaque types. To correctly handle the opaques we need to be able to pass in the current opaque_type_storage and return constraints, i.e. we need to use a proper canonical query. We should migrate all the queries used during HIR typeck and borrowck where this matters to proper canonical queries. This is

`layout_of` and `Reveal::All`

We convert the ParamEnv to Reveal::All right at the start of the layout_of query, so I've changed callers of layout_of to already use a post analysis TypingEnv when encountering it.

rust/compiler/rustc_ty_utils/src/layout.rs

Line 51 in ca87b53

let param_env = param_env.with_reveal_all_normalized(tcx);

`Ty::is_[unpin|sized|whatever]`

I haven't migrated fn is_item_raw to use TypingEnv, will do so in a followup PR, this should significantly reduce the amount of typing_env.param_env. At some point there will probably be zero such uses as using the type system while ignoring the typing_mode is incorrect.

`MirPhase` and phase-transitions

When inside of a MIR-body, we can mostly use its MirPhase to figure out the right typing_mode. This does not work during phase transitions, most notably when transitioning from Analysis to Runtime:

rust/compiler/rustc_mir_transform/src/lib.rs

Lines 606 to 625 in dae7ac1

    
           // These next passes must be executed together. 
        
           &add_call_guards::CriticalCallEdges, 
        
           // Must be done before drop elaboration because we need to drop opaque types, too. 
        
           &reveal_all::RevealAll, 
        
           // Calling this after reveal_all ensures that we don't deal with opaque types. 
        
           &add_subtyping_projections::Subtyper, 
        
           &elaborate_drops::ElaborateDrops, 
        
           // This will remove extraneous landing pads which are no longer 
        
           // necessary as well as forcing any call in a non-unwinding 
        
           // function calling a possibly-unwinding function to abort the process. 
        
           &abort_unwinding_calls::AbortUnwindingCalls, 
        
           // AddMovesForPackedDrops needs to run after drop 
        
           // elaboration. 
        
           &add_moves_for_packed_drops::AddMovesForPackedDrops, 
        
           // `AddRetag` needs to run after `ElaborateDrops` but before `ElaborateBoxDerefs`. 
        
           // Otherwise it should run fairly late, but before optimizations begin. 
        
           &add_retag::AddRetag, 
        
           &elaborate_box_derefs::ElaborateBoxDerefs, 
        
           &coroutine::StateTransform, 
        
           &Lint(known_panics_lint::KnownPanicsLint),

All these passes still run with MirPhase::Analysis, but we should only use Reveal::All once we're run the RevealAll pass. This required me to manually construct the right TypingEnv in all these passes. Given that it feels somewhat easy to accidentally miss this going forward, I would maybe like to change Body::phase to an Option and replace it at the start of phase transitions. This then makes it clear that the MIR is currently in a weird state.

r? @ghost

bors · 2024-11-01T22:37:52Z

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

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

lcnr · 2024-11-08T00:01:57Z

PseudoCanonicalInput may end up defining opaque types now, drop that from the typing mode

compiler/rustc_middle/src/ty/mod.rs

lcnr · 2024-11-08T15:14:10Z

@bors try @rust-timer queue

bors · 2024-11-08T15:15:24Z

⌛ Trying commit 1d78b92 with merge dcde587...

`TypingMode` sure seems like a good idea ## `infcx.typing_env` while defining opaque types I don't know how'll be able to correctly handle opaque types when using something taking a `TypingEnv` while defining opaque types. To correctly handle the opaques we need to be able to pass in the current `opaque_type_storage` and return constraints, i.e. we need to use a proper canonical query. We should migrate all the queries used during HIR typeck and borrowck where this matters to proper canonical queries. This is ## `layout_of` and `Reveal::All` We convert the `ParamEnv` to `Reveal::All` right at the start of the `layout_of` query, so I've changed callers of `layout_of` to already use a post analysis `TypingEnv` when encountering it. https://github.com/rust-lang/rust/blob/ca87b535a05097df6abbe2a031b057de2cefac5b/compiler/rustc_ty_utils/src/layout.rs#L51 r? `@ghost`

RalfJung · 2024-11-15T05:58:18Z

compiler/rustc_const_eval/src/interpret/eval_context.rs

        debug_assert_eq!(self.param_env.reveal(), Reveal::All);
-        TypingMode::PostAnalysis
+        ty::TypingEnv { typing_mode: ty::TypingMode::PostAnalysis, param_env: self.param_env }


This is also used by Miri. So if Miri should use TypingMode::fully_monomorphized(), then something seems wrong here?

the param_env is always empty in MIRI, so ty::TypingEnv { typing_mode: ty::TypingMode::PostAnalysis, param_env: self.param_env } is equal to ty::TypingEnv::fully_monomorphized()

RalfJung · 2024-11-15T13:38:02Z

src/tools/miri/src/eval.rs

+        rustc_span::DUMMY_SP,
+        typing_env.param_env,
+        MiriMachine::new(config, layout_cx)
+    );


Suggested change

);

);

assert_eq!(ecx.typing_env(), typing_env);

Does this make sense? Currently it seems a bit odd that we only pass part of the typing env to InterpCx.

The assert should hold, I don't like adding that assert from a style perspective however

a question here is the following:

const evaluation should always run in the PostAnalysis/Reveal::All mode, so we don't need to store the TypingMode in the interpcx

we still want to assert that users of const eval pass in the correct typing_env and don't accidentally switch to PostAnalysis without noticing

not sure about the final result from my chat with @compiler-errors, but I think we probably want to store a full TypingEnv in the InterpCx and const eval in general and just assert that the TypingMode is PostAnalysis. Storing another 16? bytes in the context really shouldn't matter I think

I don't like adding that assert from a style perspective however

It seems currently there is a non-local invariant encoded by how typing_env() is implemented on InterpCx. I'd like to ensure that we realize if this ever breaks.

we still want to assert that users of const eval pass in the correct typing_env and don't accidentally switch to PostAnalysis without noticing

That sounds reasonable, but that's a different assertion -- that would be somewhere in the const_eval queries. This here is about Miri.

I think we probably want to store a full TypingEnv in the InterpCx and const eval in general and just assert that the TypingMode is PostAnalysis. Storing another 16? bytes in the context really shouldn't matter I think

I'm fine with that as well.

do you mind me changing InterpCx to contain the typing_env directly in a followup PR?

Yeah feel free to stage this however.

compiler/rustc_middle/src/ty/mod.rs

compiler-errors · 2024-11-17T23:03:29Z

compiler/rustc_hir_analysis/src/check/wfcheck.rs

@@ -1126,7 +1126,7 @@ fn check_type_defn<'tcx>(
                    let ty = tcx.type_of(variant.tail().did).instantiate_identity();
                    let ty = tcx.erase_regions(ty);
                    assert!(!ty.has_infer());
-                    ty.needs_drop(tcx, tcx.param_env(item.owner_id))
+                    ty.needs_drop(tcx, ty::TypingEnv::non_body_analysis(tcx, item.owner_id.def_id))


why not wfcx.infcx.typing_env?

compiler-errors · 2024-11-17T23:05:19Z

compiler/rustc_hir_typeck/src/upvar.rs

@@ -1257,7 +1257,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
    ) -> Option<FxIndexSet<UpvarMigrationInfo>> {
        let ty = self.resolve_vars_if_possible(self.node_ty(var_hir_id));

-        if !ty.has_significant_drop(self.tcx, self.tcx.param_env(closure_def_id)) {
+        // FIXME(#132279): Using `non_body_analysis` here feels wrong.


i think i agree -- canonicalizing here would mean we can peek through opaques, which seems good?

compiler/rustc_hir_typeck/src/writeback.rs

compiler/rustc_infer/src/infer/mod.rs

compiler/rustc_mir_build/src/build/expr/as_rvalue.rs

compiler/rustc_middle/src/ty/context.rs

compiler-errors · 2024-11-17T23:21:11Z

i left some questions, but i think this can be rebased one more time, and i think i can do a final review of the delta, then ill approve after?

the behavior of the type system not only depends on the current assumptions, but also the currentnphase of the compiler. This is mostly necessary as we need to decide whether and how to reveal opaque types. We track this via the `TypingMode`.

compiler-errors · 2024-11-18T16:18:05Z

@bors r+ rollup=never p=1

bors · 2024-11-18T16:18:08Z

📌 Commit 2e087d2 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-11-18T21:07:10Z

⌛ Testing commit 2e087d2 with merge b71fb5e...

bors · 2024-11-19T00:18:28Z

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

rust-timer · 2024-11-19T01:36:25Z

Finished benchmarking commit (b71fb5e): comparison URL.

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

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.2%	[0.1%, 0.4%]	8
Regressions ❌ (secondary)	0.9%	[0.2%, 2.7%]	6
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-0.8%	[-0.9%, -0.7%]	5
All ❌✅ (primary)	0.1%	[-0.3%, 0.4%]	9

Max RSS (memory usage)

Results (primary 0.6%, secondary 2.4%)

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)	1.5%	[1.1%, 2.2%]	3
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[-2.1%, 2.2%]	4

Cycles

Results (secondary -2.0%)

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)	2.4%	[1.6%, 3.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.2%	[-5.1%, -3.5%]	4
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 789.97s -> 792.184s (0.28%)
Artifact size: 334.45 MiB -> 334.48 MiB (0.01%)

pnkfelix · 2024-11-20T02:39:08Z

Visiting for rustc-perf triage

primary regressions to unicode-normalization profiles and (to a lesser extent) diesel, stm32f4, and cargo.
it seems like this regression was to some extent anticipated, at least from the perf run that was done while the PR was in development.

@lcnr and @compiler-errors , is that consistent with your view of the situation? The regression here seems like is probably acceptable (and perhaps just yet another instance of measurement bias in our perf system), but I don't want to mark this case as triaged without at least getting an ACK from either of you.

lcnr · 2024-11-20T08:55:23Z

I think that a small negative perf hit is expected here but unavoidable. We need to track additional information in multiple queries

Fix required due to the following changes to Rust's internal API: - rust-lang/rust#132460 - rust-lang/rust#133212 - rust-lang/rust#131326 Resolves #3731 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses. --------- Co-authored-by: Zyad Hassan <88045115+zhassan-aws@users.noreply.github.com>

…rors Use `TypingMode` throughout the compiler instead of `ParamEnv` Hopefully the biggest single PR as part of rust-lang/types-team#128. ## `infcx.typing_env` while defining opaque types I don't know how'll be able to correctly handle opaque types when using something taking a `TypingEnv` while defining opaque types. To correctly handle the opaques we need to be able to pass in the current `opaque_type_storage` and return constraints, i.e. we need to use a proper canonical query. We should migrate all the queries used during HIR typeck and borrowck where this matters to proper canonical queries. This is ## `layout_of` and `Reveal::All` We convert the `ParamEnv` to `Reveal::All` right at the start of the `layout_of` query, so I've changed callers of `layout_of` to already use a post analysis `TypingEnv` when encountering it. https://github.com/rust-lang/rust/blob/ca87b535a05097df6abbe2a031b057de2cefac5b/compiler/rustc_ty_utils/src/layout.rs#L51 ## `Ty::is_[unpin|sized|whatever]` I haven't migrated `fn is_item_raw` to use `TypingEnv`, will do so in a followup PR, this should significantly reduce the amount of `typing_env.param_env`. At some point there will probably be zero such uses as using the type system while ignoring the `typing_mode` is incorrect. ## `MirPhase` and phase-transitions When inside of a MIR-body, we can mostly use its `MirPhase` to figure out the right `typing_mode`. This does not work during phase transitions, most notably when transitioning from `Analysis` to `Runtime`: https://github.com/rust-lang/rust/blob/dae7ac133b9eda152784c075facb31a6688c92b1/compiler/rustc_mir_transform/src/lib.rs#L606-L625 All these passes still run with `MirPhase::Analysis`, but we should only use `Reveal::All` once we're run the `RevealAll` pass. This required me to manually construct the right `TypingEnv` in all these passes. Given that it feels somewhat easy to accidentally miss this going forward, I would maybe like to change `Body::phase` to an `Option` and replace it at the start of phase transitions. This then makes it clear that the MIR is currently in a weird state. r? `@ghost`