Peel namespace-request tests, plus the -sys naming docs and a UTF-16 path-length fix

README.md

@@ -27,6 +27,32 @@ code lives behind `cfg(windows)`. The workspace's `wtf-string` crate is the
 exception -- its portable core has no `cfg(windows)` gating, so CI additionally
 builds, tests, and lints it on Linux and macOS.
 
+## Crate naming
+
+The `-sys` suffix here does **not** carry its usual ecosystem meaning. A `-sys`
+crate is normally raw FFI declarations with no abstraction over them; every
+`windows-*-sys` crate in this workspace instead wraps a great deal of `unsafe`,
+and takes its declarations from
+[`windows-sys`](https://crates.io/crates/windows-sys) rather than making its own.
+
+What the suffix marks is a **layer**:
+
+- **`windows-*-sys`** -- makes an existing Win32 API memory-safe *without adding
+  policy*. It schedules nothing, picks no delivery model, and reports the
+  platform's outcomes unnormalised.
+- **no suffix** -- decides something Win32 has no equivalent of.
+  `windows-waitable-queues` is the worked example: it chooses a slot protocol, an
+  overflow policy, and a signalling discipline, so calling it `-sys` would
+  misdescribe how much it decides on the caller's behalf.
+
+So the presence of `unsafe` wrappers is not evidence either way -- it is the job
+description of the first kind. The distinction is *policy*, and the suffix is the
+only signal a reader has for how much a crate decides for them.
+
+This section is the statement of record. The convention governs published crate
+names, so it belongs where a reader meets the crates rather than only in the
+design notes.
+
 ## Build
 
 Requires Rust `1.98` or newer.

crates/windows-file-enumeration-sys/src/path/tests.rs

@@ -181,9 +181,22 @@ fn a_device_namespace_path_is_resolved_rather_than_kept_verbatim() {
 /// An absolute path of exactly `units` UTF-16 units, already in normal form so
 /// `GetFullPathNameW` returns it unchanged and the resolved length is the input
 /// length.
+///
+/// UTF-16 units and not bytes or `char`s, because that is the unit Win32
+/// measures a path in: `MAX_PATH` is a count of `WCHAR`. The three coincide for
+/// the ASCII this builds, so counting the prefix any other way would pass today
+/// and quietly measure the wrong thing the moment a case uses a character that
+/// is not one byte, one scalar, and one unit at once.
 fn absolute_path_of_length(units: usize) -> String {
     let prefix = r"C:\";
-    format!("{prefix}{}", "a".repeat(units - prefix.len()))
+    let prefix_units = prefix.encode_utf16().count();
+    assert!(
+        units >= prefix_units,
+        "asked for a {units}-unit path, but the {prefix} prefix is already \
+         {prefix_units} units; the subtraction below would underflow and panic \
+         without saying why"
+    );
+    format!("{prefix}{}", "a".repeat(units - prefix_units))
 }
 
 #[test]
@@ -193,7 +206,7 @@ fn an_ordinary_path_of_exactly_max_path_content_is_accepted() {
     // cannot see, and it is the expensive direction: it refuses a path Windows
     // would have opened.
     let path = absolute_path_of_length(259);
-    assert_eq!(path.chars().count(), 259);
+    assert_eq!(path.encode_utf16().count(), 259);
 
     let prepared = prepare_str(&path).expect("259 units is within the ordinary limit");
     assert_eq!(text(&prepared), path);
@@ -203,7 +216,7 @@ fn an_ordinary_path_of_exactly_max_path_content_is_accepted() {
 fn an_ordinary_path_one_unit_past_max_path_content_is_rejected() {
     // 260 counts the terminator, so 260 content units do not fit.
     let path = absolute_path_of_length(260);
-    assert_eq!(path.chars().count(), 260);
+    assert_eq!(path.encode_utf16().count(), 260);
 
     let error = prepare_str(&path).expect_err("260 units leaves no room for the terminator");
     assert_eq!(error.failure(), RequestFailure::PathTooLong);

crates/windows-namespace-request-sys/README.md

@@ -32,6 +32,25 @@ and unassociated, because associating it with a completion port irreversibly
 forecloses `IoRing` use of it, and that choice belongs to a layer that knows the
 handle's destination.
 
+## Why the `-sys` suffix
+
+Not in the usual sense. A `-sys` crate elsewhere in the ecosystem is normally
+raw FFI declarations with no abstraction over them, and by that reading this
+crate is misnamed: it declares almost no FFI of its own and takes its
+declarations from [`windows-sys`](https://crates.io/crates/windows-sys).
+
+In this workspace the suffix marks a **layer**, not a linking strategy: a
+`windows-*-sys` crate makes an existing Win32 API memory-safe **without adding
+policy**, and a crate that decides something Win32 has no equivalent of drops
+the suffix. That is why `windows-waitable-queues` carries no `-sys` -- it picks
+a slot protocol and an overflow policy -- while this crate does, despite
+wrapping a great deal of `unsafe`. Everything above is the suffix being earned:
+it schedules nothing, chooses no delivery model, and reports raw Win32 outcomes
+without normalising them.
+
+The convention is stated for the whole workspace in the repository's
+[README](../../README.md#crate-naming).
+
 ## A path is copied; a handle is duplicated
 
 Several entries take a handle rather than a path, and a request owns a
@@ -127,5 +146,6 @@ operation coverage (every audited call site re-expressed) and scenario coverage
 context, many requests across concurrent workers from one shared capture, and a
 handle opened by one request carried into a later one).
 
-Design decisions are recorded in [DESIGN-NOTES.md](DESIGN-NOTES.md). Not yet
-released to crates.io.
+Design decisions are recorded in [DESIGN-NOTES.md](DESIGN-NOTES.md). Published
+on crates.io as
+[`windows-namespace-request-sys`](https://crates.io/crates/windows-namespace-request-sys).

crates/windows-namespace-request-sys/src/final_path/tests.rs

@@ -117,9 +117,9 @@ fn the_buffer_grows_for_a_path_longer_than_the_first_attempt() {
         .to_string_lossy();
 
     assert!(
-        resolved.chars().count() > 260,
-        "the fixture must actually exceed the first attempt: {} chars",
-        resolved.chars().count()
+        resolved.encode_utf16().count() > 260,
+        "the fixture must actually exceed the first attempt: {} UTF-16 units",
+        resolved.encode_utf16().count()
     );
     assert!(resolved.ends_with("f.t"), "unexpected: {resolved}");
 }

crates/windows-namespace-request-sys/src/full_path/tests.rs

@@ -102,9 +102,9 @@ fn a_deeply_nested_path_beyond_the_first_attempt_still_resolves() {
     let resolved = resolve(&path);
 
     assert!(
-        resolved.chars().count() > 260,
-        "the fixture must exceed the first attempt: {} chars",
-        resolved.chars().count()
+        resolved.encode_utf16().count() > 260,
+        "the fixture must exceed the first attempt: {} UTF-16 units",
+        resolved.encode_utf16().count()
     );
     assert!(resolved.ends_with("file.txt"), "unexpected: {resolved}");
 }

crates/windows-namespace-request-sys/src/open/tests.rs

@@ -385,3 +385,69 @@ fn a_copy_duplicates_the_template_rather_than_sharing_the_owner() {
     copy.perform()
         .expect("the copy's template outlived the original");
 }
+
+#[test]
+fn every_configured_parameter_reads_back_through_its_own_accessor() {
+    // A mutation run replaced `desired_access`, `share_mode`, and
+    // `creation_disposition` with constants and nothing failed. The tests above
+    // build requests with `with_*` and then *open* them, so they exercise the
+    // fields through Win32 -- which is exactly what cannot distinguish an
+    // accessor reporting the truth from one reporting a constant, because the
+    // open path reads the struct's fields directly rather than through them.
+    //
+    // Every value here is deliberately non-zero and pairwise distinct.
+    // `OpenFile::new` starts every parameter at zero, so a test that configured
+    // a zero -- or reused one value twice -- would be satisfied by
+    // `-> Default::default()` and by an accessor reading a neighbour's field.
+    let fixture = Fixture::new("open-accessors");
+    let request = request_for(fixture.directory())
+        .with_desired_access(FILE_GENERIC_READ)
+        .with_share_mode(FILE_SHARE_READ)
+        .with_creation_disposition(OPEN_EXISTING)
+        .with_flags_and_attributes(FILE_FLAG_BACKUP_SEMANTICS);
+
+    assert_eq!(request.desired_access(), FILE_GENERIC_READ);
+    assert_eq!(request.share_mode(), FILE_SHARE_READ);
+    assert_eq!(request.creation_disposition(), OPEN_EXISTING);
+    assert_eq!(request.flags_and_attributes(), FILE_FLAG_BACKUP_SEMANTICS);
+
+    // The property the four assertions above rest on, stated rather than left
+    // to the reader's eye: these are Win32 constants and their values are not
+    // obvious, so a collision between two of them would silently weaken the
+    // test into one that cannot tell those accessors apart.
+    let configured = [
+        FILE_GENERIC_READ,
+        FILE_SHARE_READ,
+        OPEN_EXISTING,
+        FILE_FLAG_BACKUP_SEMANTICS,
+    ];
+    for (index, value) in configured.iter().enumerate() {
+        assert_ne!(
+            *value, 0,
+            "a zero is indistinguishable from the unset default"
+        );
+        for other in &configured[index + 1..] {
+            assert_ne!(
+                value, other,
+                "two parameters share a value, so this test cannot tell their \
+                 accessors apart"
+            );
+        }
+    }
+}
+
+#[test]
+fn an_unset_parameter_reads_back_as_nothing_rather_than_as_a_plausible_open() {
+    // The contract `OpenFile::new` states: every parameter starts at "the
+    // caller said nothing" rather than at a plausible-looking open, because a
+    // plausible default is exactly what a caller cannot see they got. An
+    // accessor that invented one would hide that from them.
+    let fixture = Fixture::new("open-unset");
+    let request = request_for(fixture.directory());
+
+    assert_eq!(request.desired_access(), 0);
+    assert_eq!(request.share_mode(), 0);
+    assert_eq!(request.creation_disposition(), 0);
+    assert_eq!(request.flags_and_attributes(), 0);
+    assert!(request.security().is_none());
+}

crates/windows-namespace-request-sys/src/open_by_id/tests.rs

@@ -18,8 +18,8 @@ use windows_sys::Win32::Storage::FileSystem::{
 };
 
 use super::{FileIdentifier, OpenFileByIdentifier};
-use crate::CapturedHandle;
 use crate::handle::tests::{FILE_CONTENTS, Fixture, handle_allocation};
+use crate::{CapturedHandle, SecurityAttributes};
 
 const AUDITED_SHARE: u32 = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
 
@@ -275,3 +275,79 @@ fn a_request_performs_the_same_way_on_another_thread() {
 
     assert_eq!(length, FILE_CONTENTS.len() as u64);
 }
+
+#[test]
+fn every_configured_parameter_reads_back_through_its_own_accessor() {
+    // Four accessors -- `desired_access`, `share_mode`, `security`, and
+    // `flags_and_attributes` -- all survived replacement by constants in a
+    // mutation run. Every test above builds a request and then *performs* it,
+    // and the perform path reads the struct's fields directly, so nothing
+    // distinguished an accessor that reports the truth from one that does not.
+    //
+    // `OpenFileByIdentifier::new` starts every parameter at zero or `None`, so
+    // the values here are deliberately non-zero and pairwise distinct: a zero
+    // would be indistinguishable from the default, and a repeated value would
+    // let one accessor read a neighbour's field undetected.
+    let _allocating = handle_allocation()
+        .read()
+        .expect("the lock is not poisoned");
+    let fixture = Fixture::new("byid-accessors");
+    let file = fixture.open_file();
+    let id = file_id_of(&file);
+    let hint = open_directory_for_hint(&fixture);
+
+    let request = OpenFileByIdentifier::new(
+        CapturedHandle::capture(hint.as_handle()).expect("capture the volume hint"),
+        FileIdentifier::FileId(id),
+    )
+    .with_desired_access(FILE_GENERIC_READ)
+    .with_share_mode(FILE_SHARE_READ)
+    .with_flags_and_attributes(FILE_FLAG_BACKUP_SEMANTICS);
+
+    assert_eq!(request.desired_access(), FILE_GENERIC_READ);
+    assert_eq!(request.share_mode(), FILE_SHARE_READ);
+    assert_eq!(request.flags_and_attributes(), FILE_FLAG_BACKUP_SEMANTICS);
+    assert!(
+        request.security().is_none(),
+        "nothing was supplied, so nothing must be reported"
+    );
+
+    let configured = [
+        FILE_GENERIC_READ,
+        FILE_SHARE_READ,
+        FILE_FLAG_BACKUP_SEMANTICS,
+    ];
+    for (index, value) in configured.iter().enumerate() {
+        assert_ne!(*value, 0, "a zero is indistinguishable from the default");
+        for other in &configured[index + 1..] {
+            assert_ne!(
+                value, other,
+                "two parameters share a value, so this test cannot tell their \
+                 accessors apart"
+            );
+        }
+    }
+}
+
+#[test]
+fn supplied_security_attributes_read_back_rather_than_reporting_none() {
+    // `security -> None` is the one accessor whose default is already `None`,
+    // so it needs the opposite case: attributes that were supplied must be
+    // visible to a caller inspecting the request, not only to the open that
+    // consumes it.
+    let _allocating = handle_allocation()
+        .read()
+        .expect("the lock is not poisoned");
+    let fixture = Fixture::new("byid-security");
+    let file = fixture.open_file();
+    let id = file_id_of(&file);
+    let hint = open_directory_for_hint(&fixture);
+
+    let request = OpenFileByIdentifier::new(
+        CapturedHandle::capture(hint.as_handle()).expect("capture the volume hint"),
+        FileIdentifier::FileId(id),
+    )
+    .with_security(Some(SecurityAttributes::new(None, false)));
+
+    assert!(request.security().is_some());
+}