Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[.NET] Clean diagnostic rules #88469

Merged
merged 1 commit into from
Feb 18, 2024
Merged

[.NET] Clean diagnostic rules #88469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions ...s/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/CSharpSourceGeneratorVerifier.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop

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
Open in desktop
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
Open in desktop
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
Open in desktop
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()
));
Comment on lines +148 to +152
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like GD0303 shouldn't be a diagnostic, I was originally throwing an exception instead but neikeq was against it because it would stop the analyzer (see #64731 (comment)).

The way I see it diagnostics are problems in user code, but this is an internal error (a bug in the generator) and it should be unreachable anyway because we should handle all the kinds of type argument symbols. So I don't think it should be a diagnostic.

I would love to hear what you think we should do here, but it's not a blocker to merge this PR.


return false;
}
}
}
12 changes: 10 additions & 2 deletions modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators/ScriptPropertiesGenerator.cs
View file
Open in desktop
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
Open in desktop
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
Loading