Does NTSTATUS need an Eq implementation?

[kennykerr]

#707 (comment)

[MarijnS95]

Another question that doesn't feel worthy for a separate issue: where should type changes be requested for thing like https://docs.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-exception_record turning ExceptionCode into NTRESULT? Together with an Eq here one could easily throw record.ExceptionCode into a match and compare it against existing constants.

[kennykerr]

This is controlled by the https://github.com/microsoft/win32metadata/ project that produces the metadata describing these APIs. You can create an issue there with suggestions to improve the metadata.

[MarijnS95]

Great, it's actually documented how to set up such an override in https://github.com/microsoft/win32metadata#how-you-can-help / https://github.com/microsoft/win32metadata#changing-fieldparameter-types, without touching the underlying header source 👍

(Not sure if these overrides should, or will at some point, be propagated back into Windows headers?)

[kennykerr]

I just had a quick look and NTSTATUS does get an implementation of Eq and PartialEq. Was there something else that is missing?

#[repr(transparent)]
#[derive(:: std :: clone :: Clone, :: std :: marker :: Copy)]
pub struct NTSTATUS(pub i32);
impl NTSTATUS {}
impl ::std::default::Default for NTSTATUS {
    fn default() -> Self {
        Self(0)
    }
}
impl NTSTATUS {
    pub const NULL: Self = Self(0);
    pub fn is_null(&self) -> bool {
        self.0 == 0
    }
}
impl ::std::fmt::Debug for NTSTATUS {
    fn fmt(&self, fmt: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result {
        fmt.debug_struct("NTSTATUS")
            .field("Value", &format_args!("{:?}", self.0))
            .finish()
    }
}
impl ::std::cmp::PartialEq for NTSTATUS {
    fn eq(&self, other: &Self) -> bool {
        self.0 == other.0
    }
}
impl ::std::cmp::Eq for NTSTATUS {}
unsafe impl ::windows::Abi for NTSTATUS {
    type Abi = Self;
}

[MarijnS95]

@kennykerr Check - I misread and misreported the error. It must be implemented through #[derive] for match blocks to work:

error: to use a constant of type `NTSTATUS` in a pattern, `NTSTATUS` must be annotated with `#[derive(PartialEq, Eq)]`
  --> tests/handles/tests/null.rs:45:9
   |
45 |         SystemServices::DBG_PRINTEXCEPTION_WIDE_C | SystemServices::DBG_PRINTEXCEPTION_C
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: to use a constant of type `NTSTATUS` in a pattern, `NTSTATUS` must be annotated with `#[derive(PartialEq, Eq)]`
  --> tests/handles/tests/null.rs:45:53
   |
45 |         SystemServices::DBG_PRINTEXCEPTION_WIDE_C | SystemServices::DBG_PRINTEXCEPTION_C
   |                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

See also https://rust-lang.github.io/rfcs/1445-restrict-constants-in-patterns.html.

[rylev]

We had an issue for this at some point, but I'm failing to find it. We moved away from deriving some of these traits because it really hurt compile times. We might want to look at putting derives back in these specific cases and see how much it hurts compile times nowadays.

[kennykerr]

I'll see what I can do - there are many structs that derive doesn't support. Even something as simple as a struct with an f32 field can't be derived, and of course unions, and a few other edge cases. So I can't just switch to derive as much as I'd like to.

[kennykerr]

@rylev - I ended up going back to deriving as much as possible - its just much simpler - but for structs its quite a mess as there are just so many scenarios that aren't supported by derive.

I'll try to get these cleaned up and get this working.

[toshipiazza]

Did this regress? Version 0.38.0 defines NTSTATUS like so

#[repr(transparent)]
pub struct NTSTATUS(pub i32);

NTSTATUS derives neither Eq nor PartialEq, so I see this error message

error: to use a constant of type `NTSTATUS` in a pattern, `NTSTATUS` must be annotated with `#[derive(PartialEq, Eq)]`
   --> libs\debugger\src\debugger.rs:950:79
    |
950 | ...                   matches!(exception.ExceptionRecord.ExceptionCode, EXCEPTION_ACCESS_VIOLATION);
    |                                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^

[kgv]

No regress, NTSTATUS implements PartialEq and Eq but not derives it.

RFC 1445

The practical effect of these changes will be to prevent the use of constants in patterns unless the type of those constants is either a built-in type (like i32 or &str) or a user-defined constant for which Eq is derived (not merely implemented).

So, can two NTSTATUSes be semantically unequal with the same content? If not, then it should be declared as #[derive(PartialEq, Eq)].

[kennykerr]

@toshipiazza see #1869 for a fix.

[MarijnS95]

@kgv This did regress, #994 originally fixed this issue (#739) but it seems to have accidentally been undone ("regressed") in #1379.