mozilla / cbindgen

A project for generating C bindings from Rust code
Mozilla Public License 2.0
2.29k stars 294 forks source link

Config (still) has private fields in `0.26` #908

Closed virtualritz closed 5 months ago

virtualritz commented 7 months ago

See #841 .

This is still broken in 0.26. This makes Builder::with_config() a PITA to use.

    cbindgen::Builder::new()
        .with_crate(std::env::var("CARGO_MANIFEST_DIR").unwrap())
        .with_config(cbindgen::Config {
            language: cbindgen::Language::C,
            braces: cbindgen::Braces::SameLine,
            cpp_compat: true,
            style: cbindgen::Style::Both,
            ..Default::default()
        })
        .generate()?
        .write_to_file("binding.h");
error[E0451]: field `config_path` of struct `Config` is private
   --> foo/build.rs:104:15
    |
104 |             ..Default::default()
    |               ^^^^^^^^^^^^^^^^^^ field `config_path` is private
jschwe commented 7 months ago

cbindgen follows cargo semver, so version 0.25 -> 0.26 allows breaking changes. The previous issue was because 0.25.x -> 0.25.y should not contain any breaking changes.

virtualritz commented 7 months ago

I opened the issue not to report a breaking API change but what I (and others, see #841) deem a questionable API design decision.

That it is a breaking change is perfectly acceptable per se, just not the reason why. 😉

virtualritz commented 7 months ago

Basically, this forces one to do this:

    cbindgen::Builder::new()
        .with_crate(std::env::var("CARGO_MANIFEST_DIR").unwrap())
        .with_config({
            let mut config = cbindgen::Config::default();

            config.language = cbindgen::Language::C;
            config.braces = cbindgen::Braces::SameLine;
            config.cpp_compat = true;
            config.style = cbindgen::Style::Both;

            config
        })
        .generate()?
        .write_to_file("binding.h");

This is neither canonical Rust nor the intended use of the init-struct-pattern employed here.

Suggestion: separate the internally used (private) config struct from the public one used to configure the binding generator.

fzyzcjy commented 6 months ago

I am also seeing this issue when working on https://github.com/fzyzcjy/flutter_rust_bridge. Looking forward to seeing it fixed!

virtualritz commented 6 months ago

On that note, the clash between wanting to keep parts of a struct private and this breaking initializiation patterns has been discussed elsewhere.

There is also a related, old RFC.

My point would be that until (or if ever) this is supported by the compiler, if you have a struct that you want to support initializing with the init-struct pattern while keeping some of its fields private, the straightforward option is to make a 2nd struct, just for the pattern. I.e.:

pub struct Foo {
     pub a: u32,
     pub b: i32
     c: f32,
}

#[derive(Default)]
pub struct FooInit {
     pub a: u32,
     pub b: i32,
}

impl From<FooInit> for Foo {
     ...
}

struct Bar {
     foo: Foo,
     ...
}

impl Bar {
     pub fn with_foo(foo: Foo) {
          ...
     }
}

Then you use that:


let bar = Bar::new()
    .with_foo(FooInit {
        a: 42,
        ..default::Default()
    }.into());
virtualritz commented 6 months ago

I'm not saying I would do this.

I'd just not have partially private structs. I think the added safety the former provides is dwarfed by the cumbersome workarounds required to keep UX pleasant. See code above.