Skip to content

Commit

Permalink
Clean diagnostic rules
Browse files Browse the repository at this point in the history 
Move the following diagnostics into static readonly fields: GD0101, GD0102, GD0103, GD0104, GD0105, GD0106, GD0107, GD0201, GD0202, GD0203, GD0301, GD0302, GD0303, GD0401, GD0402.

To be more consistent, the titles for the following diagnostics were modified: GD0101, GD0105, GD0106, GD0302, GD0303, GD0401, GD0402. A subsequent update of the documentation repo is needed.

Tests for the following diagnostics were created: GD0201, GD0202, GD0203.
  • Loading branch information
@paulloz
paulloz committed Feb 18, 2024
1 parent 9ae8a0e commit 5981886
Show file tree
Hide file tree
Showing 11 changed files with 267 additions and 430 deletions.
16 changes: 8 additions & 8 deletions ...s/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/CSharpSourceGeneratorVerifier.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,15 @@ public static Test MakeVerifier(ICollection<string> sources, ICollection<string>
build_property.GodotProjectDir = {Constants.ExecutingAssemblyPath}
"""));

verifier.TestState.Sources.AddRange(sources.Select(source =>
{
return (source, SourceText.From(File.ReadAllText(Path.Combine(Constants.SourceFolderPath, source))));
}));
verifier.TestState.Sources.AddRange(sources.Select(source => (
source,
SourceText.From(File.ReadAllText(Path.Combine(Constants.SourceFolderPath, source)))
)));

verifier.TestState.GeneratedSources.AddRange(generatedSources.Select(generatedSource =>
{
return (FullGeneratedSourceName(generatedSource), SourceText.From(File.ReadAllText(Path.Combine(Constants.GeneratedSourceFolderPath, generatedSource)), Encoding.UTF8));
}));
verifier.TestState.GeneratedSources.AddRange(generatedSources.Select(generatedSource => (
FullGeneratedSourceName(generatedSource),
SourceText.From(File.ReadAllText(Path.Combine(Constants.GeneratedSourceFolderPath, generatedSource)), Encoding.UTF8)
)));

return verifier;
}
Expand Down
11 changes: 11 additions & 0 deletions ...s/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/Sources/EventSignals.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,15 @@ public partial class EventSignals : GodotObject
{
[Signal]
public delegate void MySignalEventHandler(string str, int num);

private struct MyStruct { }

[Signal]
private delegate void {|GD0201:MyInvalidSignal|}();

[Signal]
private delegate void MyInvalidParameterTypeSignalEventHandler(MyStruct {|GD0202:myStruct|});

[Signal]
private delegate MyStruct {|GD0203:MyInvalidReturnTypeSignalEventHandler|}();
}
6 changes: 3 additions & 3 deletions .../editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/Sources/GlobalClass.GD0401.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ public partial class CustomGlobalClass2 : Node
}

// This raises a GD0401 diagnostic error: global classes must inherit from GodotObject
{|GD0401:[GlobalClass]
public partial class CustomGlobalClass3
[GlobalClass]
public partial class {|GD0401:CustomGlobalClass3|}
{

}|}
}
6 changes: 3 additions & 3 deletions .../editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/TestData/Sources/GlobalClass.GD0402.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ public partial class CustomGlobalClass : GodotObject
}

// This raises a GD0402 diagnostic error: global classes can't have any generic type parameter
{|GD0402:[GlobalClass]
public partial class CustomGlobalClass<T> : GodotObject
[GlobalClass]
public partial class {|GD0402:CustomGlobalClass|}<T> : GodotObject
{

}|}
}
486 changes: 104 additions & 382 deletions modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/Common.cs
View file

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/ExtensionMethods.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,11 @@ MarshalUtils.TypeCache typeCache
}
}

public static Location? FirstLocationWithSourceTreeOrDefault(this IEnumerable<Location> locations)
{
return locations.FirstOrDefault(location => location.SourceTree != null) ?? locations.FirstOrDefault();
}

public static string Path(this Location location)
=> location.SourceTree?.GetLineSpan(location.SourceSpan).Path
?? location.GetLineSpan().Path;
Expand Down
18 changes: 15 additions & 3 deletions modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/GlobalClassAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
namespace Godot.SourceGenerators
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public class GlobalClassAnalyzer : DiagnosticAnalyzer
public sealed class GlobalClassAnalyzer : DiagnosticAnalyzer
{
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics
=> ImmutableArray.Create(
Expand All @@ -33,10 +33,22 @@ private void AnalyzeNode(SyntaxNodeAnalysisContext context)
return;

if (typeSymbol.IsGenericType)
Common.ReportGlobalClassMustNotBeGeneric(context, typeClassDecl, typeSymbol);
{
context.ReportDiagnostic(Diagnostic.Create(
Common.GlobalClassMustNotBeGenericRule,
typeSymbol.Locations.FirstLocationWithSourceTreeOrDefault(),
typeSymbol.ToDisplayString()
));
}

if (!typeSymbol.InheritsFrom("GodotSharp", GodotClasses.GodotObject))
Common.ReportGlobalClassMustDeriveFromGodotObject(context, typeClassDecl, typeSymbol);
{
context.ReportDiagnostic(Diagnostic.Create(
Common.GlobalClassMustDeriveFromGodotObjectRule,
typeSymbol.Locations.FirstLocationWithSourceTreeOrDefault(),
typeSymbol.ToDisplayString()
));
}
}
}
}
40 changes: 30 additions & 10 deletions modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/MustBeVariantAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
namespace Godot.SourceGenerators
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public class MustBeVariantAnalyzer : DiagnosticAnalyzer
public sealed class MustBeVariantAnalyzer : DiagnosticAnalyzer
{
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics
=> ImmutableArray.Create(
Expand Down Expand Up @@ -62,7 +62,11 @@ private void AnalyzeNode(SyntaxNodeAnalysisContext context)
{
if (!typeParamSymbol.GetAttributes().Any(a => a.AttributeClass?.IsGodotMustBeVariantAttribute() ?? false))
{
Common.ReportGenericTypeParameterMustBeVariantAnnotated(context, typeSyntax, typeSymbol);
context.ReportDiagnostic(Diagnostic.Create(
Common.GenericTypeParameterMustBeVariantAnnotatedRule,
typeSyntax.GetLocation(),
typeSymbol.ToDisplayString()
));
}
continue;
}
Expand All @@ -71,8 +75,11 @@ private void AnalyzeNode(SyntaxNodeAnalysisContext context)

if (marshalType is null)
{
Common.ReportGenericTypeArgumentMustBeVariant(context, typeSyntax, typeSymbol);
continue;
context.ReportDiagnostic(Diagnostic.Create(
Common.GenericTypeArgumentMustBeVariantRule,
typeSyntax.GetLocation(),
typeSymbol.ToDisplayString()
));
}
}
}
Expand Down Expand Up @@ -106,8 +113,15 @@ private bool IsInsideDocumentation(SyntaxNode? syntax)
/// <param name="parentSymbol">The symbol retrieved for the parent node syntax.</param>
/// <param name="typeArgumentSyntax">The type node syntax of the argument type to check.</param>
/// <param name="typeArgumentSymbol">The symbol retrieved for the type node syntax.</param>
/// <param name="typeArgumentIndex"></param>
/// <returns><see langword="true"/> if the type must be variant and must be analyzed.</returns>
private bool ShouldCheckTypeArgument(SyntaxNodeAnalysisContext context, SyntaxNode parentSyntax, ISymbol parentSymbol, TypeSyntax typeArgumentSyntax, ITypeSymbol typeArgumentSymbol, int typeArgumentIndex)
private bool ShouldCheckTypeArgument(
SyntaxNodeAnalysisContext context,
SyntaxNode parentSyntax,
ISymbol parentSymbol,
TypeSyntax typeArgumentSyntax,
ITypeSymbol typeArgumentSymbol,
int typeArgumentIndex)
{
ITypeParameterSymbol? typeParamSymbol = parentSymbol switch
{
Expand All @@ -120,18 +134,24 @@ IMethodSymbol methodSymbol when parentSyntax.Parent is AttributeSyntax &&

INamedTypeSymbol { TypeParameters.Length: > 0 } typeSymbol
=> typeSymbol.TypeParameters[typeArgumentIndex],

_
=> null
};

if (typeParamSymbol == null)
if (typeParamSymbol != null)
{
Common.ReportTypeArgumentParentSymbolUnhandled(context, typeArgumentSyntax, parentSymbol);
return false;
return typeParamSymbol.GetAttributes()
.Any(a => a.AttributeClass?.IsGodotMustBeVariantAttribute() ?? false);
}

return typeParamSymbol.GetAttributes()
.Any(a => a.AttributeClass?.IsGodotMustBeVariantAttribute() ?? false);
context.ReportDiagnostic(Diagnostic.Create(
Common.TypeArgumentParentSymbolUnhandledRule,
typeArgumentSyntax.GetLocation(),
parentSymbol.ToDisplayString()
));

return false;
}
}
}
12 changes: 10 additions & 2 deletions modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/ScriptPropertiesGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,14 +443,22 @@ MarshalType marshalType
if (propertySymbol.GetMethod == null)
{
// This should never happen, as we filtered WriteOnly properties, but just in case.
Common.ReportExportedMemberIsWriteOnly(context, propertySymbol);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedPropertyIsWriteOnlyRule,
propertySymbol.Locations.FirstLocationWithSourceTreeOrDefault(),
propertySymbol.ToDisplayString()
));
return null;
}

if (propertySymbol.SetMethod == null || propertySymbol.SetMethod.IsInitOnly)
{
// This should never happen, as we filtered ReadOnly properties, but just in case.
Common.ReportExportedMemberIsReadOnly(context, propertySymbol);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedMemberIsReadOnlyRule,
propertySymbol.Locations.FirstLocationWithSourceTreeOrDefault(),
propertySymbol.ToDisplayString()
));
return null;
}
}
Expand Down
72 changes: 58 additions & 14 deletions modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/ScriptPropertyDefValGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,33 +131,55 @@ void AppendPartialContainingTypeDeclarations(INamedTypeSymbol? containingType)
{
if (property.IsStatic)
{
Common.ReportExportedMemberIsStatic(context, property);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedMemberIsStaticRule,
property.Locations.FirstLocationWithSourceTreeOrDefault(),
property.ToDisplayString()
));
continue;
}

if (property.IsIndexer)
{
Common.ReportExportedMemberIsIndexer(context, property);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedMemberIsIndexerRule,
property.Locations.FirstLocationWithSourceTreeOrDefault(),
property.ToDisplayString()
));
continue;
}

// TODO: We should still restore read-only properties after reloading assembly. Two possible ways: reflection or turn RestoreGodotObjectData into a constructor overload.
// Ignore properties without a getter, without a setter or with an init-only setter. Godot properties must be both readable and writable.
// TODO: We should still restore read-only properties after reloading assembly.
// Two possible ways: reflection or turn RestoreGodotObjectData into a constructor overload.
// Ignore properties without a getter, without a setter or with an init-only setter.
// Godot properties must be both readable and writable.
if (property.IsWriteOnly)
{
Common.ReportExportedMemberIsWriteOnly(context, property);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedPropertyIsWriteOnlyRule,
property.Locations.FirstLocationWithSourceTreeOrDefault(),
property.ToDisplayString()
));
continue;
}

if (property.IsReadOnly || property.SetMethod!.IsInitOnly)
{
Common.ReportExportedMemberIsReadOnly(context, property);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedMemberIsReadOnlyRule,
property.Locations.FirstLocationWithSourceTreeOrDefault(),
property.ToDisplayString()
));
continue;
}

if (property.ExplicitInterfaceImplementations.Length > 0)
{
Common.ReportExportedMemberIsExplicitInterfaceImplementation(context, property);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedMemberIsExplicitInterfaceImplementationRule,
property.Locations.FirstLocationWithSourceTreeOrDefault(),
property.ToDisplayString()
));
continue;
}

Expand All @@ -166,7 +188,11 @@ void AppendPartialContainingTypeDeclarations(INamedTypeSymbol? containingType)

if (marshalType == null)
{
Common.ReportExportedMemberTypeNotSupported(context, property);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedMemberTypeIsNotSupportedRule,
property.Locations.FirstLocationWithSourceTreeOrDefault(),
property.ToDisplayString()
));
continue;
}

Expand All @@ -175,7 +201,10 @@ void AppendPartialContainingTypeDeclarations(INamedTypeSymbol? containingType)
if (!symbol.InheritsFrom("GodotSharp", "Godot.Node") &&
propertyType.InheritsFrom("GodotSharp", "Godot.Node"))
{
Common.ReportOnlyNodesShouldExportNodes(context, property);
context.ReportDiagnostic(Diagnostic.Create(
Common.OnlyNodesShouldExportNodesRule,
property.Locations.FirstLocationWithSourceTreeOrDefault()
));
}
}

Expand All @@ -194,7 +223,7 @@ void AppendPartialContainingTypeDeclarations(INamedTypeSymbol? containingType)
else
{
var propertyGet = propertyDeclarationSyntax.AccessorList?.Accessors
.Where(a => a.Keyword.IsKind(SyntaxKind.GetKeyword)).FirstOrDefault();
.FirstOrDefault(a => a.Keyword.IsKind(SyntaxKind.GetKeyword));
if (propertyGet != null)
{
if (propertyGet.ExpressionBody != null)
Expand Down Expand Up @@ -253,15 +282,23 @@ void AppendPartialContainingTypeDeclarations(INamedTypeSymbol? containingType)
{
if (field.IsStatic)
{
Common.ReportExportedMemberIsStatic(context, field);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedMemberIsStaticRule,
field.Locations.FirstLocationWithSourceTreeOrDefault(),
field.ToDisplayString()
));
continue;
}

// TODO: We should still restore read-only fields after reloading assembly. Two possible ways: reflection or turn RestoreGodotObjectData into a constructor overload.
// Ignore properties without a getter or without a setter. Godot properties must be both readable and writable.
if (field.IsReadOnly)
{
Common.ReportExportedMemberIsReadOnly(context, field);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedMemberIsReadOnlyRule,
field.Locations.FirstLocationWithSourceTreeOrDefault(),
field.ToDisplayString()
));
continue;
}

Expand All @@ -270,7 +307,11 @@ void AppendPartialContainingTypeDeclarations(INamedTypeSymbol? containingType)

if (marshalType == null)
{
Common.ReportExportedMemberTypeNotSupported(context, field);
context.ReportDiagnostic(Diagnostic.Create(
Common.ExportedMemberTypeIsNotSupportedRule,
field.Locations.FirstLocationWithSourceTreeOrDefault(),
field.ToDisplayString()
));
continue;
}

Expand All @@ -279,7 +320,10 @@ void AppendPartialContainingTypeDeclarations(INamedTypeSymbol? containingType)
if (!symbol.InheritsFrom("GodotSharp", "Godot.Node") &&
fieldType.InheritsFrom("GodotSharp", "Godot.Node"))
{
Common.ReportOnlyNodesShouldExportNodes(context, field);
context.ReportDiagnostic(Diagnostic.Create(
Common.OnlyNodesShouldExportNodesRule,
field.Locations.FirstLocationWithSourceTreeOrDefault()
));
}
}

Expand Down
Loading

0 comments on commit 5981886

Please sign in to comment.