Add diagnostic suppressor for the MarkMethodsAsStatic diagnostics on custom marshallers #72129
Conversation
…ppressor for suppressing any diagnostics that guide users to not follow the marshaller shapes.
…methods that are part of the stateful shape. Improve the test suite for the suppressor
jkoritzinsky
added
area-System.Runtime.InteropServices
source-generator
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/interop-contrib Issue DetailsToday, we have to suppress the diagnostic to make a method static for methods on the stateful marshallers shapes that are required to exist but are sometimes empty, such as This PR introduces a diagnostic suppressor that disables the "mark methods as static" diagnostic on methods that must be instance methods for our various different marshalling shapes. It also removes the manual suppression of the diagnostic from the various marshallers we had to add the suppression to. I decided to add a suppressor instead of updating the analyzer as this suppression works significantly differently than the existing built-in "suppressions" in the analyzer. This suppression is based on attributes on containing types, whereas the built-in "suppressions" are based on attributes applied to the methods or well-known method bodies (like Fixes #71793
|
...ies/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/MarshallerShape.cs
Show resolved
Hide resolved
...me.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs
Outdated
Show resolved
Hide resolved
…the test suite on Mono against an active issue
...ropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs
Outdated
Show resolved
Hide resolved
...ropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs
Outdated
Show resolved
Hide resolved
...me.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs
Outdated
Show resolved
Hide resolved
...me.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs
Outdated
Show resolved
Hide resolved
...ropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs
Show resolved
Hide resolved
…rics and reuse this logic from the suppressor. Add a linear collection test. PR feedback.
jkoritzinsky
force-pushed
the
no-static-shape-methods
branch
from
eff93e0 to
2ceab41
Compare
| /// <param name="entryPointType">The entry point type</param> | ||
| /// <param name="attributeMarshallerType">The marshaller type from the CustomMarshallerAttribute</param> | ||
| /// <returns>A fully constructed marshaller type</returns> | ||
| public static bool TryResolveMarshallerType(INamedTypeSymbol entryPointType, ITypeSymbol? attributeMarshallerType, Action<Diagnostic> reportDiagnostic, [NotNullWhen(true)] out ITypeSymbol? marshallerType) |
There was a problem hiding this comment.
This function came from the start of my work on the new CustomMarshallerAttribute analyzer. The reportDiagnostic parameter currently isn't used to actually report diagnostics, but will be used in that PR.
|
MSquic timeout is unrelated. |
Today, we have to suppress the diagnostic to make a method static for methods on the stateful marshallers shapes that are required to exist but are sometimes empty, such as
Free. In particular, dotnet/runtime and dotnet/aspnetcore bump the severity of this rule from info to warning, which is then converted into an error, so we can't just ignore it. Additionally, leaving the diagnostics as-is provides a bad user experience where we guide them to write code that doesn't work with the source generator.This PR introduces a diagnostic suppressor that disables the "mark methods as static" diagnostic on methods that must be instance methods for our various different marshalling shapes.
It also removes the manual suppression of the diagnostic from the various marshallers we had to add the suppression to.
I decided to add a suppressor instead of updating the analyzer as this suppression works significantly differently than the existing built-in "suppressions" in the analyzer. This suppression is based on attributes on containing types, whereas the built-in "suppressions" are based on attributes applied to the methods or well-known method bodies (like
throw NotImplementedException();). Additionally, we may extend this suppressor to cover other diagnostics in the future that interfere with our shape designs.Fixes #71793