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

Rust: Don't borrow borrowed types #676

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

rylev
Copy link
Contributor

@rylev rylev commented Sep 22, 2023

Fixes #673

This partially reverts a change now live in v0.12 that was introduced in #669. If a type has a lifetime associated with it, treat it as already borrowed and don't borrow again.

@rylev rylev changed the title Don't borrow borrowed types Rust: Don't borrow borrowed types Sep 22, 2023
@alexcrichton
Copy link
Member

Thinking through this a bit now, one consequence of this is that the generation of this:

package foo:bar

world x {
  resource z

  record x {
    y: list<u32>,
    z: borrow<z>,
  }

  import the-fn: func(x: x)
}

changes. Before this PR this generates:

pub struct X<'a> {
    pub y: wit_bindgen::rt::vec::Vec<u32>,
    pub z: &'a Z,
}

pub fn the_fn(x: &X<'_>) { ... }

whereas after this PR it generates

pub struct X<'a> {
    pub y: wit_bindgen::rt::vec::Vec<u32>,
    pub z: &'a Z,
}

pub fn the_fn(x: X<'_>) { ... }

that means that this PR requires ownership of the Vec field whereas previously it wasn't required, and I think this was one thing I was trying to fix implicitly in #673 as well.

@rylev
Copy link
Contributor Author

rylev commented Oct 5, 2023

@alexcrichton sorry for the delay in getting back to you.

I've updated the code I'm working on from wit-bindgen 11 to 12 so we can see what the code ends up looking like. I think it's hard to argue that the generated code here is more idiomatic. Any thoughts on how we might keep the implicitness you desire while reducing the amount of double/triple borrows seen in the code I've linked to?

@alexcrichton
Copy link
Member

Well hey if we've already got double and triple borrows may as well go for the quadruple and legendary quintuple borrow while we're at it!

But no my main concern here is the runtime implications of the generated signatures as opposed to feeling that the current bindings are idiomatic (those clearly aren't). I think we should probably go back to what was there prior, like with this PR, but there probably needs to be affordances for resources to handle the case above I mention. I'm a bit frustrated with how I can't seem to ever settle on a strategy of types generated here (it feels like constant whack-a-mole), but such is the experience of developing a bindings generator!

If you've run out of time to work on this I can also poke around and see if I can't restore the bindings from before too

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev force-pushed the no-borrow-borrows branch from 66bfffb to d503205 Compare October 6, 2023 08:06
@rylev
Copy link
Contributor Author

rylev commented Oct 6, 2023

@alexcrichton I've rebased - I believe this is what we've agreed upon (i.e., the original changes). Perhaps we can revisit this with more targeted improvements in the future?

@alexcrichton
Copy link
Member

Sounds good!

@alexcrichton alexcrichton merged commit 43cea9f into bytecodealliance:main Oct 6, 2023
9 checks passed
yamt added a commit to yamt/wasi-sensor-discussion that referenced this pull request Nov 30, 2023
i hoped this kind of wit changes didn't affect the API.
however, wit-bindgen rust has a bit complex rules (too complex in my taste)
to decide types, which alters the API like this.

cf. bytecodealliance/wit-bindgen#676
yamt added a commit to yamt/wasi-sensor-discussion that referenced this pull request Dec 6, 2023
i hoped this kind of wit changes didn't affect the API.
however, wit-bindgen rust has a bit complex rules (too complex in my taste)
to decide types, which alters the API like this.

cf. bytecodealliance/wit-bindgen#676
yamt added a commit to midokura/wasi-sensor-discussion that referenced this pull request Dec 6, 2023
i hoped this kind of wit changes didn't affect the API.
however, wit-bindgen rust has a bit complex rules (too complex in my taste)
to decide types, which alters the API like this.

cf. bytecodealliance/wit-bindgen#676
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.

wit-bindgen 0.12 Rust changes how explicitly borrowed param types are borrowed
2 participants