Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(derive): Report deprecations/errors in more cases #4181

Merged
merged 2 commits into from Sep 3, 2022
Merged

fix(derive): Report deprecations/errors in more cases #4181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9ad2c5
fix(derive): Guard against invalid attributes
epage Sep 3, 2022
a7ed5d0
fix(derive): Deprecate using `name` for Args
epage Sep 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
170 changes: 89 additions & 81 deletions clap_derive/src/item.rs
View file
Open in desktop
Expand Up @@ -91,19 +91,6 @@ impl Item {
res.push_attrs(attrs);
res.push_doc_comment(attrs, "about");

if let Some(value_parser) = res.value_parser.as_ref() {
abort!(
value_parser.span(),
"`value_parser` attribute is only allowed on fields"
);
}
if let Some(action) = res.action.as_ref() {
abort!(
action.span(),
"`action` attribute is only allowed on fields"
);
}

res
}

Expand All @@ -121,18 +108,6 @@ impl Item {

match &*res.kind {
Kind::Flatten => {
if let Some(value_parser) = res.value_parser.as_ref() {
abort!(
value_parser.span(),
"`value_parser` attribute is not allowed for flattened entry"
);
}
if let Some(action) = res.action.as_ref() {
abort!(
action.span(),
"`action` attribute is not allowed for flattened entry"
);
}
if res.has_explicit_methods() {
abort!(
res.kind.span(),
Expand All @@ -145,19 +120,6 @@ impl Item {
}

Kind::Subcommand(_) => {
if let Some(value_parser) = res.value_parser.as_ref() {
abort!(
value_parser.span(),
"`value_parser` attribute is not allowed for subcommand"
);
}
if let Some(action) = res.action.as_ref() {
abort!(
action.span(),
"`action` attribute is not allowed for subcommand"
);
}

use syn::Fields::*;
use syn::FieldsUnnamed;
let field_ty = match variant.fields {
Expand Down Expand Up @@ -223,19 +185,6 @@ impl Item {
res.push_attrs(&variant.attrs);
res.push_doc_comment(&variant.attrs, "help");

if let Some(value_parser) = res.value_parser.as_ref() {
abort!(
value_parser.span(),
"`value_parser` attribute is only allowed on fields"
);
}
if let Some(action) = res.action.as_ref() {
abort!(
action.span(),
"`action` attribute is only allowed on fields"
);
}

res
}

Expand All @@ -259,18 +208,6 @@ impl Item {

match &*res.kind {
Kind::Flatten => {
if let Some(value_parser) = res.value_parser.as_ref() {
abort!(
value_parser.span(),
"`value_parser` attribute is not allowed for flattened entry"
);
}
if let Some(action) = res.action.as_ref() {
abort!(
action.span(),
"`action` attribute is not allowed for flattened entry"
);
}
if res.has_explicit_methods() {
abort!(
res.kind.span(),
Expand All @@ -283,18 +220,6 @@ impl Item {
}

Kind::Subcommand(_) => {
if let Some(value_parser) = res.value_parser.as_ref() {
abort!(
value_parser.span(),
"`value_parser` attribute is not allowed for subcommand"
);
}
if let Some(action) = res.action.as_ref() {
abort!(
action.span(),
"`action` attribute is not allowed for subcommand"
);
}
if res.has_explicit_methods() {
abort!(
res.kind.span(),
Expand Down Expand Up @@ -403,8 +328,41 @@ impl Item {
}
}

fn push_method(&mut self, name: Ident, arg: impl ToTokens) {
if name == "name" || name == "id" {
fn push_method(&mut self, kind: AttrKind, name: Ident, arg: impl ToTokens) {
if name == "id" {
match kind {
AttrKind::Command | AttrKind::Value => {
self.deprecations.push(Deprecation {
span: name.span(),
id: "id_is_only_for_arg",
version: "4.0.0",
description: format!(
"`#[{}(id)] was allowed by mistake, instead use `#[{}(name)]`",
kind.as_str(),
kind.as_str()
),
});
}
AttrKind::Arg | AttrKind::Clap | AttrKind::StructOpt => {}
}
self.name = Name::Assigned(quote!(#arg));
} else if name == "name" {
match kind {
AttrKind::Arg => {
self.deprecations.push(Deprecation {
span: name.span(),
id: "id_is_only_for_arg",
version: "4.0.0",
description: format!(
"`#[{}(name)] was allowed by mistake, instead use `#[{}(id)]` or `#[{}(value_name)]`",
kind.as_str(),
kind.as_str(),
kind.as_str()
),
});
}
AttrKind::Command | AttrKind::Value | AttrKind::Clap | AttrKind::StructOpt => {}
}
self.name = Name::Assigned(quote!(#arg));
} else if name == "value_parser" {
self.value_parser = Some(ValueParser::Explicit(Method::new(name, quote!(#arg))));
Expand Down Expand Up @@ -504,24 +462,31 @@ impl Item {

if let Some(AttrValue::Call(tokens)) = &attr.value {
// Force raw mode with method call syntax
self.push_method(attr.name.clone(), quote!(#(#tokens),*));
self.push_method(*attr.kind.get(), attr.name.clone(), quote!(#(#tokens),*));
continue;
}

match &attr.magic {
Some(MagicAttrName::Short) if attr.value.is_none() => {
assert_attr_kind(attr, &[AttrKind::Arg]);

self.push_method(
*attr.kind.get(),
attr.name.clone(),
self.name.clone().translate_char(*self.casing),
);
}

Some(MagicAttrName::Long) if attr.value.is_none() => {
self.push_method(attr.name.clone(), self.name.clone().translate(*self.casing));
assert_attr_kind(attr, &[AttrKind::Arg]);

self.push_method(*attr.kind.get(), attr.name.clone(), self.name.clone().translate(*self.casing));
}

#[cfg(not(feature = "unstable-v5"))]
Some(MagicAttrName::ValueParser) if attr.value.is_none() => {
assert_attr_kind(attr, &[AttrKind::Arg]);

self.deprecations.push(Deprecation {
span: attr.name.span(),
id: "bare_value_parser",
Expand All @@ -533,6 +498,8 @@ impl Item {

#[cfg(not(feature = "unstable-v5"))]
Some(MagicAttrName::Action) if attr.value.is_none() => {
assert_attr_kind(attr, &[AttrKind::Arg]);

self.deprecations.push(Deprecation {
span: attr.name.span(),
id: "bare_action",
Expand All @@ -543,13 +510,18 @@ impl Item {
}

Some(MagicAttrName::Env) if attr.value.is_none() => {
assert_attr_kind(attr, &[AttrKind::Arg]);

self.push_method(
*attr.kind.get(),
attr.name.clone(),
self.name.clone().translate(*self.env_casing),
);
}

Some(MagicAttrName::ValueEnum) if attr.value.is_none() => {
assert_attr_kind(attr, &[AttrKind::Arg]);

self.is_enum = true
}

Expand All @@ -558,6 +530,8 @@ impl Item {
}

Some(MagicAttrName::About) if attr.value.is_none() => {
assert_attr_kind(attr, &[AttrKind::Command]);

if let Some(method) =
Method::from_env(attr.name.clone(), "CARGO_PKG_DESCRIPTION")
{
Expand All @@ -566,18 +540,24 @@ impl Item {
}

Some(MagicAttrName::Author) if attr.value.is_none() => {
assert_attr_kind(attr, &[AttrKind::Command]);

if let Some(method) = Method::from_env(attr.name.clone(), "CARGO_PKG_AUTHORS") {
self.methods.push(method);
}
}

Some(MagicAttrName::Version) if attr.value.is_none() => {
assert_attr_kind(attr, &[AttrKind::Command]);

if let Some(method) = Method::from_env(attr.name.clone(), "CARGO_PKG_VERSION") {
self.methods.push(method);
}
}

Some(MagicAttrName::DefaultValueT) => {
assert_attr_kind(attr, &[AttrKind::Arg]);

let ty = if let Some(ty) = self.ty.as_ref() {
ty
} else {
Expand Down Expand Up @@ -620,6 +600,8 @@ impl Item {
}

Some(MagicAttrName::DefaultValuesT) => {
assert_attr_kind(attr, &[AttrKind::Arg]);

let ty = if let Some(ty) = self.ty.as_ref() {
ty
} else {
Expand Down Expand Up @@ -689,6 +671,8 @@ impl Item {
}

Some(MagicAttrName::DefaultValueOsT) => {
assert_attr_kind(attr, &[AttrKind::Arg]);

let ty = if let Some(ty) = self.ty.as_ref() {
ty
} else {
Expand Down Expand Up @@ -731,6 +715,8 @@ impl Item {
}

Some(MagicAttrName::DefaultValuesOsT) => {
assert_attr_kind(attr, &[AttrKind::Arg]);

let ty = if let Some(ty) = self.ty.as_ref() {
ty
} else {
Expand Down Expand Up @@ -800,11 +786,15 @@ impl Item {
}

Some(MagicAttrName::NextDisplayOrder) => {
assert_attr_kind(attr, &[AttrKind::Command]);

let expr = attr.value_or_abort();
self.next_display_order = Some(Method::new(attr.name.clone(), quote!(#expr)));
}

Some(MagicAttrName::NextHelpHeading) => {
assert_attr_kind(attr, &[AttrKind::Command]);

let expr = attr.value_or_abort();
self.next_help_heading = Some(Method::new(attr.name.clone(), quote!(#expr)));
}
Expand All @@ -815,6 +805,8 @@ impl Item {
}

Some(MagicAttrName::RenameAllEnv) => {
assert_attr_kind(attr, &[AttrKind::Command, AttrKind::Arg]);

let lit = attr.lit_str_or_abort();
self.env_casing = CasingStyle::from_lit(lit);
}
Expand All @@ -829,14 +821,14 @@ impl Item {
| Some(MagicAttrName::Version)
=> {
let expr = attr.value_or_abort();
self.push_method(attr.name.clone(), expr);
self.push_method(*attr.kind.get(), attr.name.clone(), expr);
}

// Magic only for the default, otherwise just forward to the builder
#[cfg(not(feature = "unstable-v5"))]
Some(MagicAttrName::ValueParser) | Some(MagicAttrName::Action) => {
let expr = attr.value_or_abort();
self.push_method(attr.name.clone(), expr);
self.push_method(*attr.kind.get(), attr.name.clone(), expr);
}

// Directives that never receive a value
Expand Down Expand Up @@ -1263,6 +1255,22 @@ impl ToTokens for Deprecation {
}
}

fn assert_attr_kind(attr: &ClapAttr, possible_kind: &[AttrKind]) {
if !possible_kind.contains(attr.kind.get()) {
let options = possible_kind
.iter()
.map(|k| format!("`#[{}({})]", k.as_str(), attr.name))
.collect::<Vec<_>>();
abort!(
attr.name,
"Unknown `#[{}({})]` attribute ({} exists)",
attr.kind.as_str(),
attr.name,
options.join(", ")
);
}
}

/// replace all `:` with `, ` when not inside the `<>`
///
/// `"author1:author2:author3" => "author1, author2, author3"`
Expand Down
2 changes: 1 addition & 1 deletion tests/derive/explicit_name_no_renaming.rs
View file
Open in desktop
Expand Up @@ -27,7 +27,7 @@ fn explicit_short_long_no_rename() {
fn explicit_name_no_rename() {
#[derive(Parser, PartialEq, Debug)]
struct Opt {
#[arg(name = ".options")]
#[arg(id = ".options")]
foo: String,
}

Expand Down
8 changes: 4 additions & 4 deletions tests/derive/naming.rs
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@ fn test_custom_long_overwrites_default_name() {
fn test_standalone_long_uses_previous_defined_custom_name() {
#[derive(Parser, Debug, PartialEq)]
struct Opt {
#[arg(name = "foo", long)]
#[arg(id = "foo", long)]
foo_option: bool,
}

Expand All @@ -47,7 +47,7 @@ fn test_standalone_long_uses_previous_defined_custom_name() {
fn test_standalone_long_ignores_afterwards_defined_custom_name() {
#[derive(Parser, Debug, PartialEq)]
struct Opt {
#[arg(long, name = "foo")]
#[arg(long, id = "foo")]
foo_option: bool,
}

Expand Down Expand Up @@ -118,7 +118,7 @@ fn test_custom_short_overwrites_default_name() {
fn test_standalone_short_uses_previous_defined_custom_name() {
#[derive(Parser, Debug, PartialEq)]
struct Opt {
#[arg(name = "option", short)]
#[arg(id = "option", short)]
foo_option: bool,
}

Expand All @@ -132,7 +132,7 @@ fn test_standalone_short_uses_previous_defined_custom_name() {
fn test_standalone_short_ignores_afterwards_defined_custom_name() {
#[derive(Parser, Debug, PartialEq)]
struct Opt {
#[arg(short, name = "option")]
#[arg(short, id = "option")]
foo_option: bool,
}

Expand Down