Fix Path::with_extension dropping the file name for ..X - #160240
Fix Path::with_extension dropping the file name for ..X#160240lazureykis wants to merge 3 commits into
Path::with_extension dropping the file name for ..X#160240Conversation
rustbot
added
S-waiting-on-review
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @clarfonthey (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
`_with_extension` chopped only the previous extension off the byte slice,
leaving the trailing dot in place, and then called `set_extension` on that
intermediate. For a file name like `..foo`, `extension()` is `Some("foo")`,
so the intermediate ends in `..` -- which has no `file_name()` -- and
`set_extension` returns `false` without doing anything. The whole file name
is dropped and the result is a parent directory:
Path::new("a/..foo").with_extension("txt") // "a/.."
PathBuf::from("a/..foo").set_extension("txt") // "a/..txt"
`with_extension` is documented purely as "See `PathBuf::set_extension` for
more details", so the two must agree. They did until 1a44b45 replaced
`to_path_buf()` + `set_extension` with the hand-rolled byte path to save an
allocation; the `..X` case was not covered by the tests added there.
Copy up to the end of the file stem instead -- the same boundary
`set_extension` truncates to -- and append the dot and extension directly,
which keeps the single exact-capacity allocation. An exhaustive differential
check over all 1365 paths of length <= 5 from `{. a b /}` crossed with six
extensions finds no remaining disagreement between the two methods.
cc4de3d to
2fa9d8b
Compare
| let file_stem = file_stem.as_encoded_bytes(); | ||
| let end_file_stem = file_stem[file_stem.len()..].as_ptr().addr(); | ||
| let start = self_bytes.as_ptr().addr(); | ||
| (&self_bytes[..end_file_stem.wrapping_sub(start)], true) |
There was a problem hiding this comment.
This whole logic seems to be more complicated than it needs to be. Your intent in this block seems to be parsing away the file_stem portion of the Path as a u8 slice. You can just return (file_stem.as_encoded_bytes(), true) here, no?
| }; | ||
|
|
||
| let add_dot_and_extension = set_extension && !extension.is_empty(); | ||
| let new_capacity = |
There was a problem hiding this comment.
I liked the previous implementation of destructuring what the match block returns to (new_capacity, slice_to_copy). You can check if the extension argument is empty in the Some(_) block and then have set_extension be a part of the destructuring match pattern alongside new_capacity and slice_to_copy so that down below you use the set_extension instead of add_dot_and_extension as the condition to check to append the extension to the new path.
|
I actually want to strengthen the guarantees here, and also nominate to libs-api since this is an API change that feels reasonable. Essentially, the docs/spec for
And the docs for
I'm proposing to update the docs (and spec):
Status quo:
Proposal (
Difference from original:
|
clarfonthey
added
the
I-libs-api-nominated
It doesn't — Agreed on the behaviour change, but I'd widen it to the leading run of dots, which also covers Implemented, full |
|
Yeah, to be fair, I should have actually tested my assumptions instead of just guessing based upon a read of the code. The libs-api meeting is on Tuesday so I'll try to prepare a dedicated example matrix of what is expected to change under the new proposal. The goal is to try and rectify what we have and what seems reasonable. |
|
Okay, took some time and generated a table. Terrible code to do so below: Terrible codeProposed changes:
Note: full writeup at #160240 (comment) |
nia-e
removed
the
I-libs-api-nominated
|
Per today's libs-api meeting, this looks fine to us - maybe some tests could be added to make sure behaviour isn't changed for dots in the middle of a path segment, but otherwise looks good. ty ^^ |
|
Did the meeting land on the three-dot cap or the full leading run? @nia-e @clarfonthey |
|
We didn't explicitly discuss this, but the usecases that we considered favour treating arbitrarily many dots the same as two or three |
|
The main reason I left it at three is that "..." is a valid file name by itself and it would be peculiar to not allow "....ext" for that reason. |
|
hmm, i see. since this is a bugfix and we're not committing to any behaviour I'd say it's up to you as the reviewer, but maybe post on zulip about this ^^ |
`Path::file_stem` and `Path::extension` held back only the first `.` of a
file name, so a name of nothing but dots could become the stem: `...foo`
had stem `..`, and `set_extension("")` on it produced a parent directory.
Hold back up to three leading dots instead. The cap is what keeps the stem
a file name in its own right — never `.` or `..` — while still letting
`...`, itself a legal file name, carry an extension: `....foo` splits as
stem `...` and extension `foo`.
`file_prefix` split before the second dot of a name that began with one, so `..config.toml` had prefix `.` — the same defect the previous commit fixed for `file_stem`, and the same fix: hold back up to three leading dots, leaving the prefix a file name in its own right.
|
Thanks for the reviews. I updated the PR with the three-dot cap and the extra tests. |
5 participants
Fixes #160239.
Two changes. The first is the reported bug; the second is the spec change approved at the 2026-08-04 libs-api meeting.
1.
with_extensiondropped the file name after..Path::new("a/..foo").with_extension("txt")returneda/..— a parent directory — whereset_extensionreturneda/..txt.with_extensionis documented purely as "See [PathBuf::set_extension] for more details", so the two should agree._with_extensionchopped only the previous extension off the byte slice, leaving the trailing dot in place, then calledset_extensionon that intermediate. For..foothe intermediate ends in.., which has nofile_name(), soset_extensionreturnedfalseand did nothing — and its return value was discarded, so the file name was silently dropped. Regression from #113106, first released in 1.73.0.The fix copies up to the end of the file stem — the boundary
set_extensiontruncates to — and appends the dot and extension directly, keeping the single exact-capacity allocation.2. Up to three leading dots are part of the file stem
file_stemandextensionheld back only the first.of a file name, so a name of nothing but dots could become the stem:...foohad stem.., andset_extension("")on it produced a parent directory.file_prefixhad the matching defect —..config.tomlhad prefix..Both now hold back up to three leading dots (
MAX_LEADING_DOTS, shared byrsplit_file_at_dotandsplit_file_at_dotso the two cannot drift). The cap is what keeps the stem a file name in its own right — never.or..— while still letting..., itself a legal file name, carry an extension:....foosplits as stem...and extensionfoo.Three is the least cap that achieves this: over the 9841 file names of length ≤ 8 over
{. a b}, a cap of 1 leaves 189 names with a stem of.or.., a cap of 2 leaves 63, and a cap of 3 leaves none. Caps above 3 are behaviourally identical to 3.Behaviour changes are limited to file names whose stem was previously made up only of dots. Full before/after matrix in @clarfonthey's comment: #160240 (comment)
Tests
Tests were added to
test_with_extensionandtest_set_extension, which already mirror each other case for case, and totest_stem_ext/test_prefix_ext. They cover the cap boundary (....x,....,.....x) and, per @nia-e's request, pin that dots after the start of a file name still split as they always have (x..y,x...y,.x..y,..x..y,foo..bar,bar/foo..baz)../x test library/stdis green and./x fmt --checkis clean. Reverting the source change while keeping the tests fails all four oftest_stem_ext,test_prefix_ext,test_set_extensionandtest_with_extension.