fix: Correctly support incremental source generation #62

JKamsker · 2024-10-08T19:38:28Z

Fixes #56 by removing the requirement of the sg to use the compilation

viceroypenguin

Some nits, some foundational issues - a good start, though.

Generator.Equals/AttributesMetadata.cs

Generator.Equals.DynamicGenerationTests/UnitTest1.cs

Generator.Equals/AttributesMetadata.cs

Generator.Equals/EqualsGenerator.cs

Generator.Equals/Generator.Equals.csproj

Generator.Equals/Polyfill/IsExternalInit.cs

JKamsker · 2024-10-08T21:53:56Z

TODO: Actually do transform

diegofrata · 2024-10-09T13:09:39Z

Hi guys! I just spotted this PR. I am happy to work w/ you on getting something merged if it will solve #56. Let me know once the code is a reviewable state.

JKamsker · 2024-10-09T22:43:43Z

Hey @diegofrata !

Glad you like it :)

I think this PR is ready for review :)

Generator.Equals/EqualsGenerator.cs

Generator.Equals/Models/EqualityTypeModelTransformer.cs

Generator.Equals/Models/EqualityTypeModel.cs

Generator.Equals/Models/AttributesMetadata.cs

Generator.Equals/Models/AttributeMetadata.cs

viceroypenguin

This is a lot already, and I don't want to tear down all of it.

Would also recommend looking at the following code for getting outer classes:

https://github.com/ImmediatePlatform/Immediate.Validations/blob/d66ecfeff03ba31bfe53bd126d72fc19c46352e1/src/Immediate.Validations.Generators/ValidateTargetTransformer.cs#L89-L105

viceroypenguin · 2024-10-10T16:44:20Z

.gitignore

As a personal preference, I do two things with regards to generated code:

Generator tests, which use CSharpGeneratorDriver.Create(generator).RunGeneratorsAndUpdateCompilation(..); and Verify; the RunGeneratorsAndUpdateCompilation(..) call to verify that both the original and generated code compile correctly, and Verify to store the generated output and require confirmation of any changes to the generated output.

Functional tests, which test the behavior of the generated code.

In these functional tests, I do not use EmitCompilerGeneratedFiles, since any generated code I may wish to review is already available via the generated tests.

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.

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 😅

You still did not answer why you added DynamicGeneration. The existing layout was fine, though TopLevel is probably unnecessary; adding DynamicGeneration is the question under concern here.

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

nit: You do that by using <IsRoslynComponent /> and the correct launchSettings.json. Adding a new project entirely seems unnecessary.

Yea removed the proj for now...

Generator.Equals.Tests/Generator.Equals.Tests.csproj

Generator.Equals.Tests/Structs/CustomEquality.cs

Generator.Equals.Tests/Structs/UnorderedEquality.Sample.cs

Generator.Equals.DynamicGenerationTests/UnitTest1.cs

Generator.Equals/SymbolHelpers.cs

Generator.Equals/Extensions/SymbolExtensions.cs

Generator.Equals/Models/ComparableImmutableArray.cs

JKamsker · 2024-10-11T06:16:53Z

Would also recommend looking at the following code for getting outer classes:

https://github.com/ImmediatePlatform/Immediate.Validations/blob/d66ecfeff03ba31bfe53bd126d72fc19c46352e1/src/Immediate.Validations.Generators/ValidateTargetTransformer.cs#L89-L105

I think we also need namespaces.

Applied requested changes from the pr

JKamsker · 2024-10-11T11:23:41Z

So to sum up, a major restructuring is necessary but it should be done in a separate pr imho.

@diegofrata Can you also have a look? :D

Generator.Equals/Extensions/SymbolExtensions.cs

Generator.Equals/Models/EqualityTypeModelTransformer.cs

diegofrata

Apologies for the delay, startup life is not easy. This is a bit of a beast but it looks great! Thanks for this refactoring, it is a lot better and a lot more maintainable.

I tried to not comment on style in general, the exception being null-checks, which I agree with other commenters that it's too non-idiomatic and that's coming from someone who hates explicit modifiers but tolerates it because it's the preference of the majority. :-) Sorry!

A couple of things that still need discussion before merging are:

Is .NET 8 required here? If so, projects targeting .NET 6.0 need an update, as does the GitHub Action (that should be straight-forward)
Would be nice if package versions between the different test projects were in sync, with the addition of the DynamicGeneration test project.
I don't have a Windows machine at hand anymore, so can't check if .NET Framework 4.8 is still working after this PR and sadly I don't have an action for that. If you're on Windows, could you test it?

Let me know if you need any assistance w/ any of these things! Once again, thanks for taking the time to work on this project.

Generator.Equals/Models/AttributeMetadataExtensions.cs

Generator.Equals/Extensions/AttributeDataExtensions.cs

Generator.Equals/Generators/RecordEqualityGenerator.cs

Generator.Equals/Generators/RecordStructEqualityGenerator.cs

Generator.Equals.DynamicGenerationTests/Base_Assertions.cs

diegofrata · 2024-10-14T19:31:14Z

Generator.Equals/Models/EqualityTypeModelTransformer.cs

+        if (equatableAttributeData is null || !attributesMetadata.Equatable.Equals(equatableAttributeData.AttributeClass))
+        {
+            // TODO: Report diagnostic
+            // throw new Exception("Expected exactly one EquatableAttribute.");


Would be cool to see diagnostic report in these places rather than silently failing. Do you think you can get that in?

Didn't have the resources to research it but it seems like its alot of additional work since the reporting would be happening somewhere completely different

I will reiterate that generators cannot safely report diagnostics and still be incremental. Diagnostics should be reported by a matching analyzer.

However, I agree that silently failing is not intuitive for the user. Silently fail here and write an analyzer to report the failure.

@viceroypenguin do you have any pointers to read on this? I have been a bit out of the loop with generators since I first wrote this. Information was always a bit spread out on this topic.

https://github.com/dotnet/roslyn/blob/main/docs/features/incremental-generators.cookbook.md#issue-diagnostics

Ha. The cookbook didn't leave me any wiser. I will just consider myself happy with being told "not to do it inside an incremental generator" for now and do a bit of reading later.

For reference, I have not implemented analyzers before (obviously), and this link was helpful to me:
https://www.thinktecture.com/en/net/roslyn-source-generators-analyzers-code-fixes/

@JKamsker I think we might want to add some analyzers at some point but I don't see it as a blocker for this PR given the package already does not have one.

If you want an example of working set of Generator/Analyzer/CodeFix, please feel free to check out my source-generator packages:

https://github.com/immediateplatform/immediate.handlers

https://github.com/immediateplatform/immediate.apis

https://github.com/immediateplatform/immediate.validations

I have created an issue for this #65

Generator.Equals/Models/EqualityTypeModelTransformer.cs

Generator.Equals/Models/EquatableImmutableArray.cs

Generator.Equals.Tests/Generator.Equals.Tests.csproj

diegofrata · 2024-10-15T16:34:01Z

@JKamsker re-request a review from me once you're done and then we can try and get it merged.

JKamsker · 2024-10-15T21:38:39Z

To avoid making this pr even bigger, i have created 2 new issues:

Restructuring: Restructuring #66
Adding diagnostics: Add diagnostics #65

JKamsker · 2024-10-15T21:57:27Z

Removed test project
Upgraded to net 8.0

diegofrata · 2024-10-16T08:31:37Z

Incredible work! Thanks a lot @JKamsker. I will try to get some automatic test running for NET 4.8 later today to ensure it is supported going forward.

diegofrata · 2024-10-16T08:32:12Z

And also thanks to @viceroypenguin @333fred for insightful comments.

diegofrata · 2024-10-17T08:19:29Z

@JKamsker pre-release 3.2.0-alpha1 is out. I will ask some colleagues to test it.

Refactor Generator: Remove requirement for compilation

17ca7b0

viceroypenguin suggested changes Oct 8, 2024

View reviewed changes

JKamsker changed the title ~~Refactor Generator: Remove requirement for compilation~~ [WIP] Refactor Generator: Remove requirement for compilation Oct 8, 2024

JKamsker force-pushed the JKamsker/real_incremental_reduced branch from fd0ee6f to 5bd93fb Compare October 9, 2024 22:37

SourceGen does transform and allows for better caching

ceb45e2

JKamsker force-pushed the JKamsker/real_incremental_reduced branch from 5bd93fb to ceb45e2 Compare October 9, 2024 22:39

JKamsker requested a review from viceroypenguin October 9, 2024 22:39

JKamsker changed the title ~~[WIP] Refactor Generator: Remove requirement for compilation~~ Refactor Generator: Remove requirement for compilation Oct 9, 2024

333fred reviewed Oct 9, 2024

View reviewed changes

JKamsker force-pushed the JKamsker/real_incremental_reduced branch from 85a1c33 to ceb45e2 Compare October 10, 2024 00:40

This was referenced Oct 10, 2024

Support StringEqualityAttribute on collections of strings #60

Open

String array + Comparer #63

Open

JKamsker added 2 commits October 10, 2024 14:18

Remove throws and fix equality of the model

5797558

Add EquatableImmutableDictionary

cdff4c9

JKamsker force-pushed the JKamsker/real_incremental_reduced branch from 652ba68 to cdff4c9 Compare October 10, 2024 13:56

Remove comments

d12bf22

viceroypenguin suggested changes Oct 10, 2024

View reviewed changes

JKamsker added 6 commits October 11, 2024 08:21

Comment out EmitCompilerGeneratedFiles

018b0d8

Cleanup

afd36b0

Applied requested changes from the pr

Remove useless test class

762c681

Remove commented-out code

1eae2be

Remove comment

60fc464

Resolved warnings

cacd1dd

JKamsker force-pushed the JKamsker/real_incremental_reduced branch from f7b98e2 to cacd1dd Compare October 11, 2024 10:27

Using SymbolDisplayFormat.FullyQualifiedFormat for fullname

75f67a8

JKamsker requested review from 333fred and viceroypenguin October 11, 2024 11:23

viceroypenguin reviewed Oct 11, 2024

View reviewed changes

Generator.Equals/Extensions/SymbolExtensions.cs Show resolved Hide resolved

Generator.Equals/Models/EqualityTypeModelTransformer.cs Outdated Show resolved Hide resolved

SymbolExtensions made internal to get rid of warning

7e0e70c

diegofrata reviewed Oct 14, 2024

View reviewed changes

Applied requested changes for pr

927e2a7

Expression lambda instead of block

0df78df

JKamsker added 3 commits October 15, 2024 23:52

Consolidate package versions

6d11303

Upgrade to net8

5cadbe9

Remove unnecessary test project

df2966c

JKamsker requested a review from diegofrata October 15, 2024 21:57

diegofrata changed the title ~~Refactor Generator: Remove requirement for compilation~~ fix: Correctly support incremental source generation Oct 16, 2024

diegofrata approved these changes Oct 16, 2024

View reviewed changes

diegofrata merged commit f945469 into diegofrata:main Oct 16, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Correctly support incremental source generation #62

fix: Correctly support incremental source generation #62

JKamsker commented Oct 8, 2024

viceroypenguin left a comment

JKamsker commented Oct 8, 2024

diegofrata commented Oct 9, 2024

JKamsker commented Oct 9, 2024

viceroypenguin left a comment

viceroypenguin Oct 10, 2024

viceroypenguin Oct 10, 2024

JKamsker Oct 11, 2024

viceroypenguin Oct 11, 2024

JKamsker Oct 15, 2024

viceroypenguin Oct 15, 2024

JKamsker Oct 15, 2024

JKamsker commented Oct 11, 2024

JKamsker commented Oct 11, 2024 •

edited

Loading

diegofrata left a comment

diegofrata Oct 14, 2024

JKamsker Oct 14, 2024

viceroypenguin Oct 14, 2024

diegofrata Oct 15, 2024

viceroypenguin Oct 15, 2024

diegofrata Oct 15, 2024

viceroypenguin Oct 15, 2024

JKamsker Oct 15, 2024

diegofrata commented Oct 15, 2024

JKamsker commented Oct 15, 2024

JKamsker commented Oct 15, 2024

diegofrata commented Oct 16, 2024

diegofrata commented Oct 16, 2024

diegofrata commented Oct 17, 2024

fix: Correctly support incremental source generation #62

fix: Correctly support incremental source generation #62

Conversation

JKamsker commented Oct 8, 2024

viceroypenguin left a comment

Choose a reason for hiding this comment

JKamsker commented Oct 8, 2024

diegofrata commented Oct 9, 2024

JKamsker commented Oct 9, 2024

viceroypenguin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JKamsker commented Oct 11, 2024

JKamsker commented Oct 11, 2024 • edited Loading

diegofrata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegofrata commented Oct 15, 2024

JKamsker commented Oct 15, 2024

JKamsker commented Oct 15, 2024

diegofrata commented Oct 16, 2024

diegofrata commented Oct 16, 2024

diegofrata commented Oct 17, 2024

JKamsker commented Oct 11, 2024 •

edited

Loading