Meeting Link: https://meet.jit.si/rust-libs-meeting-crxoz2at8hiccp7b3ixf89qgxfymlbwr
Attendees:
13 rust-lang/rust T-libs-api FCPs
BurntSushi (10), joshtriplett (7), dtolnay (3), m-ou-se (7), Amanieu (5), yaahc (4)
Unresolved question: is it okay/good that .set()
skips the initialization? To make this work:
Mara: If it does exactly T.with(|x| x.set(value))
does, then it first gets initialized and then overwritten. T.set(value)
instead skips the default initialization.
David: This seems pretty nice.
Amanieu: Don't like it.
JoshT: Could be a subtle user trap.
Mara: I can't think of any situation where this would cause a bug.
JoshT: Maybe the initializer should be const. {examples of things intializers can do.} Skipping it might result in something go wrong.
David: I can see initializers do that, but not sutuations where you'd also use .set().
JoshT: What's the downside of just always running the initializer, other than losing the panic!() use case?
Mara: The panic use case is exactly the case I care about.
David: Yeah I like that use case. This basically gives us the thread-local version of OnceCell, but without needing new type/api.
The 8472: Would it help to rename it to reinit
or something?
Mara: Regardless of whether we have a .init()
, I don't think .set()
should make it do "init and destroy and set".
Amanieu: Lazy doesn't have .set(). If it had, would it skip initialization?
Mara: OnceCell does have .set() that skips init.
JoshT: In your panic use case, would you call .set() just once or multiple times?
Mara: For the ID use case, just once. (Should probably use a OnceCell.) But for other use cases perhaps multiple times.
JoshT: Should there be a set_once
or some other name that implies that you can only call it once and it bypasses initialization?
Mara: That's OnceCell, but now we're talking about a thread local Cell.
JoshT: So what's the use case for setting multiple times?
Mara: Example: output capturing. thread local dyn Write
. The default init could open a buffer or file, but if you .set() it before the first print you don't want to directly allocate/open and dealloc/close.
JoshT: That's a valid case.
JoshT: Suppose you do want lazy init and then set?
Mara: Then you just call .with(|x| x.set(..))
.
The 8472: Or .replace()
.
JoshT: Okay, I do think it might be error prone, but I agree that the situation where it matters is very unusual and involves the user doing something exceedingly "clever", in which case they hopefully read the docs. So I'd be fine with it if the docs are clear.
The 8472: They can also wrap the type if they don't want the method.
Amanieu: Agree with Josh. I don't think this will cause issues in practice. If you're exploiting this edge case you're aware of it.
Mara: Docs are clear, including the panic use case: https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set
FCP kicked off: https://github.com/rust-lang/rust/issues/92122#issuecomment-1660551128
JoshT: Maybe we should add to the docs that replace
is an alternative.
Mara: I can't think of any use case where you'd want that.
JoshT: The docs say "unlike the other methods", so that's good enough.
Amanieu: What is EINPROGRESS?
JoshT: EINPROGRESS is not like EAGAIN. It's a failure state, not a 'please wait' state.
{some discussion about when this happens with connect()}
JoshT: In conclusion, you'd handle these differently, so they should be different errors.
Mara: So then the question is if we want ErrorKind::InProgress. I'd think no, because that'd be platform specific.
JoshT: It's pretty standard on unix.
Mara: Nothing would use it on windows right?
JoshT: True, but we have that for more errors. That's fine.
The 8472: Windows would return EWOULDBLOCK in some of those situations. see https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-connect
JoshT: That doesn't seem like the same situation.
Mara: My general worry is that on non-unix platforms it leads people to believe they can handle this error separately, even when it maps to a different (more general) category on e.g. Windows.
Amanieu: Ideally we'd want to map the EAGAIN from windows connect() differently, but I don't think we can do that.
JoshT: That might be an argument for the separate code, then libraries could map them differently.
Mara: Today you need cfg(unix) to handle libc::EINPROGRESS. Adding ErrorKind::InProgress still requires cfg(windows), except you can forget about it and write non-cross-platform code that compiles on all platforms. Bad.
JoshT: We already have many, many cases where different platforms return different errno cases for the "same" error on different platforms.
The 8472: I can't find any other use cases for the error than sockets.
JoshT: There are others too, e.g. filesystem.
Mara: What's the point of having a unix-specific ErrorKind? If you're making unix-specific software, just use libc::EINPROGRESS?
JoshT: EINPROGRESS exists in Windows too. You can still use ErrorKind::InProgress on Windows.
Amanieu: Alternatively, map EINPROGRESS to WouldBlock, and then the user can decide what to do based on the api they called.
JoshT: That seems bad. You check for WouldBlock in an async loop for example.
Mara: Looks like we can't reach a conclusion on InProgress today, but it does sound like we should re-open the ACP.
Josh reopening the ACP: https://github.com/rust-lang/libs-team/issues/92
Notes from before the meeting:
- Jubilee: "I am not really aware of any other code in the standard library that deliberately relies on user-facing "post-monomorphization errors". I believe this may be the first? I believe this should be discussed by the relevant teams before accepting this PR. It's low-risk since this is user-facing, but it is a significant change. The alternative is essentially "wait for GCE to get better before allowing these functions to develop further"."
- Jubilee: Note that while inline_const isn't fully stable yet (and is expected to make this kind of thing more common), it's mostly due to hashing out some compiler details.
Mara: Jubilee notes that the title of the PR is wrong. That is confusing.
The 8472: This panic more eagerly, at compile time.
David: Yeah. We had a similar case where N=0 would not compile. We rejected that on purpose.
The 8472: We don't have the right mechanism for this. You don't want it to trigger in a removed/unevaluated branch.
Thom: We either do it at runtime or compile time, those seem like the only two options.
The 8472: Some checks are delayed until after monomorphization.
David: Link: https://github.com/rust-lang/rust/pull/99471#issuecomment-1555908856
David: Does the same pattern apply here?
Mara: Yes, I think os.
David: I think so too.
David: Someone brought up C++-style constexpr if, but that's probably not available to us.
Thom + JoshT: There are ideas for that, but not coming any time soon.
Thom: It's worth noting that
https://github.com/rust-lang/rust/issues/74985
is listed as blocked on making the N == 0 error fail at compile time. Agree it might not be desirable, but there might be mismatched expectations.
Mara: A tool we have available today is a lint. Like you get for a[100]
when it's out of bounds.
Thom: Yeah a lint sounds ideal.
The 8472: Does that work in the generic case?
Thom: That could work.
David: There is one other change in this PR: Some return values become Option.
Out of meeting time.
std::os::unix::net
, it's not confined to the inner std::sys
module.std::os::unix
but I think these should probably be reevaluated if they aren't applicable to all "Unixen". Unices? Units?Generated by fully-automatic-rust-libs-team-triage-meeting-agenda-generator