From 4a79e0121e66bd255ee4a485d474724d54d6ce37 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Mon, 4 Nov 2024 20:43:19 -0500 Subject: [PATCH 1/3] Rewrite fmt expansion to exactly preserve user-provided args --- impl/src/attr.rs | 16 +++++++- impl/src/fmt.rs | 60 +++++++++++++++--------------- tests/ui/display-underscore.stderr | 8 ++-- tests/ui/raw-identifier.stderr | 17 +++++---- 4 files changed, 59 insertions(+), 42 deletions(-) diff --git a/impl/src/attr.rs b/impl/src/attr.rs index c28761c..85af18b 100644 --- a/impl/src/attr.rs +++ b/impl/src/attr.rs @@ -24,6 +24,7 @@ pub struct Display<'a> { pub requires_fmt_machinery: bool, pub has_bonus_display: bool, pub implied_bounds: Set<(usize, Trait)>, + pub bindings: Vec<(Ident, TokenStream)>, } #[derive(Copy, Clone)] @@ -127,6 +128,7 @@ fn parse_error_attribute<'a>(attrs: &mut Attrs<'a>, attr: &'a Attribute) -> Resu requires_fmt_machinery, has_bonus_display: false, implied_bounds: Set::new(), + bindings: Vec::new(), }; if attrs.display.is_some() { return Err(Error::new_spanned( @@ -253,7 +255,7 @@ impl ToTokens for Display<'_> { // Currently `write!(f, "text")` produces less efficient code than // `f.write_str("text")`. We recognize the case when the format string // has no braces and no interpolated values, and generate simpler code. - tokens.extend(if self.requires_fmt_machinery { + let write = if self.requires_fmt_machinery { quote! { ::core::write!(__formatter, #fmt #args) } @@ -261,6 +263,18 @@ impl ToTokens for Display<'_> { quote! { __formatter.write_str(#fmt) } + }; + + tokens.extend(if self.bindings.is_empty() { + write + } else { + let locals = self.bindings.iter().map(|(local, _value)| local); + let values = self.bindings.iter().map(|(_local, value)| value); + quote! { + match (#(#values,)*) { + (#(#locals,)*) => #write + } + } }); } } diff --git a/impl/src/fmt.rs b/impl/src/fmt.rs index d049fd8..d984bfd 100644 --- a/impl/src/fmt.rs +++ b/impl/src/fmt.rs @@ -2,9 +2,9 @@ use crate::ast::{ContainerKind, Field}; use crate::attr::{Display, Trait}; use crate::scan_expr::scan_expr; use crate::unraw::{IdentUnraw, MemberUnraw}; -use proc_macro2::{Delimiter, TokenStream, TokenTree}; -use quote::{format_ident, quote, quote_spanned}; -use std::collections::{BTreeSet, HashMap as Map, HashSet}; +use proc_macro2::{Delimiter, TokenStream}; +use quote::{format_ident, quote, quote_spanned, ToTokens}; +use std::collections::{BTreeSet, HashMap, HashSet}; use std::iter; use syn::ext::IdentExt; use syn::parse::discouraged::Speculative; @@ -12,15 +12,14 @@ use syn::parse::{Error, ParseStream, Parser, Result}; use syn::{Expr, Ident, Index, LitStr, Token}; impl Display<'_> { - // Transform `"error {var}"` to `"error {}", var`. pub fn expand_shorthand(&mut self, fields: &[Field], container: ContainerKind) -> Result<()> { let raw_args = self.args.clone(); let FmtArguments { - named: mut named_args, + named: user_named_args, first_unnamed, } = explicit_named_args.parse2(raw_args).unwrap(); - let mut member_index = Map::new(); + let mut member_index = HashMap::new(); let mut extra_positional_arguments_allowed = true; for (i, field) in fields.iter().enumerate() { member_index.insert(&field.member, i); @@ -31,14 +30,10 @@ impl Display<'_> { let fmt = self.fmt.value(); let mut read = fmt.as_str(); let mut out = String::new(); - let mut args = self.args.clone(); let mut has_bonus_display = false; let mut implied_bounds = BTreeSet::new(); - - let mut has_trailing_comma = false; - if let Some(TokenTree::Punct(punct)) = args.clone().into_iter().last() { - has_trailing_comma = punct.as_char() == ','; - } + let mut bindings = Vec::new(); + let mut macro_named_args = HashSet::new(); self.requires_fmt_machinery = self.requires_fmt_machinery || fmt.contains('}'); @@ -81,19 +76,26 @@ impl Display<'_> { _ => continue, }; let formatvar = match &member { - MemberUnraw::Unnamed(index) => IdentUnraw::new(format_ident!("_{}", index)), - MemberUnraw::Named(ident) => ident.clone(), + MemberUnraw::Unnamed(index) => IdentUnraw::new(format_ident!("__field{}", index)), + MemberUnraw::Named(ident) => { + if user_named_args.contains(ident) { + // Refers to a named argument written by the user, not to field. + out += &ident.to_string(); + continue; + } + IdentUnraw::new(format_ident!("__field_{}", ident.to_string())) + } }; out += &formatvar.to_string(); - if !named_args.insert(member.clone()) { - // Already specified in the format argument list. + if !macro_named_args.insert(member.clone()) { + // Already added to scope by a previous use. continue; } - if !has_trailing_comma { - args.extend(quote_spanned!(span=> ,)); - } let local = formatvar.to_local(); - args.extend(quote_spanned!(span=> #formatvar = #local)); + let mut binding_value = ToTokens::into_token_stream(match &member { + MemberUnraw::Unnamed(index) => format_ident!("_{}", index), + MemberUnraw::Named(ident) => ident.to_local(), + }); if let Some(&field) = member_index.get(&member) { let end_spec = match read.find('}') { Some(end_spec) => end_spec, @@ -111,26 +113,26 @@ impl Display<'_> { Some(_) => Trait::Display, None => { has_bonus_display = true; - args.extend(quote_spanned!(span=> .as_display())); + binding_value.extend(quote_spanned!(span=> .as_display())); Trait::Display } }; implied_bounds.insert((field, bound)); } - has_trailing_comma = false; + bindings.push((local, binding_value)); } out += read; self.fmt = LitStr::new(&out, self.fmt.span()); - self.args = args; self.has_bonus_display = has_bonus_display; self.implied_bounds = implied_bounds; + self.bindings = bindings; Ok(()) } } struct FmtArguments { - named: HashSet, + named: BTreeSet, first_unnamed: Option, } @@ -150,7 +152,7 @@ fn explicit_named_args(input: ParseStream) -> Result { input.parse::().unwrap(); Ok(FmtArguments { - named: HashSet::new(), + named: BTreeSet::new(), first_unnamed: None, }) } @@ -158,7 +160,7 @@ fn explicit_named_args(input: ParseStream) -> Result { fn try_explicit_named_args(input: ParseStream) -> Result { let mut syn_full = None; let mut args = FmtArguments { - named: HashSet::new(), + named: BTreeSet::new(), first_unnamed: None, }; @@ -172,7 +174,7 @@ fn try_explicit_named_args(input: ParseStream) -> Result { if input.peek(Ident::peek_any) && input.peek2(Token![=]) && !input.peek2(Token![==]) { let ident: IdentUnraw = input.parse()?; input.parse::()?; - args.named.insert(MemberUnraw::Named(ident)); + args.named.insert(ident); } else { begin_unnamed = Some(input.fork()); } @@ -196,7 +198,7 @@ fn try_explicit_named_args(input: ParseStream) -> Result { fn fallback_explicit_named_args(input: ParseStream) -> Result { let mut args = FmtArguments { - named: HashSet::new(), + named: BTreeSet::new(), first_unnamed: None, }; @@ -209,7 +211,7 @@ fn fallback_explicit_named_args(input: ParseStream) -> Result { input.parse::()?; let ident: IdentUnraw = input.parse()?; input.parse::()?; - args.named.insert(MemberUnraw::Named(ident)); + args.named.insert(ident); } } diff --git a/tests/ui/display-underscore.stderr b/tests/ui/display-underscore.stderr index 36882b9..3935970 100644 --- a/tests/ui/display-underscore.stderr +++ b/tests/ui/display-underscore.stderr @@ -1,7 +1,5 @@ -error: invalid format string: invalid argument name `_` - --> tests/ui/display-underscore.rs:4:11 +error: in expressions, `_` can only be used on the left-hand side of an assignment + --> tests/ui/display-underscore.rs:4:9 | 4 | #[error("{_}")] - | ^ invalid argument name in format string - | - = note: argument name cannot be a single underscore + | ^^^^^ `_` not allowed here diff --git a/tests/ui/raw-identifier.stderr b/tests/ui/raw-identifier.stderr index a3ce94d..ffce3fe 100644 --- a/tests/ui/raw-identifier.stderr +++ b/tests/ui/raw-identifier.stderr @@ -1,13 +1,10 @@ -error: invalid format string: raw identifiers are not supported - --> tests/ui/raw-identifier.rs:4:18 +error: invalid format string: expected `}`, found `#` + --> tests/ui/raw-identifier.rs:4:9 | 4 | #[error("error: {r#fn}")] - | --^^ - | | - | raw identifier used here in format string - | help: remove the `r#` + | ^^^^^^^^^^^^^^^ expected `}` in format string | - = note: identifiers in format strings can be keywords and don't need to be prefixed with `r#` + = note: if you intended to print `{`, you can escape it using `{{` error: invalid format string: raw identifiers are not supported --> tests/ui/raw-identifier.rs:11:30 @@ -19,3 +16,9 @@ error: invalid format string: raw identifiers are not supported | help: remove the `r#` | = note: identifiers in format strings can be keywords and don't need to be prefixed with `r#` + +error[E0425]: cannot find value `r` in this scope + --> tests/ui/raw-identifier.rs:4:9 + | +4 | #[error("error: {r#fn}")] + | ^^^^^^^^^^^^^^^ not found in this scope From 58cc36e69f0a7b67b85692e8dc38a81273b839b9 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Mon, 4 Nov 2024 22:00:42 -0500 Subject: [PATCH 2/3] Improve diagnostic on {_} in format string --- impl/src/fmt.rs | 2 +- tests/ui/display-underscore.stderr | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/impl/src/fmt.rs b/impl/src/fmt.rs index d984bfd..6b1e7fa 100644 --- a/impl/src/fmt.rs +++ b/impl/src/fmt.rs @@ -78,7 +78,7 @@ impl Display<'_> { let formatvar = match &member { MemberUnraw::Unnamed(index) => IdentUnraw::new(format_ident!("__field{}", index)), MemberUnraw::Named(ident) => { - if user_named_args.contains(ident) { + if user_named_args.contains(ident) || ident == "_" { // Refers to a named argument written by the user, not to field. out += &ident.to_string(); continue; diff --git a/tests/ui/display-underscore.stderr b/tests/ui/display-underscore.stderr index 3935970..36882b9 100644 --- a/tests/ui/display-underscore.stderr +++ b/tests/ui/display-underscore.stderr @@ -1,5 +1,7 @@ -error: in expressions, `_` can only be used on the left-hand side of an assignment - --> tests/ui/display-underscore.rs:4:9 +error: invalid format string: invalid argument name `_` + --> tests/ui/display-underscore.rs:4:11 | 4 | #[error("{_}")] - | ^^^^^ `_` not allowed here + | ^ invalid argument name in format string + | + = note: argument name cannot be a single underscore From bf6efce84df7d0683bb39eea4eeb623146df469a Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Mon, 4 Nov 2024 22:03:43 -0500 Subject: [PATCH 3/3] Improve diagnostic on raw identifier in format string --- impl/src/fmt.rs | 3 +++ tests/ui/raw-identifier.stderr | 17 +++++++---------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/impl/src/fmt.rs b/impl/src/fmt.rs index 6b1e7fa..49e77c5 100644 --- a/impl/src/fmt.rs +++ b/impl/src/fmt.rs @@ -70,6 +70,9 @@ impl Display<'_> { member } 'a'..='z' | 'A'..='Z' | '_' => { + if read.starts_with("r#") { + continue; + } let ident = Ident::new(take_ident(&mut read), span); MemberUnraw::Named(IdentUnraw::new(ident)) } diff --git a/tests/ui/raw-identifier.stderr b/tests/ui/raw-identifier.stderr index ffce3fe..a3ce94d 100644 --- a/tests/ui/raw-identifier.stderr +++ b/tests/ui/raw-identifier.stderr @@ -1,10 +1,13 @@ -error: invalid format string: expected `}`, found `#` - --> tests/ui/raw-identifier.rs:4:9 +error: invalid format string: raw identifiers are not supported + --> tests/ui/raw-identifier.rs:4:18 | 4 | #[error("error: {r#fn}")] - | ^^^^^^^^^^^^^^^ expected `}` in format string + | --^^ + | | + | raw identifier used here in format string + | help: remove the `r#` | - = note: if you intended to print `{`, you can escape it using `{{` + = note: identifiers in format strings can be keywords and don't need to be prefixed with `r#` error: invalid format string: raw identifiers are not supported --> tests/ui/raw-identifier.rs:11:30 @@ -16,9 +19,3 @@ error: invalid format string: raw identifiers are not supported | help: remove the `r#` | = note: identifiers in format strings can be keywords and don't need to be prefixed with `r#` - -error[E0425]: cannot find value `r` in this scope - --> tests/ui/raw-identifier.rs:4:9 - | -4 | #[error("error: {r#fn}")] - | ^^^^^^^^^^^^^^^ not found in this scope