Skip to content

Remove dependency on os_string_truncate nightly feature #176

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.

Sign up for GitHub

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

Closed
matthew-e-brown wants to merge 4 commits into from
Closed

Remove dependency on os_string_truncate nightly feature #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7cb8608
Remove `os_string_truncate` usage from `path::normalize`
matthew-e-brown May 21, 2025
3c99160
Remove `os_string_truncate` from features lists
matthew-e-brown May 21, 2025
95e6e36
Replace `.into()` with more explicit `PathBuf::from()`
matthew-e-brown May 21, 2025
7ef4c31
Fix typo in comment
matthew-e-brown May 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/bin/edit/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

#![feature(let_chains, linked_list_cursors, os_string_truncate, string_from_utf8_lossy_owned)]
#![feature(let_chains, linked_list_cursors, string_from_utf8_lossy_owned)]

mod documents;
mod draw_editor;
Expand Down
1 change: 0 additions & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
maybe_uninit_fill,
maybe_uninit_slice,
maybe_uninit_uninit_array_transpose,
os_string_truncate
)]
#![allow(clippy::missing_transmute_annotations, clippy::new_without_default, stable_features)]

Expand Down
26 changes: 18 additions & 8 deletions src/path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

//! Path related helpers.

use std::ffi::OsStr;
use std::ffi::{OsStr, OsString};
use std::path::{Component, MAIN_SEPARATOR_STR, Path, PathBuf};

/// Normalizes a given path by removing redundant components.
Expand All @@ -23,14 +23,24 @@ pub fn normalize(path: &Path) -> PathBuf {
}
Component::CurDir => {}
Component::ParentDir => {
// Get the length up to the parent directory
if let Some(len) = res
.parent()
.map(|p| p.as_os_str().as_encoded_bytes().len())
// Ensure we don't pop the root directory
&& len >= root_len
// Get length up to the parent directory and truncate, but ensure we don't pop the root directory.
// NB: this compares the system-dependent "encoded length" as discussed in OsString's documentation.
if let Some(parent) = res.parent()
&& parent.as_os_str().len() >= root_len
Comment on lines +28 to +29
Copy link
Member

@lhecker lhecker May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep the old check here? This is 100% subjective, but I personally find it a bit more readable.

Another 110% purely subjective thing is that I think the comments could be a bit shorter. This may be because I wrote the original code and/or because I know the involved stdlib functions well, but I think the old comments were "sufficient" at least. The new ones are way better of course, but I think there's a balance in between that could be struck. I think I need to clarify again: This is purely subjective so please disagree. 😅

(I'll wait for you to respond before merging this.)

That said, ironically your new code is shorter than the old version, despite moving a temporary object around (Good job, optimizing compiler! 😄): https://godbolt.org/z/cT5Gvzb36
The missing bit is the check_public_boundary call which is part of the truncate implementation if you check it out. It checks if the given index is part of a WTF8 character boundary. I think it's okay for us to not do that check. Our truncation length comes straight from the parent path length.

Copy link
Contributor Author

@matthew-e-brown matthew-e-brown May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep the old check here?

The reason I changed the comparison was to differentiate between OsStr lengths and .as_encoded_bytes() lengths. The old version compared p.as_os_str().as_encoded_bytes().len() to root_len = res.as_os_str().len(): one with and one without the call to .as_encoded_bytes() first. Can we be sure that the two lengths are equivalent? The documentation for OsStr and OsString were kind of confusing to me on this topic.

I went down a bit of a rabbit hole with this...

  • OsStr::len() specifically states that it does not return the byte-length of the string as represented on the underlying OS, but instead returns the length of the underlying in-Rust container; the strings may be stored in a different manner to make conversions more efficient. It summarizes the supposed len-related oddities by saying that, "this value is simply useful for passing to other methods, like OsString::with_capacity." It calls back to OsString's introduction for more information.

  • OsString's introduction, then, notes that, "on Windows, strings are stores as sequences of 16-bit values." This note almost seems to imply that an OsString could be stored as such, and that it may therefore have a length different than its byte-length; but the section very quickly corrects that an OsString is still "actually stored as a sequence of 8-bit values."

  • The next section on capacity of an OsString further clarifies that OsString capacity (and therefore length) uses a combination of "units of UTF-8 bytes" and "units of bytes in an unspecified encoding."

So... OsStr::len() makes a point (a whole paragraph's worth!) of noting that .len() is not necessarily the same as the number of bytes, and that the type-level docs should be checked; but then the type-level docs explain that, no, OsStrings' lengths are always bytes anyways!

In this particular case, the root_len being compared to will always have been obtained through .as_os_str().len() right after res.push(OsStr::new(MAIN_SEPARATOR_STR))—which, according to PathBuf's docs, should always truncate the path to only ever be that MAIN_SEPARATOR_STR, meaning it should always now be either / or \... so it shouldn't even matter anyways, since both those cases should always have the same "native-length" and byte-length... And, in my testing, I couldn't come up with an OsStr whose .len() and .as_encoded_bytes().len() didn't match (though I didn't test any ill-formed Unicode or anything like that).

But, with the OsStr::len()'s documentation (combined with my lack of experience working with Win32, wchar_t, etc.) making as much of a point as it did about the length, I just couldn't shake the feeling that there was some edge-case where the difference mattered.

So... to avoid that headache, I figured it would be easier (and possibly even, more semantically correct?) to just make sure to only compare one OsStr::len() with OsStr::len, and to keep the .as_encoded_bytes().len() length only for when it was time to actually deal with bytes. This is what my NB: comment about "encoded lengths" was talking about. Doing it that way meant that I needed .as_os_str().len() for the check, but not for the truncate anymore; so mapping the option made less sense than let Some'ing the whole parent chunk again.

If none of that makes any sense or holds any water, then... I'm not particularly attached to either approach 😅 We can swap it back if you'd prefer. You probably know more than I do about the potential pitfalls of wide-chars, WTF8, and OsStrings, so please let me know if I'm missing anything obvious 🙂

Though, I did notice when coming back to this that the check can probably be > instead of >=, since there isn't much need to truncate a string to length n if its length is already exactly n...

I think the comments could be a bit shorter.

Oh yeah, they definitely could be 😄 I think that's just a result of my particular coding style. I can try and trim them down a bit. Though, I myself am not sure which ones I'd get rid of or shorten (since they're just sort of "my voice," so to speak). How would you write them? 🙂

Copy link
Member

@lhecker lhecker May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's how I'd do it: 078230f

I'm not concerned about .len() VS .as_encoded_bytes().len(), as it's the same thing under the hood. Basically, I consider it a theoretical, not practical difference. I do know that some people really don't enjoy such hot takes. 😅 I don't expect this part of the stdlib to change in the future anymore though. WTF8 is just an overall good solution.

But I did try to address your concern as you're genuinely correct about it. I did it in the opposite way though: By making everything consistently use .as_encoded_bytes().len(), this should not result in a mismatch anymore, right?

{
res.as_mut_os_string().truncate(len);
// To actually truncate the OsString, convert it to raw bytes first, truncate the Vec, then convert
// it back.
// [FIXME] Can be replaced with a plain `res.as_mut_os_string().truncate(parent_len)` once
// `os_string_truncate` is stabilized (#133262)
let byte_len = parent.as_os_str().as_encoded_bytes().len();
let mut bytes = res.into_os_string().into_encoded_bytes();
bytes.truncate(byte_len);
// SAFETY: All encoding concerns of `OsString` are met:
// - the provided bytes came directly from a known-to-be-valid `OsStr`, `res`.
// - since `parent` is also a valid `OsStr`, trimming at its byte length must also be at a valid
// boundary.
// This is very similar to the example from `OsStr::from_encoded_bytes_unchecked`'s documentation.
res = PathBuf::from(unsafe { OsString::from_encoded_bytes_unchecked(bytes) });
}
}
Component::Normal(p) => res.push(p),
Expand Down