Make tcx optional from StableMIR run macro and extend it to accept closures #119833

celinval · 2024-01-10T20:28:18Z

Change run macro to avoid sometimes unnecessary dependency on TyCtxt, and introduce run_with_tcx to capture use cases where tcx is required. Additionally, extend both macros to accept closures that may capture variables.

I've also modified the internal() method to make it safer, by accepting the type context to force the 'tcx lifetime to match the context lifetime.

These are non-backward compatible changes, but they only affect internal APIs which are provided today as helper functions until we have a stable API to start the compiler.

rustbot · 2024-01-10T20:28:26Z

r? @ouz-a

(rustbot has picked a reviewer for you, use r? to override)

celinval · 2024-01-15T19:08:13Z

r? @oli-obk

@oli-obk do you have time to review this one? Thanks

compiler/rustc_smir/src/rustc_internal/mod.rs

oli-obk · 2024-01-16T09:02:22Z

compiler/rustc_smir/src/rustc_internal/mod.rs

 pub fn internal<'tcx, S: RustcInternal<'tcx>>(item: S) -> S::T {
    with_tables(|tables| item.internal(tables))
 }

+/// Retrieve the internal Rust compiler type context.
+///
+/// # Warning
+///
+/// This function is unstable, and it's behavior may change at any point.
+///
+/// # Panics
+///
+/// This function will panic if StableMIR has not been properly initialized.
+pub fn tcx<'tcx>() -> TyCtxt<'tcx> {
+    with_tables(|tables| tables.tcx)
+}


These two functions are unsound. They trivially allow you to create a TyCtxt<'static> or a Ty<'static> and move it outside the run! invocation in safe code.

The only way I see them becoming safe is to use the with_tables scheme of passing a closure that then gets the item as an argument. This way we can have control over how long the item lives

Indeed. Let me fix that!

I removed tcx() and added the type context as an argument to internal() to avoid the issue you just mentioned.

Even though the new solution is a bit hacky, I think its safer and more usable than the with_tables solution. Let me know what you think.

Oh that's even better!

Simplify the `run` macro to avoid sometimes unnecessary dependency on `TyCtxt`. Instead, users can use the new internal method `tcx()`. Additionally, extend the macro to accept closures that may capture variables. These are non-backward compatible changes, but they only affect internal APIs which are provided today as helper functions until we have a stable API to start the compiler.

I added `tcx` argument to `internal` to force 'tcx to be the same lifetime as TyCtxt. The only other solution I could think is to change this function to be `unsafe`.

compiler/rustc_smir/src/rustc_internal/mod.rs

oli-obk · 2024-01-17T09:42:18Z

@bors r+ rollup

bors · 2024-01-17T09:42:20Z

📌 Commit ac66570 has been approved by oli-obk

It is now in the queue for this repository.

bjorn3 · 2024-01-17T10:11:41Z

compiler/rustc_smir/src/rustc_internal/mod.rs

+/// This function will panic if StableMIR has not been properly initialized.
+pub fn internal<'tcx, S: RustcInternal<'tcx>>(tcx: TyCtxt<'tcx>, item: S) -> S::T {
+    // The tcx argument ensures that the item won't outlive the type context.
+    let _ = tcx;


There can be multiple TyCtxt at the same time. You did have to use Lift to ensure that the item comes from the right TyCtxt.

can we actually have them at the same time? Or just after each other? (though I guess the same issue applies there)

But, that is not an issue here, because once the stable mir context shuts down, there is no more mapping of the stable mir type. You may get the wrong rustc type, but it will still be within the TyCtxt.

can we actually have them at the same time?

Yes, you can a new TyCtxt and enter it and then with access to that TyCtxt it should be possible to create a new stable mir context and pass the TyCtxt you entered yourself as argument to internal(). This would allow the returned value outlive the TyCtxt of the stable mir context despite referencing values that are being freed with the rest of the stable mir context.

Ok... I'm amazed our TLS doesn't panic, but just replace the old tls tcx with the new one and switch it back afterwards. I did not expect that.

Just to make sure I understand, there could exist 2 or more TyCtxt in the same thread, and an item that was interned in one TyCtxt might outlive the type in a different one.

In that case, we should invoke lift using the TyCtxt that was passed to the intern function. Is that correct?

It also looks like not everything that implements RustcInternal, implements Lift, so the intern call will have to be done in the RustcInternal implementation for types that require so.

If both facts are true, my plan is to mark the RustcInternal trait as unsafe and add TyCtxt as an argument to RustcInternal::internal as well. It will be up to the RustcInternal implementation to invoke lift.

BTW, this change won't be small. Since the changes in this PR are now unrelated to this issue, can we go ahead and merge the current PR and I can create a follow up PR with the solution I described above. I can even revert adding tcx argument to the internal function if you prefer and add it in the next one.

oli-obk · 2024-01-17T11:18:24Z

@bors r- I guess we do need to use tcx.lift(value).unwrap() to ensure that we're using the right TyCtxt

- Move fix to a separate PR

celinval · 2024-01-18T04:01:29Z

@oli-obk I reverted all changes related to internal() from this PR as I'm going to create a separate PR for it. Please let me know if there is any other change you think should be done regarding this PR. Thanks!

celinval · 2024-01-18T04:04:49Z

@rustbot label +S-waiting-on-review -S-waiting-on-author

oli-obk · 2024-01-18T05:52:44Z

@bors r+

bors · 2024-01-18T05:52:46Z

📌 Commit 6a573cb has been approved by oli-obk

It is now in the queue for this repository.

…li-obk Make tcx optional from StableMIR run macro and extend it to accept closures Change `run` macro to avoid sometimes unnecessary dependency on `TyCtxt`, and introduce `run_with_tcx` to capture use cases where `tcx` is required. Additionally, extend both macros to accept closures that may capture variables. I've also modified the `internal()` method to make it safer, by accepting the type context to force the `'tcx` lifetime to match the context lifetime. These are non-backward compatible changes, but they only affect internal APIs which are provided today as helper functions until we have a stable API to start the compiler.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#119172 (Detect `NulInCStr` error earlier.) - rust-lang#119833 (Make tcx optional from StableMIR run macro and extend it to accept closures) - rust-lang#119955 (Modify GenericArg and Term structs to use strict provenance rules) - rust-lang#120021 (don't store const var origins for known vars) - rust-lang#120038 (Don't create a separate "basename" when naming and opening a MIR dump file) - rust-lang#120057 (Don't ICE when deducing future output if other errors already occurred) - rust-lang#120073 (Remove spastorino from users_on_vacation) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#119172 (Detect `NulInCStr` error earlier.) - rust-lang#119833 (Make tcx optional from StableMIR run macro and extend it to accept closures) - rust-lang#119967 (Add `PatKind::Err` to AST/HIR) - rust-lang#119978 (Move async closure parameters into the resultant closure's future eagerly) - rust-lang#120021 (don't store const var origins for known vars) - rust-lang#120038 (Don't create a separate "basename" when naming and opening a MIR dump file) - rust-lang#120057 (Don't ICE when deducing future output if other errors already occurred) - rust-lang#120073 (Remove spastorino from users_on_vacation) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#119833 - celinval:smir-accept-closures, r=oli-obk Make tcx optional from StableMIR run macro and extend it to accept closures Change `run` macro to avoid sometimes unnecessary dependency on `TyCtxt`, and introduce `run_with_tcx` to capture use cases where `tcx` is required. Additionally, extend both macros to accept closures that may capture variables. I've also modified the `internal()` method to make it safer, by accepting the type context to force the `'tcx` lifetime to match the context lifetime. These are non-backward compatible changes, but they only affect internal APIs which are provided today as helper functions until we have a stable API to start the compiler.

The following PRs needed to be addressed: - rust-lang/rust#119833 - rust-lang/rust#119790 Note that the rustc tests are no longer passing, but the failures are unrelated to these changes and need further investigation.

rustbot assigned ouz-a Jan 10, 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 Jan 10, 2024

rustbot assigned oli-obk and unassigned ouz-a Jan 15, 2024

oli-obk reviewed Jan 16, 2024

View reviewed changes

celinval added 2 commits January 16, 2024 11:17

Remove tcx function and make internal fn safer

2564811

I added `tcx` argument to `internal` to force 'tcx to be the same lifetime as TyCtxt. The only other solution I could think is to change this function to be `unsafe`.

celinval force-pushed the smir-accept-closures branch from 8041889 to 2564811 Compare January 16, 2024 22:36

This comment has been minimized.

Sign in to view

celinval commented Jan 17, 2024

View reviewed changes

compiler/rustc_smir/src/rustc_internal/mod.rs Outdated Show resolved Hide resolved

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 Jan 17, 2024

bjorn3 reviewed Jan 17, 2024

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 17, 2024

celinval changed the title ~~Remove tcx from StableMIR run macro and accept closures~~ Make tcx optional from StableMIR run macro and extend it to accept closures Jan 17, 2024

celinval force-pushed the smir-accept-closures branch from ac66570 to cc4ac83 Compare January 18, 2024 03:58

Revert changes to internal method for now

6a573cb

- Move fix to a separate PR

celinval force-pushed the smir-accept-closures branch from cc4ac83 to 6a573cb Compare January 18, 2024 04:00

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 18, 2024

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 Jan 18, 2024

matthiaskrgr mentioned this pull request Jan 18, 2024

Rollup of 8 pull requests #120087

Closed

matthiaskrgr mentioned this pull request Jan 18, 2024

Rollup of 7 pull requests #120088

Closed

matthiaskrgr mentioned this pull request Jan 18, 2024

Rollup of 8 pull requests #120089

Merged

bors merged commit c3e237c into rust-lang:master Jan 18, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 18, 2024

celinval mentioned this pull request Jan 22, 2024

Fix compilation after changes to trait and run rust-lang/project-stable-mir#60

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make tcx optional from StableMIR run macro and extend it to accept closures #119833

Make tcx optional from StableMIR run macro and extend it to accept closures #119833

celinval commented Jan 10, 2024 •

edited

rustbot commented Jan 10, 2024

celinval commented Jan 15, 2024

oli-obk Jan 16, 2024

celinval Jan 16, 2024 •

edited

celinval Jan 16, 2024

oli-obk Jan 17, 2024

This comment has been minimized.

oli-obk commented Jan 17, 2024

bors commented Jan 17, 2024

bjorn3 Jan 17, 2024

oli-obk Jan 17, 2024

oli-obk Jan 17, 2024

bjorn3 Jan 17, 2024

oli-obk Jan 17, 2024

celinval Jan 17, 2024

celinval Jan 17, 2024

oli-obk commented Jan 17, 2024

celinval commented Jan 18, 2024

celinval commented Jan 18, 2024

oli-obk commented Jan 18, 2024

bors commented Jan 18, 2024

Make tcx optional from StableMIR run macro and extend it to accept closures #119833

Make tcx optional from StableMIR run macro and extend it to accept closures #119833

Conversation

celinval commented Jan 10, 2024 • edited

rustbot commented Jan 10, 2024

celinval commented Jan 15, 2024

Choose a reason for hiding this comment

celinval Jan 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

oli-obk commented Jan 17, 2024

bors commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jan 17, 2024

celinval commented Jan 18, 2024

celinval commented Jan 18, 2024

oli-obk commented Jan 18, 2024

bors commented Jan 18, 2024

celinval commented Jan 10, 2024 •

edited

celinval Jan 16, 2024 •

edited