provokes a warning about "args" never being used. Well, there's no meaningful way you can use it, it's a unit struct. You might have to pass it down to other functions for API contract reasons, but really it only exists because of clap's own API contracts (and as a placeholder, in case this subcommand grows options in the future).
You can easily squelch the warning with the _-prefix convention but this feels like it should be unnecessary, and that's one more thing to change in the future if the subcommand does grow some options. What do y'all think of having the warning be automatically suppressed for unused arguments whose type is unit? (Not just unit struct, because I can imagine a similar issue arising with () in generics.)
I like this on the grounds that it reduces a source of busywork to satisfy the linter without writing anything meaningful — I have written many trait impls on () or with associated types that are (), and needing to adjust all the names is tedious, especially if I have to adjust them back when changing the type to not be unit.
I dislike this on the grounds that it's letting a useless variable pass unnoticed.
Also, if the struct is intended to be some meaningful token, then the unused variable warning could be desired. Perhaps the warning should only be suppressed if the type is Copy.
This is exactly why I wouldn't want to suppress it: a unit type may still exist for the purposes of passing to some other function you call as a token (e.g. to prove you have a capability), and if you're not using it, it's very easy to write _: MyUnitType.
If you want to suppress it but also clarify to the reader that no data is lost, you can also use a pattern in the argument; never creating any unused variable either. This may be best if the name is not obviously a token type.
In terms of readability this looks odd at first but seems fine to me overall. My gut wants this to be an example motivating (limited) type deduction for arguments to avoid the duplication but that would be just a different token soup and honestly is very niche.
I've run into a related issue with trait default implementations. I don't want to rename the parameter because it shows up in docs as-is. So I end up with let _ = unused_arg; in the body of the default impl (that doesn't use/need the argument). I agree that a special case on the lint isn't very nice though, so I just kind of live with the in-body "suppression".
There is a lot of value in having warnings be precise. Noisy warnings only justify false positives if the true positives commonly identify real bugs.
In this case there seems to be a lot of speculation that the warned case is ambiguous. I don't know either way. I do know that clap already has an impl Args for (), but args: () still fires the warning. Could it be relaxed in that specific case?
Warnings should be precise is a general statement about warnings. If the compiler can be taught a special case for a warning that eliminates a whole class of false positives then (other factors aside) it should be taught that special case. Ideally when I see a warning I am expecting an issue with my code, not expecting to allow it.
The case in the OP is noise: the warning needs to be allow'd or otherwise disabled. I don't think there is an easy way for the compiler to be taught that some ZST's are tokens and some are ignorable. Perhaps an annotation on the struct, but then I wonder: if the code were then changed to add a parameter to that empty struct, I would probably remove the annotation and trigger the warning everywhere (and then either use my new parameter or silence it with let _ =). So having N places where the warning is disabled (via prefix _) doesn't seem that big a deal.
My point is that I don't think unused variables are sufficiently more likely to be noise just because they are zero-sized. We have established ways of quieting the lint (leading _), and IMO that is entirely sufficient, also for this case.
I agree lints should avoid noise. I just don't think this proposal significantly improves the signal-to-noise ratio of this lint.
It looks to me that the argument is unused in the original example? That argument to the function seems entirely useless. Perhaps it would be different in a larger more realistic example, but if so, such an example should be provided.
@RalfJung I am not entirely sure this proposal is worth it myself (which is why I didn't just file an enhancement request on the compiler) but does it make more sense to you if I say that the main thing I don't like about shutting up the warning with a leading _, in this case, is that I will then have to remove that leading _ when, inevitably, the subcommand grows some options?
@Vorpal The struct itself has to exist because that's how clap works, and it has to be supplied as an argument to the subcommand run function in order to keep the API between main and the subcommand run functions uniform. Again, the goal is to keep churn to an absolute minimum when, inevitably, options are added to subcommands that didn't have them.
Churn on introducing args would be reduced even further if you uniformly pattern matched the argument struct in every subcommand whether it has fields or not
use clap::Args;
use super::CommandError;
#[derive(Clone, Debug, Args)]
pub struct SubcommandWithNoOptionsArgs {}
pub fn subcommand_with_no_options_run(args: SubcommandWithNoOptionsArgs)
-> Result<(), CommandError> {
let SubcommandwithNoOptionsArgs { } = args;
Ok(())
}
It makes sense, but I feel like that's why, unique among many other lints, we make it possible to suppress this as simply as adding _ rather than having to write out something like expect(...). It's one character. And almost definitionally, you'll only need to change that _ in one place, because you won't have any other references to it.
I really like this solution, as it adds a place in the function where you acknowledge that it (currently) has no args.
Later if you add args, you get a nice error about missing fields, instead of a softer lint when it's no longer meeting the ZST special case.
The hard error is much more clear in my opinion.