-
Notifications
You must be signed in to change notification settings - Fork 19
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
fix: Correctly support incremental source generation #62
Changes from 12 commits
17ca7b0
ceb45e2
5797558
cdff4c9
d12bf22
018b0d8
afd36b0
762c681
1eae2be
60fc464
cacd1dd
75f67a8
7e0e70c
927e2a7
0df78df
6d11303
5cadbe9
df2966c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,3 +5,5 @@ obj | |
*.DotSettings.user | ||
.vs | ||
.DS_Store | ||
|
||
Generator.Equals.Tests/Generated/** |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
extern alias genEquals; | ||
using System.Collections.Immutable; | ||
using Microsoft.CodeAnalysis.CSharp; | ||
|
||
namespace Generator.Equals.Tests.DynamicGeneration; | ||
|
||
public class Base_Assertions | ||
{ | ||
// Check if immutable arrays are comparable | ||
[Fact] | ||
public void Immutable_Arrays_Eqatable() | ||
JKamsker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
var a = ImmutableArray.Create(1, 2, 3); | ||
var b = ImmutableArray.Create(1, 2, 3); | ||
Assert.Equal(a, b); | ||
} | ||
|
||
[Fact] | ||
public void Immutable_Arrays_Comparable() | ||
{ | ||
var a = ImmutableArray.Create(1, 2, 3); | ||
var b = ImmutableArray.Create(1, 2, 3); | ||
Assert.True(a.SequenceEqual(b)); | ||
} | ||
|
||
// Check if equality type model is comparable by value | ||
[Fact] | ||
public void EqualityTypeModel_Eqatable() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: typo |
||
{ | ||
var a = CreateEqualityTypeModelMock(); | ||
var b = CreateEqualityTypeModelMock(); | ||
Assert.Equal(a, b); | ||
} | ||
|
||
[Fact] | ||
public void AttributesMetadata_Equatable() | ||
{ | ||
var a = AttributesMetadata.CreateDefault(); | ||
var b = AttributesMetadata.CreateDefault(); | ||
Assert.True(a.Equals(b)); | ||
} | ||
|
||
internal static EqualityTypeModel CreateEqualityTypeModelMock() | ||
{ | ||
var containingSymbols = ImmutableArray.Create<ContainingSymbol>( | ||
new NamespaceContainingSymbol { Name = "Namespace1" }, | ||
new TypeContainingSymbol { Name = "ContainingType", Kind = null } | ||
); | ||
|
||
var attributesMetadata = AttributesMetadata.Instance; | ||
|
||
var equalityMemberModels = ImmutableArray.Create( | ||
new EqualityMemberModel | ||
{ | ||
PropertyName = "Property1", | ||
TypeName = "int", | ||
EqualityType = EqualityType.DefaultEquality | ||
}, | ||
new EqualityMemberModel | ||
{ | ||
PropertyName = "Property2", | ||
TypeName = "string", | ||
EqualityType = EqualityType.StringEquality, | ||
StringComparer = attributesMetadata.StringComparisonLookup[4] | ||
} | ||
); | ||
|
||
return new EqualityTypeModel | ||
{ | ||
SyntaxKind = SyntaxKind.ClassDeclaration, | ||
TypeName = "MyType", | ||
BaseTypeName = "BaseType", | ||
IsSealed = true, | ||
ContainingSymbols = containingSymbols, | ||
AttributesMetadata = attributesMetadata, | ||
ExplicitMode = false, | ||
IgnoreInheritedMembers = false, | ||
BuildEqualityModels = equalityMemberModels, | ||
Fullname = "Namespace1.MyType" | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFramework>net8.0</TargetFramework> | ||
<ImplicitUsings>enable</ImplicitUsings> | ||
<Nullable>enable</Nullable> | ||
|
||
<IsPackable>false</IsPackable> | ||
<IsTestProject>true</IsTestProject> | ||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="coverlet.collector" Version="6.0.0" /> | ||
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.8.0" /> | ||
<PackageReference Include="xunit" Version="2.5.3" /> | ||
<PackageReference Include="xunit.runner.visualstudio" Version="2.5.3" /> | ||
<PackageReference Include="SourceGeneratorTestHelpers" Version="8.1.0" /> | ||
|
||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<ProjectReference Include="..\Generator.Equals.Runtime\Generator.Equals.Runtime.csproj" /> | ||
<ProjectReference Include="..\Generator.Equals\Generator.Equals.csproj"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you using an alias for this reference? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of |
||
<Aliases>genEquals</Aliases> | ||
</ProjectReference> | ||
</ItemGroup> | ||
|
||
|
||
<ItemGroup> | ||
<Using Include="Xunit" /> | ||
</ItemGroup> | ||
|
||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
extern alias genEquals; | ||
|
||
global using genEquals::Generator.Equals.Models; | ||
global using genEquals::Generator.Equals; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
extern alias genEquals; | ||
|
||
using genEquals::System.Diagnostics.CodeAnalysis; | ||
|
||
namespace Generator.Equals.DynamicGenerationTests; | ||
|
||
internal static class SourceText | ||
{ | ||
public static string CSharp([StringSyntax("c#-test")] string source) => source; | ||
} |
diegofrata marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,4 +10,4 @@ public partial struct Sample | |
[UnorderedEquality] public List<int>? Properties { get; set; } | ||
} | ||
} | ||
} | ||
} |
This file was deleted.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opening this file in VS 2022 comes up with 9 warnings. These should all be addressed. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,88 +1,51 @@ | ||
using System; | ||
using System.CodeDom.Compiler; | ||
using System.Collections.Generic; | ||
using System.Collections.Immutable; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Text; | ||
using Microsoft.CodeAnalysis; | ||
|
||
using Generator.Equals.Models; | ||
|
||
using Microsoft.CodeAnalysis.CSharp; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
|
||
namespace Generator.Equals | ||
{ | ||
static class ContainingTypesBuilder | ||
internal static class ContainingTypesBuilder | ||
{ | ||
static IEnumerable<INamespaceOrTypeSymbol> ContainingNamespaceAndTypes(ISymbol symbol, bool includeSelf) | ||
{ | ||
foreach (var item in AllContainingNamespacesAndTypes(symbol, includeSelf)) | ||
{ | ||
yield return item; | ||
|
||
if (item.IsNamespace) | ||
yield break; | ||
} | ||
} | ||
|
||
static IEnumerable<INamespaceOrTypeSymbol> AllContainingNamespacesAndTypes(ISymbol symbol, bool includeSelf) | ||
public static string Build(ImmutableArray<ContainingSymbol> containingSymbols, Action<IndentedTextWriter> content) | ||
{ | ||
if (includeSelf && symbol is INamespaceOrTypeSymbol self) | ||
yield return self; | ||
using var buffer = new StringWriter(new StringBuilder(capacity: 4096)); | ||
using var writer = new IndentedTextWriter(buffer); | ||
|
||
while (true) | ||
foreach (var parentSymbol in containingSymbols.Reverse()) | ||
{ | ||
symbol = symbol.ContainingSymbol; | ||
|
||
if (!(symbol is INamespaceOrTypeSymbol namespaceOrTypeSymbol)) | ||
yield break; | ||
|
||
yield return namespaceOrTypeSymbol; | ||
} | ||
} | ||
|
||
public static string Build(ISymbol symbol, Action<IndentedTextWriter> content, bool includeSelf = false) | ||
{ | ||
var buffer = new StringWriter(new StringBuilder(capacity: 4096)); | ||
var writer = new IndentedTextWriter(buffer); | ||
var symbols = ContainingNamespaceAndTypes(symbol, includeSelf).ToList(); | ||
|
||
for (var i = symbols.Count - 1; i >= 0; i--) | ||
{ | ||
var s = symbols[i]; | ||
|
||
if (s.IsNamespace) | ||
if (parentSymbol is NamespaceContainingSymbol namespaceSymbol) | ||
{ | ||
writer.WriteLine(); | ||
writer.WriteLine(EqualityGeneratorBase.EnableNullableContext); | ||
writer.WriteLine(EqualityGeneratorBase.SuppressObsoleteWarningsPragma); | ||
writer.WriteLine(EqualityGeneratorBase.SuppressTypeConflictsWarningsPragma); | ||
writer.WriteLine(); | ||
|
||
var namespaceName = s.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat.WithGlobalNamespaceStyle(SymbolDisplayGlobalNamespaceStyle.Omitted)); | ||
|
||
if (!string.IsNullOrEmpty(namespaceName)) | ||
if (!string.IsNullOrEmpty(namespaceSymbol.Name)) | ||
{ | ||
writer.WriteLine($"namespace {namespaceName}"); | ||
writer.WriteLine($"namespace {namespaceSymbol.Name}"); | ||
writer.AppendOpenBracket(); | ||
} | ||
} | ||
else | ||
else if (parentSymbol is TypeContainingSymbol typeContainingSymbol) | ||
{ | ||
var typeDeclarationSyntax = s.DeclaringSyntaxReferences | ||
.Select(x => x.GetSyntax()) | ||
.OfType<TypeDeclarationSyntax>() | ||
.First(); | ||
|
||
var keyword = typeDeclarationSyntax.Kind() switch | ||
var keyword = typeContainingSymbol.Kind switch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
{ | ||
SyntaxKind.ClassDeclaration => "class", | ||
SyntaxKind.RecordDeclaration => "record", | ||
(SyntaxKind)9068 => "record struct", // RecordStructDeclaration | ||
SyntaxKind.RecordStructDeclaration => "record struct", | ||
SyntaxKind.StructDeclaration => "struct", | ||
var x => throw new ArgumentOutOfRangeException($"Syntax kind {x} not supported") | ||
}; | ||
|
||
var typeName = s.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat); | ||
writer.WriteLine($"partial {keyword} {typeName}"); | ||
writer.WriteLine($"partial {keyword} {parentSymbol.Name}"); | ||
writer.AppendOpenBracket(); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a personal preference, I do two things with regards to generated code:
CSharpGeneratorDriver.Create(generator).RunGeneratorsAndUpdateCompilation(..);
andVerify
; theRunGeneratorsAndUpdateCompilation(..)
call to verify that both the original and generated code compile correctly, andVerify
to store the generated output and require confirmation of any changes to the generated output.EmitCompilerGeneratedFiles
, since any generated code I may wish to review is already available via the generated tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opening the code in VS fully, I see that there are now four test projects. Only two are necessary, imo, using the above process.
It appears that SnapshotTests has the first bullet point, and Tests contain the functional tests. TopLevel is probably unnecessary, and I am uncertain what DynamicGeneration is supposed to add over the existing ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea i generally think the sln would require some form of restructuring. Ideally following your format (from eg Immediate.Validators).
But i would like to move that topic to another pr, since there is already another pr. Changing the folder structure would prolly require me to do everything in that other pr again 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still did not answer why you added
DynamicGeneration
. The existing layout was fine, thoughTopLevel
is probably unnecessary; addingDynamicGeneration
is the question under concern here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea i wanted to generate and debug the generator without having to debug visual studio itslef.
In the other pr i need it to verify that the issue has been fixed https://github.com/diegofrata/Generator.Equals/blob/25d158fe365fc8ca43e08498ae6c95cf72d94142/Generator.Equals.DynamicGenerationTests/Issues/Issue-60-StringEquality-Enumerables.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You do that by using
<IsRoslynComponent />
and the correctlaunchSettings.json
. Adding a new project entirely seems unnecessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea removed the proj for now...