impl Index<RangeFrom> for CStr

[1011X]

This change implements (partial) slicing for CStr.

Since a CStr must end in a null byte, it's not possible to trim from the right and still have a valid CStr. But, it is possible to trim from the left. This lets us be a bit more flexible and treat them more like strings.

let string = CStr::from_bytes_with_nul(b"Hello World!\0");
let result = CStr::from_bytes_with_nul(b"World!\0");
assert_eq!(&string[6..], result);

[rust-highfive]

r? @hanna-kruppe

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

[hanna-kruppe]

What's your use case for this method?

I'm a bit hesitant to add this API because its performance is impacted by the theoretically-planned change from representing &CStr as (ptr, len) pair to representing it like a plain char * (in C terms) with length only available by calling strlen. The same is true for to_bytes[_with_nul] but that's a rather more central operation and its documentation warns about the potential performance pitfall. Adding more APIs with the same pitfall -- especially a trait impl whose docs are hidden by default -- seems to increase the risk that code becomes accidentally quadratic if the planned change ever does happen.

[hanna-kruppe]

In any case, cc @rust-lang/libs because this would be insta-stable and hence needs an FCP.

[Amanieu]

I think this is very useful functionality!

@rfcbot fcp merge

[rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @SimonSapin
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[1011X]

@hanna-kruppe Sorry for taking so long to reply. I don't have a use-case, just thought it'd be nice to have, since it's currently kinda difficult to get CStrs from other CStrs.

I see the desire to treat &CStr the same as a *const c_char, since it means you can just use &CStr in FFI code to seemlessly handle C strings between both C and Rust, while also allowing some conveniences like, e.g., adding methods to it.

But tbh I'm not sure how I feel about that, since this breaks a couple of assumptions I have about references:

• a reference is semantically different from a pointer
• a reference to an unsized type always has a length

Say we have a C function that returns a char const * and we want to write a Rust wrapper for it. We could use &CStr, but then what happens if the function returns a null pointer? It's a perfectly valid value to return for a char const *, but now it seems Rust would run into undefined behavior.

For this feature, the only way I can think of that still allows for it without making the potential changes you mentioned more difficult is something like this:

// assume `self` is the same as `*const c_char`
let len = sys::strlen(self);
if index.start < len {
    self.offset(index.start)
} else {
    // emulate an out-of-bounds panic
    panic!("index out of bounds: the len is {} but the index is {}", len, index.start");
}

But of course this means that it'll need to calculate the length every time to make sure it's not going over the end, rather than using a cached length.

[hanna-kruppe]

@hanna-kruppe Sorry for taking so long to reply. I don't have a use-case, just thought it'd be nice to have, since it's currently kinda difficult to get CStrs from other CStrs.

I see the desire to treat &CStr the same as a *const c_char, since it means you can just use &CStr in FFI code to seemlessly handle C strings between both C and Rust, while also allowing some conveniences like, e.g., adding methods to it.

But tbh I'm not sure how I feel about that, since this breaks a couple of assumptions I have about references:

* a reference is semantically different from a pointer

* a reference to an unsized type always has a length

Frankly, I also have my doubts about the usefulness of that change at this point, but for different reasons. The way dynamically-sized types (DST) in Rust work, it's a bit sloppy to talk about &CStr specifically because all other pointers-to-CStr would work the same way, including raw pointers. And there's a more general long-standing desire to generalize DSTs from "always pointer + length or pointer + vtable" to allow almost arbitrary metadata bundled to the pointer, including no metadata at all. For example, to store the vtable pointer inside (or next to) the object (as in C++) -- such a trait object is still unsized, but the pointer to it is thin.

And yet, generalized DSTs would still have a baked-in assumption that the size is rather cheap to fetch, which would not be true for "thin CStr". And while non-null-ness of references is not necessarily an obstacle to FFI (you can just use Option<&CStr> if null is allowed), the aliasing implications of references are sometimes subtly inappropriate when interacting with C. Furthermore, many uses of C strings do benefit from a cached length, and maintaining that length manually is error-prone and unsafe. Thus, I believe there will always be room for a "C string + cached length" type (what CStr currently is) even if we add a newtype for "just the pointer" in the future -- and in that case, we might as well let CStr remain the "cached strlen" type and call the "just the pointer" type something different.

Still, despite my misgivings, the plan of record is to to make CStr lose the length (or at least keep open the possibility), so my preferred options would be giving up on those plans or not adding APIs that conflict with those plans.

Say we have a C function that returns a char const * and we want to write a Rust wrapper for it. We could use &CStr, but then what happens if the function returns a null pointer? It's a perfectly valid value to return for a char const *, but now it seems Rust would run into undefined behavior.

For this feature, the only way I can think of that still allows for it without making the potential changes you mentioned more difficult is something like this:

// assume `self` is the same as `*const c_char`
let len = sys::strlen(self);
if index.start < len {
    self.offset(index.start)
} else {
    // emulate an out-of-bounds panic
    panic!("index out of bounds: the len is {} but the index is {}", len, index.start");
}

But of course this means that it'll need to calculate the length every time to make sure it's not going over the end, rather than using a cached length.

Right, something like that. An improvement is possible when index.start is much smaller than strlen(self) (only check that there's no 0 byte before index.start, stop searching beyond that) though it's possible that this is a regression when index.start is close to the end of the string, if strlen is more efficient than strnlen_s or its open-coded equivalent.

[1011X]

are there any other changes needed on my part? Github is showing a red "changes requested" from @dtolnay, but it's already been fixed. also @RalfJung's review doesn't show the "outdated" mark like the others. is there something else i need to do?

[dtolnay]

I think this is waiting on one other libs member to sign off in #74021 (comment) or to raise objections.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[dtolnay]

@bors r+

[bors]

📌 Commit 30b8835 has been approved by dtolnay

[Mark-Simulacrum]

@rust-timer make-pr-for f305b20

[BurntSushi]

@rust-lang/libs Did we consider the impact of this change on the possible future implementation of choice of not caching the length of the C string, as mentioned by @hanna-kruppe above? In particular, with this API stabilized, I feel it is very unlikely that we would ever stop caching the length.

Note that this is about to be on the stable release in a couple days.

[dtolnay]

I considered it. I don't think this is incompatible with using a ptr-only representation for &CStr. Indexing with n.. will verify no \0 in the first n bytes and produce a &CStr beginning n bytes higher.

[BurntSushi]

@dtolnay Interesting. I think that would be our first impl of Index in std that is not constant time? That's what I was thinking would make it very unlikely to stop caching its length. (Not that it's necessarily incompatible.)

[Mark-Simulacrum]

BTreeMap Index is log n, at least, though that's "better" than n

[pietroalbini]

Release is tomorrow, we can back the stabilization out for a release if that gives y'all more time to think.

[BurntSushi]

cc @rust-lang/libs Does anyone want to second reconsidering this? If it's just me then happy to let it rest. We should at least carefully document this. Because it's going to be a potentially surprising performance footgun if we ever do decide to stop caching the length.

[sfackler]

I also feel a bit uneasy.