C#: Fix Generated ScriptProperty Error.

[magian1127]

Add "this." to prevent errors caused by duplicate variable names.
Try to find the default value of property getters.

加上"this.",防止和生成的变量名重名导致的错误.
尝试查找属性的默认值.

Bugsquad edit:

• Fixes Can't name C# variable "value" or "name" #66819.

[neikeq]

I will need some example script code to understand what this fixes.

[magian1127]

using Godot;
using Godot.Collections;
using System;

public partial class new_script : Godot.RefCounted
{
	string name = "test";

 	[Export]
	public Array<string> Skills { get; set; } = new Array<string>();

}

These codes generate these errors

test4bug.zip

[raulsntos]

I still think it would be preferable to always use fully qualified names (and not omitting global::) in generated code instead of adding usings. To clarify what this PR intends to fix is the generator for GetGodotPropertyDefaultValues which, currently, for an exported property like this:

[Export]
public Array<Node> MyNodes { get; set; } = new Array<Node>();

Generates this code:

internal new static System.Collections.Generic.Dictionary<StringName, object> GetGodotPropertyDefaultValues()
{
    var values = new System.Collections.Generic.Dictionary<StringName, object>(1);
    Godot.Collections.Array<Godot.Node> __MyNodes_default_value = new Array<Node>();
    values.Add(PropertyName.MyNodes, __MyNodes_default_value);
    return values;
}

The issue is the default value expression isn't fully qualified since it's taken as is from the source.

This PR intends to fix this by including the same usings in the generated file as the ones declared in the source, this could still conflict and I'd prefer if we didn't omit global either.

This code would still fail even with this PR:

// using Godot; // <-- No using Godot namespace

public partial class Sprite2D : Godot.Resource {} // <- class with same name as one of the classes in Godot namespace, can be defined in a different file

public partial class MyNode : Godot.Node
{
    [Godot.Export]
    public Sprite2D Sprite { get; set; } = new Sprite2D();
}

---

This PR also fixes conflicts when the exported members share the same name with a local variable defined in the generated code by adding this., I have no problem with this part of the PR.

[magian1127]

// using Godot; // <-- No using Godot namespace

public partial class Sprite2D : Godot.Resource {} // <- class with same name as one of the classes in Godot namespace, can be defined in a different file

public partial class MyNode : Godot.Node
{
    [Godot.Export]
    public Sprite2D Sprite { get; set; } = new Sprite2D();
}

In the latest PR, this paragraph is valid.

At the beginning, I really wanted to use the fully qualified name, but after a long time of searching the roslyn document, I couldn't find a good way, so I had to do it first.

To cope with various methods, the current solution also has its own advantages

A _x = new A();
A _x = A.C();
A _x = A.C(new B());
A _x = new A(new B());
//Other more and more complex
//Find and compare "fully qualified names" for each one

在最新的PR中,这段是有效的.
刚开始的时候,我确实想用完全限定名,但是翻了很久的roslyn文档,也没找到好的办法,就只能先这样了.
要应对各种方法的情况下,现在的解决方案也有它自己的优势.

[magian1127]

A new modification has been added.
Try to find the default value of property getters.
In the following cases, the default values can now be displayed normally.

    string _aaa = "test";

    [Export]
    string AAA
    {
        get
        {
            return _aaa;
        }
        set
        {
            _aaa = value;
        }
    }

    private string _newAnimation = "xxxxx";

    [Godot.Export(Godot.PropertyHint.Enum, "xxxx,xx,xx")]
    public string NewAnimation
    {
        get => _newAnimation;
        set
        {
            _newAnimation = value;
        }
    }

[magian1127]

Adjusted according to #68580

[raulsntos]

Seems to work well. Thanks for the contribution!

My main concern is the case where the default value is the result of an expensive operation because it will be executed on each call to GetGodotPropertyDefaultValues and it can be problematic if the operation has side-effects. However, this was already how it worked before this PR, I just wanted to bring attention to it.

[Export]
public int MyProperty { get; set; } = MyExpensiveMethodWithSideEffects();

Also, we may want to add an example to the Godot.SourceGenerators.Sample project with the syntax that this PR adds support for:

private int _myProperty = 42;

[Export]
public int MyProperty
{
	get => _myProperty;
	set => _myProperty = value;
}

[magian1127]

Generally speaking, the default value of the attribute should not change.
Users should be advised to avoid using dynamic default values.
MyExpensiveMethodWithSideEffects(), perhaps this approach could be adopted.

[Export]
public int MyProperty { get; set; } = MyExpensiveMethodWithSideEffects();

//generated
    private int __MyProperty = MyExpensiveMethodWithSideEffects();

    internal new static System.Collections.Generic.Dictionary<StringName, object> GetGodotPropertyDefaultValues()
    {
        ...
        string __MyPropert_default_value = __MyProperty;
        ...
    }

通常来说,属性默认值应该是不太会变动的.
应该建议用户,避免使用动态的默认值.
MyExpensiveMethodWithSideEffects()这种,或许可以采用这样的办法.

[raulsntos]

I think this looks good to merge already, @neikeq may to review this too since he self-requested a review.

[neikeq]

Yes, I'll review this tonight. Property default values are delicate so I want to make sure we're not doing anything undesirable.

[neikeq]

This doesn't make sure that the body of the getter is a single return statement, it only looks for that return statement. This can have many problems, even if the return example is just an identifier for a field. For example, a method could have more than one return statement.

IMO, the best we can support here is bodies that contain only a return statement with the identifier (as weird as it would be to write that with a block rather than an expression body). This code should make sure that's all the code does.

[magian1127]

only generate a single return, which should be applicable to most cases

[neikeq]

Looks good to merge.

I think the code to get the initializer and it's value is duplicated in 3 places now, so it's a good candidate to extract to a method. But it's not necessary in order to merge.

[neikeq]

Thanks!

[magian1127]

Looks good to merge.

I think the code to get the initializer and it's value is duplicated in 3 places now, so it's a good candidate to extract to a method. But it's not necessary in order to merge.

For me, naming the method is a very distressing thing ha.
There are 4 places where this paragraph is used, and the next time you refactor the file you can consider extracting it into the method.

var sm = context.Compilation.GetSemanticModel(initializer.SyntaxTree);
value = initializer.Value.FullQualifiedSyntax(sm);