Construct external Rust types with named fields #589

jbdutton · 2023-07-27T21:59:34Z

This pull request allows Rune scripts to construct Rust types with named fields, like so:

#[derive(Default, Debug, Any, PartialEq, Eq)]
#[rune(constructor)]
struct External {
    #[rune(get, set)]
    suite_name: String,
    #[rune(get, set)]
    room_number: usize,
}

pub fn main() {
  let external = External {
      suite_name: "Fowler",
      room_number: 1300,
  };

  external
}

I've tested this with structs and it works as expected. It should also work with enum variants that have named fields, though I haven't tested that yet.

It does this by creating a closure during the Any macro expansion, and installing that as a constructor function associated with the type:

|suite_name: String, room_number: usize| {
    External { suite_name, room_number }
}

By invoking the constructor function, the VM is placing the External type onto the stack as a Value::Any. This means you can successfully extract the typed object using the FromValue trait:

let output = vm.call(["main"], ())?;
let output: External = rune::from_value(output)?;
println!("{:?}", output);

Right now, this relies on the caller to assign the struct fields in the correct order. Otherwise the call to the closure fails due to mismatched types. There are probably a few ways to fix that, including making the closure take a single Object argument and extracting the fields from it.

This probably also doesn't work with nested structs or generics, at least for the moment.

Fixes #400.

crates/rune-macros/src/any.rs

crates/rune/src/module.rs

udoprog

Very nice, thanks for starting the work towards external struct construction!

Everything looks pretty good to me, apart that we need to deal with re-ordering. Since we want to maintain the evaluation order we unfortunately can't just re-order field value construction (which would've been nice). So we'll have to cope by swapping or copying values around for now.

I have some thoughts about changing the underlying instructions so that each has a target address they're being assembled into. It would simplify construction but is further down the line.

udoprog

Change looks nice, there's possibly a stack offset issue during assembly I'd like you to test for and possibly fix.

udoprog · 2023-07-30T14:25:15Z

crates/rune/src/compile/v1/assemble.rs

+            cx.asm.push(
+                Inst::Swap {
+                    a: stack_position,
+                    b: desired_position,


I think these will have to be offset from the base of stack construction since the stack might contain other data before this point? That is, based of cx.scopes.total(&span) before the assignments above are constructed.

Yeah, I was going to ask you about this. Currently this is using stack_bottom.checked_add to ensure the position is inside the call frame. But a lot of your stack operations are relative to the top, so I wasn't sure if you preferred Inst::Swap to also be relative to the top for consistency.

Relative to the stack frame seems fine and easy to reason about. Unless you can come up with some other reason.

crates/rune/src/compile/v1/assemble.rs

udoprog · 2023-07-30T14:30:52Z

examples/examples/external_struct.rs

+
+    let mut sources = rune::sources! {
+        entry => {
+            pub fn main() {


Add some padding in this test, like a few variable declarations before you construct the struct and it should produce the error which requires adding a stack offset (see above).

I was planning to add a true test (or tests) to exercise the new functionality, and leave the example focused on demonstrating the basics. Will push something up momentarily.

udoprog · 2023-07-30T14:31:42Z

examples/examples/external_struct.rs

+
+    let output = vm.call(["main"], ())?;
+    let output: External = rune::from_value(output)?;
+    println!("{:?}", output);


Add an assert_eq! here which checks that the constructed object has the expected values.

jbdutton · 2023-07-30T17:30:00Z

crates/rune/src/runtime/stack.rs

+        let a = self.stack.len().checked_sub(a).ok_or(StackError)?;
+        let b = self.stack.len().checked_sub(b).ok_or(StackError)?;
+        self.stack.swap(a, b);


This is using offsets from the top of the stack now. Should it check that the computed offsets don't exceed stack_bottom?

Yeah, that's necessary to ensure stack isolation between function calls, which could otherwise pose a security risk in case there are any miscompilations. Note that you get that for free if you checked_add from stack_bottom instead (relative to the stack frame), all though you still need to check that neither exceeds the size of the stack to avoid the panic when out of bounds during slice::swap.

Ah ok, I misunderstood your previous comment. Unfortunately I'm out of pocket the next few days. If you have time, feel free to push a fix and merge everything. Thanks for all the feedback!

Ok, thanks for your work and giving me the heads up!

jbdutton · 2023-07-30T17:50:04Z

crates/rune/src/compile/v1/assemble.rs

@@ -1976,6 +1976,10 @@ fn expr_object<'hir>(
        hir::ExprObjectKind::StructVariant { hash } => {
            cx.asm.push(Inst::StructVariant { hash, slot }, span);
        }
+        hir::ExprObjectKind::Constructor { hash, args } => {


This isn't a great name, IMO. Any suggestions? Maybe hir::ExprObjectKind::ExternalType, or something along those lines?

Including "External" seems good, so 👍. Not important to be perfect since it's an internal variant and can easily be renamed.

…ructs

register constructor

5d7fe00

jbdutton changed the title ~~Construct external structs~~ Construct external Rust types with named fields Jul 27, 2023

jbdutton commented Jul 27, 2023

View reviewed changes

crates/rune-macros/src/any.rs Show resolved Hide resolved

jbdutton commented Jul 27, 2023

View reviewed changes

crates/rune/src/module.rs Show resolved Hide resolved

jbdutton mentioned this pull request Jul 27, 2023

Can't construct struct with named field #400

Closed

udoprog reviewed Jul 29, 2023

View reviewed changes

udoprog added the enhancement New feature or request label Jul 29, 2023

Joshua Dutton added 2 commits July 30, 2023 10:07

call constructor when available

47c47a9

reorder fields according to type definition

4fde80e

jbdutton force-pushed the construct-external-structs branch from 3698d71 to 4fde80e Compare July 30, 2023 14:07

only reorder before constructor function

f55572a

udoprog requested changes Jul 30, 2023

View reviewed changes

Josh Dutton added 3 commits July 30, 2023 10:38

avoid non const error path

f394862

use offset from top of stack

de7a594

push var on to stack

bcf6baa

jbdutton commented Jul 30, 2023

View reviewed changes

Josh Dutton added 3 commits July 30, 2023 13:31

assert against whole response

0d5e20d

revert dbg statement

960eb50

test nested struct

86ee513

jbdutton marked this pull request as ready for review July 30, 2023 17:43

jbdutton commented Jul 30, 2023

View reviewed changes

Josh Dutton and others added 3 commits July 30, 2023 14:34

rename to external type

4f73981

Merge remote-tracking branch 'origin/main' into construct-external-st…

5902737

…ructs

Swap assignments based on the current stack frame

b8ac03b

udoprog approved these changes Jul 30, 2023

View reviewed changes

udoprog merged commit 35dded3 into rune-rs:main Jul 30, 2023

jbdutton deleted the construct-external-structs branch August 8, 2023 16:45

chenrui333 mentioned this pull request Oct 10, 2023

rune 0.13.1 Homebrew/homebrew-core#150407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Construct external Rust types with named fields #589

Construct external Rust types with named fields #589

jbdutton commented Jul 27, 2023 •

edited

Loading

udoprog left a comment •

edited

Loading

udoprog left a comment

udoprog Jul 30, 2023

jbdutton Jul 30, 2023

udoprog Jul 30, 2023

udoprog Jul 30, 2023

jbdutton Jul 30, 2023

udoprog Jul 30, 2023

jbdutton Jul 30, 2023

udoprog Jul 30, 2023 •

edited

Loading

jbdutton Jul 30, 2023

udoprog Jul 30, 2023 •

edited

Loading

jbdutton Jul 30, 2023

udoprog Jul 30, 2023

Construct external Rust types with named fields #589

Construct external Rust types with named fields #589

Conversation

jbdutton commented Jul 27, 2023 • edited Loading

udoprog left a comment • edited Loading

Choose a reason for hiding this comment

udoprog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udoprog Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udoprog Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbdutton commented Jul 27, 2023 •

edited

Loading

udoprog left a comment •

edited

Loading

udoprog Jul 30, 2023 •

edited

Loading

udoprog Jul 30, 2023 •

edited

Loading