Github Turn alloc's force_expr macro into a regular macro_rules. by m-ou-se · Pu...

Member

Why not give it a more complicated name, like __rustc_force_expr?

m-ou-se commented 9 days ago

Sure, that makes a collision less likely, but we don't have any identifiers guaranteed to be reserved for the language/library implementation like C and C++ have. But yeah, if we can't find a better way, we should probably do that.

estebank commented 9 days ago

Sure, that makes a collision less likely, but we don't have any identifiers guaranteed to be reserved for the language/library implementation

That's not something that we can do anything about today and the "namespaced" name coupled with its inherent uselessness for anything beyond improving diagnostics makes the likelihood of this being a problem low. I am more concerned about not being able to remove this ever, so lets pick a name we can live with.

m-ou-se commented 9 days ago

So our options seem to be:

• Add a new arm to the vec![] macro_rules for internal use as suggested in #61933 (comment). However, this is will be part of the stable public api, and will be visible in the documentation.
• Add a new way of specifying that a macro should always appear in the expression position as suggested in #61933 (comment). Although this feels more like 'the right solution', this is a much bigger change and is probably not worth it.
• Use a pub macro for force_expr such that it doesn't have to use #[macro_export]. That's whats currently in nightly, but breaks rust-analyzer: rust-analyzer/rust-analyzer#7349
• Use #[macro_export] macro_rules for force_expr (or e.g. __rust_force_export) like in this PR. This would be the first time an implementation detail is exported through #[macro_export], and could technically break code.
• Go back to the original vec![] macro, which gives a weird error when used in a pattern position.

If these are our only options, the change in this PR seems like the best way forward for now (with a prefix as suggested in #81241 (comment)). If some kind of #[macro_only_in_expr_position] ever gets implemented, we should update vec![] to use that instead.

I'll update this PR and mark it as ready for review.

m-ou-se commented 9 days ago

r? @oli-obk

bugadani commented 9 days ago

I am more concerned about not being able to remove this ever, so lets pick a name we can live with.

Hm, if the removal of an otherwise hidden macro is a concern, we can revert #81080 and reintroduce in its current form once rust-analyzer can handle pub macro.

oli-obk commented 8 days ago

I don't see how removing this (unstable!) macro could be a breaking change, so...

@bors r+

bors commented 8 days ago

Commit 1934eaf has been approved by oli-obk