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

Allow using 'static lifetimes in functions #3856

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

daxpedda
Copy link
Collaborator

@daxpedda daxpedda commented Feb 25, 2024

Currently #[wasm_bindgen] disallows any kind of lifetime on functions.
This change allows it to accept 'static.

Fixes #1187.

@daxpedda daxpedda force-pushed the static-lifetime branch 2 times, most recently from f2e1ceb to 97fe5df Compare February 25, 2024 20:11
@Liamolucko
Copy link
Collaborator

This applies to arguments too, where using 'static isn't okay. It's still sound because the way references work internally is that #[wasm_bindgen] generates a local variable and takes a reference to it, so you just get a borrow checker error, but it's a worse error than you'd have gotten before:

use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub fn invalid(_: &'static str) {}
error[E0597]: `*arg0` does not live long enough
 --> src/lib.rs:3:1
  |
3 | #[wasm_bindgen]
  | ^^^^^^^^^^^^^^-
  | |             |
  | |             `*arg0` dropped here while still borrowed
  | borrowed value does not live long enough
  | argument requires that `*arg0` is borrowed for `'static`
  |
  = note: this error originates in the attribute macro `wasm_bindgen` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0597`.
error: could not compile `wasm-bindgen-playground` (lib) due to 1 previous error

LGTM if you're okay with that though.

@Liamolucko
Copy link
Collaborator

Also, this fixes #1187 (the thing it was originally opened for anyway, not the struct use case someone brought up later).

Co-Authored-By: Liam Murphy <43807659+Liamolucko@users.noreply.github.com>
@daxpedda
Copy link
Collaborator Author

LGTM if you're okay with that though.

I'm fine with that considering its the same error message you already get when doing:

#[wasm_bindgen]
pub fn invalid(_: &str) {}

What kind of references are actually allowed in exported functions? Otherwise I could go ahead and outright ban them.

@daxpedda daxpedda merged commit b9ccb8f into rustwasm:main Feb 27, 2024
12 checks passed
@Liamolucko
Copy link
Collaborator

I'm fine with that considering its the same error message you already get when doing:

#[wasm_bindgen]
pub fn invalid(_: &str) {}

What do you mean? That example compiles just fine.

What kind of references are actually allowed in exported functions? Otherwise I could go ahead and outright ban them.

Up until now, just the kind above, and I assumed the whole point of this PR was to allow ones like this:

#[wasm_bindgen]
pub fn foo() -> &'static str {
    "hello world"
}

I didn't test it thoroughly enough and that actually still doesn't compile because of an extra error ('cannot return a borrowed ref with #[wasm_bindgen]'), but it now works if you wrap it in something:

#[wasm_bindgen]
pub fn foo() -> Option<&'static str> {
    Some("hello world")
}

That's also the use-case #1187 was asking for (although it should probably be reopened now that I've realised this doesn't fully fix it).

@daxpedda
Copy link
Collaborator Author

I'm fine with that considering its the same error message you already get when doing:

#[wasm_bindgen]
pub fn invalid(_: &str) {}

What do you mean? That example compiles just fine.

Yep, my mistake.

Up until now, just the kind above, and I assumed the whole point of this PR was to allow ones like this:

#[wasm_bindgen]
pub fn foo() -> &'static str {
    "hello world"
}

Sorry, should have probably mentioned this in OP. My problem was just lifetimes in pointers as parameters, e.g. par1: *const FnOnce(&'static u32). I was originally considering removing the whole lifetime check, but I wasn't really sure what other issues this was addressing, so just enabling 'static seemed safe to me.

That's also the use-case #1187 was asking for (although it should probably be reopened now that I've realised this doesn't fully fix it).

I've re-opened it.


My PR's have been pretty sloppy lately, so thank you for your diligent and careful reviews!

daxpedda added a commit to daxpedda/wasm-bindgen that referenced this pull request Mar 2, 2024
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.

Not sound to use lifetimes in function signatures
2 participants