Skip to content

Remove paste, simplify plugin macros #1069

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 11, 2025
Merged

Remove paste, simplify plugin macros #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion godot-core/src/builtin/collections/packed_array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ macro_rules! impl_packed_array {
$($trait_impls:tt)*
},
) => {
// TODO expand type names in doc comments (use e.g. `paste` crate)
#[doc = concat!("Implements Godot's `", stringify!($PackedArray), "` type,")]
#[doc = concat!("which is a space-efficient array of `", stringify!($Element), "`s.")]
///
Expand Down
2 changes: 1 addition & 1 deletion godot-core/src/private.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ pub(crate) fn iterate_plugins(mut visitor: impl FnMut(&ClassPlugin)) {
#[cfg(feature = "codegen-full")] // Remove if used in other scenarios.
pub(crate) fn find_inherent_impl(class_name: crate::meta::ClassName) -> Option<InherentImpl> {
// We do this manually instead of using `iterate_plugins()` because we want to break as soon as we find a match.
let plugins = __godot_rust_plugin___GODOT_PLUGIN_REGISTRY.lock().unwrap();
let plugins = __GODOT_PLUGIN_REGISTRY.lock().unwrap();

plugins.iter().find_map(|elem| {
if elem.class_name == class_name {
Expand Down
5 changes: 2 additions & 3 deletions godot-ffi/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,10 @@ api-4-3 = ["godot-bindings/api-4-3"]
api-4-4 = ["godot-bindings/api-4-4"]
# ]]

# TODO: get rid of paste and gensym, they are trivially implementable with proc-macros. Update cargo-deny.
# Especially gensym which pulls in entire syn for 10 LoC. See https://github.com/regiontog/gensym/blob/master/src/lib.rs.
# TODO: get rid of gensym, which is implementable with proc-macros. Update cargo-deny.
# gensym pulls in entire syn for 10 LoC. See https://github.com/regiontog/gensym/blob/master/src/lib.rs.
# Might however require godot-ffi to depend on godot-macro, which is not ideal...
[dependencies]
paste = "1"

[target.'cfg(target_os = "linux")'.dependencies]
libc = "0.2.153"
Expand Down
5 changes: 0 additions & 5 deletions godot-ffi/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,6 @@ mod plugins;
mod string_cache;
mod toolbox;

// See https://github.com/dtolnay/paste/issues/69#issuecomment-962418430
// and https://users.rust-lang.org/t/proc-macros-using-third-party-crate/42465/4
#[doc(hidden)]
pub use paste;

#[doc(hidden)]
#[cfg(target_family = "wasm")]
pub use gensym::gensym;
Expand Down
65 changes: 23 additions & 42 deletions godot-ffi/src/plugins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,53 +17,44 @@
#[macro_export]
macro_rules! plugin_registry {
($vis:vis $registry:ident: $Type:ty) => {
$crate::paste::paste! {
#[used]
#[allow(non_upper_case_globals)]
#[doc(hidden)]
$vis static [< __godot_rust_plugin_ $registry >]:
std::sync::Mutex<Vec<$Type>> = std::sync::Mutex::new(Vec::new());
}
#[used]
#[allow(non_upper_case_globals)]
#[doc(hidden)]
$vis static $registry: std::sync::Mutex<Vec<$Type>>
= std::sync::Mutex::new(Vec::new());
};
}

#[doc(hidden)]
#[macro_export]
#[allow(clippy::deprecated_cfg_attr)]
#[cfg_attr(rustfmt, rustfmt::skip)]
// ^ skip: paste's [< >] syntax chokes fmt
// Following rustfmt::skip is no longer needed, but there are 2 workarounds good to know, thus preserved.
// #[allow(clippy::deprecated_cfg_attr)]
// #[cfg_attr(rustfmt, rustfmt::skip)]
// cfg_attr: workaround for https://github.com/rust-lang/rust/pull/52234#issuecomment-976702997
macro_rules! plugin_execute_pre_main_wasm {
($gensym:ident,) => {
// Rust presently requires that statics with a custom `#[link_section]` must be a simple
// list of bytes on the wasm target (with no extra levels of indirection such as references).
// list of bytes on the Wasm target (with no extra levels of indirection such as references).
//
// As such, instead we export a fn with a random name of predictable format to be used
// by the embedder.
$crate::paste::paste! {
#[no_mangle]
extern "C" fn [< rust_gdext_registrant_ $gensym >] () {
__init();
}
Comment on lines -41 to -47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was paste not still necessary here? We need a prefix with which to search for registered functions at init time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If needed, we can use pastey instead.

Copy link
Member Author

@Bromeon Bromeon Mar 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch! This is the problem when the comments aren't expressive: "a random name of predictable format" didn't make me think that the concrete prefix mattered.

If needed, we can use pastey instead.

I don't simply trust random crates, and this one in particular had an incident where it just replaced all commits of the previous author. That after opening the RustSec advisory for paste, triggering the migration in the first place. Not a great start.

I'll see if I find another solution. Maybe a proc-macro, then we can get rid of gensym as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking. Let's do it ourselves then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #1092.

// As such, instead we export a function with a random name of predictable format to be used by the embedder.
#[no_mangle]
extern "C" fn $gensym() {
__init();
}
};
}

/// Executes a block of code before main, by utilising platform specific linker instructions.
#[doc(hidden)]
#[macro_export]
#[allow(clippy::deprecated_cfg_attr)]
#[cfg_attr(rustfmt, rustfmt::skip)]
// ^ skip: paste's [< >] syntax chokes fmt
// cfg_attr: workaround for https://github.com/rust-lang/rust/pull/52234#issuecomment-976702997
macro_rules! plugin_execute_pre_main {
($body:expr) => {
const _: () = {
#[allow(non_upper_case_globals)]
#[used]
// Windows:
#[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
// MacOS + iOS:
// macOS + iOS:
#[cfg_attr(target_os = "ios", link_section = "__DATA,__mod_init_func")]
#[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")]
// Linux, Android, BSD:
Expand All @@ -88,43 +79,33 @@ macro_rules! plugin_execute_pre_main {
};
}

/// register a plugin by executing code pre-main that adds the plugin to the plugin registry
/// Register a plugin by executing code pre-main that adds the plugin to the registry.
#[doc(hidden)]
#[macro_export]
#[allow(clippy::deprecated_cfg_attr)]
#[cfg_attr(rustfmt, rustfmt::skip)]
// ^ skip: paste's [< >] syntax chokes fmt
// cfg_attr: workaround for https://github.com/rust-lang/rust/pull/52234#issuecomment-976702997
macro_rules! plugin_add_inner {
($registry:ident; $plugin:expr; $( $path_tt:tt )* ) => {
($registry:path; $plugin:expr) => {
$crate::plugin_execute_pre_main!({
let mut guard = $crate::paste::paste!( $( $path_tt )* [< __godot_rust_plugin_ $registry >] )
.lock()
.unwrap();
let mut guard = $registry.lock().unwrap();

guard.push($plugin);
});
};
}

/// Register a plugin to a registry
/// Register a plugin to a registry.
#[doc(hidden)]
#[macro_export]
macro_rules! plugin_add {
( $registry:ident; $plugin:expr ) => {
$crate::plugin_add_inner!($registry; $plugin; );
};

( $registry:ident in $path:path; $plugin:expr ) => {
$crate::plugin_add_inner!($registry; $plugin; $path ::);
( $registry:path; $plugin:expr ) => {
$crate::plugin_add_inner!($registry; $plugin);
};
}

/// Iterate over all plugins in unspecified order
#[doc(hidden)]
#[macro_export]
macro_rules! plugin_foreach_inner {
( $registry:ident; $closure:expr; $( $path_tt:tt )* ) => {
let guard = $crate::paste::paste!( $( $path_tt )* [< __godot_rust_plugin_ $registry >] )
let guard = $( $path_tt )* $registry
.lock()
.unwrap();

Expand All @@ -135,7 +116,7 @@ macro_rules! plugin_foreach_inner {
};
}

/// Register a plugin to a registry
/// Iterate over all plugins in unspecified order.
#[doc(hidden)]
#[macro_export]
macro_rules! plugin_foreach {
Expand Down
2 changes: 1 addition & 1 deletion godot-macros/src/bench.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub fn attribute_bench(input_decl: venial::Item) -> ParseResult<TokenStream> {
}
}

::godot::sys::plugin_add!(__GODOT_BENCH in crate::framework; crate::framework::RustBenchmark {
::godot::sys::plugin_add!(crate::framework::__GODOT_BENCH; crate::framework::RustBenchmark {
name: #bench_name_str,
file: std::file!(),
line: std::line!(),
Expand Down
2 changes: 1 addition & 1 deletion godot-macros/src/class/data_models/inherent_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ pub fn transform_inherent_impl(
};

let class_registration = quote! {
::godot::sys::plugin_add!(__GODOT_PLUGIN_REGISTRY in #prv; #prv::ClassPlugin::new::<#class_name>(
::godot::sys::plugin_add!(#prv::__GODOT_PLUGIN_REGISTRY; #prv::ClassPlugin::new::<#class_name>(
#prv::PluginItem::InherentImpl(#prv::InherentImpl::new::<#class_name>(#docs))
));
};
Expand Down
2 changes: 1 addition & 1 deletion godot-macros/src/class/data_models/interface_trait_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ pub fn transform_trait_impl(original_impl: venial::Impl) -> ParseResult<TokenStr
}
}

::godot::sys::plugin_add!(__GODOT_PLUGIN_REGISTRY in #prv; #prv::ClassPlugin::new::<#class_name>(
::godot::sys::plugin_add!(#prv::__GODOT_PLUGIN_REGISTRY; #prv::ClassPlugin::new::<#class_name>(
#prv::PluginItem::ITraitImpl(#item_constructor)
));
};
Expand Down
2 changes: 1 addition & 1 deletion godot-macros/src/class/derive_godot_class.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ pub fn derive_godot_class(item: venial::Item) -> ParseResult<TokenStream> {
#( #deprecations )*
#( #errors )*

::godot::sys::plugin_add!(__GODOT_PLUGIN_REGISTRY in #prv; #prv::ClassPlugin::new::<#class_name>(
::godot::sys::plugin_add!(#prv::__GODOT_PLUGIN_REGISTRY; #prv::ClassPlugin::new::<#class_name>(
#prv::PluginItem::Struct(
#prv::Struct::new::<#class_name>(#docs)#(.#modifiers())*
)
Expand Down
2 changes: 1 addition & 1 deletion godot-macros/src/class/godot_dyn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ pub fn attribute_godot_dyn(input_decl: venial::Item) -> ParseResult<TokenStream>
}
}

::godot::sys::plugin_add!(__GODOT_PLUGIN_REGISTRY in #prv; #prv::ClassPlugin::new::<#class_path>(
::godot::sys::plugin_add!(#prv::__GODOT_PLUGIN_REGISTRY; #prv::ClassPlugin::new::<#class_path>(
#prv::PluginItem::DynTraitImpl(#prv::DynTraitImpl::new::<#class_path, dyn #trait_path #assoc_type_constraints>()))
);

Expand Down
2 changes: 1 addition & 1 deletion godot-macros/src/itest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub fn attribute_itest(input_item: venial::Item) -> ParseResult<TokenStream> {
#body
}

::godot::sys::plugin_add!(__GODOT_ITEST in crate::framework; crate::framework::RustTestCase {
::godot::sys::plugin_add!(crate::framework::__GODOT_ITEST; crate::framework::RustTestCase {
name: #test_name_str,
skipped: #skipped,
focused: #focused,
Expand Down
8 changes: 4 additions & 4 deletions itest/rust/src/register_tests/constant_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,10 @@ impl godot::obj::cap::ImplementsGodotApi for HasOtherConstants {

// TODO once this is done via proc-macro, see if `register-docs` is still used in register_docs_test.rs. Otherwise, remove feature from Cargo.toml.
godot::sys::plugin_add!(
__GODOT_PLUGIN_REGISTRY in ::godot::private;
::godot::private::ClassPlugin::new::<HasOtherConstants>(
::godot::private::PluginItem::InherentImpl(
::godot::private::InherentImpl::new::<HasOtherConstants>(
godot::private::__GODOT_PLUGIN_REGISTRY;
godot::private::ClassPlugin::new::<HasOtherConstants>(
godot::private::PluginItem::InherentImpl(
godot::private::InherentImpl::new::<HasOtherConstants>(
#[cfg(feature = "register-docs")]
Default::default()
)
Expand Down
Loading