Implement the safe_ident strategy for virtual call parameter identifier generation

[ambeeeeee]

The behavior in maybe_rename_parameter as currently implemented strips leading underscores from parameter names.

The case where _ is stripped presents a need to sanitize identifiers where we otherwise would not have to, as the removal of the leading underscore may make the identifier match a language keyword. A concrete example someone ran into was an argument named type in IEditorExportPlugin::export_file in #817.

There appears to be an existing approach to keyword-named parameters, as the parameter in the codegen-generated interface is named type_. I just implemented that exact strategy in the way maybe_rename_parameter generates identifiers after removing a prefixing _.

Codegen Comparison

With the following code from #817

use godot::{classes::{EditorExportPlugin, IEditorExportPlugin}, prelude::*};

#[derive(GodotClass)]
#[class(base=EditorExportPlugin)]
pub struct MyExportPlugin {
    base: Base<EditorExportPlugin>,
}

#[godot_api]
impl IEditorExportPlugin for MyExportPlugin {
    fn init(base: Base<EditorExportPlugin>) -> Self {
        Self {
            base,
        }
    }

    fn export_file(&mut self, _path: GString, _type: GString, _features: PackedStringArray) {
        todo!()
    }
}

I've included only the changed output from the expanded macro for brevity.

Pre-Change

fn __virtual_call(name: &str) ->  ::godot::sys::GDExtensionClassCallVirtual {
    use::godot::obj::UserClass as _;
    match name {
        "_export_file" => {
            use::godot::sys;
            type Sig = ((),GString,GString,PackedStringArray);
            unsafe extern "C" fn virtual_fn(instance_ptr:sys::GDExtensionClassInstancePtr,args_ptr: *const sys::GDExtensionConstTypePtr,ret:sys::GDExtensionTypePtr,){
                let call_ctx =  ::godot::meta::CallContext::func("MyExportPlugin","export_file");
                let _success =  ::godot::private::handle_ptrcall_panic(&call_ctx, || <Sig as ::godot::meta::PtrcallSignatureTuple> ::in_ptrcall(instance_ptr, &call_ctx,args_ptr,ret, |instance_ptr,params|{
                    let(path,type,features,) = params;
                    let storage = unsafe {
                        ::godot::private::as_storage:: <MyExportPlugin>(instance_ptr)
                    };
                    let mut instance =  ::godot::private::Storage::get_mut(storage);
                    instance.export_file(path,type,features)
                },sys::PtrcallType::Virtual,));
            }
            Some(virtual_fn)
        },
// ...

Specific lines that cause problems:

• let(path,type,features,) = params;
• instance.export_file(path,type,features)

Post-Change

fn __virtual_call(name: &str) -> ::godot::sys::GDExtensionClassCallVirtual {
    use ::godot::obj::UserClass as _;
    match name {
        "_export_file" => {
            use ::godot::sys;
            type Sig = ((), GString, GString, PackedStringArray);
            unsafe extern "C" fn virtual_fn(
                instance_ptr: sys::GDExtensionClassInstancePtr,
                args_ptr: *const sys::GDExtensionConstTypePtr,
                ret: sys::GDExtensionTypePtr,
            ) {
                let call_ctx =
                    ::godot::meta::CallContext::func("MyExportPlugin", "export_file");
                let _success = ::godot::private::handle_ptrcall_panic(&call_ctx, || {
                    <Sig as ::godot::meta::PtrcallSignatureTuple>::in_ptrcall(
                        instance_ptr,
                        &call_ctx,
                        args_ptr,
                        ret,
                        |instance_ptr, params| {
                            let (path, type_, features) = params;
                            let storage = unsafe {
                                ::godot::private::as_storage::<MyExportPlugin>(instance_ptr)
                            };
                            let mut instance = ::godot::private::Storage::get_mut(storage);
                            instance.export_file(path, type_, features)
                        },
                        sys::PtrcallType::Virtual,
                    )
                });
            }
            Some(virtual_fn)
        }
// ...

[ambeeeeee]

Another solution which would work in this case is generating the identifiers with the escaped identifier syntax, which is how I originally implemented a solution. I figured I'd just use the same solution that already exists in the codegen implementation even though prepending r# to the prefix-stripped identifier is "simpler".

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-822

[Bromeon]

Thanks a lot! 🙂

The code duplication isn't great, but I agree it's probably more consistent to stick with the existing approach for now 🤔 could you maybe add a comment in both functions (macro + codegen) to indicate there's duplicated code which would need to be kept in sync?

Would it be possible to add a small test that verifies that it compiles? You may need to extend SELECTED_CLASSES, or maybe there's an existing one which has also a type parameter or so...

[ambeeeeee]

Added documentation about the code duplication and what I hope to be a suitable test.

I'd rather have implemented more test cases, but I think in this case we can get away with one. My thinking is that the test case is testing whether we call safe_ident and whether it returns a safe identifier, and if we wanted more testing we could just test safe_ident since we know it will be called as expected as long as this case compiles.

(Sorry about the force pushes, somehow ended up duplicating the squashed commit while attempting to rebase on the latest master and things got far out of hand so very quickly)

[Bromeon]

Thanks! Don't worry about force pushes 🙂

[Bromeon]

Thanks! Once you remove the clippy/rustfmt attributes, change import and squash the commits, should be good to go!

[Bromeon]

Thanks a lot!