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

Fix taking owned resource handles in Rust imports #669

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion crates/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ pub struct TypeInfo {

/// Whether or not this type (transitively) has a borrow handle.
pub has_borrow_handle: bool,

/// Whether or not this type (transitively) has an own handle.
pub has_own_handle: bool,
}

impl std::ops::BitOrAssign for TypeInfo {
Expand All @@ -52,6 +55,7 @@ impl std::ops::BitOrAssign for TypeInfo {
self.has_list |= rhs.has_list;
self.has_resource |= rhs.has_resource;
self.has_borrow_handle |= rhs.has_borrow_handle;
self.has_own_handle |= rhs.has_own_handle;
}
}

Expand Down Expand Up @@ -164,7 +168,10 @@ impl Types {
info.has_resource = true;
}
TypeDefKind::Handle(handle) => {
info.has_borrow_handle = matches!(handle, Handle::Borrow(_));
match handle {
Handle::Borrow(_) => info.has_borrow_handle = true,
Handle::Own(_) => info.has_own_handle = true,
}
info.has_resource = true;
}
TypeDefKind::Tuple(t) => {
Expand Down
1 change: 1 addition & 0 deletions crates/go/tests/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ macro_rules! codegen_test {
(resource_borrow_in_record $name:tt $test:tt) => {};
(resource_borrow_in_record_export $name:tt $test:tt) => {};
(resources_in_aggregates $name:tt $test:tt) => {};
(issue668 $name:tt $test:tt) => {};

($id:ident $name:tt $test:tt) => {
#[test]
Expand Down
23 changes: 16 additions & 7 deletions crates/rust-lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,12 +305,21 @@ pub trait RustGenerator<'a> {
let lt = self.lifetime_for(&info, mode);
let ty = &self.resolve().types[id];
if ty.name.is_some() {
// If this type has a list internally, no lifetime is being printed,
// but we're in a borrowed mode, then that means we're in a borrowed
// context and don't want ownership of the type but we're using an
// owned type definition. Inject a `&` in front to indicate that, at
// the API level, ownership isn't required.
if (info.has_list || info.has_borrow_handle) && lt.is_none() {
// If `mode` is borrowed then that means literal ownership of the
// input type is not necessarily required. In this situation we
// ideally want to put a `&` in front to statically indicate this.
// That's not required in all situations however and is only really
// critical for lists which otherwise would transfer ownership of
// the allocation to this function.
//
// Note, though, that if the type has an `own<T>` inside of it then
// it is actually required that we take ownership since Rust is
// losing access to those handles.
//
// Check here if the type has the right shape and if we're in the
// right mode, and if those conditions are met a lifetime is
// printed.
if info.has_list && !info.has_own_handle {
if let TypeMode::AllBorrowed(lt)
| TypeMode::LeafBorrowed(lt)
| TypeMode::HandlesBorrowed(lt) = mode
Expand Down Expand Up @@ -1032,7 +1041,7 @@ pub trait RustGenerator<'a> {
TypeMode::AllBorrowed(s) | TypeMode::LeafBorrowed(s) | TypeMode::HandlesBorrowed(s) => {
s
}
_ => return None,
TypeMode::Owned => return None,
};
if info.has_borrow_handle {
return Some(lt);
Expand Down
1 change: 1 addition & 0 deletions crates/teavm-java/tests/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ macro_rules! codegen_test {
(resource_borrow_in_record_export $name:tt $test:tt) => {};
(same_names5 $name:tt $test:tt) => {};
(resources_in_aggregates $name:tt $test:tt) => {};
(issue668 $name:tt $test:tt) => {};

($id:ident $name:tt $test:tt) => {
#[test]
Expand Down
20 changes: 20 additions & 0 deletions tests/codegen/issue668.wit
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package test:test

interface test-import {
resource resource-a {
constructor(id: u32)
}

record record-a {
resource-a: resource-a,
resources: list<resource-a>,
}

resource resource-b {
make: static func(record-a: record-a)
}
}

world test-world {
import test-import
}
4 changes: 2 additions & 2 deletions tests/runtime/ownership/borrowing-duplicate-if-necessary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ impl Guest for Exports {
lists::foo(value)
);

thing_in::bar(thing_in::Thing {
thing_in::bar(&thing_in::Thing {
name: "thing 1",
value: &["some value", "another value"],
});
Expand All @@ -38,7 +38,7 @@ impl Guest for Exports {
name: "thing 1".to_owned(),
value: vec!["some value".to_owned(), "another value".to_owned()],
},
thing_in_and_out::baz(value)
thing_in_and_out::baz(&value)
);
}
}
2 changes: 1 addition & 1 deletion tests/runtime/ownership/borrowing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ impl Guest for Exports {
lists::foo(value)
);

thing_in::bar(thing_in::Thing {
thing_in::bar(&thing_in::Thing {
name: "thing 1",
value: &["some value", "another value"],
});
Expand Down