Replace From<&str> for GUID with TryFrom<&str> (#3193)

microsoft · Aug 7, 2024 · 1d977ac · 1d977ac
1 parent fdc5149
commit 1d977ac
Show file tree

Hide file tree

Showing 15 changed files with 165 additions and 124 deletions.
diff --git a/crates/libs/core/src/guid.rs b/crates/libs/core/src/guid.rs
@@ -1,5 +1,3 @@
-#![allow(clippy::many_single_char_names)]
-
 use super::*;
 
 /// A globally unique identifier ([GUID](https://docs.microsoft.com/en-us/windows/win32/api/guiddef/ns-guiddef-guid))
@@ -120,37 +118,30 @@ impl core::fmt::Debug for GUID {
     }
 }
 
-impl From<&str> for GUID {
-    fn from(value: &str) -> Self {
-        assert!(value.len() == 36, "Invalid GUID string");
-        let mut bytes = value.bytes();
-
-        let a = ((bytes.next_u32() * 16 + bytes.next_u32()) << 24)
-            + ((bytes.next_u32() * 16 + bytes.next_u32()) << 16)
-            + ((bytes.next_u32() * 16 + bytes.next_u32()) << 8)
-            + bytes.next_u32() * 16
-            + bytes.next_u32();
-        assert!(bytes.next().unwrap() == b'-', "Invalid GUID string");
-        let b = ((bytes.next_u16() * 16 + (bytes.next_u16())) << 8)
-            + bytes.next_u16() * 16
-            + bytes.next_u16();
-        assert!(bytes.next().unwrap() == b'-', "Invalid GUID string");
-        let c = ((bytes.next_u16() * 16 + bytes.next_u16()) << 8)
-            + bytes.next_u16() * 16
-            + bytes.next_u16();
-        assert!(bytes.next().unwrap() == b'-', "Invalid GUID string");
-        let d = bytes.next_u8() * 16 + bytes.next_u8();
-        let e = bytes.next_u8() * 16 + bytes.next_u8();
-        assert!(bytes.next().unwrap() == b'-', "Invalid GUID string");
-
-        let f = bytes.next_u8() * 16 + bytes.next_u8();
-        let g = bytes.next_u8() * 16 + bytes.next_u8();
-        let h = bytes.next_u8() * 16 + bytes.next_u8();
-        let i = bytes.next_u8() * 16 + bytes.next_u8();
-        let j = bytes.next_u8() * 16 + bytes.next_u8();
-        let k = bytes.next_u8() * 16 + bytes.next_u8();
-
-        Self::from_values(a, b, c, [d, e, f, g, h, i, j, k])
+impl TryFrom<&str> for GUID {
+    type Error = Error;
+
+    fn try_from(from: &str) -> Result<Self> {
+        if from.len() != 36 {
+            return Err(invalid_guid());
+        }
+
+        let bytes = &mut from.bytes();
+        let mut guid = Self::zeroed();
+
+        guid.data1 = try_u32(bytes, true)?;
+        guid.data2 = try_u16(bytes, true)?;
+        guid.data3 = try_u16(bytes, true)?;
+        guid.data4[0] = try_u8(bytes, false)?;
+        guid.data4[1] = try_u8(bytes, true)?;
+        guid.data4[2] = try_u8(bytes, false)?;
+        guid.data4[3] = try_u8(bytes, false)?;
+        guid.data4[4] = try_u8(bytes, false)?;
+        guid.data4[5] = try_u8(bytes, false)?;
+        guid.data4[6] = try_u8(bytes, false)?;
+        guid.data4[7] = try_u8(bytes, false)?;
+
+        Ok(guid)
     }
 }
 
@@ -166,28 +157,43 @@ impl From<GUID> for u128 {
     }
 }
 
-trait HexReader {
-    fn next_u8(&mut self) -> u8;
-    fn next_u16(&mut self) -> u16;
-    fn next_u32(&mut self) -> u32;
+fn invalid_guid() -> Error {
+    Error::from_hresult(imp::E_INVALIDARG)
 }
 
-impl HexReader for core::str::Bytes<'_> {
-    fn next_u8(&mut self) -> u8 {
-        let value = self.next().unwrap();
-        match value {
-            b'0'..=b'9' => value - b'0',
-            b'A'..=b'F' => 10 + value - b'A',
-            b'a'..=b'f' => 10 + value - b'a',
-            _ => panic!(),
-        }
-    }
+fn try_u32(bytes: &mut core::str::Bytes<'_>, delimiter: bool) -> Result<u32> {
+    next(bytes, 8, delimiter).ok_or_else(invalid_guid)
+}
 
-    fn next_u16(&mut self) -> u16 {
-        self.next_u8().into()
+fn try_u16(bytes: &mut core::str::Bytes<'_>, delimiter: bool) -> Result<u16> {
+    next(bytes, 4, delimiter)
+        .map(|value| value as u16)
+        .ok_or_else(invalid_guid)
+}
+
+fn try_u8(bytes: &mut core::str::Bytes<'_>, delimiter: bool) -> Result<u8> {
+    next(bytes, 2, delimiter)
+        .map(|value| value as u8)
+        .ok_or_else(invalid_guid)
+}
+
+fn next(bytes: &mut core::str::Bytes<'_>, len: usize, delimiter: bool) -> Option<u32> {
+    let mut value: u32 = 0;
+
+    for _ in 0..len {
+        let digit = bytes.next()?;
+
+        match digit {
+            b'0'..=b'9' => value = (value << 4) + (digit - b'0') as u32,
+            b'A'..=b'F' => value = (value << 4) + (digit - b'A' + 10) as u32,
+            b'a'..=b'f' => value = (value << 4) + (digit - b'a' + 10) as u32,
+            _ => return None,
+        }
     }
 
-    fn next_u32(&mut self) -> u32 {
-        self.next_u8().into()
+    if delimiter && bytes.next() != Some(b'-') {
+        None
+    } else {
+        Some(value)
     }
 }
diff --git a/crates/libs/core/src/imp/com_bindings.rs b/crates/libs/core/src/imp/com_bindings.rs
@@ -41,6 +41,7 @@ impl core::fmt::Debug for AgileReferenceOptions {
 }
 pub const CO_E_NOTINITIALIZED: windows_core::HRESULT = windows_core::HRESULT(0x800401F0_u32 as _);
 pub const E_BOUNDS: windows_core::HRESULT = windows_core::HRESULT(0x8000000B_u32 as _);
+pub const E_INVALIDARG: windows_core::HRESULT = windows_core::HRESULT(0x80070057_u32 as _);
 pub const E_NOINTERFACE: windows_core::HRESULT = windows_core::HRESULT(0x80004002_u32 as _);
 pub const E_OUTOFMEMORY: windows_core::HRESULT = windows_core::HRESULT(0x8007000E_u32 as _);
 pub const E_POINTER: windows_core::HRESULT = windows_core::HRESULT(0x80004003_u32 as _);

diff --git a/crates/libs/core/src/imp/sha1.rs b/crates/libs/core/src/imp/sha1.rs
@@ -1,5 +1,3 @@
-#![allow(clippy::many_single_char_names)]
-
 pub const fn sha1(data: &ConstBuffer) -> Digest {
     let state: [u32; 5] = [0x67452301, 0xefcdab89, 0x98badcfe, 0x10325476, 0xc3d2e1f0];
     let len: u64 = 0;

diff --git a/crates/tests/core/tests/guid.rs b/crates/tests/core/tests/guid.rs
@@ -1,4 +1,4 @@
-use windows::core::GUID;
+use windows::{core::*, Win32::Foundation::E_INVALIDARG};
 
 #[test]
 fn test_new() {
@@ -9,7 +9,7 @@ fn test_new() {
 
 #[test]
 fn from_u128() {
-    let a: GUID = "1FD63FEF-C0D2-42FE-823A-53A4052B8C8F".into();
+    let a: GUID = "1FD63FEF-C0D2-42FE-823A-53A4052B8C8F".try_into().unwrap();
     let b = GUID::from_values(
         0x1fd63fef,
         0xc0d2,
@@ -25,7 +25,51 @@ fn from_u128() {
 #[test]
 fn to_u128() {
     let num: u128 = 0x1fd63fef_c0d2_42fe_823a_53a4052b8c8f;
-    let guid: GUID = "1FD63FEF-C0D2-42FE-823A-53A4052B8C8F".into();
+    let guid: GUID = "1FD63FEF-C0D2-42FE-823A-53A4052B8C8F".try_into().unwrap();
 
     assert_eq!(u128::from(guid), num); // From<GUID>
 }
+
+#[test]
+fn parsing() {
+    assert_eq!(
+        GUID::zeroed(),
+        "00000000-0000-0000-0000-000000000000".try_into().unwrap()
+    );
+
+    // Validate invalid length and expected error information.
+    let e = GUID::try_from("wrong length").unwrap_err();
+    assert_eq!(e.code(), E_INVALIDARG);
+    assert!(e.as_ptr().is_null());
+
+    // Validate delimiter
+    GUID::try_from("00000000?0000-0000-0000-000000000000").unwrap_err();
+    GUID::try_from("00000000-0000?0000-0000-000000000000").unwrap_err();
+    GUID::try_from("00000000-0000-0000?0000-000000000000").unwrap_err();
+    GUID::try_from("00000000-0000-0000-0000?000000000000").unwrap_err();
+
+    // Validate invalid digits
+    GUID::try_from("z0000000-0000-0000-0000-000000000000").unwrap_err();
+    GUID::try_from("00000000-z000-0000-0000-000000000000").unwrap_err();
+    GUID::try_from("00000000-0000-z000-0000-000000000000").unwrap_err();
+    GUID::try_from("00000000-0000-0000-z000-000000000000").unwrap_err();
+    GUID::try_from("00000000-0000-0000-0000-z00000000000").unwrap_err();
+
+    // Validate case insensitivity
+    let value = GUID::from_u128(0x1fd63fef_c0d2_42fe_823a_53a4052b8c8f);
+    assert_eq!(
+        value,
+        "1FD63FEF-C0D2-42FE-823A-53A4052B8C8F".try_into().unwrap()
+    );
+    assert_eq!(
+        value,
+        "1fd63fef-c0d2-42fe-823a-53a4052b8c8f".try_into().unwrap()
+    );
+}
+
+#[test]
+fn debug() {
+    let value = GUID::from_u128(0x1fd63fef_c0d2_42fe_823a_53a4052b8c8f);
+
+    assert_eq!(format!("{value:?}"), "1FD63FEF-C0D2-42FE-823A-53A4052B8C8F");
+}
diff --git a/crates/tests/interface/tests/com.rs b/crates/tests/interface/tests/com.rs
@@ -61,7 +61,7 @@ struct PersistState {
 
 impl ICustomPersist_Impl for Persist_Impl {
     unsafe fn GetClassID(&self, clsid: *mut GUID) -> HRESULT {
-        *clsid = "117fb826-2155-483a-b50d-bc99a2c7cca3".into();
+        *clsid = "117fb826-2155-483a-b50d-bc99a2c7cca3".try_into().unwrap();
         S_OK
     }
 }
@@ -136,7 +136,7 @@ fn test_custom_interface() -> windows::core::Result<()> {
         let p: IPersistMemory = p.cast()?;
         assert_eq!(
             p.GetClassID()?,
-            "117fb826-2155-483a-b50d-bc99a2c7cca3".into()
+            "117fb826-2155-483a-b50d-bc99a2c7cca3".try_into()?,
         );
         assert_eq!(p.GetSizeMax()?, 10);
         assert_eq!(p.IsDirty(), S_FALSE);
@@ -165,7 +165,7 @@ fn test_custom_interface() -> windows::core::Result<()> {
         let p: ICustomPersist = p.cast()?;
         let mut b = GUID::default();
         p.GetClassID(&mut b).ok()?;
-        assert_eq!(b, "117fb826-2155-483a-b50d-bc99a2c7cca3".into());
+        assert_eq!(b, "117fb826-2155-483a-b50d-bc99a2c7cca3".try_into()?);
 
         Ok(())
     }

diff --git a/crates/tests/interface/tests/com_from_existing.rs b/crates/tests/interface/tests/com_from_existing.rs
@@ -14,7 +14,7 @@ struct Test;
 
 impl IPersist_Impl for Test_Impl {
     fn GetClassID(&self) -> Result<GUID> {
-        Ok("CEE1D356-0860-4262-90D4-C77423F0E352".into())
+        "CEE1D356-0860-4262-90D4-C77423F0E352".try_into()
     }
 }
 
@@ -30,7 +30,7 @@ fn test() -> Result<()> {
         let p: IPersist = Test.into();
         assert_eq!(
             p.GetClassID()?,
-            "CEE1D356-0860-4262-90D4-C77423F0E352".into()
+            "CEE1D356-0860-4262-90D4-C77423F0E352".try_into()?
         );
 
         let m: ITestPersistMemory = p.cast()?;

diff --git a/crates/tests/interface/tests/no_use.rs b/crates/tests/interface/tests/no_use.rs
@@ -12,7 +12,7 @@ struct Test;
 
 impl windows::Win32::System::Com::IPersist_Impl for Test_Impl {
     fn GetClassID(&self) -> windows::core::Result<windows::core::GUID> {
-        Ok("CEE1D356-0860-4262-90D4-C77423F0E352".into())
+        "CEE1D356-0860-4262-90D4-C77423F0E352".try_into()
     }
 }
 
@@ -28,7 +28,7 @@ fn test() -> windows::core::Result<()> {
         let p: windows::Win32::System::Com::IPersist = Test.into();
         assert_eq!(
             p.GetClassID()?,
-            "CEE1D356-0860-4262-90D4-C77423F0E352".into()
+            "CEE1D356-0860-4262-90D4-C77423F0E352".try_into()?
         );
 
         let m: ITestPersistMemory = windows_core::Interface::cast(&p)?;

diff --git a/crates/tests/structs/tests/propertykey.rs b/crates/tests/structs/tests/propertykey.rs
@@ -2,6 +2,9 @@ use windows::Win32::Devices::Properties::DEVPKEY_Device_BiosDeviceName;
 
 #[test]
 fn test_debug_impl() {
-    assert!(DEVPKEY_Device_BiosDeviceName.fmtid == "540B947E-8B40-45BC-A8A2-6A0B894CBDA2".into());
+    assert!(
+        DEVPKEY_Device_BiosDeviceName.fmtid
+            == "540B947E-8B40-45BC-A8A2-6A0B894CBDA2".try_into().unwrap()
+    );
     assert!(DEVPKEY_Device_BiosDeviceName.pid == 10);
 }
diff --git a/crates/tests/win32/tests/win32.rs b/crates/tests/win32/tests/win32.rs
@@ -102,7 +102,7 @@ fn constant() {
     assert!(WM_KEYUP == 257u32);
     assert!(D3D12_DEFAULT_BLEND_FACTOR_ALPHA == 1f32);
     assert!(UIA_ScrollPatternNoScroll == -1f64);
-    assert!(CLSID_D2D1Shadow == GUID::from("C67EA361-1863-4e69-89DB-695D3E9A5B6B"));
+    assert!(CLSID_D2D1Shadow == GUID::try_from("C67EA361-1863-4e69-89DB-695D3E9A5B6B").unwrap());
 
     let b: PCSTR = D3DCOMPILER_DLL_A;
     let c: PCWSTR = D3DCOMPILER_DLL_W;
@@ -264,5 +264,5 @@ fn empty_struct() {
     assert!(ldap.0 == 123);
     assert!(core::mem::size_of::<PLDAPSearch>() == core::mem::size_of::<usize>());
 
-    assert!(UIAnimationManager == GUID::from("4C1FC63A-695C-47E8-A339-1A194BE3D0B8"));
+    assert!(UIAnimationManager == GUID::try_from("4C1FC63A-695C-47E8-A339-1A194BE3D0B8").unwrap());
 }
diff --git a/crates/tests/winrt/tests/delegates.rs b/crates/tests/winrt/tests/delegates.rs
@@ -10,7 +10,7 @@ fn non_generic() -> windows::core::Result<()> {
 
     assert_eq!(
         Handler::IID,
-        windows::core::GUID::from("A4ED5C81-76C9-40BD-8BE6-B1D90FB20AE7")
+        windows::core::GUID::try_from("A4ED5C81-76C9-40BD-8BE6-B1D90FB20AE7")?
     );
 
     let (tx, rx) = std::sync::mpsc::channel();
@@ -37,7 +37,7 @@ fn generic() -> windows::core::Result<()> {
 
     assert_eq!(
         Handler::IID,
-        windows::core::GUID::from("DAE18EA9-FCF3-5ACF-BCDD-8C354CBA6D23")
+        windows::core::GUID::try_from("DAE18EA9-FCF3-5ACF-BCDD-8C354CBA6D23")?
     );
 
     let uri = Uri::CreateUri(&windows::core::HSTRING::from("http://kennykerr.ca"))?;