Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

panicbit
Copy link
Contributor

@panicbit panicbit commented Feb 4, 2018

Fixes #48004

@rust-highfive
Copy link
Contributor

r? @alexcrichton

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

@Mark-Simulacrum
Copy link
Member

Are we certain that all current platforms explicitly opt-out / don't implement Send/Sync? If so, then this seems fine to me. Please change the annotations to stable, though, as there's no way to gate trait impls today.

@panicbit panicbit force-pushed the env_unimpl_send_sync branch from 514e2a6 to b439632 Compare February 4, 2018 23:00
@panicbit
Copy link
Contributor Author

panicbit commented Feb 4, 2018

Ok, I marked the impls as stable.

Here's an overview over the Send/Sync impls:

Types not implementing Send/Sync:

Types implementing Send/Sync:

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Feb 4, 2018

So this is a breaking change for Windows and CloudABI host compilers... seems like we're going to potentially end up breaking some users here. This should be detectable with a crater run though, so let's get the artifacts for that: @bors try. Meanwhile, @rust-lang/libs, what do you think about this change?

@sfackler
Copy link
Member

sfackler commented Feb 5, 2018 via email

@alexcrichton
Copy link
Member

@panicbit are you sure that there's any platform where these types are Send or Sync? You mentioned in "Types implementing Send/Sync:" that Windows is there but it actually contains a raw pointer which means it's not. Perhaps a test could be added asserting that these types aren't send/sync?

@panicbit
Copy link
Contributor Author

panicbit commented Feb 5, 2018

@alexcrichton Right, Windows' Vars impl actually contains pointers.

I'm not sure wether it's fine to have them impl Send + Sync.

@alexcrichton
Copy link
Member

Yes It's currently intentionally conservative that they aren't send/sync I believe, but I don't believe this is a breaking change as mentioned here?

@pietroalbini pietroalbini added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-team DEPRECATED: Use the team-based variants `S-waiting-on-t-lang`, `S-waiting-on-t-compiler`, ... S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Feb 5, 2018
@kennytm
Copy link
Member

kennytm commented Feb 5, 2018

@bors try

Please don't include any punctuations when commanding bors, they don't like that 😉.

@bors
Copy link
Collaborator

bors commented Feb 5, 2018

⌛ Trying commit b439632 with merge 7046949...

bors added a commit that referenced this pull request Feb 5, 2018
Unimplement Send/Sync for ::env::{Args,ArgsOs,Vars,VarsOs}

Fixes #48004
@bors
Copy link
Collaborator

bors commented Feb 5, 2018

☀️ Test successful - status-travis
State: approved= try=True

@alexcrichton
Copy link
Member

@panicbit just to confirm, you still think this is a breaking change, right? (I'm still not so sure myself)

@panicbit
Copy link
Contributor Author

panicbit commented Feb 12, 2018

@alexcrichton I never expressed the thought of this being a breaking change. I don't think it's really breaking as the current state of things is already broken / unspecified. Though, I'm unsure wether it's best to force Send/Sync or !Send/!Sync.

If someone familiar with the Windows internals could confirm that the Windows types can be Send/Sync (without adding locks), I'd lean towards making everything Send/Sync.

@alexcrichton
Copy link
Member

Ah ok sorry I think I misunderstood. Perhaps a test could be added to assert these aren't sync/send and then we're good to go? I can confirm the windows types are not send/sync, and we can add in in later if we want.

@BatmanAoD
Copy link
Member

@alexcrichton @panicbit Looks like this is waiting on further investigation for Windows, and not waiting on crater?

@alexcrichton
Copy link
Member

@BatmanAoD oh nah I was originally thinking we'd want a test for this, but on second thought I think this is good to go

@bors: r+

@bors
Copy link
Collaborator

bors commented Feb 14, 2018

📌 Commit b439632 has been approved by alexcrichton

@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-crater Status: Waiting on a crater run to be completed. S-waiting-on-team DEPRECATED: Use the team-based variants `S-waiting-on-t-lang`, `S-waiting-on-t-compiler`, ... labels Feb 14, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Feb 14, 2018
…excrichton

Unimplement Send/Sync for ::env::{Args,ArgsOs,Vars,VarsOs}

Fixes rust-lang#48004
bors added a commit that referenced this pull request Feb 14, 2018
bors added a commit that referenced this pull request Feb 15, 2018
bors added a commit that referenced this pull request Feb 15, 2018
@kennytm kennytm merged commit b439632 into rust-lang:master Feb 15, 2018
bors added a commit that referenced this pull request Apr 9, 2018
Correct a few stability attributes

* `const_indexing` language feature was stabilized in 1.26.0 by #46882
* `Display` impls for `PanicInfo` and `Location` were stabilized in 1.26.0 by #47687
* `TrustedLen` is still unstable so its impls should be as well even though `RangeInclusive` was stabilized by #47813
* `!Send` and `!Sync` for `Args` and `ArgsOs` were stabilized in 1.26.0 by #48005
* `EscapeDefault` has been stable since 1.0.0 so should continue to show that even though it was moved to core in #48735

This could be backported to beta like #49612
@SteveLauC
Copy link
Contributor

Is this comment still true? For cli arguments, both unix and windows are defining Args as:

pub struct Args {
    args: vec::IntoIter<OsString>,
}

which seems to be Send and Sync

I am not sure about that cliabi platform as I didn't find it in the source code...

Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 24, 2025
Move `sys::pal::os::Env` into `sys::env`

Although `Env` (as `Vars`), `Args`, path functions, and OS constants are publicly exposed via `std::env`, their implementations are each self-contained. Keep them separate in `std::sys` and make a new module, `sys::env`, for `Env`.

Also fix `unsafe_op_in_unsafe_fn` for Unix and update the `!DynSend` and `!DynSync` impls which had grown out of sync with the platforms (see rust-lang#48005 for discussion on that).

r? joboet

Tracked in rust-lang#117276.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 25, 2025
Move `sys::pal::os::Env` into `sys::env`

Although `Env` (as `Vars`), `Args`, path functions, and OS constants are publicly exposed via `std::env`, their implementations are each self-contained. Keep them separate in `std::sys` and make a new module, `sys::env`, for `Env`.

Also fix `unsafe_op_in_unsafe_fn` for Unix and update the `!DynSend` and `!DynSync` impls which had grown out of sync with the platforms (see rust-lang#48005 for discussion on that).

r? joboet

Tracked in rust-lang#117276.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2025
Rollup merge of rust-lang#140143 - thaliaarchi:move-env-pal, r=joboet

Move `sys::pal::os::Env` into `sys::env`

Although `Env` (as `Vars`), `Args`, path functions, and OS constants are publicly exposed via `std::env`, their implementations are each self-contained. Keep them separate in `std::sys` and make a new module, `sys::env`, for `Env`.

Also fix `unsafe_op_in_unsafe_fn` for Unix and update the `!DynSend` and `!DynSync` impls which had grown out of sync with the platforms (see rust-lang#48005 for discussion on that).

r? joboet

Tracked in rust-lang#117276.
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request May 9, 2025
Move `sys::pal::os::Env` into `sys::env`

Although `Env` (as `Vars`), `Args`, path functions, and OS constants are publicly exposed via `std::env`, their implementations are each self-contained. Keep them separate in `std::sys` and make a new module, `sys::env`, for `Env`.

Also fix `unsafe_op_in_unsafe_fn` for Unix and update the `!DynSend` and `!DynSync` impls which had grown out of sync with the platforms (see rust-lang#48005 for discussion on that).

r? joboet

Tracked in rust-lang#117276.
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-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Send and Sync impls of ::env types depend on implementation

10 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.