Support 'swift_name' attributes on generated functions #449

alaroldai · 2020-01-08T16:13:33Z

Fixes #420

Added a parameter to bindgen::ir::Function to track attributes on generated functions. This is in addition to the existing postfix annotation, which seems to be used exclusively for user-provided strings, and so seems like a bad fit for auto-generated code.
Added a config parameter under FunctionConfig to control whether swift function names are generated

No support for documentation annotations, but adding support for something like the following may be a good follow-up task:

impl FooRef {
  // cbindgen:swift-name=FooRef.doTheThing(with arg:)
  fn do_thing(arg: u8) {
  }
}

emilio

Hi! thanks for the patch! Some review comments below.

Some questions on top:

This needs docs. It's unclear how this should be tied together. Could you write some docs on how to use this? I naively expected that swift support would entail generating swift code, but it seems I was wrong. I don't know much of swift but this should be documented somewhere, it's non-obvious. Also, the configuration changes should be put in docs.rs and co.
I think the API should be more consistent with the way must_use and other attribute-generating thing works. That way we don't hardcode the attribute name, and allow supporting swift and other platforms easily.
I don't know why the swift name is generated out of the syn ast rather than the cbindgen IR. I suspect moving it to the cbindgen IR would avoid the need for the attributes member and such altogether, and make the patch simpler / more consistent over-all...

Ideally this patch would be split into two separate PRs, one that parses and generates the functions for impl blocks, and one that generates the swift attributes. That would make it much easier to review, I think but if it's a lot of work then no worries, I can live with that :)

emilio · 2020-01-08T21:41:39Z

src/bindgen/ir/function.rs

+            let attributes = func
+                .attributes
+                .iter()
+                .map(|s| format!(" __attribute__(({}))", s))


We've generally avoided hardcoding attribute names and __attribute__ declarations here. I think a slightly more consistent approach would be something like an [fn] swift_name_macro = "FOO" config. This would get the raw content of the string just like CF_SWIFT_NAME does. So that to use this feature the only thing you need to do would be [fn] swift_name_macro = "CF_SWIFT_NAME", for example.

Yes, that's a much better solution. It doesn't work quite so well for the __attribute__ version of this behaviour because of the double-parentheses needed, but most users will probably prefer CF_SWIFT_NAME or similar instead (and a macro can be defined and included separately if necessary).

emilio · 2020-01-08T21:44:08Z

src/bindgen/utilities.rs

 pub trait SynItemFnHelpers: SynItemHelpers {
    fn exported_name(&self) -> Option<String>;
+
+    fn swift_name(&self, type_name: Option<&syn::Ident>) -> String;


Seems like this should belong to our IR instead of syn... Why is this here?

You're right, this doesn't need to be here. Moved it into bindgen::ir::Function.

Fixes mozilla#420

alaroldai

Thanks for the review!

This needs docs

Switched to using a config item under fn as you suggested, and added docs both to the config struct and in docs.md on how to use this feature. Let me know what you think.

I'd like to add an example, but there's no examples directory present - should I create one, or upload a separate repository and link to it from the readme?

emilio

Thanks! Still need to take a closer look at the rest but this looks reasonable so far.

A couple more questions below.

Regarding the example, I think I'm good with your explanation, but feel free to link to an example repo or what not :)

I didn't know that swift had built-in parsing of C / C++ to generate "bindings" or such. I feel it's a mix of pretty cool and pretty scary...

emilio · 2020-01-09T01:04:54Z

src/bindgen/ir/function.rs

+        let item_args = {
+            let mut items = vec![];
+            for (arg, _) in self.args.iter() {
+                match arg.as_str() {


This is match is just items.push(format!("{}:", arg.as_str()) right?

emilio · 2020-01-09T01:05:29Z

src/bindgen/ir/function.rs

+
+            if let Some(ref swift_name_macro) = config.function.swift_name_macro {
+                let swift_name = func.swift_name();
+                if !swift_name.is_empty() {


this can never be empty, right?

emilio · 2020-01-09T01:06:01Z

src/bindgen/ir/function.rs

+            if let Some(ref swift_name_macro) = config.function.swift_name_macro {
+                let swift_name = func.swift_name();
+                if !swift_name.is_empty() {
+                    out.write(" ");


Instead of this, just put the space in the string literal below? These two comments apply to the following block as well.

emilio · 2020-01-09T01:07:58Z

src/bindgen/utilities.rs

+            .attr_name_value_lookup("export_name")
+            .or_else(|| {
+                if self.is_no_mangle() {
+                    Some(self.sig.ident.to_string())


Does this really work for impl items? Like, does impl Foo { extern fn bar(&self) { ... } } really produce a bar symbol?

emilio · 2020-01-09T01:08:56Z

tests/rust/swift_name.rs

+  #[export_name="FooRef_doThing"]
+  pub extern fn do_thing(self: FooRef) -> i32 {
+    if let Some(nonnull) = std::ptr::NonNull::new(self.ptr) {
+      nonnull.as_ref().bar


Does this really compile as rust? I'm ~sure there's at least an unsafe block missing around.

Also four-space indent please?

emilio · 2020-01-09T01:10:23Z

tests/rust/swift_name.rs

+  }
+
+  #[export_name="FooRef_getBar"]
+  pub extern fn get_bar(self: FooRef) -> i32 {


So what happens when you use &self / &mut self / etc? What about self: Box<Self> or such?

Can we have tests for that? We should at least not generate broken code.

emilio · 2020-01-09T01:14:06Z

src/bindgen/ir/function.rs

    pub cfg: Option<Cfg>,
    pub annotations: AnnotationSet,
    pub documentation: Documentation,
+    pub attributes: Vec<String>,


This can be removed now.

emilio · 2020-01-09T01:15:44Z

src/bindgen/ir/function.rs

 #[derive(Debug, Clone)]
 pub struct Function {
    pub path: Path,
+    pub self_type_path: Option<Path>,


Please add a comment saying something like "If we're a method, this will be Some, and the path will be the path of the type in the impl block" or such.

It'd be nice to make something more similar to what associated constants do, but it may be overkill for this.

TODO (alaroldai) - figure out why functions are being generated for functions with generic receivers (e.g. `fn foo(bar: Box<Bar>)`). AFAIK such a function is not callable because the generated struct Box_Bar will be a forward declaration.

emilio

I pushed this with a couple follow-up commits. Thanks!

emilio · 2020-01-12T15:14:56Z

src/bindgen/parser.rs

            return;
        }

+        let loggable_item_name = {


This is a bit unfortunate in the sense that it runs this code unconditionally... But can improve in a follow-up.

emilio · 2020-01-12T15:56:45Z

Fixed by:

cf406e9 (this PR squashed)
688407c
b740343 (something I thought was worth doing, as the logging should happen in the uncommon case)

* Support 'swift_name' attributes on generated functions (#449) * Add [export.pre_body] to config (#452) * Handle new line in doc attribute (#454) * Add support for `Self` in tagged enums, structs and unions (#455, #455, #456) * Make sentinel variant respect regular config (#459) * Fix layout of tagged enums with size under some configurations (#463) * Add an option to allow configuring the order of function names in generated headers (#466) Thanks to all the awesome contributors.

232: Bump cbindgen from 0.12.2 to 0.13.0 r=MikailBag a=dependabot-preview[bot] Bumps [cbindgen](https://github.com/eqrion/cbindgen) from 0.12.2 to 0.13.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/eqrion/cbindgen/blob/master/CHANGES">cbindgen's changelog</a>.</em></p> <blockquote> <h2>0.13.0</h2> <pre><code> * Support 'swift_name' attributes on generated functions ([#449](mozilla/cbindgen#449)) * Add [export.pre_body] to config ([#452](mozilla/cbindgen#452)) * Handle new line in doc attribute ([#454](mozilla/cbindgen#454)) * Add support for `Self` in tagged enums, structs and unions ([#455](mozilla/cbindgen#455), [#455](mozilla/cbindgen#455), [#456](mozilla/cbindgen#456)) * Make sentinel variant respect regular config ([#459](mozilla/cbindgen#459)) * Fix layout of tagged enums with size under some configurations ([#463](mozilla/cbindgen#463)) * Add an option to allow configuring the order of function names in generated headers ([#466](mozilla/cbindgen#466)) </code></pre> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/eqrion/cbindgen/commit/5e667158a144e59c221327d12cfc9f7f1c0e744b"><code>5e66715</code></a> Release v0.13.0</li> <li><a href="https://github.com/eqrion/cbindgen/commit/723e690027209c8e613c729def6a9367e197b00f"><code>723e690</code></a> Add test cases for 'sort_by' option</li> <li><a href="https://github.com/eqrion/cbindgen/commit/f5edc2c2abf474df886975b8d83a7c46332ce352"><code>f5edc2c</code></a> Document usage of 'sort_by' option</li> <li><a href="https://github.com/eqrion/cbindgen/commit/2fb3e1d9c4ef83a3014c28f21771546a2485699b"><code>2fb3e1d</code></a> Add option to specify ordering of generated C function prototypes</li> <li><a href="https://github.com/eqrion/cbindgen/commit/4cb762ec8f24f8ef3e12fcd716326a1207a88018"><code>4cb762e</code></a> Do not emit <code>enum TagName tag</code> when a sized representation is present.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/80487484196026d2efd11131d58859e2da8beee4"><code>8048748</code></a> Add a test that demonstrates Style::Tag can emit structures with incorrect si...</li> <li><a href="https://github.com/eqrion/cbindgen/commit/12248c2fef57d596dfa9245bd5fec2eabe050002"><code>12248c2</code></a> tests: Add a simple testing-helpers headers that tests can include.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/bd831ded1945ebb28cac8560a570c5522c05b945"><code>bd831de</code></a> tests: Provide style definition to the C / C++ tests.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/ec18611e583ceda551d6e0a926eac95a69b78e44"><code>ec18611</code></a> bindings: Don't add stray newline in trailer.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/e0d4bc17a54fb38857d89bdc10d24bb9242131aa"><code>e0d4bc1</code></a> Make sentinel variant respect regular config</li> <li>Additional commits viewable in <a href="https://github.com/eqrion/cbindgen/compare/v0.12.2...v0.13.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=cbindgen&package-manager=cargo&previous-version=0.12.2&new-version=0.13.0)](https://dependabot.com/compatibility-score.html?dependency-name=cbindgen&package-manager=cargo&previous-version=0.12.2&new-version=0.13.0) You can trigger a rebase of this PR by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com): - Update frequency (including time of day and day of week) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

Bump cbindgen from 0.9.1 to 0.14.1 Bumps [cbindgen](https://github.com/eqrion/cbindgen) from 0.9.1 to 0.14.1. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/eqrion/cbindgen/blob/master/CHANGES">cbindgen's changelog</a>.</em></p> <blockquote> <h2>0.14.1</h2> <pre><code> * Handle mangling pointers. ([#508](mozilla/cbindgen#508)) * Unconditionally generate a return statement in partialeq implementations. ([#509](mozilla/cbindgen#509)) </code></pre> <h2>0.14.0</h2> <pre><code> * Minor tweak at how [export.exclude] is handled to allow excluding generic instantiations in C mode. ([#501](mozilla/cbindgen#501)) * Documented cpp_compat option. ([#496](mozilla/cbindgen#496)) * Fixed a panic when parsing associated constants for a built-in type. ([#494](mozilla/cbindgen#494)) </code></pre> <h2>0.13.2</h2> <pre><code> * Constants now have suitable documentation. ([#471](mozilla/cbindgen#471)) * Fixed some C warnings by emitting void when there are no arguments. ([#470](mozilla/cbindgen#470)) * Avoids reading cargo.toml when not needed, which can cause panics in workspace situations. * Only write `default` cases if the switch is not exhaustive. ([#475](mozilla/cbindgen#475)) * Some warnings have been refined. ([#477](mozilla/cbindgen#477)) * Code generation for static arrays has been fixed. ([#479](mozilla/cbindgen#479)) * Opt-in support for constexpr in constants. ([#481](mozilla/cbindgen#481)) * Fix C code generation and some warnings when extremely large constants are used. ([#490](mozilla/cbindgen#490)) * Proper escaping of enum variants and fields. ([#483](mozilla/cbindgen#483)) * Added support for RefCell (as an opaque type) and Cell. ([#489](mozilla/cbindgen#489)) </code></pre> <h2>0.13.1</h2> <pre><code> * Support `#[cfg]` on individual enum variants. ([#469](mozilla/cbindgen#469)) </code></pre> <h2>0.13.0</h2> <pre><code> * Support 'swift_name' attributes on generated functions ([#449](mozilla/cbindgen#449)) * Add [export.pre_body] to config ([#452](mozilla/cbindgen#452)) * Handle new line in doc attribute ([#454](mozilla/cbindgen#454)) * Add support for `Self` in tagged enums, structs and unions ([#455](mozilla/cbindgen#455), [#455](mozilla/cbindgen#455), [#456](mozilla/cbindgen#456)) * Make sentinel variant respect regular config ([#459](mozilla/cbindgen#459)) * Fix layout of tagged enums with size under some configurations ([#463](mozilla/cbindgen#463)) * Add an option to allow configuring the order of function names in generated headers ([#466](mozilla/cbindgen#466)) </code></pre> <h2>0.12.2</h2> <pre><code> * Fixed version detection with lockfile v2. mozilla/cbindgen#446 * Added support for export_name on functions. mozilla/cbindgen#447 </code></pre> <h2>0.12.1</h2> <pre><code> * Added support for #[repr*64)] on enums. mozilla/cbindgen#441 * Added support to generate plain enums instead of enum classes for C++. mozilla/cbindgen#443 * Fixed dependency resolution with lockfile v2. mozilla/cbindgen#438 </code></pre> </tr></table> ... (truncated) </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/eqrion/cbindgen/commit/0761b9bbe48d01ded1bbec45bbeea5544b3b1002"><code>0761b9b</code></a> Release 0.14.1</li> <li><a href="https://github.com/eqrion/cbindgen/commit/33d9ecf11f300bd03f431e9f3417c063e681380c"><code>33d9ecf</code></a> Handle mangling pointers.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/0fed9eebc47abcd31e0e69357cd39262c8a5a6cc"><code>0fed9ee</code></a> enum: Unconditionally generate a return statement in partialeq implementations.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/a519f1bda444a0c6c01464296a052d9a05e1d9c4"><code>a519f1b</code></a> Actually use the temp dir for temporary compiled objects.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/4a38a48937264d6ec162b2fd47ffd0e7a091ca72"><code>4a38a48</code></a> Release 0.14.0</li> <li><a href="https://github.com/eqrion/cbindgen/commit/39bae60ad7ac4711d8fccc0d098ecef98b37d559"><code>39bae60</code></a> Generate test object files in a temporary directory instead of in-tree</li> <li><a href="https://github.com/eqrion/cbindgen/commit/ec1631f3e47686e6e0c1b7641bb08b0e39c59250"><code>ec1631f</code></a> Allow excluding monomorph structs in C mode.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/6fd245096dcd5c50c1065b4bd6ce62a09df0b39b"><code>6fd2450</code></a> Add missing cpp_compat documentation.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/c265a7562a5ee815057efcc8b1aca31cd3d5318d"><code>c265a75</code></a> parser: Don't panic when finding associated constants to a primitive.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/17d7aad7d07dce8aa665aedbc75c39953afe1600"><code>17d7aad</code></a> Release v0.13.2</li> <li>Additional commits viewable in <a href="https://github.com/eqrion/cbindgen/compare/v0.9.1...v0.14.1">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=cbindgen&package-manager=cargo&previous-version=0.9.1&new-version=0.14.1)](https://dependabot.com/compatibility-score/?dependency-name=cbindgen&package-manager=cargo&previous-version=0.9.1&new-version=0.14.1) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com): - Update frequency (including time of day and day of week) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details>

alaroldai force-pushed the swift-name branch from d4eb372 to f2e3016 Compare January 8, 2020 16:54

emilio requested changes Jan 8, 2020

View reviewed changes

Support 'swift_name' attributes on generated functions

49b8ad5

Fixes mozilla#420

alaroldai force-pushed the swift-name branch from f2e3016 to 49b8ad5 Compare January 9, 2020 00:53

alaroldai commented Jan 9, 2020

View reviewed changes

emilio reviewed Jan 9, 2020

View reviewed changes

emilio approved these changes Jan 12, 2020

View reviewed changes

emilio closed this Jan 12, 2020

Support 'swift_name' attributes on generated functions #449

Support 'swift_name' attributes on generated functions #449

Uh oh!

Conversation

alaroldai commented Jan 8, 2020

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alaroldai left a comment

Choose a reason for hiding this comment

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio commented Jan 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants