godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.47k stars 20.26k forks source link

.NET 6 C# - ScriptPropertiesGenerator fail with overridden initialized property on master #71102

Open AlmightyLaxz opened 1 year ago

AlmightyLaxz commented 1 year ago

Godot version

v4.0.beta.mono.custom_build [56522f7f8] (with double)

System information

Arch Linux, Vulkan

Issue description

C# projects with a parent class including a virtual property that is then overridden & initialized in a child class appear to cause ScriptPropertiesGenerator & ScriptSerializationGenerator to fail to generate source.

The problem appears to be caused by #70483, the Source Generators run fine if built immediately before this PR was committed. I'm not sure how to debug the Source Generators for a more useful error/stack trace than below, sorry.

Parent Class

using Godot;
public partial class TestParentClass : RigidBody3D
{
    public virtual int NodeType { get; set; } = 1;
}

Child Class

If the initialized property override below is commented then the Source Generators work fine.

public partial class TestChildClass : TestParentClass
{
    public override int NodeType => 10;
}

Warnings/Errors produced

Warnings produced when a child class overrides the initialized property from its parent:

0>CSC: Warning CS8785 : Generator 'ScriptPropertiesGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'NullReferenceException' with message 'Object reference not set to an instance of an object.'
0>CSC: Warning CS8785 : Generator 'ScriptSerializationGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'NullReferenceException' with message 'Object reference not set to an instance of an object.'

Any [Export] properties then throw build errors as ScriptPropertiesGenerator failed:

0>csharpbug_03c26d6/Godot.SourceGenerators/Godot.SourceGenerators.ScriptPropertyDefValGenerator/Nothing_ScriptPropertyDefVal.generated.cs(9,33): Error CS0117 : 'Node.PropertyName' does not contain a definition for 'TestInt'
0>csharpbug_03c26d6/Godot.SourceGenerators/Godot.SourceGenerators.ScriptPropertyDefValGenerator/Nothing_ScriptPropertyDefVal.generated.cs(11,33): Error CS0117 : 'Node.PropertyName' does not contain a definition for 'TestFloat'

Steps to reproduce

Build the solution in the minimal reproduction project.

Minimal reproduction project

csharpbug_03c26d6.zip

raulsntos commented 1 year ago

I didn't consider that there may be non-readonly properties with a null SetMethod. https://github.com/godotengine/godot/pull/71128 should fix it.

Framebuffers commented 2 days ago

[Context: This bug is so elusive and hard to catch, that I cannot reproduce it reliably enough to isolate a proper issue. Here's a way it happened to me if it helps anyone. Using Godot 4.3-stable and a self-compiled build of 4.3-stable with PR #79731 baked in. Both of them worked the same. This makes it C#/Mono exclusive.]

The one thing that triggered this bug is this asset from the Asset Library. It's not the asset itself that causes this. This bug happened unexpectedly (while on a game jam), even if under the same conditions it worked before.

The conditions were:

The only solution was:

It might've been a game-breaker for many. Some steps might look unnecessary but they were in this particular case. The game was so tainted by several days of having this time bomb lurking that I did lose some code, and thankfully I did have some working backups to fall back into.

I did find a comment on the ScriptPropertyDefValGenerator.cs file that kind of makes me think of something.

I am not a professional C# programmer, nor expert in the field, but I did find a pattern:

I write this after quite a while of figuring out the details of this bug... but I cannot reproduce them 100% reliably, mainly because the aforementioned evasive nature of it. But my gut feeling is that it's not resolving this specific combination of abstraction correctly, and that, worst case, we should get a warning this pattern is not recommended and can cause later harm.