Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macro: ensure that cfg attrs are set for type alias verify generation #1464

Merged
merged 1 commit into from
Mar 12, 2025

Conversation

ahayzen-kdab
Copy link
Contributor

mod ffi {
  unsafe extern "C++" {
    #[cfg(enabled)]
    type A = crate::A;
  }
}

When using the bridge above we need to ensure that all generated code also has the cfg attribute set. Before this change the following error would occur cannot find type A in this scope. This appears to be due to the generated verify block below that was not copying the attributes from the original type.

const _: fn() = ::cxx::private::verify_external_type::<A, ...>;

After this change the attributes are copied and therefore when the type is not compiled the verify block is also not compiled.

```rust
mod ffi {
  unsafe extern "C++" {
    #[cfg(enabled)]
    type A = crate::A;
  }
}
```

When using the bridge above we need to ensure that all generated code
also has the cfg attribute set. Before this change the following error
would occur `cannot find type A in this scope`. This appears to be
due to the generated verify block below that was not copying the
attributes from the original type.

```rust
const _: fn() = ::cxx::private::verify_external_type::<A, ...>;
````

After this change the attributes are copied and therefore when the type
is not compiled the verify block is also not compiled.
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that you have tested that this works? I would not expect cfg attributes to be present in attrs so this PR as currently written should have no effect on what you are describing.

@ahayzen-kdab
Copy link
Contributor Author

I could see the cfg attribute appearing via cargo expand, but i would personally prefer this to be in a test to be sure as well.

@dtolnay do you have any recommendations as to where i could write a test for this ? As i had a quick look but it wasn't obvious if i should put this in tests/ or in an inline #[test] etc.

If you could point me to an example of a test taking a Rust bridge as input and asserting the generated Rust output that would be useful.

@dtolnay
Copy link
Owner

dtolnay commented Mar 12, 2025

Here is where I got mixed up: I forgot that cfg attributes end up going into both cfg and attrs (unlike some other attributes, which are partitioned into only the Doc, or only the Vec<Derive>, or only the OtherAttrs). The PR makes perfect sense now. Thank you!

cxx/syntax/mod.rs

Lines 167 to 174 in 82765ef

pub(crate) struct TypeAlias {
#[allow(dead_code)] // only used by cxx-build, not cxxbridge-macro
pub cfg: CfgExpr,
#[allow(dead_code)] // only used by cxxbridge-macro, not cxx-build
pub doc: Doc,
pub derives: Vec<Derive>,
#[allow(dead_code)] // only used by cxxbridge-macro, not cxx-build
pub attrs: OtherAttrs,

cxx/syntax/attrs.rs

Lines 132 to 139 in 82765ef

} else if attr_path.is_ident("cfg") {
match cfg::parse_attribute(&attr) {
Ok(cfg_expr) => {
if let Some(cfg) = &mut parser.cfg {
cfg.merge(cfg_expr);
passthrough_attrs.push(attr);
continue;
}

@dtolnay dtolnay merged commit e2d0d2c into dtolnay:master Mar 12, 2025
22 checks passed
@dtolnay
Copy link
Owner

dtolnay commented Mar 12, 2025

Published in 1.0.144.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants