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

[Question] Why does parse.expand use an unstable flag to rustc? #1015

Open
bavalpey opened this issue Oct 21, 2024 · 5 comments
Open

[Question] Why does parse.expand use an unstable flag to rustc? #1015

bavalpey opened this issue Oct 21, 2024 · 5 comments

Comments

@bavalpey
Copy link

Currently, [parse.expand] uses the -Z unstable pretty=expand option to rustc. This requires using the nightly rustc compiler. There is nothing in the documentation that mentions why this is required, and there isn't much discussion as to why. If macro expansion is needed, is there a reason that cbindgen can't just do cargo expand which does not require any nightly rustc options?

I think that macro expansion is a relatively common requirement, and gating it behind having to use a nightly toolchain is a big cost.

@bavalpey
Copy link
Author

I've been looking into this more and I think that cbindgen should set RUSTC_BOOTSTRAP in the command line where it invokes rustc. This is what cargo-expand does.
That way, we can avoid having needless error messages when using cbindgen without a nightly compiler.

@emilio
Copy link
Collaborator

emilio commented Dec 6, 2024

Well the point of it being unstable is that you shouldn't rely on it. Using RUSTC_BOOTSTRAP defeats that purpose.

@bavalpey
Copy link
Author

bavalpey commented Dec 6, 2024

Cargo-expand is one of the more popular crates. It uses this option, and has for a long time.

You also can follow the same logic it uses to determine if rustc bootstrap is even needed for invocation.

Also, a user should already understand that expanding macros with this is unstable. Sort of. Macro expansion can change in minor ways anyway.

Currently, builds that rely on this will fail with misleading error messages. I had to invoke a build script that sets RUSTC_BOOTSTRAP anyway. It's already isolated to just the macro expansion invocation, and stability shouldn't matter for this portion of cbindgen.

The macro expansion with unpretty-expanded is also essentially stable anyway, if you look at its history. It would be much, much better to use this option and document the flag as unstable in the first place.
Users are opting into macro expansion by specifying it in cbindgen.toml.

In other words, this answer doesn't satisfy me.

@emilio
Copy link
Collaborator

emilio commented Dec 6, 2024

Setting RUSTC_BOOTSTRAP from a tool is a big no-no. The right thing to do, if this option is stable-ish, would be to stabilize it on the rust side (rust-lang/rust#43364)

@bavalpey
Copy link
Author

bavalpey commented Dec 6, 2024

Setting RUSTC_BOOTSTRAP from a tool is a big no-no.

You're not setting it from the tool permanently. You are setting it only for your invocation of rustc, via the command line, that is used for macro expansion, which isn't used in any way to compile the code or impact downstream. It's also kind of a big no no to have an error manifest without documentation.

The right thing to do, if this option is stable-ish, would be to stabilize it on the rust side (rust-lang/rust#43364)

I agree, but we don't have control over that.

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

No branches or pull requests

2 participants