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

[panicbit]

Fixes #48004

[rust-highfive]

r? @alexcrichton

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

[Mark-Simulacrum]

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]

Ok, I marked the impls as stable.

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

Types not implementing Send/Sync:

• unix Args/ArgsOs
• unix Vars
• windows Args/ArgsOs
• windows Vars
• redox Args/ArgsOs
• redox Vars

Types implementing Send/Sync:

• windows Vars
• cloadabi Args/ArgsOs (shim)
• cloudabi Vars (shim)

[Mark-Simulacrum]

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]

Can we not make these types Send and Sync on all platforms?

…

On Sun, Feb 4, 2018 at 3:14 PM Mark Simulacrum ***@***.***> wrote: So this is a breaking change for windows::Vars and cloudabi in general... 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 <https://github.com/bors> try. Meanwhile, @rust-lang/libs <https://github.com/orgs/rust-lang/teams/libs>, what do you think about this change? — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#48005 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABY2UVJWeFMGJ45erjBl7c0-4e5nRRHcks5tRjnBgaJpZM4R4s49> .

[alexcrichton]

@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]

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

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

[alexcrichton]

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?

[kennytm]

@bors try

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

[bors]

⌛ Trying commit b439632 with merge 7046949...

[bors]

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

[alexcrichton]

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

[panicbit]

@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]

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]

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

[alexcrichton]

@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]

📌 Commit b439632 has been approved by alexcrichton

[SteveLauC]

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...