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.

Sign up for GitHub

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

Allow function aliases #218

Closed
wants to merge 7 commits into from
Closed

Allow function aliases #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5c91efe
Allow function aliases
Jun 11, 2020
8f7891c
Function overloading
Jun 12, 2020
f70508b
rust_name function attribute
Jun 12, 2020
2aceb7e
cxx_name function attribute
Jun 12, 2020
f81a2d7
rename parse function
Jun 12, 2020
fa9799b
attribute context validation
Jun 13, 2020
6d63840
Merge branch 'master' into new_branch
adetaylor Oct 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions gen/src/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,9 +516,10 @@ fn write_cxx_function_shim(
writeln!(out, ") noexcept {{");
write!(out, " ");
write_return_type(out, &efn.ret);
let ident = if let Some(alias) = &efn.alias { alias } else { &efn.ident };
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like this implementation of functions having an "ident" and an optional "alias", and especially counting on each use site to remember whether they should be checking for presence of the alias.

In my mind every function should have a non-optional rust name and non-optional c++ name. That's how we should store them post-parsing. Every place which uses of a function name will pick which of the two they want.

match &efn.receiver {
None => write!(out, "(*{}$)(", efn.ident),
Some(receiver) => write!(out, "({}::*{}$)(", receiver.ty, efn.ident),
None => write!(out, "(*{}$)(", ident),
Some(receiver) => write!(out, "({}::*{}$)(", receiver.ty, ident),
}
for (i, arg) in efn.args.iter().enumerate() {
if i > 0 {
Expand Down Expand Up @@ -562,8 +563,8 @@ fn write_cxx_function_shim(
_ => {}
}
match &efn.receiver {
None => write!(out, "{}$(", efn.ident),
Some(_) => write!(out, "(self.*{}$)(", efn.ident),
None => write!(out, "{}$(", ident),
Some(_) => write!(out, "(self.*{}$)(", ident),
}
for (i, arg) in efn.args.iter().enumerate() {
if i > 0 {
Expand Down Expand Up @@ -696,9 +697,10 @@ fn write_rust_function_shim(out: &mut OutFile, efn: &ExternFn, types: &Types) {
for line in efn.doc.to_string().lines() {
writeln!(out, "//{}", line);
}
let ident = if let Some(alias) = &efn.alias { alias } else { &efn.ident };
let local_name = match &efn.sig.receiver {
None => efn.ident.to_string(),
Some(receiver) => format!("{}::{}", receiver.ty, efn.ident),
None => ident.to_string(),
Some(receiver) => format!("{}::{}", receiver.ty, ident),
};
let invoke = mangle::extern_fn(&out.namespace, efn);
let indirect_call = false;
Expand Down
7 changes: 4 additions & 3 deletions macro/src/expand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ fn expand_cxx_type(namespace: &Namespace, ety: &ExternType) -> TokenStream {
}

fn expand_cxx_function_decl(namespace: &Namespace, efn: &ExternFn, types: &Types) -> TokenStream {
let ident = &efn.ident;
let ident = if let Some(alias) = &efn.alias { alias } else { &efn.ident };
let receiver = efn.receiver.iter().map(|receiver| {
let receiver_type = receiver.ty();
quote!(_: #receiver_type)
Expand Down Expand Up @@ -245,7 +245,7 @@ fn expand_cxx_function_decl(namespace: &Namespace, efn: &ExternFn, types: &Types
}

fn expand_cxx_function_shim(namespace: &Namespace, efn: &ExternFn, types: &Types) -> TokenStream {
let ident = &efn.ident;
let ident = if let Some(alias) = &efn.alias { alias } else { &efn.ident };
let doc = &efn.doc;
let decl = expand_cxx_function_decl(namespace, efn, types);
let receiver = efn.receiver.iter().map(|receiver| {
Expand Down Expand Up @@ -454,7 +454,8 @@ fn expand_function_pointer_trampoline(
let c_trampoline = mangle::c_trampoline(namespace, efn, var);
let r_trampoline = mangle::r_trampoline(namespace, efn, var);
let local_name = parse_quote!(__);
let catch_unwind_label = format!("::{}::{}", efn.ident, var);
let ident = if let Some(alias) = &efn.alias { alias } else { &efn.ident };
let catch_unwind_label = format!("::{}::{}", ident, var);
let shim = expand_rust_function_shim_impl(
sig,
types,
Expand Down
20 changes: 19 additions & 1 deletion syntax/attrs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub struct Parser<'a> {
pub doc: Option<&'a mut Doc>,
pub derives: Option<&'a mut Vec<Derive>>,
pub repr: Option<&'a mut Option<Atom>>,
pub alias: Option<&'a mut Option<(Ident, bool)>>,
Copy link
Owner

Choose a reason for hiding this comment

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

This needs a comment indicating what the bool means, or use an enum with two variants instead.

}

pub(super) fn parse_doc(cx: &mut Errors, attrs: &[Attribute]) -> Doc {
Expand Down Expand Up @@ -57,11 +58,28 @@ pub(super) fn parse(cx: &mut Errors, attrs: &[Attribute], mut parser: Parser) {
}
Err(err) => return cx.push(err),
}
} else if attr.path.is_ident("rust_name") || attr.path.is_ident("cxx_name") {
match parse_function_alias_attribute.parse2(attr.tokens.clone()) {
Ok(alias) => {
if let Some(a) = &mut parser.alias {
**a = Some((alias, attr.path.is_ident("rust_name")));
continue;
}
}
Err(err) => return cx.push(err),
}
} else {
return cx.error(attr, "unsupported attribute");
}
return cx.error(attr, "unsupported attribute");
Comment on lines +71 to -61
Copy link
Owner

Choose a reason for hiding this comment

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

This was correct before the PR. The change made in the PR makes well-formed attributes silently accepted anywhere where not expected, like putting a #[derive()] attribute on a function or a #[repr()] attribute on a function.

}
}

fn parse_function_alias_attribute(input: ParseStream) -> Result<Ident> {
input.parse::<Token![=]>()?;
let lit: LitStr = input.parse()?;
Ok(lit.parse::<Ident>()?)
}

fn parse_doc_attribute(input: ParseStream) -> Result<LitStr> {
input.parse::<Token![=]>()?;
let lit: LitStr = input.parse()?;
Expand Down
3 changes: 2 additions & 1 deletion syntax/ident.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ pub(crate) fn check_all(cx: &mut Check, namespace: &Namespace, apis: &[Api]) {
check(cx, &ety.ident);
}
Api::CxxFunction(efn) | Api::RustFunction(efn) => {
check(cx, &efn.ident);
let ident = if let Some(alias) = &efn.alias { alias } else { &efn.ident };
check(cx, ident);
for arg in &efn.args {
check(cx, &arg.ident);
}
Expand Down
5 changes: 3 additions & 2 deletions syntax/mangle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ macro_rules! join {
}

pub fn extern_fn(namespace: &Namespace, efn: &ExternFn) -> Symbol {
let ident = if let Some(alias) = &efn.alias { alias } else { &efn.ident };
match &efn.receiver {
Some(receiver) => join!(namespace, CXXBRIDGE, receiver.ty, efn.ident),
None => join!(namespace, CXXBRIDGE, efn.ident),
Some(receiver) => join!(namespace, CXXBRIDGE, receiver.ty, ident),
None => join!(namespace, CXXBRIDGE, ident),
}
}

Expand Down
1 change: 1 addition & 0 deletions syntax/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ pub struct ExternFn {
pub lang: Lang,
pub doc: Doc,
pub ident: Ident,
pub alias: Option<Ident>,
pub sig: Signature,
pub semi_token: Token![;],
}
Expand Down
15 changes: 15 additions & 0 deletions syntax/parse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,20 @@ fn parse_extern_fn(cx: &mut Errors, foreign_fn: &ForeignItemFn, lang: Lang) -> R
let unsafety = foreign_fn.sig.unsafety;
let fn_token = foreign_fn.sig.fn_token;
let ident = foreign_fn.sig.ident.clone();
let mut attr = None;
attrs::parse(cx, &foreign_fn.attrs, attrs::Parser { alias: Some(&mut attr), ..Default::default() });
let mut alias = None;
if let Some((ident, is_rust)) = attr {
match lang {
Lang::Cxx if !is_rust => {
return Err(Error::new_spanned(foreign_fn, "C/C++ functions with 'cxx_name' are not allowed"));
},
Lang::Rust if is_rust => {
return Err(Error::new_spanned(foreign_fn, "Rust functions with 'rust_name' are not allowed"));
},
Comment on lines +368 to +373
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is overly restrictive. This would make people need to remember which attribute is legal on which kind of function, and it's not needed -- any function should be able to have either attribute. We'll use the function's "real" name as the one that isn't specified by an attribute.

_ => alias = Some(ident),
}
}
let paren_token = foreign_fn.sig.paren_token;
let semi_token = foreign_fn.semi_token;
let api_function = match lang {
Expand All @@ -371,6 +385,7 @@ fn parse_extern_fn(cx: &mut Errors, foreign_fn: &ForeignItemFn, lang: Lang) -> R
lang,
doc,
ident,
alias,
sig: Signature {
unsafety,
fn_token,
Expand Down
2 changes: 1 addition & 1 deletion syntax/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ impl<'a> Types<'a> {
rust.insert(ident);
}
Api::CxxFunction(efn) | Api::RustFunction(efn) => {
let ident = &efn.ident;
let ident = if let Some(alias) = &efn.alias { alias } else { &efn.ident };
if !function_names.insert((&efn.receiver, ident)) {
duplicate_name(cx, efn, ident);
}
Comment on lines +119 to 122
Copy link
Owner

Choose a reason for hiding this comment

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

As implemented in this PR, the alias of an extern Rust function is its C++ name and the alias of an extern C++ function is its Rust name. So this check isn't correct:

  • It is going to have false positives on extern Rust functions having the same C++ name, which we should allow because C++ has function overloading.
  • It is going to have false negatives on an extern C++ function with alias f colliding with an extern Rust function named f if the Rust one also has an alias.
  • It is going to have false positives on the Rust name of a C++ function being the same as the C++ name of a Rust function, which should in general have no bearing on each other.

Expand Down
16 changes: 16 additions & 0 deletions tests/ffi/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,15 @@ pub mod ffi {
fn set2(&mut self, n: usize) -> usize;
fn set_succeed(&mut self, n: usize) -> Result<usize>;
fn get_fail(&mut self) -> Result<usize>;

Copy link
Owner

Choose a reason for hiding this comment

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

Remove trailing whitespace. (Elsewhere as well)

Suggested change

#[rust_name = "i32_overloaded_method"]
fn cOverloadedMethod(&self, x: i32) -> String;
#[rust_name = "str_overloaded_method"]
fn cOverloadedMethod(&self, x: &str) -> String;
#[rust_name = "i32_overloaded_function"]
fn cOverloadedFunction(x: i32) -> String;
#[rust_name = "str_overloaded_function"]
fn cOverloadedFunction(x: &str) -> String;
}

extern "C" {
Expand Down Expand Up @@ -160,6 +169,9 @@ pub mod ffi {
fn r_return_r2(n: usize) -> Box<R2>;
fn get(self: &R2) -> usize;
fn set(self: &mut R2, n: usize) -> usize;

#[cxx_name = "rAliasedFunction"]
fn r_aliased_function(x: i32) -> String;
}
}

Expand Down Expand Up @@ -358,3 +370,7 @@ fn r_fail_return_primitive() -> Result<usize, Error> {
fn r_return_r2(n: usize) -> Box<R2> {
Box::new(R2(n))
}

fn r_aliased_function(x: i32) -> String {
x.to_string()
}
18 changes: 18 additions & 0 deletions tests/ffi/tests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,22 @@ extern "C" std::string *cxx_test_suite_get_unique_ptr_string() noexcept {
return std::unique_ptr<std::string>(new std::string("2020")).release();
}

rust::String C::cOverloadedMethod(int32_t x) const {
return rust::String(std::to_string(x));
}

rust::String C::cOverloadedMethod(rust::Str x) const {
return rust::String(std::string(x));
}

rust::String cOverloadedFunction(int x) {
return rust::String(std::to_string(x));
}

rust::String cOverloadedFunction(rust::Str x) {
return rust::String(std::string(x));
}

extern "C" const char *cxx_run_test() noexcept {
#define STRINGIFY(x) #x
#define TOSTRING(x) STRINGIFY(x)
Expand Down Expand Up @@ -420,6 +436,8 @@ extern "C" const char *cxx_run_test() noexcept {
ASSERT(r2->get() == 2021);
ASSERT(r2->set(2020) == 2020);
ASSERT(r2->get() == 2020);

ASSERT(std::string(rAliasedFunction(2020)) == "2020");

cxx_test_suite_set_correct();
return nullptr;
Expand Down
5 changes: 5 additions & 0 deletions tests/ffi/tests.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ class C {
size_t get_fail();
const std::vector<uint8_t> &get_v() const;
std::vector<uint8_t> &get_v();
rust::String cOverloadedMethod(int32_t x) const;
rust::String cOverloadedMethod(rust::Str x) const;

private:
size_t n;
Expand Down Expand Up @@ -101,4 +103,7 @@ rust::Vec<uint8_t> c_try_return_rust_vec();
rust::Vec<rust::String> c_try_return_rust_vec_string();
const rust::Vec<uint8_t> &c_try_return_ref_rust_vec(const C &c);

rust::String cOverloadedFunction(int32_t x);
rust::String cOverloadedFunction(rust::Str x);

} // namespace tests
9 changes: 9 additions & 0 deletions tests/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,3 +181,12 @@ extern "C" fn cxx_test_suite_get_box() -> *mut cxx_test_suite::R {
unsafe extern "C" fn cxx_test_suite_r_is_correct(r: *const cxx_test_suite::R) -> bool {
*r == 2020
}

#[test]
fn test_rust_name_attribute() {
assert_eq!("2020".to_string(), ffi::i32_overloaded_function(2020));
assert_eq!("2020".to_string(), ffi::str_overloaded_function("2020"));
let unique_ptr = ffi::c_return_unique_ptr();
assert_eq!("2020".to_string(), unique_ptr.i32_overloaded_method(2020));
assert_eq!("2020".to_string(), unique_ptr.str_overloaded_method("2020"));
}