Various "flags" consts should be type-associated consts

[Ben-Lichtman]

While using this library I was attempting to call functions such as

OpenProcess and CreateToolhelp32Snapshot - these function calls take in:
PROCESS_ACCESS_RIGHTS and CREATE_TOOLHELP_SNAPSHOT_FLAGS types respectively.

There are various consts within the modules which hold these types such as:
Windows::Win32::System::Threading::PROCESS_ALL_ACCESS which are an easy and ergonomic way to construct the types without using the raw integer values.

The ergonomic problem with this is that you have to pull every one of these consts into scope in order to use them, or pull Windows::Win32::System::Threading into scope and then use eg. Threading::PROCESS_ALL_ACCESS to create instances of these structs with the descriptive properties.

I propose that these consts be instead moved into the impl of the structs they create (in the same way as u32::MAX is associated with u32). This would make using these types much easier since now only the struct needs to be in scope to easily and ergonomically use these consts.

There are many other similar "flags" types with this kind of issue present that I have not mentioned here also.

[MarijnS95]

Most of this discussion has been ongoing in #727.

[Ben-Lichtman]

@MarijnS95 that discussion seems to be more about using enums vs consts, not moving consts from module level to struct level

[MarijnS95]

@Ben-Lichtman Those "enums" have never been real enums but constants associated to their type. Rust enums have never been an option because they're inconvenient to map to integers and back, whereas newtypes over integers with some associated constants allow to use this naturally (ie. use the appropriate type directly in an FFI struct).

Specifically that issue (and the resulting PR/change) proposed and implemented the inverse of what is suggested here, in order to be able to specify exactly what constants one wants instead of getting them all for free (±2000 on WIN32_ERROR iirc) when the type is generated. Additionally most Win32 enum-constants were supposed to be used that way, for example by restating the type-name in every variant.

[Ben-Lichtman]

Ah I see. I didn't realise that it was always a new type over integers.

I believe that issue was the wrong thing to do - it doesn't seem to be mentioned anywhere that it is possible to bring associated consts into scope using a use - for example https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=e668c48630e0cdc94b62ec8ebf564815

This is the same as how Some, None, Ok and Err are brought into scope by the stdlib.

I argue that the downside of extra generated code due to always having these consts be present for their associated type is completely offset by the upside of having the semantic meaning of each flag value being attached to its type / context.

I also argue that the problem of LONG_TYPE_NAME::LONG_TYPE_NAME_CONST_NAME is offset by the ability to just add a "use LONG_TYPE_NAME::LONG_TYPE_NAME_CONST_NAME" at the top of your file.

Though I can now see why people have argued for the opposite...

I guess it's up to the maintainers, but there is my argument.

[MarijnS95]

Bringing them in scope with use was never an issue, but representing that in the bindings macro might have been the tricky part. Though that should have been easily fixable, I even suggested that.

I argue that the downside of extra generated code due to always having these consts be present for their associated type is completely offset by the upside of having the semantic meaning of each flag value being attached to its type / context.

Same here, I'm not too sure if 2000 lines of constants really make that much of a difference to compile time, compared to actual codegen (and maybe macros).

My alternate suggestion is to additionally have the constant available as type-associated if it is required/demanded in the surrounding scope, as an alias to the module-level constant (or the other way around, it doesn't really matter). Furthermore common-prefix stripping makes things more pleasant to read while still having the full name in the surrounding module if desired.

[kennykerr]

I also argue that the problem of LONG_TYPE_NAME::LONG_TYPE_NAME_CONST_NAME is offset by the ability to just add a "use LONG_TYPE_NAME::LONG_TYPE_NAME_CONST_NAME" at the top of your file.

Can you provide an example? The following doesn't quite work so I may be missing something:

struct LONG_TYPE_NAME(pub i32);

impl LONG_TYPE_NAME {
    pub const LONG_TYPE_NAME_CONST_NAME : Self = Self(1);
}

use LONG_TYPE_NAME::LONG_TYPE_NAME_CONST_NAME;

fn main() {
    let _value = LONG_TYPE_NAME_CONST_NAME;
}

[MarijnS95]

Oh...

I believe that issue was the wrong thing to do - it doesn't seem to be mentioned anywhere that it is possible to bring associated consts into scope using a use - for example https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=e668c48630e0cdc94b62ec8ebf564815

This example only works because std::u8 is a module, not the primitive type.

This is the same as how Some, None, Ok and Err are brought into scope by the stdlib.

And iirc this only works because those are enum variants, not associated constants.

In the end I think that's why my suggestion didn't work well, but it would have been a "custom" lookup in windows-rs' gen either way.

[kennykerr]

The current approach is not without cost - it takes about 200ms to index these constants (even if they're never called for). #831 would solve that if it only let you specify the type name. I'm inclined to do that if adding 3000+ constants (for WIN32_ERROR for example) doesn't measurably impact build time.

[Ben-Lichtman]

This example only works because std::u8 is a module, not the primitive type.

Oops, well that's an embarrassing oversight...

Looks like std::u8 is also deprecated in favor of using associated consts but you can't bring them into scope...

Maybe this could be a lang issue hah

[kennykerr]

This should be a little easier as of #843.

[Ben-Lichtman]

I have created an issue on rust to hopefully address this shortcoming rust-lang/rust#86129