diff --git a/sdk/typespec/typespec_client_core/src/fmt.rs b/sdk/typespec/typespec_client_core/src/fmt.rs index 097f5b64b1..86c0447e47 100644 --- a/sdk/typespec/typespec_client_core/src/fmt.rs +++ b/sdk/typespec/typespec_client_core/src/fmt.rs @@ -5,31 +5,6 @@ use std::borrow::Cow; -/// Derive to help prevent leaking personally identifiable information (PII) that deriving [`Debug`](std::fmt::Debug) might otherwise. -/// -/// `SafeDebug` is not a trait and cannot be implemented, nor should you derive `Debug` explicitly. -/// Only when you derive `SafeDebug` will types help prevent leaking PII because, by default, only the type name is printed. -/// Only when you import `typespec_client_core` with feature `debug` will it derive `Debug` normally. -/// -/// # Examples -/// -/// ``` -/// use typespec_client_core::fmt::SafeDebug; -/// -/// #[derive(SafeDebug)] -/// struct MyModel { -/// name: Option, -/// } -/// -/// let model = MyModel { -/// name: Some("Kelly Smith".to_string()), -/// }; -/// if cfg!(feature = "debug") { -/// assert_eq!(format!("{model:?}"), r#"MyModel { name: Some("Kelly Smith") }"#); -/// } else { -/// assert_eq!(format!("{model:?}"), "MyModel { .. }"); -/// } -/// ``` #[cfg(feature = "derive")] pub use typespec_macros::SafeDebug; diff --git a/sdk/typespec/typespec_macros/src/lib.rs b/sdk/typespec/typespec_macros/src/lib.rs index 157a501b80..f1ed6db207 100644 --- a/sdk/typespec/typespec_macros/src/lib.rs +++ b/sdk/typespec/typespec_macros/src/lib.rs @@ -91,25 +91,66 @@ pub fn derive_model(input: proc_macro::TokenStream) -> proc_macro::TokenStream { /// Only when you derive `SafeDebug` will types help prevent leaking PII because, by default, only the type name is printed. /// Only when you enable the `debug` feature will it derive `Debug` normally. /// +/// You can attribute types, fields, and variants with `#[safe(true)]` or `#[safe(false)]` to optionally show or hide members. +/// The default is that no members are shown. The inner most `#[safe(..)]` attribute determines whether to show or hide a member. +/// /// # Examples /// /// ``` /// # use typespec_macros::SafeDebug; /// #[derive(SafeDebug)] -/// struct MyModel { -/// name: Option, +/// struct Person { +/// name: String, +/// } +/// +/// let person = Person { +/// name: "Kelly Smith".to_string(), +/// }; +/// if cfg!(feature = "debug") { +/// assert_eq!(format!("{person:?}"), r#"Person { name: "Kelly Smith" }"#); +/// } else { +/// assert_eq!(format!("{person:?}"), "Person { .. }"); +/// } +/// ``` +/// +/// Using the `#[safe(..)]` attribute, you can selectively show or hide members. +/// The default, when not present or inherited, is to always hide members unless the `debug` feature is enabled. +/// +/// ``` +/// # use typespec_macros::SafeDebug; +/// use std::ops::Range; +/// +/// #[derive(SafeDebug)] +/// struct Employee { +/// name: String, +/// #[safe(true)] +/// position: Position, +/// } +/// +/// #[derive(SafeDebug)] +/// #[safe(true)] +/// struct Position { +/// id: i32, +/// title: String, +/// #[safe(false)] +/// salary: Range, /// } /// -/// let model = MyModel { -/// name: Some("Kelly Smith".to_string()), +/// let employee = Employee { +/// name: "Kelly Smith".to_string(), +/// position: Position { +/// id: 12, +/// title: "Staff Engineer".to_string(), +/// salary: 200_000..250_000, +/// }, /// }; /// if cfg!(feature = "debug") { -/// assert_eq!(format!("{model:?}"), r#"MyModel { name: Some("Kelly Smith") }"#); +/// assert_eq!(format!("{employee:?}"), r#"Employee { name: "Kelly Smith", position: Position { id: 12, title: "Staff Engineer", salary: 200000..250000 } }"#); /// } else { -/// assert_eq!(format!("{model:?}"), "MyModel { .. }"); +/// assert_eq!(format!("{employee:?}"), r#"Employee { position: Position { id: 12, title: "Staff Engineer", .. }, .. }"#); /// } /// ``` -#[proc_macro_derive(SafeDebug)] +#[proc_macro_derive(SafeDebug, attributes(safe))] pub fn derive_safe_debug(input: proc_macro::TokenStream) -> proc_macro::TokenStream { run_derive_macro(input, safe_debug::derive_safe_debug_impl) } diff --git a/sdk/typespec/typespec_macros/src/safe_debug.rs b/sdk/typespec/typespec_macros/src/safe_debug.rs index 159c1b88b7..0f57a2d35e 100644 --- a/sdk/typespec/typespec_macros/src/safe_debug.rs +++ b/sdk/typespec/typespec_macros/src/safe_debug.rs @@ -5,7 +5,7 @@ use crate::Result; use proc_macro2::{Span, TokenStream}; use quote::{quote, ToTokens}; use syn::{ - punctuated::Punctuated, spanned::Spanned, token::Comma, Data, DataEnum, DataStruct, + punctuated::Punctuated, spanned::Spanned, token::Comma, Attribute, Data, DataEnum, DataStruct, DeriveInput, Error, Field, Fields, FieldsNamed, FieldsUnnamed, Ident, Path, }; @@ -26,14 +26,21 @@ fn generate_body(ast: DeriveInput) -> Result { let (impl_generics, ty_generics, where_clause) = ast.generics.split_for_impl(); let name = &ast.ident; + let type_attrs = Attrs::from_attrs(&ast.attrs)?; let body = match &ast.data { Data::Enum(DataEnum { variants, .. }) => { - let variants = variants.iter().map(|v| { - let variant_name = &v.ident; - let path = to_path(&[name, variant_name]); + let variants = variants + .iter() + .map(|v| -> Result { + let variant_name = &v.ident; + let path = to_path(&[name, variant_name]); + + let mut enum_attrs = Attrs::from_attrs(&v.attrs)?; + enum_attrs.update(&type_attrs); - generate_fields(&path, &v.fields) - }); + generate_fields(&path, &enum_attrs, &v.fields) + }) + .collect::>>()?; quote! { match self { @@ -43,7 +50,7 @@ fn generate_body(ast: DeriveInput) -> Result { } Data::Struct(DataStruct { fields, .. }) => { let path = to_path(&[name]); - let fields = generate_fields(&path, fields); + let fields = generate_fields(&path, &type_attrs, fields)?; quote! { match self { @@ -64,22 +71,28 @@ fn generate_body(ast: DeriveInput) -> Result { }) } -fn generate_fields(path: &Path, fields: &Fields) -> TokenStream { +fn generate_fields(path: &Path, type_attrs: &Attrs, fields: &Fields) -> Result { let name = &path.segments.last().expect("expected identifier").ident; let name_str = name.to_string(); match fields { Fields::Named(FieldsNamed { ref named, .. }) => { - let names: Vec<&Ident> = if cfg!(feature = "debug") { - named - .iter() - .map(|f| f.ident.as_ref().expect("expected named field")) - .collect() - } else { - // Should we ever add a `#[safe(bool)]` helper attribute to denote which fields we can safely include, - // filter the fields to match and emit based on the inherited value or field attribute value. - Vec::new() - }; + let names: Vec<&Ident> = named + .iter() + .filter_map(|f| -> Option> { + if cfg!(feature = "debug") { + return Some(Ok(f.ident.as_ref().expect("expected named field"))); + } + + match Attrs::from_attrs(&f.attrs) { + Err(err) => Some(Err(err)), + Ok(attrs) if type_attrs.is_safe(&attrs) => { + Some(Ok(f.ident.as_ref().expect("expected named field"))) + } + Ok(_) => None, + } + }) + .collect::>>()?; let fields: Vec = names .iter() .map(|field_name| { @@ -90,37 +103,45 @@ fn generate_fields(path: &Path, fields: &Fields) -> TokenStream { // Use an "and the rest" matcher as needed, along with the appropriate `DebugStruct` finisher. let (matcher, finisher) = finish(&fields, named, false); - quote! { + Ok(quote! { #path { #(#names),* #matcher } => f .debug_struct(#name_str) #(#fields)* #finisher - } + }) } - Fields::Unit => quote! {#path => f.write_str(#name_str)}, + Fields::Unit => Ok(quote! {#path => f.write_str(#name_str)}), Fields::Unnamed(FieldsUnnamed { ref unnamed, .. }) => { - let indices: Vec = if cfg!(feature = "debug") { - unnamed - .iter() - .enumerate() - .map(|(i, _)| { - Ident::new(&format!("f{i}"), Span::call_site()).into_token_stream() - }) - .collect() - } else { - // Should we ever add a `#[safe(bool)]` helper attribute to denote which fields we can safely include, - // filter the fields to match and emit based on the inherited value or field attribute value. - Vec::new() - }; + let indices: Vec = unnamed + .iter() + .enumerate() + .filter_map(|(i, f)| { + if cfg!(feature = "debug") { + return Some(Ok( + Ident::new(&format!("f{i}"), Span::call_site()).into_token_stream() + )); + } + + match Attrs::from_attrs(&f.attrs) { + Err(err) => Some(Err(err)), + Ok(attrs) if type_attrs.is_safe(&attrs) => { + Some(Ok( + Ident::new(&format!("f{i}"), Span::call_site()).into_token_stream() + )) + } + Ok(_) => None, + } + }) + .collect::>>()?; // Use an "and the rest" matcher as needed, along with the appropriate `DebugTuple` finisher. let (matcher, finisher) = finish(&indices, unnamed, true); - quote! { + Ok(quote! { #path(#(#indices),* #matcher) => f .debug_tuple(#name_str) #(.field(&#indices))* #finisher - } + }) } } } @@ -166,3 +187,194 @@ fn to_path(idents: &[&Ident]) -> Path { segments, } } + +#[derive(Debug, Default)] +struct Attrs { + safe: Option, +} + +impl Attrs { + fn from_attrs(attributes: &[Attribute]) -> Result { + let mut attrs = Attrs::default(); + let mut result = Ok(()); + for attribute in attributes.iter().filter(|a| a.path().is_ident("safe")) { + result = match (result, parse_attr(attribute, &mut attrs)) { + (Ok(()), Err(e)) => Err(e), + (Err(mut e1), Err(e2)) => { + e1.combine(e2); + Err(e1) + } + (e, Ok(())) => e, + }; + } + result.map(|_| attrs) + } + + fn is_safe(&self, other: &Attrs) -> bool { + match other.safe { + Some(val) => val, + None => self.safe.unwrap_or(false), + } + } + + fn update(&mut self, other: &Attrs) { + if let Some(val) = other.safe { + self.safe = Some(val); + } + } +} + +const INVALID_SAFE_ATTRIBUTE_MESSAGE: &str = + "invalid safe attribute, expected attribute in form #[safe(false)] or #[safe(true)]"; + +fn parse_attr(attribute: &Attribute, attrs: &mut Attrs) -> Result<()> { + let meta_list = attribute + .meta + .require_list() + .map_err(|_| Error::new(attribute.span(), INVALID_SAFE_ATTRIBUTE_MESSAGE))?; + let lit: syn::LitBool = meta_list + .parse_args() + .map_err(|_| Error::new(meta_list.span(), INVALID_SAFE_ATTRIBUTE_MESSAGE))?; + attrs.safe = Some(lit.value); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn attrs_safe_requires_arg() { + let attr: Attribute = syn::parse_quote! { + #[safe] + }; + assert!( + matches!(Attrs::from_attrs(&[attr]), Err(err) if err.to_string() == INVALID_SAFE_ATTRIBUTE_MESSAGE) + ); + } + + #[test] + fn attrs_safe_requires_bool() { + let attr: Attribute = syn::parse_quote! { + #[safe(false)] + }; + assert_eq!(Attrs::from_attrs(&[attr]).unwrap().safe, Some(false)); + + let attr: Attribute = syn::parse_quote! { + #[safe(true)] + }; + assert_eq!(Attrs::from_attrs(&[attr]).unwrap().safe, Some(true)); + + let attr: Attribute = syn::parse_quote! { + #[safe(other)] + }; + assert!( + matches!(Attrs::from_attrs(&[attr]), Err(err) if err.to_string() == INVALID_SAFE_ATTRIBUTE_MESSAGE) + ); + } + + #[test] + fn attrs_is_safe() { + let mut type_attrs = Attrs::default(); + let mut field_attrs = Attrs::default(); + + // Both None + assert!(!type_attrs.is_safe(&field_attrs)); + + // None, Some(false) + field_attrs.safe = Some(false); + assert!(!type_attrs.is_safe(&field_attrs)); + + // None, Some(true) + field_attrs.safe = Some(true); + assert!(type_attrs.is_safe(&field_attrs)); + + // Some(false), None + type_attrs.safe = Some(false); + field_attrs.safe = None; + assert!(!type_attrs.is_safe(&field_attrs)); + + // Some(true), None + type_attrs.safe = Some(true); + field_attrs.safe = None; + assert!(type_attrs.is_safe(&field_attrs)); + + // Some(false), Some(false) + type_attrs.safe = Some(false); + field_attrs.safe = Some(false); + assert!(!type_attrs.is_safe(&field_attrs)); + + // Some(true), Some(false) + type_attrs.safe = Some(true); + field_attrs.safe = Some(false); + assert!(!type_attrs.is_safe(&field_attrs)); + + // Some(false), Some(true) + type_attrs.safe = Some(false); + field_attrs.safe = Some(true); + assert!(type_attrs.is_safe(&field_attrs)); + + // Some(true), Some(true) + type_attrs.safe = Some(true); + field_attrs.safe = Some(true); + assert!(type_attrs.is_safe(&field_attrs)); + } + + #[test] + fn attrs_update() { + let mut type_attrs = Attrs::default(); + let mut enum_attrs = Attrs::default(); + + // Both None + type_attrs.update(&enum_attrs); + assert_eq!(type_attrs.safe, None); + + // None, Some(false) + type_attrs.safe = None; + enum_attrs.safe = Some(false); + type_attrs.update(&enum_attrs); + assert_eq!(type_attrs.safe, Some(false)); + + // None, Some(true) + type_attrs.safe = None; + enum_attrs.safe = Some(true); + type_attrs.update(&enum_attrs); + assert_eq!(type_attrs.safe, Some(true)); + + // Some(false), None + type_attrs.safe = Some(false); + enum_attrs.safe = None; + type_attrs.update(&enum_attrs); + assert_eq!(type_attrs.safe, Some(false)); + + // Some(true), None + type_attrs.safe = Some(true); + enum_attrs.safe = None; + type_attrs.update(&enum_attrs); + assert_eq!(type_attrs.safe, Some(true)); + + // Some(false), Some(false) + type_attrs.safe = Some(false); + enum_attrs.safe = Some(false); + type_attrs.update(&enum_attrs); + assert_eq!(type_attrs.safe, Some(false)); + + // Some(true), Some(false) + type_attrs.safe = Some(true); + enum_attrs.safe = Some(false); + type_attrs.update(&enum_attrs); + assert_eq!(type_attrs.safe, Some(false)); + + // Some(false), Some(true) + type_attrs.safe = Some(false); + enum_attrs.safe = Some(true); + type_attrs.update(&enum_attrs); + assert_eq!(type_attrs.safe, Some(true)); + + // Some(true), Some(true) + type_attrs.safe = Some(true); + enum_attrs.safe = Some(true); + type_attrs.update(&enum_attrs); + assert_eq!(type_attrs.safe, Some(true)); + } +} diff --git a/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/src/lib.rs b/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/src/lib.rs index e99cde7bb7..444392b0c7 100644 --- a/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/src/lib.rs +++ b/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/src/lib.rs @@ -4,13 +4,15 @@ use typespec_client_core::fmt::SafeDebug; #[derive(SafeDebug)] -pub struct Tuple(pub i32, pub &'static str); +#[safe(false)] +pub struct Tuple(#[safe(true)] pub i32, pub &'static str); #[derive(SafeDebug)] pub struct EmptyTuple(); #[derive(SafeDebug)] pub struct Struct { + #[safe(true)] pub a: i32, pub b: &'static str, } @@ -26,6 +28,19 @@ pub enum Enum { Unit, Tuple(i32, &'static str), EmptyTuple(), - Struct { a: i32, b: &'static str }, + #[safe(true)] + Struct { + a: i32, + #[safe(false)] + b: &'static str, + }, EmptyStruct {}, } + +#[derive(SafeDebug)] +#[safe(true)] +pub struct MostlySafeStruct { + #[safe(false)] + pub name: &'static str, + pub title: &'static str, +} diff --git a/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/tests/debug.rs b/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/tests/debug.rs index 8057b046dc..66c775b3c3 100644 --- a/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/tests/debug.rs +++ b/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/tests/debug.rs @@ -62,3 +62,15 @@ fn debug_enum_empty_struct() { let x = Enum::EmptyStruct {}; assert_eq!(format!("{x:?}"), r#"EmptyStruct"#); } + +#[test] +fn debug_mostly_safe_struct() { + let x = MostlySafeStruct { + name: "Kelly Smith", + title: "Staff Engineer", + }; + assert_eq!( + format!("{x:?}"), + r#"MostlySafeStruct { name: "Kelly Smith", title: "Staff Engineer" }"# + ); +} diff --git a/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/tests/safe-debug.rs b/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/tests/safe-debug.rs index 5c4de33b6f..bbb6817a85 100644 --- a/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/tests/safe-debug.rs +++ b/sdk/typespec/typespec_macros/tests/data/safe-debug-tests/tests/safe-debug.rs @@ -16,9 +16,9 @@ static RUSTC_VERSION: LazyLock = fn safe_debug_tuple() { let x = Tuple(1, "foo"); if *RUSTC_VERSION < MIN { - assert_eq!(format!("{x:?}"), r#"Tuple"#); + assert_eq!(format!("{x:?}"), r#"Tuple(1)"#); } else { - assert_eq!(format!("{x:?}"), r#"Tuple(..)"#); + assert_eq!(format!("{x:?}"), r#"Tuple(1, ..)"#); } } @@ -31,7 +31,7 @@ fn safe_debug_empty_tuple() { #[cfg_attr(not(feature = "debug"), test)] fn safe_debug_struct() { let x = Struct { a: 1, b: "foo" }; - assert_eq!(format!("{x:?}"), r#"Struct { .. }"#); + assert_eq!(format!("{x:?}"), r#"Struct { a: 1, .. }"#); } #[test] @@ -71,7 +71,7 @@ fn safe_debug_enum_empty_tuple() { #[cfg_attr(not(feature = "debug"), test)] fn safe_debug_enum_struct() { let x = Enum::Struct { a: 1, b: "foo" }; - assert_eq!(format!("{x:?}"), r#"Struct { .. }"#); + assert_eq!(format!("{x:?}"), r#"Struct { a: 1, .. }"#); } #[test] @@ -79,3 +79,15 @@ fn safe_debug_enum_empty_struct() { let x = Enum::EmptyStruct {}; assert_eq!(format!("{x:?}"), r#"EmptyStruct"#); } + +#[test] +fn safe_debug_mostly_safe_struct() { + let x = MostlySafeStruct { + name: "Kelly Smith", + title: "Staff Engineer", + }; + assert_eq!( + format!("{x:?}"), + r#"MostlySafeStruct { title: "Staff Engineer", .. }"# + ); +}