Derive API doesn't respect id attribute

[survived]

Please complete the following tasks

• I have searched the discussions
• I have searched the open and rejected issues

Rust Version

rustc 1.60.0 (7737e0b5c 2022-04-04)

Clap Version

3.1.18

Minimal reproducible code

playground

#[derive(clap::Parser)]
struct App {
    #[clap(id = "some-argument", long = "some-argument")]
    field: String,
}

fn main() {
    let _parsed: App = clap::Parser::parse_from(["app", "--some-argument", "123"]);
}

Steps to reproduce the bug with the above code

cargo run

Actual Behaviour

Panics with a message:

thread 'main' panicked at '`field` is not a name of an argument or a group.
Make sure you're using the name of the argument itself and not the name of short or long flags.', src/main.rs:5:5

Expected Behaviour

In the code above, you can replace id with name and it will work just fine. Arg::id was introduced in 3.1.0, it's supposed to replace Arg::name. However, derive api doesn't work with id attribute, it keeps supporting name attribute that refers to deprecated method.

I expect that id attribute will have the same behaviour as name. Moreover, to be aligned with changes in library API, derive API ideally should yield a warning that name attribute is deprecated.

Additional Context

No response

Debug Output

No response

[danielparks]

I wrote a quick fix for this, but I have a couple of questions before I submit a PR:

1. Should the name and id attributes really do the same thing? I see that Arg and Command both have separate name and id fields, though I haven’t taken the time to figure out what exactly they do.
2. Should this change only apply to fields? Given that Command has an id field as well I’m inclined to make it universal, which seems simpler.
3. Would you prefer a separate PR to update documentation, or should I combine them into one?

[epage]

Should the name and id attributes really do the same thing? I see that Arg and Command both have separate name and id fields, though I haven’t taken the time to figure out what exactly they do.

While this is murkier in clap v3, clap v4 is working to make this more obvious (some deprecations move us in this direction): name and id are distinct. a command's name is both an identifier and a user-visible value. An argument's ID is mostly just an identifier though it might be used as a default for Arg::value_name.

Us conflating the two has led to maintainer confusion in some of the design and user confusion in how they are used.

Should this change only apply to fields? Given that Command has an id field as well I’m inclined to make it universal, which seems simpler.

Could you clarify what you mean? Are you speaking in terms of should this apply to both Arg and Command? Yes, we should respect explicit overrides in both

Would you prefer a separate PR to update documentation, or should I combine them into one?

Commits should be atomic. If the code is deviating from the documentation, the documentation should be updated at the same time.

What documentation updates do you see being needed?

[danielparks]

Thanks for the quick response. I‘m not very familiar with clap’s internals, so I appreciate your clarifications.

It sounds like the attribute behavior should be different for Command and Arg. Specifically:

1. Command: set only the id field. This will probably involve adding a public id() method to the Command struct.
2. Arg: set both id and name, much as the name attribute does now.

There are a few derive reference doc updates needed:

1. Add id attribute for Args.
2. Depreciate name attribute on Args.
3. Add id attribute for Commands.

[epage]

Command: set only the id field. This will probably involve adding a public id() method to the Command struct.

From a users perspective, Command only has name. Ignore the id field (it will likely go away in clap v4).

Arg: set both id and name, much as the name attribute does now.

From a users perspective, Arg only has id. Ignore the name field (it will likely go away in clap v4)

Something that confuses the matter further is Arg also has value_name[s], e.g. #3709

[danielparks]

Seems like we should disallow the id attribute on Command, then? Unfortunately, that’s going to be either messy or complicated.

Here is the solution I have that effectively just recognizes the id attribute as an alias for name.

[epage]

I'd say that solution is good enough for now. Note that we would merge for master first which would has all deprecated functionality removed. We'd want to add back in the name attribute in the docs if/when its backported to v3-master.

If we want to go for the error, short term, we could augment push_attrs with what type of attribute it is Command, Arg, etc) and do error checking in the case for PushMethod

Longer term, I'm going to be splitting up the attributes like #[command(name = "foo")] and it'll be easier to do such checks

[danielparks]

Thanks for all of your clarifications and help. I’ve submitted PR #4049; let me know if it needs updates.