Open Beliar83 opened 1 year ago
I should add that I already have been working on this and was about to make a commit and PR when I saw that one should add a feature proposal first.
During 4.0 pre-alpha we used to have a [DisableGodotGenerators]
attribute. It was removed in https://github.com/godotengine/godot/commit/4d710bf659c0bea5b8f3d6ec65eda047bada0e02 because I thought most users wouldn't want to disable source generators, since it would require them to implement all the methods that the source generators implement for them. This use case seemed extremely unlikely at the time.
we want the person that uses our library to still be able to use the C# generators normally.
This doesn't sound right. If a library project disables source generators, that should not be affecting consumer projects of that library. Maybe I'm misunderstanding what you mean, but each project decides the source generators that will be used when building that project and this generation only affects types defined in that project.
@raulsntos We already generate the glue code on the F# side, as it allows us generate code that works better with F#, but we want the users to be able to generate the C# file in the main project (it is not possible to mix C# and F# files in the same project). For this reason we don't want to disable generators for the whole project. This Attribute would only apply to a specific class, and, since it is not inherited, would still allow Nodes that inherit from it to make additions that would be picked up by the generators even if it is in the same project.
It would be used like this, for example:
[DisableGenerators(new[]{"ScriptSerialization"})]
public abstract partial class Test : MyNode
{
/// <inheritdoc />
protected override void SaveGodotObjectData(GodotSerializationInfo info)
{
_SaveGodotObjectData(info);
}
/// <inheritdoc />
protected override void RestoreGodotObjectData(GodotSerializationInfo info)
{
_RestoreGodotObjectData(info);
}
}
I am aware of that PR, but as I said, it would disable the generators for the whole project.
F# can not spit out classes which contain protected methods. So, the generated F# glue code can not create the 2 methods that need to be overridden as protected.
This means that any Node written in F# can not be extended in the C# side because the source generators will unconditionally override those 2 methods again with protected permissions causing an compile error.
To make matters worse, with how Godot works we need C# classes at least at this moment to make the F# nodes attachable. Now, considering we are going to make our own generators as the C# source generators don't work for F# at all one could argue that just turning the C# generators off entirely is ok.
But then a project can either be made in F# or C#, it can not use both. Which to me doesn't sound good.
Now, in an ideal world we wouldn't have to turn it off at all, but alas as already said F# can't spit out protected methods and C#'s source generators can't properly switch its behavior based on F# classes.
So, turning it off for a specific C# class is then the next best thing. It allows nodes in F# to work and allows people to use both C# and F# in the same project with the only downside being that there is a C# file in between the C# code by the user and the F# code by the user that they can't really use.
The way I've used my change in https://github.com/godotengine/godot/pull/71049 is to replace specific generator implementations (specifically ScriptMethods
) with my own, and I feel strongly that should indeed be project wide for consistency.
A nice followup would be to split up Godot.SourceGenerators so the base generation logic is reusable from other generators, and perhaps in that refactor there can be some common utils made suitable for the F# case too.
C#'s source generators can't properly switch its behavior based on F# classes
This seems like the real problem you have here that is worth solving. If the current C# generators are made to only do work on C# classes (perhaps they need to move to be Incremental Generators as that has some more control), then your F# generators and C# generators can live happily side by side ?
C#'s source generators can't properly switch its behavior based on F# classes
This seems like the real problem you have here that is worth solving. If the current C# generators are made to only do work on C# classes (perhaps they need to move to be Incremental Generators as that has some more control), then your F# generators and C# generators can live happily side by side ?
The problem there is, that there is no information about them. The analyzer data tells that there is a base class, but other than that, there is no data about it. Not even the correct namespace/assembly. But yeah, I guess that could also make things easier, though that seems like something that would be far away.
Also, just fyi, our F# Generators are using a third party library, as there are no official generators like for C#.
The way I've used my change in https://github.com/godotengine/godot/pull/71049 is to replace specific generator implementations (specifically ScriptMethods) with my own, and I feel strongly that should indeed be project wide for consistency.
And though that sounds good, our F# generators are only targeting F#, just like how C#'s source generators are only targeting C#. Only C# classes that directly extend a node written in F# need this.
This seems like the real problem you have here that is worth solving. If the current C# generators are made to only do work on C# classes (perhaps they need to move to be Incremental Generators as that has some more control), then your F# generators and C# generators can live happily side by side ?
The current C# source generators don't touch F# files, at all. That is why we need to make our own glue code. However due to how Godot works we are going to need to make empty C# classes that extend our F# classes. And this is where the conflict happens.
This use case seemed extremely unlikely at the time.
Although I understand that to be the case from the perspective of yourself, does the F# community try to get support into Godot since years.
And despite many significant contributions to other parts of the engine, particularly from Will, does it seem like a cooperation is not really welcomed.
It would really be nice, that we can work together, so we can merge support for a language, that tries it's best to participate in the community.
@ShalokShalom when it comes to "This use case seemed extremely unlikely at the time." I agree with raulsntos that the need to disable specific generators for specific classes was unlikely. I don't think anyone would've spotted the problem, even we didn't think much of the protected
thing until we happen to run into it.
No need to call people out or to act in this somewhat hostile way. Shit happens. Besides, it to me at first glance looks like that basically did the opposite, requiring an attribute to be added to a class at which point every source generator runs on it. If that behaviour was kept we would've still ran into the same problem anyway.
Describe the project you are working on
As mentioned by @ShalokShalom in https://github.com/godotengine/godot-proposals/issues/191#issuecomment-1439220171 there is an ongoing effort to make it easier to use Godot with F#. It was decided to use source generators to generate glue similar to what the C# generators on the F# side and also generate an C# class for the engine to pick up.
Describe the problem or limitation you are having in your project
Recently we came across the problem that, when overriding methods, F# changes the accessibility from
protected
topublic
- as it has no protected accessibility modifier. When the C# source generators generate sources for that class (and inheriting classes) they assume that the modifier still isprotected
and the compiler will generate an error as changing the accessibility is not allowed. We could generate new methods in F# and have the C# class call them, but the C# source generator will also add these methods and this is again not allowed.Describe the feature / enhancement and how it helps to overcome the problem or limitation
Allow disabling of all or specific C# source generators for a specific class without disabling them for inheritors.
This would keep the C# generators from generating the methods for that class, but still allow other C# nodes to inherit and add on to it.
Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams
Add a
DisableGenerators
attribute that allows the disabling of all or specific source generators. It can only be added to classes and will not automatically be inherited.The attribute will have 2 constructors: An empty one, and one with
string[] generators
as a parameter.The C# generators would check for this attribute and skip over classes that have it added either for all or for that specific generator.
If this enhancement will not be used often, can it be worked around with a few lines of script?
One could disable all generators by setting a build variable, but we need those to generate the base glue and also we want the person that uses our library to still be able to use the C# generators normally.
Is there a reason why this should be core and not an add-on in the asset library?
It needs a change to the existing dotnet/mono module.