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.

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

feat: Roslyn Diagnostics Analyzer for empty constructor #2104

Merged
merged 32 commits into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions sources/Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
<PackageVersion Include="Microsoft.Management.Infrastructure" Version="3.0.0-preview.4" />
<PackageVersion Include="Microsoft.NETCore.Platforms" Version="7.0.4" />
<PackageVersion Include="Microsoft.SourceLink.GitHub" Version="8.0.0" />
<PackageVersion Include="PolySharp" Version="1.14.1" />
<PackageVersion Include="ServiceWire" Version="5.5.4" />
<PackageVersion Include="SharpDX" Version="4.2.0" />
<PackageVersion Include="SharpDX.D3DCompiler" Version="4.2.0" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public void Error_On_file_scope_Class_with_DataContract()
string sourceCode = "using Stride.Core; [DataContract] file class FileScopeClass { }";
TestHelper.ExpectDiagnosticsError(sourceCode, STRDIAG001InvalidDataContract.DiagnosticId);
}
[Fact(Skip = "file scoped classes won't compile")]
[Fact]
public void No_Error_On_file_scope_Class_without_DataContract()
{
string sourceCode = "using Stride.Core; file class FileScopeClass { }";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Stride.Core.CompilerServices.Analyzers;
using Xunit;

namespace Stride.Core.CompilerServices.Tests.AnalyzerTests;
public class STRDIAG010_Test
{
[Fact]
public void No_Error_On_Default_Constructor()
{
string sourceCode = string.Format(ClassTemplates.BasicClassTemplate, " ");
TestHelper.ExpectNoDiagnosticsErrors(sourceCode);
}
[Fact]
public void No_Error_On_Empty_Constructor()
{
string sourceCode = string.Format(ClassTemplates.BasicClassTemplate, "public ValidCollection() { }");
TestHelper.ExpectNoDiagnosticsErrors(sourceCode);
}
[Fact]
public void No_Error_On_Empty_And_Other_Constructor()
{
string sourceCode = string.Format(ClassTemplates.BasicClassTemplate, "public ValidCollection() { } public ValidCollection(int x) { }");
TestHelper.ExpectNoDiagnosticsErrors(sourceCode);
}
[Fact]
public void Error_On_No_Empty_Constructor()
{
string sourceCode = string.Format(ClassTemplates.BasicClassTemplate, "public ValidCollection(int x) { }");
TestHelper.ExpectDiagnosticsError(sourceCode,STRDIAG010InvalidConstructor.DiagnosticId);
}
[Fact]
public void Error_On_DataContract_Inherited()
{
string sourceCode = string.Format(ClassTemplates.InheritedDataContract, "public Inherited(int x) { }");
TestHelper.ExpectDiagnosticsError(sourceCode, STRDIAG010InvalidConstructor.DiagnosticId);
}
[Fact]
public void Error_On_Primary_Constructor()
{
string sourceCode = string.Format(ClassTemplates.PrimaryConstructorTemplate, "");
TestHelper.ExpectDiagnosticsError(sourceCode, STRDIAG010InvalidConstructor.DiagnosticId);
}
[Fact]
public void No_Error_On_Flipped_DataContract_Parameters()
{
string sourceCode = string.Format(ClassTemplates.DataContractArgumentsTemplate, "");
TestHelper.ExpectNoDiagnosticsErrors(sourceCode);
}
}
31 changes: 31 additions & 0 deletions sources/core/Stride.Core.CompilerServices.Tests/ClassTemplates.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,37 @@ public class ValidCollection

{0} {1} X {{ get; set; }}
}}
";
public const string InheritedDataContract = @"
using Stride.Core;
using System;
[DataContract(Inherited = true)]
public class Base {{ }}

public class Inherited : Base
{{
{0}
}}
";

public const string PrimaryConstructorTemplate = @"
using Stride.Core;
using System;
[DataContract]
public class ValidCollection(int x)
{{
{0}
}}
";
public const string DataContractArgumentsTemplate = @"
[DataContract(Inherited = true,DefaultMemberMode = DataMemberMode.Assign)]
public struct ValidCollection
{{
}}
[DataContract(DefaultMemberMode = DataMemberMode.Assign,Inherited = true)]
public struct ValidCollection2
{{
{0}
}}
";
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ private static Compilation CreateCompilation(string assemblyName, string source)
// System.Private.CoreLib.dll
MetadataReference.CreateFromFile($"{assembliesDirectory}/System.Private.CoreLib.dll"),
// Stride.Core.dll
MetadataReference.CreateFromFile($"{assembliesDirectory}/Stride.Core.dll"),
MetadataReference.CreateFromFile(typeof(Stride.Core.DataContractAttribute).GetTypeInfo().Assembly.Location),
},
new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary));
public static DiagnosticAnalyzer[] AllAnalyzers => typeof(DiagnosticsAnalyzerHelper).Assembly.GetTypes()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\..\tests\xunit.runner.stride\xunit.runner.stride.csproj" />
<ProjectReference Include="..\Stride.Core\Stride.Core.csproj" ReferenceOutputAssembly="false" OutputItemType="Content" CopyToOutputDirectory="Always" />
<ProjectReference Include="..\Stride.Core\Stride.Core.csproj" ReferenceOutputAssembly="true" OutputItemType="Content" CopyToOutputDirectory="Always" />
IXLLEGACYIXL marked this conversation as resolved.
Show resolved Hide resolved
<ProjectReference Include="..\Stride.Core.CompilerServices\Stride.Core.CompilerServices.csproj" />

<PackageReference Include="Microsoft.CodeAnalysis.CSharp" PrivateAssets="all" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
; Shipped analyzer releases
; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md

## Release 1.0

### New Rules

Rule ID | Category | Severity | Notes
--------|----------|----------|-------
STRDIAG000 | Serialization | Warning | STRDIAG000AttributeContradiction
STRDIAG001 | Serialization | Warning | STRDIAG001InvalidDataContract
STRDIAG002 | Serialization | Warning | STRDIAG002InvalidContentMode
STRDIAG003 | Serialization | Warning | STRDIAG003InaccessibleMember
STRDIAG004 | Serialization | Warning | STRDIAG004PropertyWithNoGetter
STRDIAG005 | Serialization | Warning | STRDIAG005ReadonlyMemberTypeIsNotSupported
STRDIAG006 | Serialization | Warning | STRDIAG006InvalidAssignMode
STRDIAG007 | Serialization | Warning | STRDIAG007DataMemberOnDelegate
STRDIAG008 | Serialization | Warning | STRDIAG008FixedFieldInStructs
STRDIAG009 | Serialization | Warning | STRDIAG009InvalidDictionaryKey
STRDIAG010 | Serialization | Warning | STRDIAG010InvalidConstructor
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
; Unshipped analyzer release
; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md

Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,10 @@ private static void AnalyzeCompilationStart(CompilationStartAnalysisContext cont
{
var dataContractAttribute = WellKnownReferences.DataContractAttribute(context.Compilation);


if (dataContractAttribute is null)
return;

context.RegisterSymbolAction(symbolContext => AnalyzeSymbol(symbolContext, dataContractAttribute), SymbolKind.NamedType);

}

private static void AnalyzeSymbol(SymbolAnalysisContext context, INamedTypeSymbol dataContractAttribute)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ public class STRDIAG004PropertyWithNoGetter : DiagnosticAnalyzer
{
public const string DiagnosticId = "STRDIAG004";
private const string Title = "Property with no Getter";
private const string NonExistentGetterMessageFormat = "The property '{0}' with [DataMember] does not have a getter which is required for serialization.";
private const string NonExistentGetterMessageFormat = "The property '{0}' with [DataMember] does not have a getter which is required for serialization";
private const string InvalidAccessibilityOnGetterMessageFormat = "The property '{0}' with [DataMember] does not have an accessible getter which is required for serialization. A public/internal/internal protected getter is expected.";
private const string Category = DiagnosticCategory.Serialization;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ private static void AnalyzeField(SymbolAnalysisContext context, INamedTypeSymbol
if (!symbol.IsVisibleToSerializer(dataMemberAttribute))
return;


if (!symbol.IsReadOnly)
return;
var fieldType = symbol.Type;
Expand All @@ -66,7 +65,6 @@ private static void AnalyzeProperty(SymbolAnalysisContext context, INamedTypeSym
{
var propertySymbol = (IPropertySymbol)context.Symbol;


if (!propertySymbol.HasAttribute(dataMemberAttribute))
return;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ private static void AnalyzeSymbol(SymbolAnalysisContext context, INamedTypeSymbo
{
var propertySymbol = (IPropertySymbol)context.Symbol;


if (!propertySymbol.HasAttribute(dataMemberAttribute))
return;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ public class STRDIAG008FixedFieldInStructs : DiagnosticAnalyzer
{
public const string DiagnosticId = "STRDIAG008";
private const string Title = "Invalid Struct Member";
private const string MessageFormat = "Struct members with the 'fixed' Modifier are not supported as a Serialization target on member '{0}'.";
private const string MessageFormat = "Struct members with the 'fixed' Modifier are not supported as a Serialization target on member '{0}'";
private const string Category = DiagnosticCategory.Serialization;

private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Text;
using Microsoft.CodeAnalysis;
using Stride.Core.CompilerServices.Common;

namespace Stride.Core.CompilerServices.Analyzers;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public class STRDIAG010InvalidConstructor : DiagnosticAnalyzer
{
public const string DiagnosticId = "STRDIAG010";
private const string Title = "Invalid Constructor";
private const string MessageFormat = "The Type '{0}' doesn't have a public parameterless constructor, which is needed for Serialization";
private const string Category = DiagnosticCategory.Serialization;

private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(
DiagnosticId,
Title,
MessageFormat,
Category,
DiagnosticSeverity.Warning,
isEnabledByDefault: true,
helpLinkUri: string.Format(DiagnosticCategory.LinkFormat, DiagnosticId));

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get { return ImmutableArray.Create(Rule); } }

public override void Initialize(AnalysisContext context)
{
context.EnableConcurrentExecution();
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics);
context.RegisterCompilationStartAction(AnalyzeCompilationStart);
}

private static void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
{
var dataContractAttribute = WellKnownReferences.DataContractAttribute(context.Compilation);

if (dataContractAttribute is null)
return;

context.RegisterSymbolAction(symbolContext => AnalyzeSymbol(symbolContext, dataContractAttribute), SymbolKind.NamedType);

}

private static void AnalyzeSymbol(SymbolAnalysisContext context, INamedTypeSymbol dataContractAttribute)
{
var symbol = (INamedTypeSymbol)context.Symbol;
if (symbol.IsAbstract)
return;

if (symbol.HasAttribute(dataContractAttribute))
{
TryReportDiagnostics(symbol, context);
return;
}

var type = symbol.BaseType;
bool isInherited = false;
while (type != null)
{
// Check if the type has the specified DataContractAttribute through inheritance
if (type.TryGetAttribute(dataContractAttribute,out var datacontractData) && datacontractData.AttributeConstructor is not null)
{
if (datacontractData is { NamedArguments: [.., { Key: "Inherited" , Value: TypedConstant inherited } ] })
{
isInherited = (bool)inherited.Value!;
}
break;
}
type = type.BaseType;
}
if(isInherited)
IXLLEGACYIXL marked this conversation as resolved.
Show resolved Hide resolved
{
TryReportDiagnostics(symbol, context);
}
}
private static void TryReportDiagnostics(INamedTypeSymbol symbol,SymbolAnalysisContext context)
{
if (HasPublicEmptyConstructor(symbol))
{
return;
}
else
{
Rule.ReportDiagnostics(context, symbol);
}
}
private static bool HasPublicEmptyConstructor(INamedTypeSymbol type)
=> type.Constructors.Any(x => x.Parameters.Length == 0 && x.DeclaredAccessibility == Accessibility.Public);
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using System.Diagnostics.CodeAnalysis;

namespace Stride.Core.CompilerServices.Common;
internal static class SymbolExtensions
{
Expand All @@ -13,10 +15,23 @@ public static bool IsVisibleToSerializer(this ISymbol symbol, bool hasDataMember
var accessibility = symbol.DeclaredAccessibility;

if (hasDataMemberAttribute)
return accessibility == Accessibility.Public || accessibility == Accessibility.Internal || accessibility == Accessibility.ProtectedOrInternal;
return accessibility is Accessibility.Public or Accessibility.Internal or Accessibility.ProtectedOrInternal;
return accessibility == Accessibility.Public;
}

/// <summary>
/// Tries to get <paramref name="attribute"/> on the <paramref name="symbol"/>
/// </summary>
/// <param name="symbol">The Symbol to search on</param>
/// <param name="attribute">The attribute looking for</param>
/// <param name="attributeData">The <see cref="AttributeData"/> of the Attribute if it is found</param>
/// <returns>true if the attribute is found, else false</returns>
public static bool TryGetAttribute(this ISymbol symbol, INamedTypeSymbol attribute, [MaybeNullWhen(false)] out AttributeData attributeData)
{
attributeData = symbol.GetAttributes().FirstOrDefault(attr => attr.AttributeClass?.OriginalDefinition.Equals(attribute, SymbolEqualityComparer.Default) ?? false)!;
return attributeData is not null;
}

/// <summary>
/// An Immutable Type is treated if its a non Reference Type ie class
/// A struct and a string are treated as Immutable as the Yaml Serializer can't handle value Types with it's reflection.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<Description>Code generators for Stride.Core and its dependents</Description>
<LangVersion>preview</LangVersion>
<LangVersion>latest</LangVersion>
<EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
<StrideSkipAutoPack>true</StrideSkipAutoPack>
<Nullable>enable</Nullable>
<!--<DefineConstants>LAUNCH_DEBUGGER;$(DefineConstants)</DefineConstants>-->
Expand All @@ -18,10 +19,15 @@
<Link>Properties\SharedAssemblyInfo.cs</Link>
</Compile>
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.CodeAnalysis.Analyzers" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" />
<PackageReference Include="PolySharp">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>

Comment on lines +25 to +29
Copy link
Member

Choose a reason for hiding this comment

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

Where is it used? Is it required at runtime?

Copy link
Collaborator Author

@IXLLEGACYIXL IXLLEGACYIXL Jan 13, 2024

Choose a reason for hiding this comment

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

https://github.com/stride3d/stride/pull/2104/files#diff-015a4d048579b446685ff8c8dbddb5302ff100cd3fbfc1df1cef57c74a4032b6R66

its used here for pattern matching as i got told that i should do it that way ( funny thing is that this part is the only one that doesnt work )

Copy link
Member

Choose a reason for hiding this comment

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

Patter-matching is a feature of the language, it doesn't require an additional dependency. Your code seems to compile without Polysharp what does it add?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

grafik
System.Index and System.Index.GetOffset

ive changed it to a foreach loop now and added the analyzer removal back in

Copy link
Member

Choose a reason for hiding this comment

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

Just to keep a trace of what we discussed on Discord:

  1. netstandard2.0 is a requirement for analyzers/source generators. Nothing we can do about.
  2. because of that, certain languages features aren't available (like index/spread operations on collections in C# 12), unless we use a library such as PolySharp to fill the gaps.


</ItemGroup>

<ItemGroup>
Expand Down