Skip to content

Commit 7a0e659

Browse files
akien-mgaakien-mga
committed
Merge pull request #102836 from raulsntos/dotnet/export-tool-button-no-storage
[.NET] Disallow `[ExportToolButton]` on members thay may store the Callable
2 parents e20f01e + f4094b5 commit 7a0e659

14 files changed

+263
-38
lines changed

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/ExportDiagnosticsTests.cs

+9
Original file line numberDiff line numberDiff line change
@@ -101,4 +101,13 @@ await CSharpSourceGeneratorVerifier<ScriptPropertiesGenerator>.Verify(
101101
new string[] { "ExportDiagnostics_GD0110_ScriptProperties.generated.cs" }
102102
);
103103
}
104+
105+
[Fact]
106+
public async void ExportToolButtonStoringCallable()
107+
{
108+
await CSharpSourceGeneratorVerifier<ScriptPropertiesGenerator>.Verify(
109+
new string[] { "ExportDiagnostics_GD0111.cs" },
110+
new string[] { "ExportDiagnostics_GD0111_ScriptProperties.generated.cs" }
111+
);
112+
}
104113
}

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/GeneratedSources/ExportDiagnostics_GD0108_ScriptProperties.generated.cs

+1-11
Original file line numberDiff line numberDiff line change
@@ -9,22 +9,12 @@ partial class ExportDiagnostics_GD0108
99
/// </summary>
1010
public new class PropertyName : global::Godot.Node.PropertyName {
1111
/// <summary>
12-
/// Cached name for the 'MyButton' field.
12+
/// Cached name for the 'MyButton' property.
1313
/// </summary>
1414
public new static readonly global::Godot.StringName @MyButton = "MyButton";
1515
}
1616
/// <inheritdoc/>
1717
[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
18-
protected override bool SetGodotClassPropertyValue(in godot_string_name name, in godot_variant value)
19-
{
20-
if (name == PropertyName.@MyButton) {
21-
this.@MyButton = global::Godot.NativeInterop.VariantUtils.ConvertTo<global::Godot.Callable>(value);
22-
return true;
23-
}
24-
return base.SetGodotClassPropertyValue(name, value);
25-
}
26-
/// <inheritdoc/>
27-
[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
2818
protected override bool GetGodotClassPropertyValue(in godot_string_name name, out godot_variant value)
2919
{
3020
if (name == PropertyName.@MyButton) {

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/GeneratedSources/ExportDiagnostics_GD0109_ScriptProperties.generated.cs

+1-11
Original file line numberDiff line numberDiff line change
@@ -9,22 +9,12 @@ partial class ExportDiagnostics_GD0109
99
/// </summary>
1010
public new class PropertyName : global::Godot.Node.PropertyName {
1111
/// <summary>
12-
/// Cached name for the 'MyButton' field.
12+
/// Cached name for the 'MyButton' property.
1313
/// </summary>
1414
public new static readonly global::Godot.StringName @MyButton = "MyButton";
1515
}
1616
/// <inheritdoc/>
1717
[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
18-
protected override bool SetGodotClassPropertyValue(in godot_string_name name, in godot_variant value)
19-
{
20-
if (name == PropertyName.@MyButton) {
21-
this.@MyButton = global::Godot.NativeInterop.VariantUtils.ConvertTo<global::Godot.Callable>(value);
22-
return true;
23-
}
24-
return base.SetGodotClassPropertyValue(name, value);
25-
}
26-
/// <inheritdoc/>
27-
[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
2818
protected override bool GetGodotClassPropertyValue(in godot_string_name name, out godot_variant value)
2919
{
3020
if (name == PropertyName.@MyButton) {

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/GeneratedSources/ExportDiagnostics_GD0110_ScriptProperties.generated.cs

+2-12
Original file line numberDiff line numberDiff line change
@@ -9,26 +9,16 @@ partial class ExportDiagnostics_GD0110
99
/// </summary>
1010
public new class PropertyName : global::Godot.Node.PropertyName {
1111
/// <summary>
12-
/// Cached name for the 'MyButton' field.
12+
/// Cached name for the 'MyButton' property.
1313
/// </summary>
1414
public new static readonly global::Godot.StringName @MyButton = "MyButton";
1515
}
1616
/// <inheritdoc/>
1717
[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
18-
protected override bool SetGodotClassPropertyValue(in godot_string_name name, in godot_variant value)
19-
{
20-
if (name == PropertyName.@MyButton) {
21-
this.@MyButton = global::Godot.NativeInterop.VariantUtils.ConvertTo<string>(value);
22-
return true;
23-
}
24-
return base.SetGodotClassPropertyValue(name, value);
25-
}
26-
/// <inheritdoc/>
27-
[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
2818
protected override bool GetGodotClassPropertyValue(in godot_string_name name, out godot_variant value)
2919
{
3020
if (name == PropertyName.@MyButton) {
31-
value = global::Godot.NativeInterop.VariantUtils.CreateFrom<string>(this.@MyButton);
21+
value = global::Godot.NativeInterop.VariantUtils.CreateFrom<int>(this.@MyButton);
3222
return true;
3323
}
3424
return base.GetGodotClassPropertyValue(name, out value);

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/GeneratedSources/ExportDiagnostics_GD0111_ScriptProperties.generated.cs

+116
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
using Godot;
2+
using Godot.NativeInterop;
3+
4+
partial class ExportDiagnostics_GD0111
5+
{
6+
#pragma warning disable CS0109 // Disable warning about redundant 'new' keyword
7+
/// <summary>
8+
/// Cached StringNames for the properties and fields contained in this class, for fast lookup.
9+
/// </summary>
10+
public new class PropertyName : global::Godot.Node.PropertyName {
11+
/// <summary>
12+
/// Cached name for the 'MyButtonGet' property.
13+
/// </summary>
14+
public new static readonly global::Godot.StringName @MyButtonGet = "MyButtonGet";
15+
/// <summary>
16+
/// Cached name for the 'MyButtonGetSet' property.
17+
/// </summary>
18+
public new static readonly global::Godot.StringName @MyButtonGetSet = "MyButtonGetSet";
19+
/// <summary>
20+
/// Cached name for the 'MyButtonGetWithBackingField' property.
21+
/// </summary>
22+
public new static readonly global::Godot.StringName @MyButtonGetWithBackingField = "MyButtonGetWithBackingField";
23+
/// <summary>
24+
/// Cached name for the 'MyButtonGetSetWithBackingField' property.
25+
/// </summary>
26+
public new static readonly global::Godot.StringName @MyButtonGetSetWithBackingField = "MyButtonGetSetWithBackingField";
27+
/// <summary>
28+
/// Cached name for the 'MyButtonOkWithCallableCreationExpression' property.
29+
/// </summary>
30+
public new static readonly global::Godot.StringName @MyButtonOkWithCallableCreationExpression = "MyButtonOkWithCallableCreationExpression";
31+
/// <summary>
32+
/// Cached name for the 'MyButtonOkWithImplicitCallableCreationExpression' property.
33+
/// </summary>
34+
public new static readonly global::Godot.StringName @MyButtonOkWithImplicitCallableCreationExpression = "MyButtonOkWithImplicitCallableCreationExpression";
35+
/// <summary>
36+
/// Cached name for the 'MyButtonOkWithCallableFromExpression' property.
37+
/// </summary>
38+
public new static readonly global::Godot.StringName @MyButtonOkWithCallableFromExpression = "MyButtonOkWithCallableFromExpression";
39+
/// <summary>
40+
/// Cached name for the '_backingField' field.
41+
/// </summary>
42+
public new static readonly global::Godot.StringName @_backingField = "_backingField";
43+
}
44+
/// <inheritdoc/>
45+
[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
46+
protected override bool SetGodotClassPropertyValue(in godot_string_name name, in godot_variant value)
47+
{
48+
if (name == PropertyName.@MyButtonGetSet) {
49+
this.@MyButtonGetSet = global::Godot.NativeInterop.VariantUtils.ConvertTo<global::Godot.Callable>(value);
50+
return true;
51+
}
52+
if (name == PropertyName.@MyButtonGetSetWithBackingField) {
53+
this.@MyButtonGetSetWithBackingField = global::Godot.NativeInterop.VariantUtils.ConvertTo<global::Godot.Callable>(value);
54+
return true;
55+
}
56+
if (name == PropertyName.@_backingField) {
57+
this.@_backingField = global::Godot.NativeInterop.VariantUtils.ConvertTo<global::Godot.Callable>(value);
58+
return true;
59+
}
60+
return base.SetGodotClassPropertyValue(name, value);
61+
}
62+
/// <inheritdoc/>
63+
[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
64+
protected override bool GetGodotClassPropertyValue(in godot_string_name name, out godot_variant value)
65+
{
66+
if (name == PropertyName.@MyButtonGet) {
67+
value = global::Godot.NativeInterop.VariantUtils.CreateFrom<global::Godot.Callable>(this.@MyButtonGet);
68+
return true;
69+
}
70+
if (name == PropertyName.@MyButtonGetSet) {
71+
value = global::Godot.NativeInterop.VariantUtils.CreateFrom<global::Godot.Callable>(this.@MyButtonGetSet);
72+
return true;
73+
}
74+
if (name == PropertyName.@MyButtonGetWithBackingField) {
75+
value = global::Godot.NativeInterop.VariantUtils.CreateFrom<global::Godot.Callable>(this.@MyButtonGetWithBackingField);
76+
return true;
77+
}
78+
if (name == PropertyName.@MyButtonGetSetWithBackingField) {
79+
value = global::Godot.NativeInterop.VariantUtils.CreateFrom<global::Godot.Callable>(this.@MyButtonGetSetWithBackingField);
80+
return true;
81+
}
82+
if (name == PropertyName.@MyButtonOkWithCallableCreationExpression) {
83+
value = global::Godot.NativeInterop.VariantUtils.CreateFrom<global::Godot.Callable>(this.@MyButtonOkWithCallableCreationExpression);
84+
return true;
85+
}
86+
if (name == PropertyName.@MyButtonOkWithImplicitCallableCreationExpression) {
87+
value = global::Godot.NativeInterop.VariantUtils.CreateFrom<global::Godot.Callable>(this.@MyButtonOkWithImplicitCallableCreationExpression);
88+
return true;
89+
}
90+
if (name == PropertyName.@MyButtonOkWithCallableFromExpression) {
91+
value = global::Godot.NativeInterop.VariantUtils.CreateFrom<global::Godot.Callable>(this.@MyButtonOkWithCallableFromExpression);
92+
return true;
93+
}
94+
if (name == PropertyName.@_backingField) {
95+
value = global::Godot.NativeInterop.VariantUtils.CreateFrom<global::Godot.Callable>(this.@_backingField);
96+
return true;
97+
}
98+
return base.GetGodotClassPropertyValue(name, out value);
99+
}
100+
/// <summary>
101+
/// Get the property information for all the properties declared in this class.
102+
/// This method is used by Godot to register the available properties in the editor.
103+
/// Do not call this method.
104+
/// </summary>
105+
[global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
106+
internal new static global::System.Collections.Generic.List<global::Godot.Bridge.PropertyInfo> GetGodotPropertyList()
107+
{
108+
var properties = new global::System.Collections.Generic.List<global::Godot.Bridge.PropertyInfo>();
109+
properties.Add(new(type: (global::Godot.Variant.Type)25, name: PropertyName.@_backingField, hint: (global::Godot.PropertyHint)0, hintString: "", usage: (global::Godot.PropertyUsageFlags)4096, exported: false));
110+
properties.Add(new(type: (global::Godot.Variant.Type)25, name: PropertyName.@MyButtonOkWithCallableCreationExpression, hint: (global::Godot.PropertyHint)39, hintString: "", usage: (global::Godot.PropertyUsageFlags)4, exported: true));
111+
properties.Add(new(type: (global::Godot.Variant.Type)25, name: PropertyName.@MyButtonOkWithImplicitCallableCreationExpression, hint: (global::Godot.PropertyHint)39, hintString: "", usage: (global::Godot.PropertyUsageFlags)4, exported: true));
112+
properties.Add(new(type: (global::Godot.Variant.Type)25, name: PropertyName.@MyButtonOkWithCallableFromExpression, hint: (global::Godot.PropertyHint)39, hintString: "", usage: (global::Godot.PropertyUsageFlags)4, exported: true));
113+
return properties;
114+
}
115+
#pragma warning restore CS0109
116+
}

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/Sources/ExportDiagnostics_GD0108.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,5 @@
44
public partial class ExportDiagnostics_GD0108 : Node
55
{
66
[ExportToolButton("")]
7-
public Callable {|GD0108:MyButton|};
7+
public Callable {|GD0108:MyButton|} => new Callable();
88
}

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/Sources/ExportDiagnostics_GD0109.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,5 @@
55
public partial class ExportDiagnostics_GD0109 : Node
66
{
77
[Export, ExportToolButton("")]
8-
public Callable {|GD0109:MyButton|};
8+
public Callable {|GD0109:MyButton|} => new Callable();
99
}

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/Sources/ExportDiagnostics_GD0110.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,5 @@
55
public partial class ExportDiagnostics_GD0110 : Node
66
{
77
[ExportToolButton("")]
8-
public string {|GD0110:MyButton|};
8+
public int {|GD0110:MyButton|} => new();
99
}

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/Sources/ExportDiagnostics_GD0111.cs

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
using Godot;
2+
using Godot.Collections;
3+
4+
[Tool]
5+
public partial class ExportDiagnostics_GD0111 : Node
6+
{
7+
private Callable _backingField;
8+
9+
[ExportToolButton("")]
10+
public Callable {|GD0111:MyButtonGet|} { get; }
11+
12+
[ExportToolButton("")]
13+
public Callable {|GD0111:MyButtonGetSet|} { get; set; }
14+
15+
[ExportToolButton("")]
16+
public Callable {|GD0111:MyButtonGetWithBackingField|} { get => _backingField; }
17+
18+
[ExportToolButton("")]
19+
public Callable {|GD0111:MyButtonGetSetWithBackingField|} { get => _backingField; set => _backingField = value; }
20+
21+
[ExportToolButton("")]
22+
public Callable MyButtonOkWithCallableCreationExpression => new Callable(this, "");
23+
24+
[ExportToolButton("")]
25+
public Callable MyButtonOkWithImplicitCallableCreationExpression => new(this, "");
26+
27+
[ExportToolButton("")]
28+
public Callable MyButtonOkWithCallableFromExpression => Callable.From(null);
29+
}

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/AnalyzerReleases.Unshipped.md

+1
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@ GD0003 | Usage | Error | ScriptPathAttributeGenerator, [Documentation](ht
66
GD0108 | Usage | Error | ScriptPropertiesGenerator, [Documentation](https://docs.godotengine.org/en/latest/tutorials/scripting/c_sharp/diagnostics/GD0108.html)
77
GD0109 | Usage | Error | ScriptPropertiesGenerator, [Documentation](https://docs.godotengine.org/en/latest/tutorials/scripting/c_sharp/diagnostics/GD0109.html)
88
GD0110 | Usage | Error | ScriptPropertiesGenerator, [Documentation](https://docs.godotengine.org/en/latest/tutorials/scripting/c_sharp/diagnostics/GD0110.html)
9+
GD0111 | Usage | Error | ScriptPropertiesGenerator, [Documentation](https://docs.godotengine.org/en/latest/tutorials/scripting/c_sharp/diagnostics/GD0111.html)

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/Common.cs

+10
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,16 @@ public static partial class Common
137137
"The exported tool button is not a Callable. The '[ExportToolButton]' attribute is only supported on members of type Callable.",
138138
helpLinkUri: string.Format(_helpLinkFormat, "GD0110"));
139139

140+
public static readonly DiagnosticDescriptor ExportToolButtonMustBeExpressionBodiedProperty =
141+
new DiagnosticDescriptor(id: "GD0111",
142+
title: "The exported tool button must be an expression-bodied property",
143+
messageFormat: "The exported tool button '{0}' must be an expression-bodied property",
144+
category: "Usage",
145+
DiagnosticSeverity.Error,
146+
isEnabledByDefault: true,
147+
"The exported tool button must be an expression-bodied property. The '[ExportToolButton]' attribute is only supported on expression-bodied properties with a 'new Callable(...)' or 'Callable.From(...)' expression.",
148+
helpLinkUri: string.Format(_helpLinkFormat, "GD0111"));
149+
140150
public static readonly DiagnosticDescriptor SignalDelegateMissingSuffixRule =
141151
new DiagnosticDescriptor(id: "GD0201",
142152
title: "The name of the delegate must end with 'EventHandler'",

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/GodotClasses.cs

+1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ public static class GodotClasses
44
{
55
public const string GodotObject = "Godot.GodotObject";
66
public const string Node = "Godot.Node";
7+
public const string Callable = "Godot.Callable";
78
public const string AssemblyHasScriptsAttr = "Godot.AssemblyHasScriptsAttribute";
89
public const string ExportAttr = "Godot.ExportAttribute";
910
public const string ExportCategoryAttr = "Godot.ExportCategoryAttribute";

‎modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/ScriptPropertiesGenerator.cs

+89
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using System.Linq;
44
using System.Text;
55
using Microsoft.CodeAnalysis;
6+
using Microsoft.CodeAnalysis.CSharp;
67
using Microsoft.CodeAnalysis.CSharp.Syntax;
78
using Microsoft.CodeAnalysis.Text;
89

@@ -465,6 +466,94 @@ MarshalType marshalType
465466
return null;
466467
}
467468

469+
if (exportToolButtonAttr != null && propertySymbol != null)
470+
{
471+
if (!PropertyIsExpressionBodiedAndReturnsNewCallable(context.Compilation, propertySymbol))
472+
{
473+
context.ReportDiagnostic(Diagnostic.Create(
474+
Common.ExportToolButtonMustBeExpressionBodiedProperty,
475+
propertySymbol.Locations.FirstLocationWithSourceTreeOrDefault(),
476+
propertySymbol.ToDisplayString()
477+
));
478+
return null;
479+
}
480+
481+
static bool PropertyIsExpressionBodiedAndReturnsNewCallable(Compilation compilation, IPropertySymbol? propertySymbol)
482+
{
483+
if (propertySymbol == null)
484+
{
485+
return false;
486+
}
487+
488+
var propertyDeclarationSyntax = propertySymbol.DeclaringSyntaxReferences
489+
.Select(r => r.GetSyntax() as PropertyDeclarationSyntax).FirstOrDefault();
490+
if (propertyDeclarationSyntax == null || propertyDeclarationSyntax.Initializer != null)
491+
{
492+
return false;
493+
}
494+
495+
if (propertyDeclarationSyntax.AccessorList != null)
496+
{
497+
var accessors = propertyDeclarationSyntax.AccessorList.Accessors;
498+
foreach (var accessor in accessors)
499+
{
500+
if (!accessor.IsKind(SyntaxKind.GetAccessorDeclaration))
501+
{
502+
// Only getters are allowed.
503+
return false;
504+
}
505+
506+
if (!ExpressionBodyReturnsNewCallable(compilation, accessor.ExpressionBody))
507+
{
508+
return false;
509+
}
510+
}
511+
}
512+
else if (!ExpressionBodyReturnsNewCallable(compilation, propertyDeclarationSyntax.ExpressionBody))
513+
{
514+
return false;
515+
}
516+
517+
return true;
518+
}
519+
520+
static bool ExpressionBodyReturnsNewCallable(Compilation compilation, ArrowExpressionClauseSyntax? expressionSyntax)
521+
{
522+
if (expressionSyntax == null)
523+
{
524+
return false;
525+
}
526+
527+
var semanticModel = compilation.GetSemanticModel(expressionSyntax.SyntaxTree);
528+
529+
switch (expressionSyntax.Expression)
530+
{
531+
case ImplicitObjectCreationExpressionSyntax creationExpression:
532+
// We already validate that the property type must be 'Callable'
533+
// so we can assume this constructor is valid.
534+
return true;
535+
536+
case ObjectCreationExpressionSyntax creationExpression:
537+
var typeSymbol = semanticModel.GetSymbolInfo(creationExpression.Type).Symbol as ITypeSymbol;
538+
if (typeSymbol != null)
539+
{
540+
return typeSymbol.FullQualifiedNameOmitGlobal() == GodotClasses.Callable;
541+
}
542+
break;
543+
544+
case InvocationExpressionSyntax invocationExpression:
545+
var methodSymbol = semanticModel.GetSymbolInfo(invocationExpression).Symbol as IMethodSymbol;
546+
if (methodSymbol != null && methodSymbol.Name == "From")
547+
{
548+
return methodSymbol.ContainingType.FullQualifiedNameOmitGlobal() == GodotClasses.Callable;
549+
}
550+
break;
551+
}
552+
553+
return false;
554+
}
555+
}
556+
468557
var memberType = propertySymbol?.Type ?? fieldSymbol!.Type;
469558

470559
var memberVariantType = MarshalUtils.ConvertMarshalTypeToVariantType(marshalType)!.Value;

0 commit comments

Comments
 (0)