diff --git a/core/src/ast/attrs.rs b/core/src/ast/attrs.rs index 2cc2c0d59..6d2180d98 100644 --- a/core/src/ast/attrs.rs +++ b/core/src/ast/attrs.rs @@ -32,14 +32,15 @@ impl Attrs { Attr::Cfg(attr) => self.cfg.push(attr), Attr::DiplomatBackend(attr) => self.attrs.push(attr), Attr::SkipIfUnsupported => self.skip_if_unsupported = true, - Attr::CRename(rename) => self.c_rename.extend(&rename), + Attr::CRename(rename) => self.c_rename.extend(&rename, AttrExtendMode::Override), } } /// Merge attributes that should be inherited from the parent pub(crate) fn merge_parent_attrs(&mut self, other: &Attrs) { self.cfg.extend(other.cfg.iter().cloned()); - self.c_rename.extend(&other.c_rename); + self.c_rename + .extend(&other.c_rename, AttrExtendMode::Inherit); } pub(crate) fn add_attrs(&mut self, attrs: &[Attribute]) { for attr in syn_attr_to_ast_attr(attrs) { @@ -81,7 +82,7 @@ fn syn_attr_to_ast_attr(attrs: &[Attribute]) -> impl Iterator + '_ .expect("Failed to parse malformed diplomat::attr"), )) } else if a.path() == &crename_attr { - Some(Attr::CRename(RenameAttr::from_syn(a).unwrap())) + Some(Attr::CRename(RenameAttr::from_meta(&a.meta).unwrap())) } else if a.path() == &skipast { Some(Attr::SkipIfUnsupported) } else { @@ -209,6 +210,16 @@ impl Parse for DiplomatBackendAttr { } } +/// When calling attr.extend(other), when both attributes specify +/// some setting which one to prefer? +#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd)] +pub(crate) enum AttrExtendMode { + /// Prefer data from `self`, the data from `other` is only used if `self` is null + Inherit, + /// Prefer data from `other`, the data from `self` is only used if `other` is null + Override, +} + /// A pattern for use in rename attributes, like `#[diplomat::c_rename]` /// /// This can be parsed from a string, typically something like `icu4x_{0}`. @@ -223,7 +234,7 @@ pub struct RenameAttr { impl RenameAttr { /// Apply all renames to a given string - pub fn apply<'a>(&'a self, name: &'a str) -> Cow<'a, str> { + pub fn apply<'a>(&'a self, name: Cow<'a, str>) -> Cow<'a, str> { if let Some(ref pattern) = self.pattern { let replacement = &pattern.replacement; if let Some(index) = pattern.insertion_index { @@ -232,7 +243,7 @@ impl RenameAttr { replacement.into() } } else { - name.into() + name } } @@ -241,10 +252,13 @@ impl RenameAttr { self.pattern.is_none() } - fn extend(&mut self, parent: &Self) { - // Patterns override each other on inheritance - if self.pattern.is_none() { - self.pattern = parent.pattern.clone(); + pub(crate) fn extend(&mut self, other: &Self, extend_mode: AttrExtendMode) { + if (extend_mode == AttrExtendMode::Inherit && self.pattern.is_none()) + || (extend_mode == AttrExtendMode::Override && other.pattern.is_some()) + { + // Copy over the new pattern either when inheriting (and self is empty) + // or when overriding (and other is nonempty) + self.pattern = other.pattern.clone(); } // In the future if we support things like to_lower_case they may inherit separately @@ -258,10 +272,10 @@ impl RenameAttr { } } - fn from_syn(a: &Attribute) -> Result> { + pub(crate) fn from_meta(meta: &Meta) -> Result> { static C_RENAME_ERROR: &str = "#[diplomat::c_rename] must be given a string value"; - match a.meta { + match meta { Meta::Path(..) => Err(C_RENAME_ERROR.into()), Meta::NameValue(ref nv) => { // Support a shortcut `c_rename = "..."` @@ -274,7 +288,7 @@ impl RenameAttr { Ok(RenameAttr::from_pattern(&lit.value())) } // The full syntax to which we'll add more things in the future, `c_rename("")` - Meta::List(..) => a.parse_args().map_err(|e| { + Meta::List(list) => list.parse_args().map_err(|e| { format!("Failed to parse malformed #[diplomat::c_rename(...)]: {e}").into() }), } @@ -351,10 +365,10 @@ mod tests { #[test] fn test_rename() { let attr: syn::Attribute = syn::parse_quote!(#[diplomat::c_rename = "foobar_{0}"]); - let attr = RenameAttr::from_syn(&attr).unwrap(); + let attr = RenameAttr::from_meta(&attr.meta).unwrap(); insta::assert_yaml_snapshot!(attr); let attr: syn::Attribute = syn::parse_quote!(#[diplomat::c_rename("foobar_{0}")]); - let attr = RenameAttr::from_syn(&attr).unwrap(); + let attr = RenameAttr::from_meta(&attr.meta).unwrap(); insta::assert_yaml_snapshot!(attr); } } diff --git a/core/src/ast/methods.rs b/core/src/ast/methods.rs index 7e8619c76..5cfc47840 100644 --- a/core/src/ast/methods.rs +++ b/core/src/ast/methods.rs @@ -56,7 +56,7 @@ impl Method { let method_ident = &m.sig.ident; let concat_method_ident = format!("{self_ident}_{method_ident}"); let extern_ident = syn::Ident::new( - &attrs.c_rename.apply(&concat_method_ident), + &attrs.c_rename.apply(concat_method_ident.into()), m.sig.ident.span(), ); diff --git a/core/src/hir/attrs.rs b/core/src/hir/attrs.rs index 2758d0a14..c70d27cd8 100644 --- a/core/src/hir/attrs.rs +++ b/core/src/hir/attrs.rs @@ -1,11 +1,10 @@ //! #[diplomat::attr] and other attributes use crate::ast; -use crate::ast::attrs::DiplomatBackendAttrCfg; +use crate::ast::attrs::{AttrExtendMode, DiplomatBackendAttrCfg}; use crate::hir::LoweringError; -use quote::ToTokens; -use syn::{LitStr, Meta}; +use syn::Meta; pub use crate::ast::attrs::RenameAttr; @@ -13,7 +12,7 @@ pub use crate::ast::attrs::RenameAttr; #[derive(Clone, Default, Debug)] pub struct Attrs { pub disable: bool, - pub rename: Option, + pub rename: RenameAttr, pub c_rename: RenameAttr, // more to be added: rename, namespace, etc } @@ -46,65 +45,46 @@ impl Attrs { let support = validator.attrs_supported(); for attr in &ast.attrs { if validator.satisfies_cfg(&attr.cfg) { - match &attr.meta { - Meta::Path(p) => { - if p.is_ident("disable") { - if this.disable { - errors.push(LoweringError::Other( - "Duplicate `disable` attribute".into(), - )); - } else if !support.disabling { - errors.push(LoweringError::Other(format!( - "`disable` not supported in backend {}", - validator.primary_name() - ))) - } else if context == AttributeContext::EnumVariant { - errors.push(LoweringError::Other( - "`disable` cannot be used on enum variants".into(), - )) - } else { - this.disable = true; - } - } else { + let path = attr.meta.path(); + + if path.is_ident("disable") { + if let Meta::Path(_) = attr.meta { + if this.disable { + errors + .push(LoweringError::Other("Duplicate `disable` attribute".into())); + } else if !support.disabling { errors.push(LoweringError::Other(format!( - "Unknown diplomat attribute {p:?}: expected one of: `disable, rename`" - ))); - } - } - Meta::NameValue(nv) => { - let p = &nv.path; - if p.is_ident("rename") { - if this.rename.is_some() { - errors.push(LoweringError::Other( - "Duplicate `rename` attribute".into(), - )); - } else if !support.renaming { - errors.push(LoweringError::Other(format!( - "`rename` not supported in backend {}", - validator.primary_name() - ))) - } else { - let v = nv.value.to_token_stream(); - let l = syn::parse2::(v); - if let Ok(ref l) = l { - this.rename = Some(l.value()) - } else { - errors.push(LoweringError::Other(format!( - "Found diplomat attribute {p:?}: expected string as `rename` argument" - ))); - } - } + "`disable` not supported in backend {}", + validator.primary_name() + ))) + } else if context == AttributeContext::EnumVariant { + errors.push(LoweringError::Other( + "`disable` cannot be used on enum variants".into(), + )) } else { - errors.push(LoweringError::Other(format!( - "Unknown diplomat attribute {p:?}: expected one of: `disable, rename`" - ))); + this.disable = true; } + } else { + errors.push(LoweringError::Other( + "`disable` must be a simple path".into(), + )) } - other => { - errors.push(LoweringError::Other(format!( - "Unknown diplomat attribute {other:?}: expected one of: `disable, rename`" - ))); + } else if path.is_ident("rename") { + match RenameAttr::from_meta(&attr.meta) { + Ok(rename) => { + // We use the override extend mode: a single ast::Attrs + // will have had these attributes inherited into the list by appending + // to the end; so a later attribute in the list is more pertinent. + this.rename.extend(&rename, AttrExtendMode::Override); + } + Err(e) => errors.push(LoweringError::Other(format!( + "`rename` attr failed to parse: {e:?}" + ))), } + } else { + errors.push(LoweringError::Other(format!( + "Unknown diplomat attribute {path:?}: expected one of: `disable, rename`" + ))); } } } diff --git a/core/src/hir/snapshots/diplomat_core__hir__elision__tests__simple_mod.snap b/core/src/hir/snapshots/diplomat_core__hir__elision__tests__simple_mod.snap index c3ee88d34..7c1a22f86 100644 --- a/core/src/hir/snapshots/diplomat_core__hir__elision__tests__simple_mod.snap +++ b/core/src/hir/snapshots/diplomat_core__hir__elision__tests__simple_mod.snap @@ -92,7 +92,9 @@ TypeContext { ), attrs: Attrs { disable: false, - rename: None, + rename: RenameAttr { + pattern: None, + }, c_rename: RenameAttr { pattern: None, }, @@ -101,7 +103,9 @@ TypeContext { ], attrs: Attrs { disable: false, - rename: None, + rename: RenameAttr { + pattern: None, + }, c_rename: RenameAttr { pattern: None, }, @@ -206,7 +210,9 @@ TypeContext { ), attrs: Attrs { disable: false, - rename: None, + rename: RenameAttr { + pattern: None, + }, c_rename: RenameAttr { pattern: None, }, @@ -215,7 +221,9 @@ TypeContext { ], attrs: Attrs { disable: false, - rename: None, + rename: RenameAttr { + pattern: None, + }, c_rename: RenameAttr { pattern: None, }, @@ -242,7 +250,9 @@ TypeContext { methods: [], attrs: Attrs { disable: false, - rename: None, + rename: RenameAttr { + pattern: None, + }, c_rename: RenameAttr { pattern: None, }, diff --git a/feature_tests/cpp2/include/AttrOpaque1Renamed.d.hpp b/feature_tests/cpp2/include/AttrOpaque1Renamed.d.hpp index 2ddfd0006..fc4e2583c 100644 --- a/feature_tests/cpp2/include/AttrOpaque1Renamed.d.hpp +++ b/feature_tests/cpp2/include/AttrOpaque1Renamed.d.hpp @@ -14,7 +14,7 @@ class AttrOpaque1Renamed { public: - inline static std::unique_ptr new_(); + inline static std::unique_ptr totally_not_new(); inline uint8_t method_renamed() const; diff --git a/feature_tests/cpp2/include/AttrOpaque1Renamed.hpp b/feature_tests/cpp2/include/AttrOpaque1Renamed.hpp index e3d5f15b6..7e1475beb 100644 --- a/feature_tests/cpp2/include/AttrOpaque1Renamed.hpp +++ b/feature_tests/cpp2/include/AttrOpaque1Renamed.hpp @@ -13,7 +13,7 @@ #include "AttrOpaque1.h" -inline std::unique_ptr AttrOpaque1Renamed::new_() { +inline std::unique_ptr AttrOpaque1Renamed::totally_not_new() { auto result = capi::namespace_AttrOpaque1_new(); return std::unique_ptr(AttrOpaque1Renamed::FromFFI(result)); } diff --git a/feature_tests/cpp2/tests/attrs.cpp b/feature_tests/cpp2/tests/attrs.cpp index 37de0809a..b545fd12a 100644 --- a/feature_tests/cpp2/tests/attrs.cpp +++ b/feature_tests/cpp2/tests/attrs.cpp @@ -3,7 +3,7 @@ #include "assert.hpp" int main(int argc, char *argv[]) { - std::unique_ptr r = AttrOpaque1Renamed::new_(); + std::unique_ptr r = AttrOpaque1Renamed::totally_not_new(); simple_assert_eq("method should call", r->method_renamed(), 77); simple_assert_eq("method should call", r->crenamed(), 123); diff --git a/feature_tests/src/attrs.rs b/feature_tests/src/attrs.rs index 8feab53ad..6cf1d5cbc 100644 --- a/feature_tests/src/attrs.rs +++ b/feature_tests/src/attrs.rs @@ -6,6 +6,7 @@ pub mod ffi { pub struct AttrOpaque1; impl AttrOpaque1 { + #[diplomat::attr(cpp2, rename = "totally_not_{0}")] pub fn new() -> Box { Box::new(AttrOpaque1) } diff --git a/tool/src/c2/formatter.rs b/tool/src/c2/formatter.rs index fd530d4db..dcbda7e2d 100644 --- a/tool/src/c2/formatter.rs +++ b/tool/src/c2/formatter.rs @@ -79,7 +79,7 @@ impl<'tcx> CFormatter<'tcx> { let ty_name = self.fmt_type_name(ty); let method_name = method.name.as_str(); let put_together = format!("{ty_name}_{method_name}"); - method.attrs.c_rename.apply(&put_together).into() + method.attrs.c_rename.apply(put_together.into()).into() } pub fn fmt_ptr<'a>(&self, ident: &'a str, mutability: hir::Mutability) -> Cow<'a, str> { diff --git a/tool/src/cpp2/formatter.rs b/tool/src/cpp2/formatter.rs index fd92cae1a..e370d9ac9 100644 --- a/tool/src/cpp2/formatter.rs +++ b/tool/src/cpp2/formatter.rs @@ -27,11 +27,11 @@ impl<'tcx> Cpp2Formatter<'tcx> { /// Resolve and format a named type for use in code pub fn fmt_type_name(&self, id: TypeId) -> Cow<'tcx, str> { let resolved = self.c.tcx().resolve_type(id); - if let Some(rename) = resolved.attrs().rename.as_ref() { - rename.into() - } else { - resolved.name().as_str().into() - } + + resolved + .attrs() + .rename + .apply(resolved.name().as_str().into()) } /// Resolve and format a named type for use in diagnostics @@ -60,11 +60,7 @@ impl<'tcx> Cpp2Formatter<'tcx> { /// Format an enum variant. pub fn fmt_enum_variant(&self, variant: &'tcx hir::EnumVariant) -> Cow<'tcx, str> { - if let Some(rename) = variant.attrs.rename.as_ref() { - rename.into() - } else { - variant.name.as_str().into() - } + variant.attrs.rename.apply(variant.name.as_str().into()) } pub fn fmt_c_enum_variant<'a>( &self, @@ -148,15 +144,15 @@ impl<'tcx> Cpp2Formatter<'tcx> { /// Format a method pub fn fmt_method_name<'a>(&self, method: &'a hir::Method) -> Cow<'a, str> { + let name = method.attrs.rename.apply(method.name.as_str().into()); + // TODO(#60): handle other keywords - if let Some(rename) = method.attrs.rename.as_ref() { - rename.into() - } else if method.name == "new" { + if name == "new" { "new_".into() - } else if method.name == "default" { + } else if name == "default" { "default_".into() } else { - method.name.as_str().into() + name } } diff --git a/tool/src/dart/formatter.rs b/tool/src/dart/formatter.rs index 5aae71a2b..720f89b0d 100644 --- a/tool/src/dart/formatter.rs +++ b/tool/src/dart/formatter.rs @@ -78,9 +78,8 @@ impl<'tcx> DartFormatter<'tcx> { /// Resolve and format a named type for use in code pub fn fmt_type_name(&self, id: TypeId) -> Cow<'tcx, str> { let resolved = self.c.tcx().resolve_type(id); - let candidate: Cow<'tcx, str> = if let Some(rename) = resolved.attrs().rename.as_ref() { - rename.into() - } else if let Some(strip_prefix) = self.strip_prefix.as_ref() { + + let candidate: Cow = if let Some(strip_prefix) = self.strip_prefix.as_ref() { resolved .name() .as_str() @@ -95,7 +94,7 @@ impl<'tcx> DartFormatter<'tcx> { panic!("{candidate:?} is not a valid Dart type name. Please rename."); } - candidate + resolved.attrs().rename.apply(candidate) } /// Resolve and format a named type for use in diagnostics @@ -106,11 +105,8 @@ impl<'tcx> DartFormatter<'tcx> { /// Format an enum variant. pub fn fmt_enum_variant(&self, variant: &'tcx hir::EnumVariant) -> Cow<'tcx, str> { - if let Some(rename) = variant.attrs.rename.as_ref() { - rename.into() - } else { - variant.name.as_str().to_lower_camel_case().into() - } + let name = variant.name.as_str().to_lower_camel_case().into(); + variant.attrs.rename.apply(name) } /// Format a field name or parameter name @@ -126,15 +122,14 @@ impl<'tcx> DartFormatter<'tcx> { /// Format a method pub fn fmt_method_name<'a>(&self, method: &'a hir::Method) -> Cow<'a, str> { // TODO(#60): handle other keywords - if let Some(rename) = method.attrs.rename.as_ref() { - rename.into() + + // TODO: we should give attrs.rename() control over the camelcasing + let name = method.name.as_str().to_lower_camel_case(); + let name = method.attrs.rename.apply(name.into()); + if INVALID_METHOD_NAMES.contains(&&*name) { + format!("{name}_").into() } else { - let name = method.name.as_str().to_lower_camel_case(); - if INVALID_METHOD_NAMES.contains(&name.as_str()) { - format!("{name}_").into() - } else { - name.into() - } + name } }