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

[edition 2024] missing_unsafe_on_extern in macros with older editions #132425

Closed
daxpedda opened this issue Oct 31, 2024 · 7 comments
Closed

[edition 2024] missing_unsafe_on_extern in macros with older editions #132425

daxpedda opened this issue Oct 31, 2024 · 7 comments
Labels
A-edition-2024 Area: The 2024 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. F-unsafe_extern_blocks `#![feature(unsafe_extern_blocks)]` L-missing_unsafe_on_extern Lint: missing_unsafe_on_extern

Comments

@daxpedda
Copy link
Contributor

I tried this code:

use wasm_bindgen::prelude::*;

#[wasm_bindgen]
unsafe extern {
    pub fn test();
}

I expected to see this happen: it should compile.
Instead, this happened: it fails to compile.

Meta

rustc --version --verbose:

rustc 1.84.0-nightly (1e4f10ba6 2024-10-29)
binary: rustc
commit-hash: 1e4f10ba6476e48a42a79b9f846a2d9366525b9e
commit-date: 2024-10-29
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.1
Compile Error

error: extern blocks must be unsafe
  --> src/main.rs:5:12
  |
5 |     pub fn test();
  |            ^^^^


Basically the problem is that the #[wasm-bindgen] proc-macro generates extern "C" blocks which aren't prefixed with the unsafe keyword.
However, my understanding is that proc-macros should carry the edition from the crate they are coming from, which in this case is edition 2021. The rules of edition 2024 should not be applied to the generated code from this edition 2021 proc-macro.

That said, this is easily fixable in wasm-bindgen, but the bug will probably break other libraries as well.

Cc rustwasm/wasm-bindgen#4218.

@daxpedda daxpedda added the C-bug Category: This is a bug. label Oct 31, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 31, 2024
@ehuss ehuss added A-edition-2024 Area: The 2024 edition F-unsafe_extern_blocks `#![feature(unsafe_extern_blocks)]` labels Oct 31, 2024
@ehuss
Copy link
Contributor

ehuss commented Oct 31, 2024

cc @spastorino Do you by chance know what is happening? Is it possible that wasm-bindgen is using the wrong hygiene?

Or maybe the validation is looking at a span injected from the user's code?

@spastorino
Copy link
Member

spastorino commented Nov 4, 2024

Just gave it a very quick look ... this error is emitted in https://github.com/spastorino/rust/blob/2dece5bb62f234f5622a08289c5a3d1555cd7843/compiler/rustc_ast_passes/src/ast_validation.rs#L964-L977.
The if part is using span.at_least_rust_2024 https://github.com/spastorino/rust/blob/2dece5bb62f234f5622a08289c5a3d1555cd7843/compiler/rustc_ast_passes/src/ast_validation.rs#L965-L966 so it shouldn't be the problem.
What is probably being executed is https://github.com/spastorino/rust/blob/2dece5bb62f234f5622a08289c5a3d1555cd7843/compiler/rustc_ast_passes/src/ast_validation.rs#L968-L975 as a forward incompatible lint (allow by default).
I'm not sure why this would be failing though. Would need to try things out and debug it to get a better understanding of what's going out. Unless I'm missing something obvious.

@ehuss
Copy link
Contributor

ehuss commented Nov 4, 2024

I believe the problem is here.

wasm-bindgen is modifying the tokens of the generated extern block to use the span of the identifier from the input. That identifier will have the 2024 hygiene. Thus the entire extern block will have 2024 hygiene, and thus the validation will think it is a 2024 extern block.

I think this might need to be fixed on the wasm-bindgen side. I don't know how the edition hygiene of the item span is computed if the item is built from a mix of tokens, but I'm wondering if wasm-bindgen could not modify the span of the extern "C" { tokens. That is, only respan this line and these lines, and not the surrounding stuff like extern "C".

@ehuss
Copy link
Contributor

ehuss commented Nov 4, 2024

Another alternative solution is for wasm-bindgen to include the unsafe keyword if the input includes it. That way, the caller can update their extern block to include unsafe. However, cargo fix probably won't work with that.

@spastorino
Copy link
Member

@ehuss should we close this issue then?

@ehuss
Copy link
Contributor

ehuss commented Nov 5, 2024

I'm not sure, yet. We'll discuss in our call today. Not working with wasm-bindgen I think is a pretty big deal, and will want to at least follow up to see if we can fix it on their side.

I was also wondering if it is in the realm of possibility to check the ExpnKind of the span to suppress the error? Do we ever do that anywhere else? Does that even fix this particular scenario?

@ehuss
Copy link
Contributor

ehuss commented Nov 15, 2024

I'm going to close this particular issue as resolved by rustwasm/wasm-bindgen#4259.

As for the general problem of proc-macros re-spanning their output (particularly with quote_spanned! to generate better error messages), I'm going to link to my comment over at #132963 (comment). I don't know if that is fixable (I have no ideas yet), or if there are possible mitigations. I'm concerned about how common that seems to be, and it breaks our assumptions about how macros are compatible with editions.

@ehuss ehuss closed this as completed Nov 15, 2024
@jieyouxu jieyouxu added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. L-missing_unsafe_on_extern Lint: missing_unsafe_on_extern and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. F-unsafe_extern_blocks `#![feature(unsafe_extern_blocks)]` L-missing_unsafe_on_extern Lint: missing_unsafe_on_extern
Projects
None yet
Development

No branches or pull requests

6 participants