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

Add os::windows::ffi::OsStrExt::system_cmp and system_eq #86008

Closed
wants to merge 2 commits into from

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Jun 4, 2021

Adds two methods to the extension trait OsStrExt on Windows for dealing with case-insensitive strings: system_cmp and system_eq. These methods use the system case-insensitive implementation CompareStringOrdinal with bIgnoreCase, which differs from a pure Unicode case-insensitive comparison (which is why I explicitly named these methods system_ instead of something like case_insensitive_).

These methods are the correct way for comparing environment variable keys, registry keys, resource handle names etc. on Windows, e.g. "Path" and "PATH" are treated the same by the system and should compare equal.

This is the minimal way I thought of for enabling the use of Windows specific case-insensitive comparisons. There are several ways this could be extended further if necessary:

  • Also have extension traits with system_cmp and system_eq for Path, [u8]?, [u16]? (This is not the correct way to compare paths: Tracking Issue for windows_case_insensitive #86007 (comment))
  • Introduce a trait SystemOrd and SystemEq in std::os::windows.
  • Introduce a wrapper type like num::Wrapping for OsStr that overrides the Ord and Eq implementation.

Inspired by #85270

Tracking issue: #86007

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2021
Comment on lines 135 to 136
/// This is the correct way to compare various strings on Windows:
/// environment variable keys, registry keys, file paths and resource handle names are all case-insensitive.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per #86007 (comment) this might still not be the correct way for dealing with file paths. What should we say about this in the documentation?

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I'm uncertain. Perhaps remove "file paths" from that list but then add a note that file paths may have different rules? I think system_cmp could still perhaps be useful as a heuristic (i.e. these two paths may be equal) so long as it isn't relied upon for file paths.

But I don't think there's any OS support for comparing file paths other than opening both files and seeing if they point to the same file. Which obviously can't be done if one of the paths doesn't yet exist (or they can't be opened for some other reason). I mean, system_cmp may work for file paths most of the time but when it doesn't it could be a footgun. Or at least surprising when two "equal" paths point to different files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah comparing file paths is more complex, also due to the possibility of relative paths, verbatim paths, UNC prefixes, symlinks, etc; there are many ways in which two different paths can be equivalent.

It seems more appropriate to find a separate correct solution for that, so I'll leave it out of the documentation in this PR.

@rustbot rustbot added A-FFI Area: Foreign function interface (FFI) O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 5, 2021
@nagisa
Copy link
Member

nagisa commented Jun 6, 2021

Also have extension traits with system_cmp and system_eq for Path, [u8]?, [u16]?

I believe that the case sensitivity is a filesystem (and nowadays, per-directory) property, rather than a OS-specified one, so naming them system_* would likely be quite confusing. If adding comparison methods like these for Path is a goal, I think it is worthwhile to prototype an addition now (in order to see if it is at all possible to implement anything reasonable) and if so, consider if system_* naming is good for the functionality included in this PR.

@CDirkx
Copy link
Contributor Author

CDirkx commented Jun 6, 2021

Yes for paths it seems better to offer a fs function similar to the C++ std::filesystem::equivalent, which opens the two paths and compares the results of stat. Therefore I think it would be best to only add these methods to OsStr, and when such an equivalent function exist in Rust explicitly call it out as the correct way for comparing file paths (Currently the proposed docs in this PR have: "This is the correct way to compare various strings on Windows (...) Note that this does not include file names or paths; those can be case-sensitive depending on the system, file system or directory settings").

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 31, 2021
/// let b = OsString::from("PATH");
///
/// assert!(a.eq(&b) == false);
/// assert!(a.system_eq(&b).unwrap() == true);
Copy link
Member

Choose a reason for hiding this comment

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

I think you're missing a closing ```?

@camelid camelid added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2021
@JohnCSimon
Copy link
Member

@CDirkx can you address the comment from Camelid

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2021
@JohnCSimon JohnCSimon removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2021
@JohnCSimon JohnCSimon added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 19, 2021
@JohnCSimon
Copy link
Member

ping from triage:
@CDirkx

returning to author to address comments
thanks.

@bors
Copy link
Contributor

bors commented Oct 30, 2021

☔ The latest upstream changes (presumably #89174) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

@CDirkx
I'm closing this PR is inactive, please feel free to reopen when you're ready to continue.
@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Nov 30, 2021
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-FFI Area: Foreign function interface (FFI) O-windows Operating system: Windows S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants