godot icon indicating copy to clipboard operation
godot copied to clipboard

C#: Add `[ScriptMethodExclude]`. An attribute that excluding method generation from source generators

Open kkjinping opened this issue 1 year ago • 5 comments

C#: Add [ScriptMethodExclude] . An attribute that excluding method generation from source generators.

kkjinping avatar Apr 15 '24 06:04 kkjinping

Hello, and thank you for the contribution 😀 Can you explain a bit more the use case behind this? I understand the finality of it (avoid generating metadata for the annotated methods), but I'm not sure that I understand the necessity (why one would need to avoid this small-ish part of the source generation). I am not fundamentally against the idea of giving more control over the generation process, but I'd like to understand the reasoning. Thanks.

paulloz avatar Apr 20 '24 00:04 paulloz

Hello, and thank you for the contribution 😀 Can you explain a bit more the use case behind this? I understand the finality of it (avoid generating metadata for the annotated methods), but I'm not sure that I understand the necessity (why one would need to avoid this small-ish part of the source generation). I am not fundamentally against the idea of giving more control over the generation process, but I'd like to understand the reasoning. Thanks.

I apologize for taking so long to get back to you.

Here are my reasons for submitting this PR:

  1. When there are too many methods, the generated code has a lot of if and equals, this can cause some performance impact.
  2. Private methods are automatically exposed, allowing call, however the developer has no control over this.

kkjinping avatar Apr 22 '24 12:04 kkjinping

No need to apologize, there's no rush at all.

I don't think impact on performance is in consideration here. The amount of code we'd have to generate for this to actually matter would be massive. I do agree that automatically exposing non-public members to the Call / Set / Get API is something we could do without, though.

Can I ask if you have a concrete example as to what prompted you to open this PR? Sorry if it feels like I'm pushing against the change (I'm not). But, even if this is a small change, it would introduce a new attribute that we'd eventually need to deprecate, since the long-term goal is to make the bindings entirely opt-in (opposed to the current no control / opt-out model). So I'm trying to assess where the short-term benefits lie against the long-term drawbacks.

paulloz avatar Apr 22 '24 18:04 paulloz

I actually have a use case for this attribute, I have worked on a C# Wrapper Generator for GDExtension, and one specific GDExtension, Steam, have produced a single class with 732 methods in it, which generates this MEGA 2200 lines of if and else statement that even bricks the compiler: Error CS8078,

This is what that compiler-breaking method looks like: image ...... image

Compile my included C# wrapper file will sometimes cause the following compiler issue: image Translate to: An expression is too long or complex to compile.

I wish we could have this attribute that I can add for each wrapper method, to at least make it compile (as these methods shouldn't be called by others anyway).

Delsin-Yu avatar Apr 24 '24 01:04 Delsin-Yu

I think this is actually a good addition to the API. Arguably, I'd make it [GodotIgnore], so it could be used in other generators. One possible use case: I want to implement the access to static properties across interop, and it would probably be good to avoid exposing stuff like GodotSharpEditor.Instance if we can.

cc @raulsntos

paulloz avatar Apr 30 '24 19:04 paulloz

The .NET team has discussed this PR and agree that this is a good addition, but as suggested by @paulloz we'd like to extend the attribute to work for properties as well.

We also bikeshed a bit about the name:

  • We decided the attribute should not have a Godot prefix, to be consistent with the rest of our attributes.
  • We considered the word Ignore and Exclude, and settled on Ignore because it matches the [JsonIgnore] attribute.
  • We considered adding a Member suffix (i.e.: [IgnoreMember]), because the word Ignore alone may be too vague.

In the end, we think both [Ignore] and [IgnoreMember] are acceptable options.

We also intend to merge this PR for 4.4, so I'm adjusting the milestone accordingly. Keep in mind this is not a guarantee that it will be merged for 4.4, and may still be pushed to a later milestone if we can't finish it in time.

raulsntos avatar Sep 11 '24 22:09 raulsntos