Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

celinval
Copy link
Contributor

@celinval celinval commented Jan 10, 2024

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
Copy link
Collaborator

rustbot commented Jan 10, 2024

r? @ouz-a

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

@rustbot 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
@celinval
Copy link
Contributor Author

r? @oli-obk

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

@rustbot rustbot assigned oli-obk and unassigned ouz-a Jan 15, 2024
compiler/rustc_smir/src/rustc_internal/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_smir/src/rustc_internal/mod.rs Outdated Show resolved Hide resolved
Comment on lines 53 to 68
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)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@celinval celinval Jan 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Let me fix that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 17, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 17, 2024

📌 Commit ac66570 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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
/// 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

oli-obk commented Jan 17, 2024

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

@bors 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 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
- Move fix to a separate PR
@celinval
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@rustbot 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
@oli-obk
Copy link
Contributor

oli-obk commented Jan 18, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jan 18, 2024

📌 Commit 6a573cb has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2024
…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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2024
…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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
…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
@bors bors merged commit c3e237c into rust-lang:master Jan 18, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 18, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
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.
celinval added a commit to celinval/stable-mir-dev that referenced this pull request Jan 22, 2024
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.
celinval added a commit to celinval/stable-mir-dev that referenced this pull request Jan 22, 2024
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.
oli-obk pushed a commit to rust-lang/project-stable-mir that referenced this pull request Jan 26, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants