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

Support calling C++ and Rust methods #121

Merged
merged 5 commits into from
Apr 17, 2020
Merged

Conversation

jgalenson
Copy link
Contributor

This adds support for calling C++ and Rust methods with the syntax suggested in #51:

fn get_name(self: &ThingC) -> &CxxString;
fn print(self: &ThingR);

It requires Rust 1.43 to use this syntax.

These methods can be declared in the bridge by naming the first
argument self and making it a reference to the containing class, e.g.,
  fn get(self: &C) -> usize;
  fn set(self: &mut C, n: usize);
This syntax requires Rust 1.43.

Note that the implementation also changes the internal naming of shim
functions so that they also contain the name of the owning class, if
any.  This allows defining multiple methods with the same name on
different objects.
These methods can be declared in the bridge by naming the first
argument self and making it a reference to the containing class, e.g.,
  fn get(self: &R) -> usize;
  fn set(self: &mut R, n: usize);
This syntax requires Rust 1.43.
@dtolnay
Copy link
Owner

dtolnay commented Apr 17, 2020

Thanks, this is great! I'll get a chance to review the implementation tomorrow.

If someone needs method support on 1.42 and isn't satisfied waiting for the 1.43 release, it would be possible for us to accept this as a synonym for self. But I would lean toward sticking with just self until someone asks for it.

@myronahn
Copy link
Contributor

This is great, I can really use this.

gen/write.rs Outdated
Comment on lines 328 to 336
writeln!(out, "struct {} final {{", ety.ident);
for method in methods {
write!(out, " ");
let sig = &method.sig;
let local_name = method.ident.to_string();
write_rust_function_shim_decl(out, &local_name, sig, None, false);
writeln!(out, ";");
}
writeln!(out, "}};");
Copy link
Owner

@dtolnay dtolnay Apr 17, 2020

Choose a reason for hiding this comment

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

We need a private or deleted default constructor and copy constructor in here. Otherwise this allows C++ to obtain an opaque Rust type by value and call methods on it, which is unsound.

Specifically the R2 struct from tests/ffi/lib.rs currently turns into:

struct R2 final {
  size_t get() noexcept;
  size_t set(size_t n) noexcept;
};

and this allows C++ to write R2{}.get() which becomes UB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that!

@dtolnay dtolnay marked this pull request as ready for review April 17, 2020 23:43
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.

Thanks, this looks good to me. I'm not sure if it's marked Draft because you are still working on it but feel free to send follow-up PRs as needed.

One note: I am going to remove the changes from demo-rs and the readme, since I'd like to keep that first intro code snippet as focused as possible on the most important concepts. We'll need to figure out somewhere else to put exhaustive documentation of the full feature set (namespace is another; #86) rather than frontloading everything in that first snippet.

@dtolnay dtolnay merged commit af74d66 into dtolnay:master Apr 17, 2020
@jgalenson
Copy link
Contributor Author

I marked it as a draft because I thought we wanted to wait until Rust 1.43 was released, but it looks like you dealt with that as a follow-up. Thanks!

@jgalenson jgalenson deleted the methods branch April 18, 2020 00:06
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.

3 participants