Run debug_asserts during compile time for derives

epage commented 2 years ago

Issue by glittershark Friday Aug 27, 2021 at 14:00 GMT Originally opened as https://github.com/clap-rs/clap/issues/2740

Please complete the following tasks

[X] I have searched the discussions
[X] I have searched the existing issues

Clap Version

3.0.0-beta.4

Describe your use case

Consider the following app:

use clap::Clap;

#[derive(Clap)]
struct Opts {
    #[clap(long, requires("foo"))]
    bar: i32
}

That app has a bug - the foo argument doesn't exist - but you don't have any way of finding out about it unless you actually run the application! In the real world, outside of the realm of contrived example code, this happened at $work where one parallel branch renamed an argument, while another added a new argument that required the old name - and since we didn't (previously, we do now) have a test covering option parsing, this went uncaught until we actually tried to run the binary.

Describe the solution you'd like

I was thinking it'd be awesome if #[derive(Clap)] could detect usages of requires (and other things that reference the name of another argument) and emit a compilation error if they reference arguments that don't exist. This might get a little hairy in the presence of #[clap(flatten)] et al, though, and I'm not super familiar with the internals of derive macros so the information may well just not be available at this point

Alternatives, if applicable

Maybe if it's not feasible to do this inside the macro itself (or not desired, because of the weird interaction with #[clap(flatten)] we could instead have some sort of custom clippy lint that detects this?

Additional Context

No response

epage commented 2 years ago

Comment by epage Friday Aug 27, 2021 at 14:05 GMT

Yes, this would not be possible with flatten

I was thinking maybe we could if flatten isn't present but that wouldn't work if we were deriving a mix-in.

It looks like we do have a debug_assert for this case, unless there is a corner we are missing.

epage commented 2 years ago

Comment by glittershark Friday Aug 27, 2021 at 14:08 GMT

I wonder how frequently people use requires on a field that isn't actually defined in the struct they're defining - I'd bet it's a minority of cases. I wonder if we could get away with something like

use clap::Clap;

#[derive(Clap)]
struct Opts {
    #[clap(long, requires("foo", allow_missing))]
    bar: i32
}

to opt-out of the compile-time check for the field existing.

epage commented 2 years ago

Comment by glittershark Friday Aug 27, 2021 at 14:10 GMT

It looks like we do have a debug_assert for this case, unless there is a corner we are missing.

I'm not familiar with the structure of clap - is assert_app run as part of the derive macro (so at compile-time) or at runtime?

epage commented 2 years ago

Comment by epage Friday Aug 27, 2021 at 14:20 GMT

I'm not familiar with the structure of clap - is assert_app run as part of the derive macro (so at compile-time) or at runtime?

It happens at runtime with debug builds. You could have your CI run --help to ensure all debug asserts pass which gives you at least some level of enforcement.

epage commented 2 years ago

Comment by glittershark Friday Aug 27, 2021 at 18:33 GMT

You could have your CI run --help to ensure all debug asserts pass which gives you at least some level of enforcement.

Yeah, I just wrote a test that calls Opts::parse_from(...), but it'd be nice to have it as a compile-time check without having to remember to write a test.

epage commented 2 years ago

Comment by epage Friday Aug 27, 2021 at 18:34 GMT

Thats understandable

epage / clapng