Use new RenameAttr for HIR renaming too (#417)

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<Item = Attr> + '_
                     .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<Self, Cow<'static, str>> {
+    pub(crate) fn from_meta(meta: &Meta) -> Result<Self, Cow<'static, str>> {
         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);
     }
 }

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(),
         );
 

core/src/hir/attrs.rs

@@ -1,19 +1,18 @@
 //! #[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;
 
 #[non_exhaustive]
 #[derive(Clone, Default, Debug)]
 pub struct Attrs {
     pub disable: bool,
-    pub rename: Option<String>,
+    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::<LitStr>(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`"
+                    )));
                 }
             }
         }

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,
                 },

feature_tests/cpp2/tests/attrs.cpp

@@ -3,7 +3,7 @@
 #include "assert.hpp"
 
 int main(int argc, char *argv[]) {
-    std::unique_ptr<AttrOpaque1Renamed> r = AttrOpaque1Renamed::new_();
+    std::unique_ptr<AttrOpaque1Renamed> r = AttrOpaque1Renamed::totally_not_new();
     simple_assert_eq("method should call", r->method_renamed(), 77);
     simple_assert_eq("method should call", r->crenamed(), 123);
 

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<AttrOpaque1> {
             Box::new(AttrOpaque1)
         }

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> {

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
         }
     }