Improve caching in XmlToDescriptionGenerator #1010
Conversation
| ImmutableArray<string> TypeParameters, ImmutableArray<ParameterInfo> Parameters, | ||
| string? MethodDescription, string? ReturnDescription, ImmutableArray<DiagnosticInfo> Diagnostics) |
There was a problem hiding this comment.
Immutable array is likely to hurt equality here. I personally always reuse EquatableArray from @Sergio0694 for this. https://github.com/Sergio0694/ComputeSharp/blob/main/src/ComputeSharp.SourceGeneration/Helpers/EquatableArray%7BT%7D.cs
There was a problem hiding this comment.
Yep. This model, as written, will very rarely be equatable.
| private readonly record struct DiagnosticInfo(string Id, Location? Location, params object?[] MessageArgs); | ||
|
|
||
| /// <summary>Holds extracted XML documentation for a method.</summary> | ||
| private sealed record XmlDocumentation(string MethodDescription, string Returns, Dictionary<string, string> Parameters); |
There was a problem hiding this comment.
This Dictionary should still be a problem, isn't it?
There was a problem hiding this comment.
Yes. In one of my personal repos I've created a number of immutable collection types that implement structural equality including a dictionary type.
| private readonly record struct MethodToGenerate(MethodDeclarationSyntax MethodDeclaration, IMethodSymbol MethodSymbol); | ||
| private readonly record struct TypeDeclarationInfo(string Name, string TypeKeyword); | ||
|
|
||
| private readonly record struct DiagnosticInfo(string Id, Location? Location, params object?[] MessageArgs); |
There was a problem hiding this comment.
This array here is likely to be a problem as well?
| // Combine with compilation to get well-known type symbols. | ||
| var compilationAndMethods = context.CompilationProvider.Combine(allMethods); | ||
| // Report diagnostics. | ||
| context.RegisterSourceOutput(allMethods, static (spc, methods) => |
There was a problem hiding this comment.
This separation of diagnostic reporting into a separate callback is an interesting approach. I think it might provide a viable workaround for dotnet/runtime#92509. Curious if you've applied this approach elsewhere before, or if there are any drawbacks to it.
There was a problem hiding this comment.
I haven't. copilot suggested it.
There was a problem hiding this comment.
Note that diagnostic objects are not equatable, and the recommendation of the team (e.g. @CyrusNajmabadi) is to not include them in incremental steps, or they would break them. Meaning that you effectively lose incrementality at each re-run as long as there's any diagnostic being produced. From our previous talks there's 3 ways to fix this:
- If possible, just move them al to analyzers
- Emit them immediately from the transform step
- Introduce some equatable model to carry diagnostic info (e.g. I do this here)
There was a problem hiding this comment.
If there are any diagnostics, I expect that equality will be completely broken here. Our recommendation is to have separate analyzers for diagnostics and not do any reporting in the generator.
5 participants
Closes #1003