-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Document that OsString
and OsStr
are bytes; provide conversions to bytes
#95290
Conversation
f1086bb
to
ccd297b
Compare
This comment has been minimized.
This comment has been minimized.
ccd297b
to
6f2b6c1
Compare
This comment has been minimized.
This comment has been minimized.
6f2b6c1
to
ff78e5e
Compare
This comment has been minimized.
This comment has been minimized.
ff78e5e
to
08dae9d
Compare
This comment has been minimized.
This comment has been minimized.
08dae9d
to
84e64bd
Compare
This comment has been minimized.
This comment has been minimized.
84e64bd
to
e59b2cf
Compare
This comment has been minimized.
This comment has been minimized.
It seems like this only requires that there is some variant of OsStr that is byte-based encoded as a superset of UTF-8, right? It would be possible for OsStr to wrap something like I mostly ask/note this because I want to make sure that this kind of option is explicitly mentioned in the writeup (and potentially rejected, either for complexity or infeasibility if (for example) we can't think of an encoding that will work), so that we can point back to it when thinking about this in the future. |
Fair enough; strictly speaking, the requirement would be that We could, for example, use some encoding that has a second variant for a native |
This comment has been minimized.
This comment has been minimized.
629aa32
to
20090db
Compare
This comment has been minimized.
This comment has been minimized.
…o bytes This commit consists almost entirely of documentation, updating the module documentation in `std::ffi` and the type documentation on `OsString`. This only adds two methods: `OsStr::as_bytes` and `OsString::into_vec`, and plumbs those methods down to the existing implementations for both UNIX and Windows.
20090db
to
af7b7da
Compare
Note that this crate doesn't rely on the internal representation at all. It decodes the encoded Also, the preferred way to use the crate is to use I think this PR would still be a good idea, since many people rely on the representation anyway. However, I wanted to clarify those points.
One option would be to do the same as os_str_bytes and document limited invariants about the encoding. This would allow many of the same changes that could be made now to the internal representation. |
There is one problem if this PR gets merged and starts getting used. Without a wrapper, it becomes very easy to mix WTF-8 and UTF-8 bytes. For searching and concatenation, that would be a problem, since both need special handling of WTF-8 bytes. Also, if storage is permitted, the encoding could never change. |
I have no objections to documenting a lesser guarantee and making the encoding less specified, if that's needed in order to get consensus to make this change. |
rustdoc: Early doc link resolution fixes and refactorings A subset of rust-lang#94857 that shouldn't cause perf regressions, but should fix some issues like https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/ICE.20in.20collect_intra_doc_links.2Ers rust-lang#95290 and improve performance in cases like rust-lang#95694.
I appreciate the progress on this and recognize that other crates can benefit from this but I figure it'd be worth documenting where this is deficient for at least one use case, clap. At a high level, clap needs
There are a couple of ways to unblock clap:
If there is anything I can do to help move along any of the parts to unblock clap, let me know! |
We are doing direct transmutes between `OsStr` and `[u8]`. rust-lang/rust#95290 would make this natively supported but I got tired of waitin for it. This only saves about 1/4s off of `cargo build`. This took 2.9 KiB off of `cargo bloat --release --example git`
I'm very interested in the status of this. I currently use unsafe mem::transmute calls (inspired by Clap) in my command line parsing function, and I frankly don't think it's acceptable that correct argument handling (whether it's options parsing or the only uses people have mentioned here) is impossible in portable and safe rust. Has there been any progress? |
`OsStr` has historically kept its implementation details private out of concern for locking us into a specific encoding on Windows. This is an alternative to rust-lang#95290 which proposed specifying the encoding on Windows. Instead, this only specifies that for cross-platform code, `OsStr`'s encoding is a superset of UTF-8 and defines rules for safely interacting with it At minimum, this can greatly simplify the `os_str_bytes` crate and every arg parser that interacts with `OsStr` directly (which is most of those that support invalid UTF-8).
Allow limited access to `OsStr` bytes `OsStr` has historically kept its implementation details private out of concern for locking us into a specific encoding on Windows. This is an alternative to rust-lang#95290 which proposed specifying the encoding on Windows. Instead, this only specifies that for cross-platform code, `OsStr`'s encoding is a superset of UTF-8 and defines rules for safely interacting with it At minimum, this can greatly simplify the `os_str_bytes` crate and every arg parser that interacts with `OsStr` directly (which is most of those that support invalid UTF-8). Tracking issue: rust-lang#111544
Allow limited access to `OsStr` bytes `OsStr` has historically kept its implementation details private out of concern for locking us into a specific encoding on Windows. This is an alternative to rust-lang#95290 which proposed specifying the encoding on Windows. Instead, this only specifies that for cross-platform code, `OsStr`'s encoding is a superset of UTF-8 and defines rules for safely interacting with it At minimum, this can greatly simplify the `os_str_bytes` crate and every arg parser that interacts with `OsStr` directly (which is most of those that support invalid UTF-8). Tracking issue: rust-lang#111544
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this has been superseded by #109698.
The
OsString
andOsStr
types are wrappers around bytes, and are required by existing stable APIs to be byte-based encodings that are supersets of UTF-8 (since a&str
can become an&OsStr
without allocation). However, both types are opaque, and do not provide access to raw bytes, primarily because on Windows they use the WTF-8 encoding, the standard for which recommends against exposing it.Nonetheless, many crates do rely on
OsStr
andOsString
being bytes. We already provide theOsStrExt
andOsStringExt
extension traits, which get used reasonably widely despite being non-portable. And theos_str_bytes
crate has millions of downloads, and tens of thousands of new downloads everyday.This PR is an effort to seek consensus for making the contents of an
OsStr
orOsString
available as bytes, inspired by recent discussions within the libs-api team.This is a minimal PR to acheive that goal. This PR consists almost entirely of documentation, updating the module documentation in
std::ffi
and the type documentation onOsString
. This PR only adds two methods:OsStr::as_bytes
andOsString::into_vec
, and plumbs those methods down to the existing implementations for both UNIX and Windows. Both methods are unstable (and I'll file a tracking issue for them as soon as I take this PR out of draft status).In particular, this PR does not add any new
From
orTryFrom
orInto
orTryInto
impls, nor does it add methods to constructOsStr
orOsString
from bytes (which would require error handling), nor does it attempt to do any further unification of implementations between Windows and UNIX, nor does it extend these types to have more of the functionality of bstr. All of those things could happen in potential future changes, and I'd be happy to make some of those changes myself, but I'd like to focus on the initial consensus for making the change before further work that depends on the change.As one further note of the implications of this change: there has been a proposal to change OsStr/OsString on Windows from WTF-8 to a more complex encoding that can potentially speed up matching for use with the Pattern API. Since that proposal, we've decided to seal the Pattern API and avoid stabilizing it. I would propose that we not switch the encoding of OsStr/OsString, and proceed with exposing bytes using the current encoding. Another option would be to expose bytes but not specify the exact encoding beyond "superset of UTF-8".
r? @ghost