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.

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

fix: ambiguous virtual method calls #1020

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix: ambiguous virtual method calls #1020

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: 1 addition & 0 deletions godot-macros/src/class/data_models/field_var.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ impl GetterSetterImpl {
is_script_virtual: false,
rpc_info: None,
},
None,
);

let export_token = export_token.expect("getter/setter generation should not fail");
Expand Down
25 changes: 21 additions & 4 deletions godot-macros/src/class/data_models/func.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,12 @@ pub fn make_virtual_callback(
class_name: &Ident,
signature_info: &SignatureInfo,
before_kind: BeforeKind,
interface_trait: Option<&venial::TypeExpr>,
) -> TokenStream {
let method_name = &signature_info.method_name;

let wrapped_method = make_forwarding_closure(class_name, signature_info, before_kind);
let wrapped_method =
make_forwarding_closure(class_name, signature_info, before_kind, interface_trait);
let sig_tuple = signature_info.tuple_type();

let call_ctx = make_call_context(
Expand Down Expand Up @@ -75,6 +77,7 @@ pub fn make_virtual_callback(
pub fn make_method_registration(
class_name: &Ident,
func_definition: FuncDefinition,
interface_trait: Option<&venial::TypeExpr>,
) -> ParseResult<TokenStream> {
let signature_info = &func_definition.signature_info;
let sig_tuple = signature_info.tuple_type();
Expand All @@ -85,8 +88,12 @@ pub fn make_method_registration(
Err(msg) => return bail_fn(msg, &signature_info.method_name),
};

let forwarding_closure =
make_forwarding_closure(class_name, signature_info, BeforeKind::Without);
let forwarding_closure = make_forwarding_closure(
class_name,
signature_info,
BeforeKind::Without,
interface_trait,
);

// String literals
let method_name = &signature_info.method_name;
Expand Down Expand Up @@ -209,6 +216,7 @@ fn make_forwarding_closure(
class_name: &Ident,
signature_info: &SignatureInfo,
before_kind: BeforeKind,
interface_trait: Option<&venial::TypeExpr>,
) -> TokenStream {
let method_name = &signature_info.method_name;
let params = &signature_info.param_idents;
Expand Down Expand Up @@ -238,7 +246,16 @@ fn make_forwarding_closure(
let method_call = if matches!(before_kind, BeforeKind::OnlyBefore) {
TokenStream::new()
} else {
quote! { instance.#method_name( #(#params),* ) }
match interface_trait {
Some(interface_trait) => {
let instance_ref = match signature_info.receiver_type {
ReceiverType::Mut => quote! { &mut instance },
_ => quote! { &instance },
Copy link
Member

Choose a reason for hiding this comment

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

do we want to just silently fail here if an incorrect receiver is passed somehow? it should generate a compile error either way, but maybe we'd wanna explicitly pass a compile_error! instead?

Copy link
Author

@adalinesimonian adalinesimonian Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is inside of a match arm against receiver_type that matches ReceiverType::Mut and ReceiverType::Ref, so it is impossible in this case for it to be anything else.

Copy link
Member

Choose a reason for hiding this comment

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

i think it would be more clear to write that using an unreachable! then

};
quote! { <#class_name as #interface_trait>::#method_name( #instance_ref, #(#params),* ) }
}
None => quote! { instance.#method_name( #(#params),* ) },
}
};

quote! {
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 @@ -98,7 +98,7 @@ pub fn transform_inherent_impl(

let method_registrations: Vec<TokenStream> = funcs
.into_iter()
.map(|func_def| make_method_registration(&class_name, func_def))
.map(|func_def| make_method_registration(&class_name, func_def, None))
.collect::<ParseResult<Vec<TokenStream>>>()?;

let constant_registration = make_constant_registration(consts, &class_name, &class_name_obj)?;
Expand Down
11 changes: 9 additions & 2 deletions 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 @@ -246,6 +246,7 @@ pub fn transform_trait_impl(original_impl: venial::Impl) -> ParseResult<TokenStr
hash_constant: quote! { hashes::#method_name_ident },
signature_info,
before_kind,
interface_trait: Some(trait_path.clone()),
});
}
}
Expand All @@ -266,6 +267,7 @@ pub fn transform_trait_impl(original_impl: venial::Impl) -> ParseResult<TokenStr
hash_constant: quote! { ::godot::sys::known_virtual_hashes::Node::ready },
signature_info: SignatureInfo::fn_ready(),
before_kind: BeforeKind::OnlyBefore,
interface_trait: None,
};

overridden_virtuals.push(match_arm);
Expand Down Expand Up @@ -366,6 +368,7 @@ struct OverriddenVirtualFn<'a> {
hash_constant: TokenStream,
signature_info: SignatureInfo,
before_kind: BeforeKind,
interface_trait: Option<venial::TypeExpr>,
}

impl OverriddenVirtualFn<'_> {
Expand All @@ -383,8 +386,12 @@ impl OverriddenVirtualFn<'_> {
let pattern = method_name_str;

// Lazily generate code for the actual work (calling user function).
let method_callback =
make_virtual_callback(class_name, &self.signature_info, self.before_kind);
let method_callback = make_virtual_callback(
class_name,
&self.signature_info,
self.before_kind,
self.interface_trait.as_ref(),
);

quote! {
#(#cfg_attrs)*
Expand Down
3 changes: 2 additions & 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 @@ -279,7 +279,8 @@ fn make_user_class_impl(
let tool_check = util::make_virtual_tool_check();
let signature_info = SignatureInfo::fn_ready();

let callback = make_virtual_callback(class_name, &signature_info, BeforeKind::OnlyBefore);
let callback =
make_virtual_callback(class_name, &signature_info, BeforeKind::OnlyBefore, None);

// See also __virtual_call() codegen.
// This doesn't explicitly check if the base class inherits from Node (and thus has `_ready`), but the derive-macro already does
Expand Down
Loading