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

Config (still) has private fields in 0.26 #908

Closed
virtualritz opened this issue Dec 16, 2023 · 6 comments · Fixed by #911
Closed

Config (still) has private fields in 0.26 #908

virtualritz opened this issue Dec 16, 2023 · 6 comments · Fixed by #911

Comments

@virtualritz
Copy link

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
Copy link
Contributor

jschwe commented Dec 18, 2023

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
Copy link
Author

virtualritz commented Dec 18, 2023

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
Copy link
Author

virtualritz commented Dec 18, 2023

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
Copy link

fzyzcjy commented Jan 14, 2024

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

@virtualritz
Copy link
Author

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
Copy link
Author

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.

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 a pull request may close this issue.

3 participants