godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 93 forks source link

Add compile-time checks for arguments when emitting signals (C#) #7891

Open davor-skontra opened 1 year ago

davor-skontra commented 1 year ago

Describe the project you are working on

A game implemented in C# that makes use of signals

Describe the problem or limitation you are having in your project

When emitting signals it's easy to forget an argument or add an argument with a wrong type. Especially if the signal has been refactored. This can lead to runtime errors since there is no indication that there is something wrong during compile time.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

When calling EmitSignal with an incorrect number of parameters or parameters of wrong type we should show an Error (or a Warning, Hint etc) at compile time.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

A Roslyn Code Analyser that checks all EmitSignal calls at compile time could be added to the project.

The analyser would keep a list of existing signal delegates and invocations of EmitSignal method. Using the SignalName parameter of the emit method in the analyser we could easily figure out the correct parameter amounts and types. Then, if there is a mismatch we could show and Error .

Since this would be done using Roslyn Code Analyzers the errors would be visible in most IDE's right away.

Example of a situation in which an error would be displayed since the EmitSignal call is missing an argument of type Int:

[Signal]
public delegate void MySignalEventHandler(string foo, int bar);

EmitSignal(SignalName.MySignal, "some string");

If this enhancement will not be used often, can it be worked around with a few lines of script?

Code analysers can be disabled easily with a SuppressMessageAttribute or with more broadly with EditorConfig files but I'm not sure why somebody would want to suppress this particular one

Is there a reason why this should be core and not an add-on in the asset library?

It seems an important core feature that should be enabled by default since it would be useful to any C# project that is using signals.

davor-skontra commented 1 year ago

Additional notes:

I saw similar proposals that would aim to fix this in GDScript and apparently it's not easy to do for the language, but since GDScript is more or less dynamically typed and C# isn't it might make sense to implement at least in C#.

I have some experience with Roslyn and I'm willing to implement this if it's approved.

poohcom1 commented 1 year ago

I believe this issue would be addressed if https://github.com/godotengine/godot/pull/68233 or https://github.com/godotengine/godot/pull/70424 gets merged, but I do think code analyzers could be a good workaround in the meantime.