From d73860e05b1990b3e57e514a4a53c8ebf64320d0 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Fri, 23 Aug 2019 15:39:08 +0300 Subject: [PATCH 1/3] Change behavior of `about/author/version` and ad `no_version` --- examples/no_version.rs | 4 +- structopt-derive/src/attrs.rs | 142 +++++++++++++++++++++++--------- structopt-derive/src/lib.rs | 20 +++-- structopt-derive/src/parse.rs | 92 ++++++++++++++++----- structopt-derive/src/spanned.rs | 6 ++ tests/author_version_about.rs | 4 +- tests/doc-comments-help.rs | 2 +- 7 files changed, 199 insertions(+), 71 deletions(-) diff --git a/examples/no_version.rs b/examples/no_version.rs index 8d388cb1..8cf2550b 100644 --- a/examples/no_version.rs +++ b/examples/no_version.rs @@ -4,9 +4,7 @@ use structopt::StructOpt; #[derive(StructOpt, Debug)] #[structopt( name = "no_version", - about = "", - version = "", - author = "", + no_version, global_settings = &[AppSettings::DisableVersion] )] struct Opt {} diff --git a/structopt-derive/src/attrs.rs b/structopt-derive/src/attrs.rs index 4f82eaf3..8881b63f 100755 --- a/structopt-derive/src/attrs.rs +++ b/structopt-derive/src/attrs.rs @@ -5,18 +5,18 @@ // , at your // option. This file may not be copied, modified, or distributed // except according to those terms. + use crate::spanned::Sp; + +use std::env; + use heck::{CamelCase, KebabCase, MixedCase, ShoutySnakeCase, SnakeCase}; use proc_macro2::{Span, TokenStream}; -use proc_macro_error::span_error; +use proc_macro_error::{call_site_error, span_error}; use quote::quote; -use std::{env, mem}; -use syn::spanned::Spanned as _; - -use syn::Type::Path; use syn::{ - self, AngleBracketedGenericArguments, Attribute, GenericArgument, Ident, LitStr, MetaNameValue, - PathArguments, PathSegment, TypePath, + self, spanned::Spanned, AngleBracketedGenericArguments, Attribute, GenericArgument, Ident, + LitStr, MetaNameValue, PathArguments, PathSegment, Type::Path, TypePath, }; use crate::parse::*; @@ -39,12 +39,13 @@ pub enum Ty { Other, } +#[derive(Clone)] pub struct Method { name: Ident, args: TokenStream, } -#[derive(Debug, PartialEq)] +#[derive(Debug, PartialEq, Clone)] pub enum Parser { FromStr, TryFromStr, @@ -70,12 +71,17 @@ pub enum CasingStyle { Verbatim, } +#[derive(Clone)] pub struct Attrs { name: Sp, cased_name: String, casing: Sp, methods: Vec, parser: Sp<(Sp, TokenStream)>, + author: Option<(Ident, LitStr)>, + about: Option<(Ident, LitStr)>, + version: Option<(Ident, LitStr)>, + no_version: Option, has_custom_parser: bool, kind: Sp, } @@ -141,17 +147,19 @@ impl Attrs { Sp::call_site(Parser::TryFromStr), quote!(::std::str::FromStr::from_str), )), + about: None, + author: None, + version: None, + no_version: None, + has_custom_parser: false, kind: Sp::call_site(Kind::Arg(Sp::call_site(Ty::Other))), } } + /// push `.method("str literal")` fn push_str_method(&mut self, name: Sp, arg: Sp) { match (&**name, &**arg) { - ("about", "") | ("version", "") | ("author", "") => { - let methods = mem::replace(&mut self.methods, vec![]); - self.methods = methods.into_iter().filter(|m| m.name != name).collect(); - } ("name", _) => { self.cased_name = self.casing.translate(&arg); self.name = arg; @@ -166,6 +174,21 @@ impl Attrs { fn push_attrs(&mut self, attrs: &[Attribute]) { use crate::parse::StructOptAttr::*; + fn from_lit_or_env( + ident: Ident, + lit: Option, + env_var: &str, + ) -> Option<(Ident, LitStr)> { + let lit = lit.unwrap_or_else(|| { + let gen = env::var(env_var) + .unwrap_or_else(|_| + span_error!(ident.span(), "`{}` environment variable is not defined, use `{} = \"{}\"` to set it manually", env_var, env_var, env_var)); + LitStr::new(&gen, Span::call_site()) + }); + + Some((ident, lit)) + } + for attr in parse_structopt_attributes(attrs) { match attr { Short(ident) => { @@ -194,6 +217,22 @@ impl Attrs { self.set_kind(kind); } + NoVersion(ident) => self.no_version = Some(ident), + + About(ident, about) => { + self.about = from_lit_or_env(ident, about, "CARGO_PKG_DESCRIPTION") + } + + Author(ident, author) => { + self.author = + from_lit_or_env(ident, author, "CARGO_PKG_AUTHORS").map(|(ident, lit)| { + let value = lit.value().replace(":", ", "); + (ident.clone(), LitStr::new(&value, ident.span())) + }) + } + + Version(ident, version) => self.version = Some((ident, version)), + NameLitStr(name, lit) => { self.push_str_method(name.into(), lit.into()); } @@ -272,6 +311,7 @@ impl Attrs { return None; } let value = s.value(); + let text = value .trim_start_matches("//!") .trim_start_matches("///") @@ -335,33 +375,16 @@ impl Attrs { }); } } + pub fn from_struct( attrs: &[Attribute], name: Sp, argument_casing: Sp, ) -> Self { let mut res = Self::new(name, argument_casing); - let attrs_with_env = [ - ("version", "CARGO_PKG_VERSION"), - ("about", "CARGO_PKG_DESCRIPTION"), - ("author", "CARGO_PKG_AUTHORS"), - ]; - attrs_with_env - .iter() - .filter_map(|&(m, v)| env::var(v).ok().and_then(|arg| Some((m, arg)))) - .filter(|&(_, ref arg)| !arg.is_empty()) - .for_each(|(name, arg)| { - let new_arg = if name == "author" { - arg.replace(":", ", ") - } else { - arg - }; - let name = Sp::call_site(name.to_string()); - let new_arg = Sp::call_site(new_arg.to_string()); - res.push_str_method(name, new_arg); - }); - res.push_doc_comment(attrs, "about"); res.push_attrs(attrs); + res.push_doc_comment(attrs, "about"); + if res.has_custom_parser { span_error!( res.parser.span(), @@ -379,6 +402,7 @@ impl Attrs { Kind::Arg(_) => res, } } + fn ty_from_field(ty: &syn::Type) -> Sp { let t = |kind| Sp::new(kind, ty.span()); if let Path(TypePath { @@ -404,6 +428,7 @@ impl Attrs { t(Ty::Other) } } + pub fn from_field(field: &syn::Field, struct_casing: Sp) -> Self { let name = field.ident.clone().unwrap(); let mut res = Self::new(name.into(), struct_casing); @@ -463,7 +488,7 @@ impl Attrs { span_error!(m.name.span(), "methods are not allowed for skipped fields"); } } - Kind::Arg(_) => { + Kind::Arg(orig_ty) => { let mut ty = Self::ty_from_field(&field.ty); if res.has_custom_parser { match *ty { @@ -510,7 +535,7 @@ impl Attrs { _ => (), } - res.kind = Sp::call_site(Kind::Arg(ty)); + res.kind = Sp::new(Kind::Arg(ty), orig_ty.span()); } } @@ -536,16 +561,59 @@ impl Attrs { self.methods.iter().find(|m| m.name == name) } - pub fn methods(&self) -> TokenStream { + /// generate methods from attributes on top of struct or enum + pub fn top_level_methods(&self) -> TokenStream { + let version = match (&self.no_version, &self.version) { + (Some(no_version), Some(_)) => span_error!( + no_version.span(), + "`no_version` and `version = \"version\"` can't be used together" + ), + + (None, Some((_, version))) => quote!(.version(#version)), + + (None, None) => { + let version = env::var("CARGO_PKG_VERSION").unwrap_or_else(|_|{ + call_site_error!("`CARGO_PKG_VERSION` environment variable is not defined, use `version = \"version\" to set it manually or `no_version` to not set it at all") + }); + quote!(.version(#version)) + } + + (Some(_), None) => TokenStream::new(), + }; + + let version = Some(version); + let author = self + .author + .as_ref() + .map(|(_, version)| quote!(.author(#version))); + let about = self + .about + .as_ref() + .map(|(_, version)| quote!(.about(#version))); + + let methods = self + .methods + .iter() + .map(|&Method { ref name, ref args }| quote!( .#name(#args) )) + .chain(version) + .chain(author) + .chain(about); + + quote!( #(#methods)* ) + } + + /// generate methods on top of a field + pub fn field_methods(&self) -> TokenStream { let methods = self .methods .iter() .map(|&Method { ref name, ref args }| quote!( .#name(#args) )); + quote!( #(#methods)* ) } - pub fn cased_name(&self) -> &str { - &self.cased_name + pub fn cased_name(&self) -> String { + self.cased_name.to_string() } pub fn parser(&self) -> &(Sp, TokenStream) { diff --git a/structopt-derive/src/lib.rs b/structopt-derive/src/lib.rs index 21716c83..2035a4cb 100644 --- a/structopt-derive/src/lib.rs +++ b/structopt-derive/src/lib.rs @@ -16,15 +16,15 @@ mod attrs; mod parse; mod spanned; -use crate::attrs::{sub_type, Attrs, CasingStyle, Kind, Parser, Ty}; -use crate::spanned::Sp; +use crate::{ + attrs::{sub_type, Attrs, CasingStyle, Kind, Parser, Ty}, + spanned::Sp, +}; + use proc_macro2::{Span, TokenStream}; use proc_macro_error::{call_site_error, filter_macro_errors, span_error}; use quote::{quote, quote_spanned}; -use syn::punctuated::Punctuated; -use syn::spanned::Spanned; -use syn::token::Comma; -use syn::*; +use syn::{punctuated::Punctuated, spanned::Spanned, token::Comma, *}; /// Default casing style for generated arguments. const DEFAULT_CASING: CasingStyle = CasingStyle::Kebab; @@ -154,8 +154,9 @@ fn gen_augmentation( quote!( .takes_value(true).multiple(false).required(#required) #validator ) } }; - let methods = attrs.methods(); + let name = attrs.cased_name(); + let methods = attrs.field_methods(); Some(quote! { let #app_var = #app_var.arg( @@ -282,7 +283,7 @@ fn gen_clap(attrs: &[Attribute]) -> GenOutput { let attrs = Attrs::from_struct(attrs, Sp::call_site(name), Sp::call_site(DEFAULT_CASING)); let tokens = { let name = attrs.cased_name(); - let methods = attrs.methods(); + let methods = attrs.top_level_methods(); quote!(::structopt::clap::App::new(#name)#methods) }; @@ -372,7 +373,8 @@ fn gen_augment_clap_enum( }; let name = attrs.cased_name(); - let from_attrs = attrs.methods(); + let from_attrs = attrs.top_level_methods(); + quote! { .subcommand({ let #app_var = ::structopt::clap::SubCommand::with_name(#name); diff --git a/structopt-derive/src/parse.rs b/structopt-derive/src/parse.rs index df7c7eaf..74185883 100755 --- a/structopt-derive/src/parse.rs +++ b/structopt-derive/src/parse.rs @@ -1,8 +1,14 @@ +use std::iter::FromIterator; + use proc_macro_error::{span_error, ResultExt}; -use syn::parse::{Parse, ParseStream}; -use syn::punctuated::Punctuated; -use syn::spanned::Spanned; -use syn::{self, parenthesized, parse2, Attribute, Expr, Ident, LitStr, Token}; +use syn::{ + self, parenthesized, + parse::{Parse, ParseStream}, + parse2, + punctuated::Punctuated, + spanned::Spanned, + Attribute, Expr, ExprLit, Ident, Lit, LitBool, LitStr, Token, +}; pub struct StructOptAttributes { pub paren_token: syn::token::Paren, @@ -21,16 +27,31 @@ impl Parse for StructOptAttributes { } pub enum StructOptAttr { + // single-identifier attributes Short(Ident), Long(Ident), Flatten(Ident), Subcommand(Ident), Skip(Ident), - Parse(Ident, ParserSpec), + NoVersion(Ident), + + // ident [= "string literal"] + About(Ident, Option), + Author(Ident, Option), + + // ident = "string literal" + Version(Ident, LitStr), RenameAll(Ident, LitStr), NameLitStr(Ident, LitStr), + + // parse(parser_kind [= parser_func]) + Parse(Ident, ParserSpec), + + // ident = arbitrary_expr NameExpr(Ident, Expr), - MethodCall(Ident, Punctuated), + + // ident(arbitrary_expr,*) + MethodCall(Ident, Vec), } impl Parse for StructOptAttr { @@ -42,22 +63,45 @@ impl Parse for StructOptAttr { if input.peek(Token![=]) { // `name = value` attributes. - input.parse::()?; // skip '=' + let assign_token = input.parse::()?; // skip '=' - match name_str.as_ref() { - "rename_all" => { - let casing_lit: LitStr = input.parse()?; - Ok(RenameAll(name, casing_lit)) - } + if input.peek(LitStr) { + let lit: LitStr = input.parse()?; + let lit_str = lit.value(); - _ => { - if input.peek(LitStr) { - let lit: LitStr = input.parse()?; - Ok(NameLitStr(name, lit)) - } else { - let expr: Expr = input.parse()?; - Ok(NameExpr(name, expr)) + let check_empty_lit = |s| { + if lit_str.is_empty() { + span_error!(lit.span(), "`#[structopt({} = \"\") is deprecated in structopt 3.0, now it's default behavior", s); } + }; + + match &*name_str.to_string() { + "rename_all" => Ok(RenameAll(name, lit)), + + "version" => { + check_empty_lit("version"); + Ok(Version(name, lit)) + } + + "author" => { + check_empty_lit("author"); + Ok(Author(name, Some(lit))) + } + + "about" => { + check_empty_lit("about"); + Ok(About(name, Some(lit))) + } + + _ => Ok(NameLitStr(name, lit)), + } + } else { + match input.parse::() { + Ok(expr) => Ok(NameExpr(name, expr)), + Err(_) => span_error! { + assign_token.span(), + "expected `string literal` or `expression` after `=`" + }, } } } else if input.peek(syn::token::Paren) { @@ -90,6 +134,16 @@ impl Parse for StructOptAttr { "flatten" => Ok(Flatten(name)), "subcommand" => Ok(Subcommand(name)), "skip" => Ok(Skip(name)), + "no_version" => Ok(NoVersion(name)), + + "about" => (Ok(About(name, None))), + "author" => (Ok(Author(name, None))), + + "version" => { + span_error!(name.span(), + "#[structopt(version)] is invalid attribute, structopt 3.0 inherits version from Cargo.toml by default, no attribute needed") + }, + _ => span_error!(name.span(), "unexpected attribute: {}", name_str), } } diff --git a/structopt-derive/src/spanned.rs b/structopt-derive/src/spanned.rs index 5b781f18..dce37ded 100644 --- a/structopt-derive/src/spanned.rs +++ b/structopt-derive/src/spanned.rs @@ -65,6 +65,12 @@ impl From for Sp { } } +impl<'a> From> for Sp { + fn from(sp: Sp<&'a str>) -> Self { + Sp::new(sp.val.into(), sp.span) + } +} + impl PartialEq for Sp { fn eq(&self, other: &Sp) -> bool { self.val == other.val diff --git a/tests/author_version_about.rs b/tests/author_version_about.rs index 0872ea39..f28147c2 100644 --- a/tests/author_version_about.rs +++ b/tests/author_version_about.rs @@ -11,7 +11,7 @@ use structopt::StructOpt; #[test] fn no_author_version_about() { #[derive(StructOpt, PartialEq, Debug)] - #[structopt(name = "foo", about = "", author = "", version = "")] + #[structopt(name = "foo", no_version)] struct Opt {} let mut output = Vec::new(); @@ -24,7 +24,7 @@ fn no_author_version_about() { #[test] fn use_env() { #[derive(StructOpt, PartialEq, Debug)] - #[structopt()] + #[structopt(author, about)] struct Opt {} let mut output = Vec::new(); diff --git a/tests/doc-comments-help.rs b/tests/doc-comments-help.rs index 4c7f2d61..1c9e6b62 100644 --- a/tests/doc-comments-help.rs +++ b/tests/doc-comments-help.rs @@ -57,7 +57,7 @@ fn empty_line_in_doc_comment_is_double_linefeed() { /// /// Bar #[derive(StructOpt, PartialEq, Debug)] - #[structopt(name = "lorem-ipsum", author = "", version = "")] + #[structopt(name = "lorem-ipsum", no_version)] struct LoremIpsum {} let mut output = Vec::new(); From aca77d2a6f198b39119abbd5c41ff4d107345f74 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Fri, 23 Aug 2019 15:39:48 +0300 Subject: [PATCH 2/3] Emit error about `raw` removal --- structopt-derive/src/attrs.rs | 2 +- structopt-derive/src/parse.rs | 17 +++++++++++++++-- tests/raw_bool_literal.rs | 30 ++++++++++++++++++++++++++++++ tests/ui/raw.rs | 17 +++++++++++++++++ tests/ui/raw.stderr | 5 +++++ 5 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 tests/raw_bool_literal.rs create mode 100644 tests/ui/raw.rs create mode 100644 tests/ui/raw.stderr diff --git a/structopt-derive/src/attrs.rs b/structopt-derive/src/attrs.rs index 8881b63f..8358b0e0 100755 --- a/structopt-derive/src/attrs.rs +++ b/structopt-derive/src/attrs.rs @@ -244,7 +244,7 @@ impl Attrs { MethodCall(name, args) => self.methods.push(Method { name: name.into(), - args: quote!(#args), + args: quote!(#(#args),*), }), RenameAll(_, casing_lit) => { diff --git a/structopt-derive/src/parse.rs b/structopt-derive/src/parse.rs index 74185883..6714811c 100755 --- a/structopt-derive/src/parse.rs +++ b/structopt-derive/src/parse.rs @@ -121,9 +121,22 @@ impl Parse for StructOptAttr { } } + "raw" => { + match nested.parse::() { + Ok(bool_token) => { + let expr = ExprLit { attrs: vec![], lit: Lit::Bool(bool_token) }; + let expr = Expr::Lit(expr); + Ok(MethodCall(name, vec![expr])) + } + + Err(_) => span_error!(name.span(), + "`#[structopt(raw(...))` attributes are deprecated in structopt 3.0, only `raw(true)` and `raw(false)` are allowed") + } + } + _ => { - let method_args = nested.parse_terminated(Expr::parse)?; - Ok(MethodCall(name, method_args)) + let method_args: Punctuated<_, Token![,]> = nested.parse_terminated(Expr::parse)?; + Ok(MethodCall(name, Vec::from_iter(method_args))) } } } else { diff --git a/tests/raw_bool_literal.rs b/tests/raw_bool_literal.rs new file mode 100644 index 00000000..4084b854 --- /dev/null +++ b/tests/raw_bool_literal.rs @@ -0,0 +1,30 @@ +// Copyright 2018 Guillaume Pinot (@TeXitoi) +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use structopt::clap; +use structopt::StructOpt; + +#[test] +fn raw_bool_literal() { + #[derive(StructOpt, Debug, PartialEq)] + #[structopt(no_version, name = "raw_bool")] + struct Opt { + #[structopt(raw(false))] + a: String, + #[structopt(raw(true))] + b: String, + } + + assert_eq!( + Opt { + a: "one".into(), + b: "--help".into() + }, + Opt::from_iter(&["test", "one", "--", "--help"]) + ); +} diff --git a/tests/ui/raw.rs b/tests/ui/raw.rs new file mode 100644 index 00000000..cf75a444 --- /dev/null +++ b/tests/ui/raw.rs @@ -0,0 +1,17 @@ +// Copyright 2018 Guillaume Pinot (@TeXitoi) +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use structopt::StructOpt; + +#[derive(StructOpt, Debug)] +struct Opt { + #[structopt(raw(case_insensitive = "true"))] + s: String, +} + +fn main() {} diff --git a/tests/ui/raw.stderr b/tests/ui/raw.stderr new file mode 100644 index 00000000..e380dcd0 --- /dev/null +++ b/tests/ui/raw.stderr @@ -0,0 +1,5 @@ +error: `#[structopt(raw(...))` attributes are deprecated in structopt 3.0, only `raw(true)` and `raw(false)` are allowed + --> $DIR/raw.rs:13:17 + | +13 | #[structopt(raw(case_insensitive = "true"))] + | ^^^ From cf05f04d2489136f368cc70c52988f79a65341a8 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Fri, 23 Aug 2019 15:40:07 +0300 Subject: [PATCH 3/3] Update changelog --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 716c2848..446a15af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,17 +3,28 @@ ## Breaking changes * Bump minimum rust version to 1.34 by [@TeXitoi](https://github.com/TeXitoi) + * Support optional vectors of arguments for distinguishing between `-o 1 2`, `-o` and no option provided at all by [@sphynx](https://github.com/sphynx) ([#180](https://github.com/TeXitoi/structopt/issues/188)). This is a breaking change as `Option>` is handled differently. If you need to have a `Option>` handled the old was, you can `type Something = Vec;` and then use `Option` as your structopt field. + * Change default case from 'Verbatim' into 'Kebab' by [@0ndorio](https://github.com/0ndorio) ([#202](https://github.com/TeXitoi/structopt/issues/202)). This is a breaking change. If you rely on the old behavior you need to add `#[structopt(rename_all = "verbatim")]` as an attribute to each data structure deriving the `StructOpt` trait. +* Change `version`, `author` and `about` attributes behavior. `version/author/about = ""` is no longer a + valid syntax. `author` and `about` are no longer derived from `Cargo.toml` by default, i.e when no + attributes mentioned. `version` is still to be derived from `Cargo.toml` by default. + Introduced explicit `author` and `about` attributes (with no arguments, i.e `#[structopt(author)]`) + for explicit requests to deduce author/about fields from `Cargo.toml`. + `version/author/about = "version/author/about"` is still valid syntax. + Introduced `no_version` attribute (no args) which prevents version auto deducing by default. + Proposed by [@TeXitoi](https://github.com/TeXitoi) [(#217)](https://github.com/TeXitoi/structopt/issues/217), implemented by [@CreepySkeleton](https://github.com/CreepySkeleton) [(#229)](https://github.com/TeXitoi/structopt/pull/229). + ## improvements * Support skipping struct fields by [@sphynx](https://github.com/sphynx) @@ -22,6 +33,9 @@ * Add optional feature to support `paw` by [@gameldar](https://github.com/gameldar) ([#187](https://github.com/TeXitoi/structopt/issues/187)) +* Significantly improve error reporting by [@CreepySkeleton](https://github.com/CreepySkeleton) + ([#225](https://github.com/TeXitoi/structopt/pull/225/)) + # v0.2.16 (2019-05-29) * Support optional options with optional argument, allowing `cmd [--opt[=value]]`