Refactor and fix parse_prefix on Windows
#78833
Merged
+141
−84
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Refactor `get_first_two_components` to `get_next_component`. Fixes the following behaviour of `parse_prefix`: - series of separator bytes in a prefix are correctly parsed as a single separator - device namespace prefixes correctly recognize both `\\` and `/` as separators
|
r? @dtolnay |
|
@rustbot ping windows This PR changes the behavior of |
|
Hey Windows Group! This bug has been identified as a good "Windows candidate". cc @arlosi @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @retep998 @rylev @sivadeilra |
|
Sure, I'll take a look on Monday. |
|
Windows dev, here. On principle, this change seems sound. There's always the risk of breaking some obscure corner case when it comes to parsing paths on Windows, but I don't think that should stop this PR. Agreed w/ @dtolnay on getting some form of sanity check for the behavior of |
|
By sanity check I just meant someone else's opinion familiar with Windows paths. The implementation and test updates in the PR look good to me. Thanks @CDirkx and @sivadeilra! |
dtolnay
approved these changes
Dec 13, 2020
|
@bors r+ |
|
📌 Commit 94d73d4 has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Dec 14, 2020
Refactor and fix `parse_prefix` on Windows This PR is an extension of rust-lang#78692 as well as a general refactor of `parse_prefix`: **Fixes**: There are two errors in the current implementation of `parse_prefix`: Firstly, in the current implementation only `\` is recognized as a separator character in device namespace prefixes. This behavior is only correct for verbatim paths; `"\\.\C:/foo"` should be parsed as `"C:"` instead of `"C:/foo"`. Secondly, the current implementation only handles single separator characters. In non-verbatim paths a series of separator characters should be recognized as a single boundary, e.g. the UNC path `"\\localhost\\\\\\C$\foo"` should be parsed as `"\\localhost\\\\\\C$"` and then `UNC(server: "localhost", share: "C$")`, but currently it is not parsed at all, because it starts being parsed as `\\localhost\` and then has an invalid empty share location. Paths like `"\\.\C:/foo"` and `"\\localhost\\\\\\C$\foo"` are valid on Windows, they are equivalent to just `"C:\foo"`. **Refactoring**: All uses of `&[u8]` within `parse_prefix` are extracted to helper functions and`&OsStr` is used instead. This reduces the number of places unsafe is used: - `get_first_two_components` is adapted to the more general `parse_next_component` and used in more places - code for parsing drive prefixes is extracted to `parse_drive`
|
⌛ Testing commit 94d73d4 with merge 906a5b35869e78b67c0fc3890065bb570bd0b4f1... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Dec 15, 2020
Refactor and fix `parse_prefix` on Windows This PR is an extension of rust-lang#78692 as well as a general refactor of `parse_prefix`: **Fixes**: There are two errors in the current implementation of `parse_prefix`: Firstly, in the current implementation only `\` is recognized as a separator character in device namespace prefixes. This behavior is only correct for verbatim paths; `"\\.\C:/foo"` should be parsed as `"C:"` instead of `"C:/foo"`. Secondly, the current implementation only handles single separator characters. In non-verbatim paths a series of separator characters should be recognized as a single boundary, e.g. the UNC path `"\\localhost\\\\\\C$\foo"` should be parsed as `"\\localhost\\\\\\C$"` and then `UNC(server: "localhost", share: "C$")`, but currently it is not parsed at all, because it starts being parsed as `\\localhost\` and then has an invalid empty share location. Paths like `"\\.\C:/foo"` and `"\\localhost\\\\\\C$\foo"` are valid on Windows, they are equivalent to just `"C:\foo"`. **Refactoring**: All uses of `&[u8]` within `parse_prefix` are extracted to helper functions and`&OsStr` is used instead. This reduces the number of places unsafe is used: - `get_first_two_components` is adapted to the more general `parse_next_component` and used in more places - code for parsing drive prefixes is extracted to `parse_drive`
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Dec 15, 2020
Refactor and fix `parse_prefix` on Windows This PR is an extension of rust-lang#78692 as well as a general refactor of `parse_prefix`: **Fixes**: There are two errors in the current implementation of `parse_prefix`: Firstly, in the current implementation only `\` is recognized as a separator character in device namespace prefixes. This behavior is only correct for verbatim paths; `"\\.\C:/foo"` should be parsed as `"C:"` instead of `"C:/foo"`. Secondly, the current implementation only handles single separator characters. In non-verbatim paths a series of separator characters should be recognized as a single boundary, e.g. the UNC path `"\\localhost\\\\\\C$\foo"` should be parsed as `"\\localhost\\\\\\C$"` and then `UNC(server: "localhost", share: "C$")`, but currently it is not parsed at all, because it starts being parsed as `\\localhost\` and then has an invalid empty share location. Paths like `"\\.\C:/foo"` and `"\\localhost\\\\\\C$\foo"` are valid on Windows, they are equivalent to just `"C:\foo"`. **Refactoring**: All uses of `&[u8]` within `parse_prefix` are extracted to helper functions and`&OsStr` is used instead. This reduces the number of places unsafe is used: - `get_first_two_components` is adapted to the more general `parse_next_component` and used in more places - code for parsing drive prefixes is extracted to `parse_drive`
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Dec 15, 2020
Refactor and fix `parse_prefix` on Windows This PR is an extension of rust-lang#78692 as well as a general refactor of `parse_prefix`: **Fixes**: There are two errors in the current implementation of `parse_prefix`: Firstly, in the current implementation only `\` is recognized as a separator character in device namespace prefixes. This behavior is only correct for verbatim paths; `"\\.\C:/foo"` should be parsed as `"C:"` instead of `"C:/foo"`. Secondly, the current implementation only handles single separator characters. In non-verbatim paths a series of separator characters should be recognized as a single boundary, e.g. the UNC path `"\\localhost\\\\\\C$\foo"` should be parsed as `"\\localhost\\\\\\C$"` and then `UNC(server: "localhost", share: "C$")`, but currently it is not parsed at all, because it starts being parsed as `\\localhost\` and then has an invalid empty share location. Paths like `"\\.\C:/foo"` and `"\\localhost\\\\\\C$\foo"` are valid on Windows, they are equivalent to just `"C:\foo"`. **Refactoring**: All uses of `&[u8]` within `parse_prefix` are extracted to helper functions and`&OsStr` is used instead. This reduces the number of places unsafe is used: - `get_first_two_components` is adapted to the more general `parse_next_component` and used in more places - code for parsing drive prefixes is extracted to `parse_drive`
This was referenced Dec 15, 2020
|
☀️ Test successful - checks-actions |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is an extension of #78692 as well as a general refactor of
parse_prefix:Fixes:
There are two errors in the current implementation of
parse_prefix:Firstly, in the current implementation only
\is recognized as a separator character in device namespace prefixes. This behavior is only correct for verbatim paths;"\\.\C:/foo"should be parsed as"C:"instead of"C:/foo".Secondly, the current implementation only handles single separator characters. In non-verbatim paths a series of separator characters should be recognized as a single boundary, e.g. the UNC path
"\\localhost\\\\\\C$\foo"should be parsed as"\\localhost\\\\\\C$"and thenUNC(server: "localhost", share: "C$"), but currently it is not parsed at all, because it starts being parsed as\\localhost\and then has an invalid empty share location.Paths like
"\\.\C:/foo"and"\\localhost\\\\\\C$\foo"are valid on Windows, they are equivalent to just"C:\foo".Refactoring:
All uses of
&[u8]withinparse_prefixare extracted to helper functions and&OsStris used instead. This reduces the number of places unsafe is used:get_first_two_componentsis adapted to the more generalparse_next_componentand used in more placesparse_drive