Skip to content
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

ACP: std::ffi::c and std::ffi::os submodules #134

Closed
clarfonthey opened this issue Nov 15, 2022 · 14 comments
Closed

ACP: std::ffi::c and std::ffi::os submodules #134

clarfonthey opened this issue Nov 15, 2022 · 14 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@clarfonthey
Copy link

clarfonthey commented Nov 15, 2022

Proposal

Note: this ACP will reference the std crate and submodules of it, although in cases where types are also available in core, equivalent modules should also be added there as well.

Problem statement + Motivation, use cases

(It's difficult to separate these two, so, I combined them into one section for this ACP.)

As part of rust-lang/rust#104353 (ACP: #135), I mention that adding a Bytes iterator type to std::ffi could potentially be ambiguous, since both C strings and OS strings could theoretically offer one. This problem exists for any iterators or other types added adjacent to C strings and OS strings that could be potentially ambiguous.

There is precedent for this among other standard library types, since essentially all collection and sequence types have their own submodule so they can be paired with their iterators. Even std::option and std::result exist for this purpose, since otherwise these modules would only contain the types Option and Result respectively.

Solution sketches

The recommendation of this ACP is to add std::ffi::c and std::ffi::os modules, although std::c_str and std::os_str could technically be added as an alternative.

Proposed moves:

  • std::ffi::CStr => std::ffi::c::CStr
  • std::ffi::CString => std::ffi::c::CString
  • std::ffi::CStrBytes (Tracking Issue for CStr::bytes rust#112115) => std::ffi::c::Bytes
  • std::ffi::OsStr => std::ffi::os::OsStr
  • std::ffi::OsString => std::ffi::os::OsString

To ensure backwards-compatibility, use self::{c::{CStr, CString}, os::{OsStr, OsString}} would be added into std::ffi to re-export the moved items.

This would be similar to how, for example, std::collections::BinaryHeap is just a re-export of std::collections::binary_heap::BinaryHeap, although there are still types only included in std::collections::binary_heap like std::collections::binary_heap::Iter.

Links and related work

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

@clarfonthey clarfonthey added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Nov 15, 2022
@SUPERCILEX
Copy link

I don't think this is worth it due to backwards compat. How would users know to use one import over the other? I guess we could add a clippy lint or deprecate the old imports, but that would break a lot of code. Calling it CStrBytes seems perfectly reasonable to me.

@clarfonthey
Copy link
Author

The answer is that both would be valid, and there's no need to deprecate the original aliases. This is the same as how collections work today: you can import std::collections::VecDeque or std::collections::vec_deque::VecDeque and there aren't any specific complaints about using one or the other, and I'd imagine a lot of people would continue to use std::ffi::CStr and friends as well.

@SUPERCILEX
Copy link

Hmmm, I see how that would make sense. And ffi presumably wouldn't export the Bytes so you'd have to take it from the c module? I still don't like the churn, but if you're ok with that then this seems like a good idea.

@clarfonthey
Copy link
Author

Yup! You would have to explicitly use std::ffi::c::Bytes instead of use std::ffi::CStrBytes, although in most cases you just wouldn't import it at all since I'd imagine most uses won't need to know anything beyond impl Iterator<Item = u8>.

@clarfonthey
Copy link
Author

Reworded the original description to include newly approved CStr::bytes iterator, and to say how this would be done via a bulleted list instead of prose for clarity.

@dtolnay
Copy link
Member

dtolnay commented May 31, 2023

I am not on board with c and os as module names. I feel like they unnecessarily obscure the point behind this change that's described in the Motivation section, which is to provide grouping for iterator types and error types related to CStr/CString and OsStr/OsString respectively.

For examples does c_int belong in std::ffi::c because it is a C thing / does VaList belong in std::ffi::c because it is an extern "C" thing? According to this API they do not, because the c in std::ffi::c does not refer to C-related APIs, it refers to CStr-/CString-related APIs, despite "str" not appearing in the module path. A module named c might be justifiable in the context of something like std::ffi::str::c or std::str::c or std::ffi::c::str but I don't think those are the ideal organization here.

I don't like the mentioned alternative of std::c_str+std::os_str either. For example we also do not have std::binary_heap which is a type mentioned in the ACP. None of these are ubiquitous enough types for that the way path or vec or option are.

I do see the need for type-specific modules for categorization of iterators and error types inside of std::ffi, for the same reason that std::collections is also done this way.

std::ffi::c_str and std::ffi::os_str would work, I think.

@clarfonthey
Copy link
Author

I think that std::c_str and std::os_str were genuinely typos, and I meant to type std::ffi::c_str and std::ffi::os_str like you suggested. I even thought that was what I had wrote initially, and had to double-check.

I'm okay with updating the proposal to use those instead.

@dtolnay dtolnay added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label May 31, 2023
@dtolnay
Copy link
Member

dtolnay commented May 31, 2023

Marking this ACP-accepted API Change Proposal is accepted (seconded with no objections) specifically in regard to std::ffi::c_str.

There are enough CStr-specific types with nondescript names floating around in std::ffi that really belonged in a c_str module to begin with.

And of course CStr and CString themselves would also go there, and the iterator type from:

On std::ffi::os_str I am less sold. There is nothing currently proposed to go there beyond OsStr and OsString, right? If so, I don't see the need for now. We can do it when those types begin accumulating more surface area, as might happen following this change:

@dtolnay
Copy link
Member

dtolnay commented May 31, 2023

Implementation suggestion: play with #[doc(inline)] / #[doc(no_inline)] to nudge reader toward importing CStr as use std::ffi::CStr but importing e.g. FromBytesWithNulError as use std::ffi::c_str::FromBytesWithNulError.

I see std::collections already uses #[doc(inline)] in places.

Screenshot from 2023-05-31 00-47-21

@clarfonthey
Copy link
Author

Noted! Will open up a PR for the std::ffi::c_str module with the changes you suggested.

@clarfonthey
Copy link
Author

clarfonthey commented May 31, 2023

Side note from actually implementing this: libstd makes a distinction between str and string modules, and for the implementation, I decided to separate c_str and c_string for this reason. Will also include whether this separation is meaningful in the tracking issue.

I personally like it because it makes clear what types correspond to CStr versus CString, but again, that's still important to discuss.

@dtolnay
Copy link
Member

dtolnay commented May 31, 2023

Not a fan of c_string. I don't see the need.

Similar existing cases: there is also no need for a path_buf module, or a ref_cell module.

CString is an owned mutable CStr so it works well enough in std::ffi::c_str in my opinion.

@rust-lang/libs-api?

@clarfonthey
Copy link
Author

The lack of a path_buf module is a good point. I'm fine either way.

@dtolnay
Copy link
Member

dtolnay commented Oct 17, 2023

We discussed this in today's T-libs-api meeting and affirmed that we do like std::ffi::c_str (encompassing both CStr and CString related types), and would hold off on a std::ffi::os_str for now.

Thank you for driving this!

@dtolnay dtolnay closed this as completed Oct 17, 2023
jhpratt added a commit to jhpratt/rust that referenced this issue Mar 9, 2024
Add std::ffi::c_str module

ACP: rust-lang/libs-team#134

`std::ffi` docs before change:
![Structs: VaList, VaListImpl, CStr, CString, FromBytesWithNulError, FromVecWithNulError, IntoStringError, NulError, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/b2cf3534-30f9-4ef0-a655-bacdc3a19e17)

`std::ffi` docs after change:
![Re-exports: self::c_str::{FromBytesWithNulError, FromBytesUntilNulError, FromVecWithNulError, NulError, IntoStringError} ; Modules: c_str ; Structs: VaList, VaListImpl, CStr, CString, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/23aa6964-da7a-4942-bbf7-42bde2146f9e)

(note: I'm omitting the `c_int`, etc. stuff from the screenshots since it's the same in both. this doesn't just delete those types)
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Mar 10, 2024
Add std::ffi::c_str module

ACP: rust-lang/libs-team#134

`std::ffi` docs before change:
![Structs: VaList, VaListImpl, CStr, CString, FromBytesWithNulError, FromVecWithNulError, IntoStringError, NulError, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/b2cf3534-30f9-4ef0-a655-bacdc3a19e17)

`std::ffi` docs after change:
![Re-exports: self::c_str::{FromBytesWithNulError, FromBytesUntilNulError, FromVecWithNulError, NulError, IntoStringError} ; Modules: c_str ; Structs: VaList, VaListImpl, CStr, CString, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/23aa6964-da7a-4942-bbf7-42bde2146f9e)

(note: I'm omitting the `c_int`, etc. stuff from the screenshots since it's the same in both. this doesn't just delete those types)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 10, 2024
Add std::ffi::c_str module

ACP: rust-lang/libs-team#134

`std::ffi` docs before change:
![Structs: VaList, VaListImpl, CStr, CString, FromBytesWithNulError, FromVecWithNulError, IntoStringError, NulError, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/b2cf3534-30f9-4ef0-a655-bacdc3a19e17)

`std::ffi` docs after change:
![Re-exports: self::c_str::{FromBytesWithNulError, FromBytesUntilNulError, FromVecWithNulError, NulError, IntoStringError} ; Modules: c_str ; Structs: VaList, VaListImpl, CStr, CString, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/23aa6964-da7a-4942-bbf7-42bde2146f9e)

(note: I'm omitting the `c_int`, etc. stuff from the screenshots since it's the same in both. this doesn't just delete those types)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 10, 2024
Add std::ffi::c_str module

ACP: rust-lang/libs-team#134

`std::ffi` docs before change:
![Structs: VaList, VaListImpl, CStr, CString, FromBytesWithNulError, FromVecWithNulError, IntoStringError, NulError, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/b2cf3534-30f9-4ef0-a655-bacdc3a19e17)

`std::ffi` docs after change:
![Re-exports: self::c_str::{FromBytesWithNulError, FromBytesUntilNulError, FromVecWithNulError, NulError, IntoStringError} ; Modules: c_str ; Structs: VaList, VaListImpl, CStr, CString, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/23aa6964-da7a-4942-bbf7-42bde2146f9e)

(note: I'm omitting the `c_int`, etc. stuff from the screenshots since it's the same in both. this doesn't just delete those types)
jhpratt added a commit to jhpratt/rust that referenced this issue Mar 10, 2024
Add std::ffi::c_str module

ACP: rust-lang/libs-team#134

`std::ffi` docs before change:
![Structs: VaList, VaListImpl, CStr, CString, FromBytesWithNulError, FromVecWithNulError, IntoStringError, NulError, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/b2cf3534-30f9-4ef0-a655-bacdc3a19e17)

`std::ffi` docs after change:
![Re-exports: self::c_str::{FromBytesWithNulError, FromBytesUntilNulError, FromVecWithNulError, NulError, IntoStringError} ; Modules: c_str ; Structs: VaList, VaListImpl, CStr, CString, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/23aa6964-da7a-4942-bbf7-42bde2146f9e)

(note: I'm omitting the `c_int`, etc. stuff from the screenshots since it's the same in both. this doesn't just delete those types)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 10, 2024
Add std::ffi::c_str module

ACP: rust-lang/libs-team#134

`std::ffi` docs before change:
![Structs: VaList, VaListImpl, CStr, CString, FromBytesWithNulError, FromVecWithNulError, IntoStringError, NulError, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/b2cf3534-30f9-4ef0-a655-bacdc3a19e17)

`std::ffi` docs after change:
![Re-exports: self::c_str::{FromBytesWithNulError, FromBytesUntilNulError, FromVecWithNulError, NulError, IntoStringError} ; Modules: c_str ; Structs: VaList, VaListImpl, CStr, CString, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/23aa6964-da7a-4942-bbf7-42bde2146f9e)

(note: I'm omitting the `c_int`, etc. stuff from the screenshots since it's the same in both. this doesn't just delete those types)
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 10, 2024
Rollup merge of rust-lang#112136 - clarfonthey:ffi-c_str, r=cuviper

Add std::ffi::c_str module

ACP: rust-lang/libs-team#134

`std::ffi` docs before change:
![Structs: VaList, VaListImpl, CStr, CString, FromBytesWithNulError, FromVecWithNulError, IntoStringError, NulError, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/b2cf3534-30f9-4ef0-a655-bacdc3a19e17)

`std::ffi` docs after change:
![Re-exports: self::c_str::{FromBytesWithNulError, FromBytesUntilNulError, FromVecWithNulError, NulError, IntoStringError} ; Modules: c_str ; Structs: VaList, VaListImpl, CStr, CString, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/23aa6964-da7a-4942-bbf7-42bde2146f9e)

(note: I'm omitting the `c_int`, etc. stuff from the screenshots since it's the same in both. this doesn't just delete those types)
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Mar 12, 2024
Add std::ffi::c_str module

ACP: rust-lang/libs-team#134

`std::ffi` docs before change:
![Structs: VaList, VaListImpl, CStr, CString, FromBytesWithNulError, FromVecWithNulError, IntoStringError, NulError, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/b2cf3534-30f9-4ef0-a655-bacdc3a19e17)

`std::ffi` docs after change:
![Re-exports: self::c_str::{FromBytesWithNulError, FromBytesUntilNulError, FromVecWithNulError, NulError, IntoStringError} ; Modules: c_str ; Structs: VaList, VaListImpl, CStr, CString, OsStr, OsString](https://github.com/rust-lang/rust/assets/15850505/23aa6964-da7a-4942-bbf7-42bde2146f9e)

(note: I'm omitting the `c_int`, etc. stuff from the screenshots since it's the same in both. this doesn't just delete those types)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 13, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 13, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 13, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 14, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
jhpratt added a commit to jhpratt/rust that referenced this issue Mar 14, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 14, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 14, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 14, 2024
Rollup merge of rust-lang#104353 - clarfonthey:cstr-bytes-iter, r=cuviper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
bors pushed a commit to rust-lang/miri that referenced this issue Mar 15, 2024
Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

3 participants