HackMD
Libs-API Meeting 2023-08-22
tags: Libs Meetings Minutes
Meeting Link: https://meet.jit.si/rust-libs-meeting-crxoz2at8hiccp7b3ixf89qgxfymlbwr
Attendees: David, JoshT, Mara, The 8472, Chris Denton
Agenda
- Triage
- Anything else?
Triage
FCPs
1 rust-lang/rfcs T-libs-api FCPs
18 rust-lang/rust T-libs-api FCPs
m-ou-se (9), joshtriplett (8), Amanieu (6), dtolnay (6), BurntSushi (6)
rust.tf/114986 FCP process: Require 2/3 majority for FCP
This would require 4/5 checkboxes for us (instead of 3/5 today).
(nominated) rust.tf/85122 Tracking Issue for inherent unchecked integer methods
David: For shifting, is it clear which part is not checked? (The result or the number of bits to shift)
Mara: I think that's clear enough.
David: Would you ever want one of those saturating and another unchecked or something?
The 8472: Unchecked is for very low level stuff
[..]
Unresolved question 2: naming: we already have 'get_unchecked', so seems okay.
JoshT: Would you ever use unchecked_add over wrapping_add? Doesn't that compile to the same?
The 8472: Useful restrictions for e.g. optimziations.
David: Useful for miri too.
Unresolved question 3:
JoshT: Is the UB about both the input and output?
Mara: About the input (# bits to shift), just like wrapping_shl.
Mara: I don't think we can do something else, because we already have wrapping_shl.
The 8472: UB on output overflow is also useful.
Mara: Could just use unsafe unreachable on a checked_shl.
The 8472: Optimizer needs to recognize that.
Amanieu: the unchecked shift is to eliminate the masking on the shift amount, which some platforms need.
David: I'm on board with this.
The 8472: For some use cases it would be useful to also communicate that the result is not overflowing. E.g. shifting related to pointers and alignment. Although that could also be done with unchecked_mul() depending on the situation.
Mara: for the native << operator we don't panic for 5u32 << 31 but we do (in debug mode) for 5u32 << 35. Just like wrapping_shl, this is only about the shift amount, not about the result.
The 8472: There's still use cases where not overflowing the result is a useful guarantee.
Mara: Yes, but doing it only for the input is more consistent. And unchecked_mul(1<<N) is a good alternative.
Mara: So, do we think we have consensus on the shift behaviour, or should we only FCP add/sub/mul/neg?
David: I think we can approve the shift behaviour.
The 8472: Would be good to have an overview of what we're stabilizing and what is missing, and what the existing alternatives are.
Mara to reply, asking for an overview.
(nominated) rust.tf/91345 Tracking Issue for `result_option_inspect`
Mara: I nominated this, because this gets a lot of attention and we should either close or accept this.
Amanieu: It's basically .map(), returning the input from the closure.
It takes self by value, but the closure gets the T by reference.
Josh: That's mildly annoying if you already have a Option<&T>.
David: I'm mildly against this. I'd advise using map.
Josh: Also mildly against that. Not convinced we want something chainable.
The 8472: People want this for long chains.
Josh: Yes, for chains map is good enough.
The 8472: The issue is that it gets more verbose, as you need a block with two expressions. I don't like it, but i understand why someone wants this.
Mara: I think there is value in keeping the 'basic vocabulary' of Option and Result small, to keep code readable.
Josh: Thoughts on a more general 'tap' functionality?
Mara: I think tap is fine as a separate crate. If there's a strong need for that, we might want to have a lang feature for that problem instead (like let-else!).
Mara: Sounds like no one is in favour?
Amanieu: I'm tempted to be in favour. {LOUD TRAIN NOISES} It doesn't cost much to add it. I don't want to fall into the trap where we reject things because we don't find use cases for it. We're just a very small sample of the rust community, so we don't have full view of how this would be used.
Josh: It seems like we're seeing the use case, we just don't want to see people using it.
The 8472: You can't stop someone from doing it themselves, it's just a matter of providing the convenience.
Mara: For inclusion in std, it's important to consider those reading code instead of just those writing code. Would people have a harder time reading rust code if Option::inspect is widely used?
Amanieu: Looking at use cases of map_err.. i have one liners that just return the original value. e.g. to log something before returning.
Josh: Would those use cases by satisfied by a log_error function? (Not to be provided by std.)
Amanieu in chat:
Josh: I think this would be improved less by inspect_err and more by a dedicated method (ptrace().blah("..")).
The 8472: anyhow has .context() through an extension trait.
Mara: It seems we have a lack of strong opinions, only mildly against or in favor ^^'
Amanieu: I can be strongly in favour.
Josh: Would anyone check a box for closing or for merging?
Mara: I'd check my box for closing, and probably leave it open for merging (without raising concerns).
Josh: Same
David: Same.
David: burntsushi probably too.
Josh: So would there be any objections to closing?
Mara: Well a lot of people seem to be in favour, so we need to handle that well.
Amanieu: I still think we should merge this. It's generally useful, and there is very little downside.
The 8472: Wasn't there a proposal to make chaining with patterns more ergonomic? postfix macro with matches, or something?
Mara: Would be nice if we had some magical language feature that would cover map+inspect+filter+… but we don't have anything.
Mara to reply.
(nominated) rust.tf/111090 Tracking Issue for const `[u8]::is_ascii` (`const_slice_is_ascii`)
Seems fine? Done.
(nominated) rust.tf/114737 `std::process::Command::env_clear` is unusable on Windows
From chris' comment:
- As noted above, Go's equivalent to env_clear makes an exception for SystemRoot. We could do the same.
- Alternatively we could reset the environment to the user's default as suggested here. But this is very far from a "clear" environment so may be better as another method.
Josh: Neither of those seem like the right behavior for env_clear. Maybe for a new method that isn't env_clear.
The 8472: Platform specific method?
Josh: Not necessarily. It could be portable, like env_reset or env_default (not proposing names).
The 8472: On Unix there is .profile and systemd stuff and more..
Josh: On unix i'd just make it clear.
Amanieu: looked on github code search for env_clear. I'm seeing quite a few that do that and then just set the PATH variable. None of them seem to be aimed at Windows.
Amanieu: Found one for Windows. building for msys2. clears everything and explicitly inherits PATH.
Josh: like with ACPs, we shouldn't be the one to design the right API, but just give feedback on proposals. We could say we don't want to change env_clear but are open to a new method.
Mara: I don't know enough of Windows, but I could imagine that changing env_clear might make sense, to do what Go does for example.
Josh: We can say we don't have consensus to change env_clear.
Mara: We can ping the windows ping group.
Amanieu: Yeah let's do that.
(nominated) rust.tf/114564 Attempt to describe the intent behind the `From` trait further
(nominated) rust.tf/114780 add more explicit I/O safety documentation
(waiting on team) rust.tf/87920 Tracking Issue for `Saturating` type
(waiting on team) rust.tf/114149 `read_dir` has unexpected behavior for `""`
(new change proposal) rust.tf/libs256 Add a `CStr::len()` method
Amanieu: This is now O(1) but might later become O(N)
David: We could forcefully make it O(N) with black_box.
Amanieu: noo
Josh: i don't think we should, but i can understand why we would.
Josh: I don't think it's a problem to have .len() and have it become O(N) later.
Mara: I think having a O(N) .len() is an issue. we have to_bytes(), not as_bytes(). subtle difference, but to shows a conversion is going on.
The 8472: .count_bytes()?
Amanieu: Anything other than len would be confusing
The 8472: That's good. Because len won't do the the intuitive thing
Josh: Perhaps a clippy lint to not use .len() multiple times could help.
The 8472: len is very short, which is useful if you call it often. This one should not be called often. So it's reasonable this is longer.
Josh: We can tell people .to_bytes().len() is the recommended way.
Amanieu: If we don't have any method for this, people will keep asking.
Mara: How common is this operation? You can't index into a CStr..
Mara: The problem statement in the ACP says: "[this] may not be the best option when CStr becomes a thin pointer." But that seems false? It is the most efficient way to do that.
Amanieu: How do y'all feel about count_bytes? So, with a verb in it?
Josh: Reasonable.
Amanieu: let's ask for another name.
Mara: +1, just something that doesn't imply O(1).
Mara to reply.
(new change proposal) rust.tf/libs257 Implement `From<&'a &'static str>` for `Arguments<'a>`
Mara: I'll one-up this ACP: How about from &'static str rather than from &'a &'static str. ^^ We can't do that with today's implementation, but we can make that work with a new implementation.
Mara: Not a reason to block the conversion from &'a &'static str.
Amanieu: Please don't use From, but a method instead.
Mara: In the future, the representation of &'static str and fmt::Arguments could be identical.
David: you can already write format_args!("asdf")?
Mara: you can't write format_args!(s). also s might contain {} etc.
Josh: fmt::Arguments::from_str("{a}") would not parse {a}.
Mara: We have from_str(&"a") today, but not from_str("a").
Mara: With Arguments::new_str("{}") you might expect that to parse {}. With a From impl it's clearer it represents the same value.
Amanieu: We can postpone until the new fmt::Arguments impl exists.
Mara to reply.
(new change proposal) rust.tf/libs258 Generify `BorrowedBuf` and `BorrowedCursor`
Josh: These types are used in code that's already somewhat complex. Having to specialize in order to optimize for u8 isn't great. We should keep this specific to bytes. We could consider a type for the more general case, but it shouldn't be BorrowedBuf.
Amanieu: If you're not working with read/write API, what you what in practice is a function that takes a slice of [MaybeUninit<T>] and returns a subslice of [T]
Mara: It's also about using i8 or c_char with read and write.
Amanieu: Converting that to u8 is easy enough.
Mara: Fair.
The 8472: do we support platforms where c_char is not 8 bit?
Josh: No, but could be either signed or unsigned: always u8 or i8. (We should make that a hard guarantee in the future.)
Mara: I think it'd be easy enough for us to make BorrowedBuf generic.
Amanieu: We wouldn't use it in std though.
Mara: True. good point.
Josh to reply.
(new change proposal) rust.tf/libs259 Add openat/unlinkat/etc. abstractions to ReadDir/DirEntry/OpenOptions
The 8472: Unix has this. Windows "doesn't" but actually does (and we use it in std). Important for security.
The 8472: Alternative, we could just expose the file descriptor/handle from ReadDir, but that's questionable because it's technically UB to use the fd during a unix readdir().
Mara: Can we expect support for this from all platforms?
The 8472: Perhaps not on some niche platforms. A fallback would not be secure.
Mara: What should be the fallback behaviour? Should we error, or .join() the path ourselves, losing the security?
The 8472: Joining and not erroring would be good for adoption.
Mara: Depends a bit on which platforms don't support it. I might feel okay if we do non-secure behaviour or some niche platforms.
Mara: I think we can support this on pretty much all platforms, and it seems that all new platforms have these syscalls anyway.
Amanieu: How does DirEntry get the dir fd?
The 8472: The DirEntry already has an Arc to the dir.
Amanieu: can we point people at cap-std instead?
Mara: It's different: you can do dir.open("../something"), but you shouldn't with capabilities.
