Skip to content

Commit

Permalink
Merge pull request #4181 from epage/error
Browse files Browse the repository at this point in the history 
fix(derive): Report deprecations/errors in more cases
  • Loading branch information
@epage
epage committed Sep 3, 2022
2 parents f513862 + a7ed5d0 commit 7ed7f71
Show file tree
Hide file tree
Showing 8 changed files with 101 additions and 93 deletions.
170 changes: 89 additions & 81 deletions clap_derive/src/item.rs
View file
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
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
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

0 comments on commit 7ed7f71

Please sign in to comment.