From 0a6bc511dbd0fb994bb0e39539acf9b33d3a4419 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 11 Jun 2020 20:02:44 -0400 Subject: [PATCH 1/2] Use proper span when generating `matches` token In `struct-opt-derive`, a function is generated with a parameter named `matches`. Since `quote!` is used to generate the function, the `matches` token will be resolved using `Span::call_site`. However, the literal identifier `matches` is also used inside several `quote_spanned!` expressions. Such a `matches` identifier will be resolved using the `Span` passed to `quote_spanned!`, which may not be the same as `Span::call_site`. Currently, this is difficult to observe in practice, due to rust-lang/rust#43081 . However, once PR rust-lang/rust#73084 is merged, proc macros will see properly spanned tokens in more cases, which will cause these incorrect uses of `quote_spanned!` to break. This PR uses `quote! { matches }` to generate a correctly spanned `matches` token, which is then include in the `quote_spanned!` expressions using `#matches`. --- structopt-derive/src/lib.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/structopt-derive/src/lib.rs b/structopt-derive/src/lib.rs index 820a4c8e..1630864c 100644 --- a/structopt-derive/src/lib.rs +++ b/structopt-derive/src/lib.rs @@ -248,6 +248,7 @@ fn gen_constructor(fields: &Punctuated, parent_attribute: &Attrs) ); let field_name = field.ident.as_ref().unwrap(); let kind = attrs.kind(); + let matches = quote! { matches }; match &*kind { Kind::ExternalSubcommand => abort!( kind.span(), @@ -265,13 +266,13 @@ fn gen_constructor(fields: &Punctuated, parent_attribute: &Attrs) }; quote_spanned! { kind.span()=> #field_name: <#subcmd_type as ::structopt::StructOptInternal>::from_subcommand( - matches.subcommand()) + #matches.subcommand()) #unwrapper } } Kind::Flatten => quote_spanned! { kind.span()=> - #field_name: ::structopt::StructOpt::from_clap(matches) + #field_name: ::structopt::StructOpt::from_clap(#matches) }, Kind::Skip(val) => match val { @@ -318,24 +319,24 @@ fn gen_constructor(fields: &Punctuated, parent_attribute: &Attrs) let occurrences = *attrs.parser().kind == ParserKind::FromOccurrences; let name = attrs.cased_name(); let field_value = match **ty { - Ty::Bool => quote_spanned!(ty.span()=> matches.is_present(#name)), + Ty::Bool => quote_spanned!(ty.span()=> #matches.is_present(#name)), Ty::Option => quote_spanned! { ty.span()=> - matches.#value_of(#name) + #matches.#value_of(#name) .map(#parse) }, Ty::OptionOption => quote_spanned! { ty.span()=> - if matches.is_present(#name) { - Some(matches.#value_of(#name).map(#parse)) + if #matches.is_present(#name) { + Some(#matches.#value_of(#name).map(#parse)) } else { None } }, Ty::OptionVec => quote_spanned! { ty.span()=> - if matches.is_present(#name) { - Some(matches.#values_of(#name) + if #matches.is_present(#name) { + Some(#matches.#values_of(#name) .map_or_else(Vec::new, |v| v.map(#parse).collect())) } else { None @@ -343,20 +344,20 @@ fn gen_constructor(fields: &Punctuated, parent_attribute: &Attrs) }, Ty::Vec => quote_spanned! { ty.span()=> - matches.#values_of(#name) + #matches.#values_of(#name) .map_or_else(Vec::new, |v| v.map(#parse).collect()) }, Ty::Other if occurrences => quote_spanned! { ty.span()=> - #parse(matches.#value_of(#name)) + #parse(#matches.#value_of(#name)) }, Ty::Other if flag => quote_spanned! { ty.span()=> - #parse(matches.is_present(#name)) + #parse(#matches.is_present(#name)) }, Ty::Other => quote_spanned! { ty.span()=> - matches.#value_of(#name) + #matches.#value_of(#name) .map(#parse) .unwrap() }, From 83894e2a6b6b4d943eee6560084375ad7e55db65 Mon Sep 17 00:00:00 2001 From: CreepySkeleton Date: Fri, 12 Jun 2020 13:08:07 +0300 Subject: [PATCH 2/2] Some tweaks --- structopt-derive/src/lib.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/structopt-derive/src/lib.rs b/structopt-derive/src/lib.rs index 1630864c..a5cd7c26 100644 --- a/structopt-derive/src/lib.rs +++ b/structopt-derive/src/lib.rs @@ -28,7 +28,7 @@ use crate::{ use proc_macro2::{Span, TokenStream}; use proc_macro_error::{abort, abort_call_site, proc_macro_error, set_dummy}; -use quote::{quote, quote_spanned}; +use quote::{format_ident, quote, quote_spanned}; use syn::{punctuated::Punctuated, spanned::Spanned, token::Comma, *}; /// Default casing style for generated arguments. @@ -239,6 +239,16 @@ fn gen_augmentation( } fn gen_constructor(fields: &Punctuated, parent_attribute: &Attrs) -> TokenStream { + // This ident is used in several match branches below, + // and the `quote[_spanned]` invocations have different spans. + // + // Given that this ident is used in several places and + // that the branches are located inside of a loop, it is possible that + // this ident will be given _different_ spans in different places, and + // thus will not be the _same_ ident anymore. To make sure the `matches` + // is always the same, we factor it out. + let matches = format_ident!("matches"); + let fields = fields.iter().map(|field| { let attrs = Attrs::from_field( field, @@ -248,7 +258,6 @@ fn gen_constructor(fields: &Punctuated, parent_attribute: &Attrs) ); let field_name = field.ident.as_ref().unwrap(); let kind = attrs.kind(); - let matches = quote! { matches }; match &*kind { Kind::ExternalSubcommand => abort!( kind.span(),