Fix incremental source generator #22
Conversation
|
Thanks so much for the effort. Will be a look at this PR soon, in the meantime I've enabled the build for this. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #22 +/- ##
==========================================
- Coverage 94.27% 93.95% -0.32%
==========================================
Files 7 8 +1
Lines 874 894 +20
Branches 194 192 -2
==========================================
+ Hits 824 840 +16
- Misses 23 25 +2
- Partials 27 29 +2 ☔ View full report in Codecov by Sentry. |
|
sorry to bother... any prognosis to see this merged? I'm very interested to see this ISG to be fully functional and healthy! |
| @@ -3,6 +3,7 @@ | |||
| <PackageVersion Include="coverlet.collector" Version="6.0.0" /> | |||
| <PackageVersion Include="Fody" Version="6.6.4" /> | |||
| <PackageVersion Include="IsExternalInit" Version="1.0.3" /> | |||
| <PackageVersion Include="Microsoft.Bcl.HashCode" Version="1.1.1" /> | |||
There was a problem hiding this comment.
Does this work without including the dependency in the NuGet package?
There was a problem hiding this comment.
@lsoft, soon. Most likely this week.
There was a problem hiding this comment.
@GerardSmit, thanks for pointing that out. Will ensure it gets included. BTW, if any of the commits I made after your original PR don't make sense, let me know. We should have this merged soon.
There was a problem hiding this comment.
Everything else look OK to me; using a ushort instead of a hacky wrapper struct is probably better. 😉
|
So, I have switched our network subsystem from sync mode to sync+async mode with this ISG. Things looks amazing! 👍 I'm concerned about potential performance issues because of "broken" ISG pipeline. This is the only thing I want to wait for before merging my PR. @virzak Sorry to bother, absolutely(!) no pressing here, but if you are able to share some prognosis, it would help me to build delivery plan :) |
|
Hey, no problem. Thanks for the push. Looking as we speak, will merge today. Also this came out: https://twitter.com/andrewlocknet/status/1747312256931516497?t=zgQQi6TdVqzRPz6faKW60w&s=19 |
| @@ -25,162 +27,161 @@ public void Initialize(IncrementalGeneratorInitializationContext context) | |||
| context.RegisterPostInitializationOutput(ctx => ctx.AddSource( | |||
| $"{CreateSyncVersionAttribute}.g.cs", SourceText.From(SourceGenerationHelper.CreateSyncVersionAttributeSource, Encoding.UTF8))); | |||
|
|
|||
| IncrementalValuesProvider<MethodDeclarationSyntax> methodDeclarations = context.SyntaxProvider | |||
| var disableNullable = | |||
| context.CompilationProvider.Select((c, _) | |||
There was a problem hiding this comment.
@GerardSmit, if you're good with this change, I'll merge.
Following this example as well as this section of the tutorial.
There was a problem hiding this comment.
Looks good 👍
The unit test is still succeeding, so the caching still works. 😄
|
Thanks a lot, @GerardSmit !!! |
|
@GerardSmit Thanks you! Your PR helped a real people to fix their real problems! 👍 |
Fixes #20
This was quite annoying, but luckily comments from @Sergio0694 popped up in my research to resolve them 😉
Because of this, I had to introduce
EquatableArray<T>that Sergio wrote:ImmutableArray<T> struct implements IEquatable<T> interface but does not stick to the rules of properly implementing value semantics dotnet/runtime#77183 (comment)
I had to move the methods around to make the code-style analyzer happy.
Sergio hinted that we shouldn't use Diagnostic itself, but a trimmed down one:
Update generator to really be incremental k94ll13nn3/AutoConstructor#83 (comment)
Enum doesn't implement IEquatable<T>Because of this I couldn't use ImmutableArray<T>, so I made a wrapper called EquatableEnum<T>.
I don't know if this is the best way. 😅
Changed to an
ushortinffc85b3I used the unit test from k94ll13nn3/AutoConstructor#83 to validate if the source generator is incremental.
All unit tests (Generator.Tests and GenerationSandbox.Tests) are succeeding, without modifying them.