From efecfa47f7544f7de27c3ab61e02cb9eab21107f Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 8 Jul 2022 11:16:58 -0700 Subject: [PATCH 1/4] Remove explicit suppressions in source. Add a prototype diagnostic suppressor for suppressing any diagnostics that guide users to not follow the marshaller shapes. --- .../InteropServices/HandleRefMarshaller.cs | 1 - .../src/System/Drawing/Graphics.cs | 1 - .../src/System/Drawing/Image.cs | 1 - .../Drawing/Imaging/MetafileHeaderWmf.cs | 1 - .../ShapeBreakingDiagnosticSuppressor.cs | 59 ++++++++++++ .../LibraryImportGenerator.Unit.Tests.csproj | 16 +++- .../ShapeBreakingDiagnosticSuppressorTests.cs | 93 +++++++++++++++++++ 7 files changed, 167 insertions(+), 5 deletions(-) create mode 100644 src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs create mode 100644 src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs diff --git a/src/libraries/Common/src/System/Runtime/InteropServices/HandleRefMarshaller.cs b/src/libraries/Common/src/System/Runtime/InteropServices/HandleRefMarshaller.cs index ccdfa9d55d984..3ed1c26baf760 100644 --- a/src/libraries/Common/src/System/Runtime/InteropServices/HandleRefMarshaller.cs +++ b/src/libraries/Common/src/System/Runtime/InteropServices/HandleRefMarshaller.cs @@ -23,7 +23,6 @@ public void FromManaged(HandleRef handle) public void OnInvoked() => GC.KeepAlive(_handle.Wrapper); - [SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "This method is part of the marshaller shape and is required to be an instance method.")] public void Free() { } } } diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.cs index 34ccdb709b3a2..0c80860cbf4bf 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Graphics.cs @@ -89,7 +89,6 @@ public void OnInvoked() GC.KeepAlive(_managed); } - [SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "This method is part of the marshaller shape and is required to be an instance method.")] public void Free() { } } } diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Image.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Image.cs index 0cb8b907aa498..078eb6d5352d9 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Image.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Image.cs @@ -77,7 +77,6 @@ public void OnInvoked() GC.KeepAlive(_managed); } - [SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "This method is part of the marshaller shape and is required to be an instance method.")] public void Free() { } } } diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/MetafileHeaderWmf.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/MetafileHeaderWmf.cs index f984b287c0f00..2f5b386e1e04a 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/MetafileHeaderWmf.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/MetafileHeaderWmf.cs @@ -157,7 +157,6 @@ public MetafileHeaderWmf ToManaged() return _managed; } - [SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "This method is part of the marshaller shape and is required to be an instance method.")] public void Free() { } } } diff --git a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs new file mode 100644 index 0000000000000..4ddeef6cadd77 --- /dev/null +++ b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs @@ -0,0 +1,59 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using System.Text; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Microsoft.Interop.Analyzers +{ + [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] + public class ShapeBreakingDiagnosticSuppressor : DiagnosticSuppressor + { + public static readonly SuppressionDescriptor MarkMethodsAsStaticSuppression = new SuppressionDescriptor("SYSLIBSUPPRESS1001", "CA1822", "Do not offer to make methods static when they need to be instance methods."); + + public override ImmutableArray SupportedSuppressions => + ImmutableArray.Create(MarkMethodsAsStaticSuppression); + + public override void ReportSuppressions(SuppressionAnalysisContext context) + { + foreach (var diagnostic in context.ReportedDiagnostics) + { + if (diagnostic.Id == MarkMethodsAsStaticSuppression.SuppressedDiagnosticId) + { + SuppressMarkMethodsAsStaticDiagnosticIfNeeded(context, diagnostic); + } + } + } + + private static void SuppressMarkMethodsAsStaticDiagnosticIfNeeded(SuppressionAnalysisContext context, Diagnostic diagnostic) + { + SemanticModel model = context.GetSemanticModel(diagnostic.Location.SourceTree); + ISymbol symbol = model.GetDeclaredSymbol(diagnostic.Location.SourceTree.GetRoot(context.CancellationToken).FindNode(diagnostic.Location.SourceSpan), context.CancellationToken); + if (symbol.Name == "Free" && symbol.Kind == SymbolKind.Method) // TODO: Extend to all names recognized in the shape + { + if (symbol.ContainingType is { TypeKind: TypeKind.Struct }) + { + bool isCustomTypeMarshaller = GetAllContainingTypes(symbol).Any(type => type.GetAttributes().Any( + attr => attr.AttributeClass?.ToDisplayString() == TypeNames.CustomMarshallerAttribute + && attr.AttributeConstructor is not null + && attr.ConstructorArguments[2].Value is INamedTypeSymbol marshallerType + && SymbolEqualityComparer.Default.Equals(marshallerType, symbol.ContainingType))); + context.ReportSuppression(Suppression.Create(MarkMethodsAsStaticSuppression, diagnostic)); + } + } + } + + private static IEnumerable GetAllContainingTypes(ISymbol symbol) + { + for (INamedTypeSymbol containingType = symbol.ContainingType; containingType is not null; containingType = containingType.ContainingType) + { + yield return containingType; + } + } + } +} diff --git a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/LibraryImportGenerator.Unit.Tests.csproj b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/LibraryImportGenerator.Unit.Tests.csproj index 6771823334a40..44d2e790099e9 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/LibraryImportGenerator.Unit.Tests.csproj +++ b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/LibraryImportGenerator.Unit.Tests.csproj @@ -1,4 +1,4 @@ - + $(NetCoreAppCurrent) @@ -25,4 +25,18 @@ + + + + + + + + + + + + + + diff --git a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs new file mode 100644 index 0000000000000..fb6de5f16781e --- /dev/null +++ b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs @@ -0,0 +1,93 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using LibraryImportGenerator.UnitTests.Verifiers; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Testing; +using Microsoft.Interop.Analyzers; +using Xunit; + +namespace LibraryImportGenerator.Unit.Tests +{ + public class ShapeBreakingDiagnosticSuppressorTests + { + [Fact] + public async Task EmptyParameterlessFreeMethodHasSuppressedDiagnostic() + { + await VerifySuppressorAsync(""" + using System.Runtime.InteropServices.Marshalling; + + struct S + { + public bool b; + }; + + [CustomMarshaller(typeof(S), MarshalMode.ManagedToUnmanagedIn, typeof(ManagedToUnmanagedIn))] + static class Marshaller + { + public struct ManagedToUnmanagedIn + { + private int i; + public void FromManaged(S s) { i = s.b ? 1 : 0; } + public ManagedToUnmanagedIn ToUnmanaged() => this; + + public void {|#0:Free|}() {} + } + } + """, + new DiagnosticResult(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression.SuppressedDiagnosticId, DiagnosticSeverity.Info).WithLocation(0).WithIsSuppressed(true)); + } + + private static async Task VerifySuppressorAsync(string source, params DiagnosticResult[] expected) + { + var test = new Test + { + TestCode = source, + }; + + test.ExpectedDiagnostics.AddRange(expected); + await test.RunAsync(CancellationToken.None); + } + + class Test : CSharpCodeFixVerifier.Test + { + public Test() + { + // Don't verify that we don't emit any diagnostics when they're disabled with #pragma warning disable + // This check doesn't work when we set up the CompilationWithAnalyzers object to report suppressed diagnostics + TestBehaviors |= TestBehaviors.SkipSuppressionCheck; + } + protected override IEnumerable GetDiagnosticAnalyzers() + { + return new DiagnosticAnalyzer[] + { + // Our diagnostic suppressor + new ShapeBreakingDiagnosticSuppressor(), + // Each of the analyzers the suppressor supports suppressing diagnostics from + new Microsoft.CodeQuality.Analyzers.QualityGuidelines.MarkMembersAsStaticAnalyzer() + }; + } + + protected override CompilationWithAnalyzers CreateCompilationWithAnalyzers(Compilation compilation, ImmutableArray analyzers, AnalyzerOptions options, CancellationToken cancellationToken) + { + return new CompilationWithAnalyzers( + compilation, + analyzers, + new CompilationWithAnalyzersOptions( + options, + onAnalyzerException: null, + concurrentAnalysis: true, + logAnalyzerExecutionTime: true, + reportSuppressedDiagnostics: true)); // We're specifically testing a DiagnosticSuppressor here, so we want to test that we find suppressed diagnostics. + } + } + } +} From 2269e52a017fb808917ba4437228cf51afb681ca Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 13 Jul 2022 13:40:31 -0700 Subject: [PATCH 2/4] Improve suppressor to only suppress diagnostics that are actually on methods that are part of the stateful shape. Improve the test suite for the suppressor --- .../ShapeBreakingDiagnosticSuppressor.cs | 37 ++++-- .../MarshallerShape.cs | 23 +++- .../ShapeBreakingDiagnosticSuppressorTests.cs | 107 +++++++++++++++++- 3 files changed, 148 insertions(+), 19 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs index 4ddeef6cadd77..c90904a959090 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs @@ -33,27 +33,40 @@ public override void ReportSuppressions(SuppressionAnalysisContext context) private static void SuppressMarkMethodsAsStaticDiagnosticIfNeeded(SuppressionAnalysisContext context, Diagnostic diagnostic) { SemanticModel model = context.GetSemanticModel(diagnostic.Location.SourceTree); - ISymbol symbol = model.GetDeclaredSymbol(diagnostic.Location.SourceTree.GetRoot(context.CancellationToken).FindNode(diagnostic.Location.SourceSpan), context.CancellationToken); - if (symbol.Name == "Free" && symbol.Kind == SymbolKind.Method) // TODO: Extend to all names recognized in the shape + ISymbol diagnosedSymbol = model.GetDeclaredSymbol(diagnostic.Location.SourceTree.GetRoot(context.CancellationToken).FindNode(diagnostic.Location.SourceSpan), context.CancellationToken); + + if (diagnosedSymbol.Kind == SymbolKind.Method) { - if (symbol.ContainingType is { TypeKind: TypeKind.Struct }) + if (FindContainingEntryPointTypeAndManagedType(diagnosedSymbol.ContainingType) is (INamedTypeSymbol entryPointMarshallerType, INamedTypeSymbol managedType)) { - bool isCustomTypeMarshaller = GetAllContainingTypes(symbol).Any(type => type.GetAttributes().Any( - attr => attr.AttributeClass?.ToDisplayString() == TypeNames.CustomMarshallerAttribute - && attr.AttributeConstructor is not null - && attr.ConstructorArguments[2].Value is INamedTypeSymbol marshallerType - && SymbolEqualityComparer.Default.Equals(marshallerType, symbol.ContainingType))); - context.ReportSuppression(Suppression.Create(MarkMethodsAsStaticSuppression, diagnostic)); + bool isLinearCollectionMarshaller = entryPointMarshallerType.GetAttributes().Any(attr => attr.AttributeClass?.ToDisplayString() == TypeNames.ContiguousCollectionMarshallerAttribute); + (MarshallerShape _, StatefulMarshallerShapeHelper.MarshallerMethods methods) = StatefulMarshallerShapeHelper.GetShapeForType(diagnosedSymbol.ContainingType, managedType, isLinearCollectionMarshaller, context.Compilation); + if (methods.IsShapeMethod((IMethodSymbol)diagnosedSymbol)) + { + // If we are a method of the shape on the stateful marshaller shape, then we need to be our current shape. + // So, suppress the diagnostic to make this method static, as that would break the shape. + context.ReportSuppression(Suppression.Create(MarkMethodsAsStaticSuppression, diagnostic)); + } } } } - private static IEnumerable GetAllContainingTypes(ISymbol symbol) + private static (INamedTypeSymbol EntryPointType, INamedTypeSymbol ManagedType)? FindContainingEntryPointTypeAndManagedType(INamedTypeSymbol marshallerType) { - for (INamedTypeSymbol containingType = symbol.ContainingType; containingType is not null; containingType = containingType.ContainingType) + for (INamedTypeSymbol containingType = marshallerType; containingType is not null; containingType = containingType.ContainingType) { - yield return containingType; + AttributeData? attrData = containingType.GetAttributes().FirstOrDefault( + attr => attr.AttributeClass?.ToDisplayString() == TypeNames.CustomMarshallerAttribute + && attr.AttributeConstructor is not null + && !attr.ConstructorArguments[0].IsNull + && attr.ConstructorArguments[2].Value is INamedTypeSymbol marshallerTypeInAttribute + && SymbolEqualityComparer.Default.Equals(marshallerTypeInAttribute, marshallerType)); + if (attrData is not null) + { + return (containingType, (INamedTypeSymbol)attrData.ConstructorArguments[0].Value); + } } + return null; } } } diff --git a/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/MarshallerShape.cs b/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/MarshallerShape.cs index 10e7c432aeba5..d624584f5d8a6 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/MarshallerShape.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/MarshallerShape.cs @@ -411,12 +411,30 @@ public record MarshallerMethods public IMethodSymbol? ToUnmanaged { get; init; } public IMethodSymbol? Free { get; init; } public IMethodSymbol? OnInvoked { get; init; } + public IMethodSymbol? StatefulGetPinnableReference { get; init; } // Linear collection public IMethodSymbol? ManagedValuesSource { get; init; } public IMethodSymbol? UnmanagedValuesDestination { get; init; } public IMethodSymbol? ManagedValuesDestination { get; init; } public IMethodSymbol? UnmanagedValuesSource { get; init; } + + public bool IsShapeMethod(IMethodSymbol method) + { + return SymbolEqualityComparer.Default.Equals(method, FromManaged) + || SymbolEqualityComparer.Default.Equals(method, FromManagedWithBuffer) + || SymbolEqualityComparer.Default.Equals(method, ToManaged) + || SymbolEqualityComparer.Default.Equals(method, ToManagedGuranteed) + || SymbolEqualityComparer.Default.Equals(method, FromUnmanaged) + || SymbolEqualityComparer.Default.Equals(method, ToUnmanaged) + || SymbolEqualityComparer.Default.Equals(method, Free) + || SymbolEqualityComparer.Default.Equals(method, OnInvoked) + || SymbolEqualityComparer.Default.Equals(method, StatefulGetPinnableReference) + || SymbolEqualityComparer.Default.Equals(method, ManagedValuesSource) + || SymbolEqualityComparer.Default.Equals(method, UnmanagedValuesDestination) + || SymbolEqualityComparer.Default.Equals(method, ManagedValuesDestination) + || SymbolEqualityComparer.Default.Equals(method, UnmanagedValuesSource); + } } public static (MarshallerShape shape, MarshallerMethods methods) GetShapeForType(ITypeSymbol marshallerType, ITypeSymbol managedType, bool isLinearCollectionMarshaller, Compilation compilation) @@ -518,9 +536,12 @@ public static (MarshallerShape shape, MarshallerMethods methods) GetShapeForType { shape |= MarshallerShape.StatelessPinnableReference; } - if (GetStatefulGetPinnableReference(marshallerType) is not null) + + IMethodSymbol statefulGetPinnableReference = GetStatefulGetPinnableReference(marshallerType); + if (statefulGetPinnableReference is not null) { shape |= MarshallerShape.StatefulPinnableReference; + methods = methods with { StatefulGetPinnableReference = statefulGetPinnableReference }; } return (shape, methods); diff --git a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs index fb6de5f16781e..dd9885209d6d2 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs @@ -20,9 +20,11 @@ namespace LibraryImportGenerator.Unit.Tests public class ShapeBreakingDiagnosticSuppressorTests { [Fact] - public async Task EmptyParameterlessFreeMethodHasSuppressedDiagnostic() + public async Task StatefulValueMarshallerMethodsThatDoNotUseInstanceState_SuppressesDiagnostic() { await VerifySuppressorAsync(""" + using System; + using System.Runtime.CompilerServices; using System.Runtime.InteropServices.Marshalling; struct S @@ -35,15 +37,108 @@ static class Marshaller { public struct ManagedToUnmanagedIn { - private int i; - public void FromManaged(S s) { i = s.b ? 1 : 0; } - public ManagedToUnmanagedIn ToUnmanaged() => this; + public static int BufferSize { get; } = 1; - public void {|#0:Free|}() {} + public void {|#0:FromManaged|}(S s) + { + } + + public void {|#1:FromManaged|}(S s, Span buffer) + { + } + + public ManagedToUnmanagedIn {|#2:ToUnmanaged|}() + { + return default; + } + + public void {|#3:FromUnmanaged|}(ManagedToUnmanagedIn unmanaged) + { + } + + public S {|#4:ToManaged|}() + { + return default; + } + + public void {|#5:Free|}() {} + + public void {|#6:OnInvoked|}() {} + + public ref byte {|#7:GetPinnableReference|}() => ref Unsafe.NullRef(); + } + } + """, + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(0), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(1), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(2), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(3), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(4), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(5), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(6), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(7)); + } + + [Fact] + public async Task MethodWithShapeMatchingNameButDifferingSignature_DoesNotSuppressDiagnostic() + { + await VerifySuppressorAsync(""" + using System.Runtime.InteropServices.Marshalling; + + struct S + { + public bool b; + }; + + [CustomMarshaller(typeof(S), MarshalMode.ManagedToUnmanagedIn, typeof(ManagedToUnmanagedIn))] + static class Marshaller + { + public struct ManagedToUnmanagedIn + { + public void {|#0:Free|}(int i) {} } } """, - new DiagnosticResult(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression.SuppressedDiagnosticId, DiagnosticSeverity.Info).WithLocation(0).WithIsSuppressed(true)); + Diagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression.SuppressedDiagnosticId, DiagnosticSeverity.Info).WithLocation(0)); + } + + [Fact] + public async Task StatefulValueMarshallerMethodsForMarshallerInNonNestedMarshaller_DoesNotSuppressDiagnostics() + { + // For performance reasons, we only look on containing types to find a CustomMarshallerAttribute. + // As a result, we miss some cases of the bad diagnostics. + // Since we're going to recommend people make their marshallers nested types of their entry-point type, + // this limitation isn't unreasonable. If the user isn't following our best practices, then they're going + // to have a worse dev experience. + await VerifySuppressorAsync(""" + using System.Runtime.InteropServices.Marshalling; + + struct S + { + public bool b; + }; + + [CustomMarshaller(typeof(S), MarshalMode.ManagedToUnmanagedIn, typeof(ManagedToUnmanagedIn))] + static class Marshaller + { + } + + public struct ManagedToUnmanagedIn + { + public void {|#0:Free|}() {} + } + """, + Diagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression.SuppressedDiagnosticId, DiagnosticSeverity.Info).WithLocation(0)); + } + + private static DiagnosticResult Diagnostic(string id, DiagnosticSeverity originalDiagnosticSeverity) + { + return new DiagnosticResult(id, originalDiagnosticSeverity); + } + + private static DiagnosticResult SuppressedDiagnostic(SuppressionDescriptor descriptor, DiagnosticSeverity originalDiagnosticSeverity) + { + return new DiagnosticResult(descriptor.SuppressedDiagnosticId, originalDiagnosticSeverity).WithIsSuppressed(true); } private static async Task VerifySuppressorAsync(string source, params DiagnosticResult[] expected) From c474b9c84f383fc6ea3948999d176188c53118a4 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 14 Jul 2022 10:41:28 -0700 Subject: [PATCH 3/4] Add the suppression id to our diagnostic list markdown file. Disable the test suite on Mono against an active issue --- docs/project/list-of-diagnostics.md | 6 ++++++ .../Analyzers/ShapeBreakingDiagnosticSuppressor.cs | 2 +- .../ShapeBreakingDiagnosticSuppressorTests.cs | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/project/list-of-diagnostics.md b/docs/project/list-of-diagnostics.md index 0c40bc3c7abad..750e556d6d9ac 100644 --- a/docs/project/list-of-diagnostics.md +++ b/docs/project/list-of-diagnostics.md @@ -173,3 +173,9 @@ The diagnostic id values reserved for .NET Libraries analyzer warnings are `SYSL | __`SYSLIB1062`__ | *_`SYSLIB1062`-`SYSLIB1064` reserved for Microsoft.Interop.LibraryImportGenerator._* | | __`SYSLIB1063`__ | *_`SYSLIB1062`-`SYSLIB1064` reserved for Microsoft.Interop.LibraryImportGenerator._* | | __`SYSLIB1064`__ | *_`SYSLIB1062`-`SYSLIB1064` reserved for Microsoft.Interop.LibraryImportGenerator._* | + +### Diagnostic Suppressions (`SYSLIBSUPPRESS****`) + +| Suppression ID | Suppressed Diagnostic ID | Description | +| :----------------- | :----------------------- | :---------- | +| __`SYSLIBSUPPRESS0001`__ | CA1822 | Do not offer to make methods static when the methods need to be instance methods for a custom marshaller shape. | \ No newline at end of file diff --git a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs index c90904a959090..a55fd92dd9991 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs @@ -14,7 +14,7 @@ namespace Microsoft.Interop.Analyzers [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] public class ShapeBreakingDiagnosticSuppressor : DiagnosticSuppressor { - public static readonly SuppressionDescriptor MarkMethodsAsStaticSuppression = new SuppressionDescriptor("SYSLIBSUPPRESS1001", "CA1822", "Do not offer to make methods static when they need to be instance methods."); + public static readonly SuppressionDescriptor MarkMethodsAsStaticSuppression = new SuppressionDescriptor("SYSLIBSUPPRESS0001", "CA1822", "Do not offer to make methods static when the methods need to be instance methods for a custom marshaller shape."); public override ImmutableArray SupportedSuppressions => ImmutableArray.Create(MarkMethodsAsStaticSuppression); diff --git a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs index dd9885209d6d2..1850321f96b42 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs @@ -17,6 +17,7 @@ namespace LibraryImportGenerator.Unit.Tests { + [ActiveIssue("https://github.com/dotnet/runtime/issues/60650", TestRuntimes.Mono)] public class ShapeBreakingDiagnosticSuppressorTests { [Fact] From 2ceab41cb349d629abe799e843a8584d518657a1 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 14 Jul 2022 15:29:00 -0700 Subject: [PATCH 4/4] Enhance handling of resolving the marshaller type in presence of generics and reuse this logic from the suppressor. Add a linear collection test. PR feedback. --- .../ShapeBreakingDiagnosticSuppressor.cs | 25 ++--- .../ManualTypeMarshallingHelper.cs | 92 ++++++++++++++----- .../ShapeBreakingDiagnosticSuppressorTests.cs | 76 +++++++++++---- 3 files changed, 142 insertions(+), 51 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs index a55fd92dd9991..b4d6fbc1632b4 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ShapeBreakingDiagnosticSuppressor.cs @@ -35,18 +35,20 @@ private static void SuppressMarkMethodsAsStaticDiagnosticIfNeeded(SuppressionAna SemanticModel model = context.GetSemanticModel(diagnostic.Location.SourceTree); ISymbol diagnosedSymbol = model.GetDeclaredSymbol(diagnostic.Location.SourceTree.GetRoot(context.CancellationToken).FindNode(diagnostic.Location.SourceSpan), context.CancellationToken); - if (diagnosedSymbol.Kind == SymbolKind.Method) + if (diagnosedSymbol.Kind != SymbolKind.Method) { - if (FindContainingEntryPointTypeAndManagedType(diagnosedSymbol.ContainingType) is (INamedTypeSymbol entryPointMarshallerType, INamedTypeSymbol managedType)) + return; + } + + if (FindContainingEntryPointTypeAndManagedType(diagnosedSymbol.ContainingType) is (INamedTypeSymbol entryPointMarshallerType, INamedTypeSymbol managedType)) + { + bool isLinearCollectionMarshaller = ManualTypeMarshallingHelper.IsLinearCollectionEntryPoint(entryPointMarshallerType); + (MarshallerShape _, StatefulMarshallerShapeHelper.MarshallerMethods methods) = StatefulMarshallerShapeHelper.GetShapeForType(diagnosedSymbol.ContainingType, managedType, isLinearCollectionMarshaller, context.Compilation); + if (methods.IsShapeMethod((IMethodSymbol)diagnosedSymbol)) { - bool isLinearCollectionMarshaller = entryPointMarshallerType.GetAttributes().Any(attr => attr.AttributeClass?.ToDisplayString() == TypeNames.ContiguousCollectionMarshallerAttribute); - (MarshallerShape _, StatefulMarshallerShapeHelper.MarshallerMethods methods) = StatefulMarshallerShapeHelper.GetShapeForType(diagnosedSymbol.ContainingType, managedType, isLinearCollectionMarshaller, context.Compilation); - if (methods.IsShapeMethod((IMethodSymbol)diagnosedSymbol)) - { - // If we are a method of the shape on the stateful marshaller shape, then we need to be our current shape. - // So, suppress the diagnostic to make this method static, as that would break the shape. - context.ReportSuppression(Suppression.Create(MarkMethodsAsStaticSuppression, diagnostic)); - } + // If we are a method of the shape on the stateful marshaller shape, then we need to be our current shape. + // So, suppress the diagnostic to make this method static, as that would break the shape. + context.ReportSuppression(Suppression.Create(MarkMethodsAsStaticSuppression, diagnostic)); } } } @@ -60,7 +62,8 @@ private static (INamedTypeSymbol EntryPointType, INamedTypeSymbol ManagedType)? && attr.AttributeConstructor is not null && !attr.ConstructorArguments[0].IsNull && attr.ConstructorArguments[2].Value is INamedTypeSymbol marshallerTypeInAttribute - && SymbolEqualityComparer.Default.Equals(marshallerTypeInAttribute, marshallerType)); + && ManualTypeMarshallingHelper.TryResolveMarshallerType(containingType, marshallerTypeInAttribute, _ => { }, out ITypeSymbol? constructedMarshallerType) + && SymbolEqualityComparer.Default.Equals(constructedMarshallerType, marshallerType)); if (attrData is not null) { return (containingType, (INamedTypeSymbol)attrData.ConstructorArguments[0].Value); diff --git a/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/ManualTypeMarshallingHelper.cs b/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/ManualTypeMarshallingHelper.cs index 89a11309070ff..9fe7724de2722 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/ManualTypeMarshallingHelper.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/ManualTypeMarshallingHelper.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Linq.Expressions; using System.Runtime.CompilerServices; @@ -62,6 +63,8 @@ public static class MarshalUsingProperties public const string ConstantElementCount = nameof(ConstantElementCount); } + private static void IgnoreDiagnostic(Diagnostic diagnostic) { } + public static bool IsLinearCollectionEntryPoint(INamedTypeSymbol entryPointType) { return entryPointType.IsGenericType @@ -149,28 +152,9 @@ private static bool TryGetMarshallersFromEntryType( continue; ITypeSymbol marshallerType = marshallerTypeOnAttr; - if (isLinearCollectionMarshalling && marshallerTypeOnAttr is INamedTypeSymbol namedMarshallerType) + if (!TryResolveMarshallerType(entryPointType, marshallerType, IgnoreDiagnostic, out marshallerType)) { - // Update the marshaller type with resolved type arguments based on the entry point type - // We expect the entry point to already have its type arguments updated based on the managed type - Stack nestedTypeNames = new Stack(); - INamedTypeSymbol currentType = namedMarshallerType; - while (currentType is not null) - { - if (currentType.IsConstructedFromEqualTypes(entryPointType)) - break; - - nestedTypeNames.Push(currentType.Name); - currentType = currentType.ContainingType; - } - - currentType = entryPointType; - foreach (string name in nestedTypeNames) - { - currentType = currentType.GetTypeMembers(name).First(); - } - - marshallerType = currentType; + continue; } // TODO: Report invalid shape for mode @@ -198,6 +182,72 @@ private static bool TryGetMarshallersFromEntryType( return true; } + /// + /// Resolve the (possibly unbound generic) marshaller type to a fully constructed type based on the entry point type's generic parameters. + /// + /// The entry point type + /// The marshaller type from the CustomMarshallerAttribute + /// A fully constructed marshaller type + public static bool TryResolveMarshallerType(INamedTypeSymbol entryPointType, ITypeSymbol? attributeMarshallerType, Action reportDiagnostic, [NotNullWhen(true)] out ITypeSymbol? marshallerType) + { + if (attributeMarshallerType is null) + { + marshallerType = null; + return false; + } + + if (attributeMarshallerType is not INamedTypeSymbol namedMarshallerType) + { + marshallerType = attributeMarshallerType; + return true; + } + + // Update the marshaller type with resolved type arguments based on the entry point type + // We expect the entry point to already have its type arguments updated based on the managed type + Stack nestedTypes = new(); + INamedTypeSymbol currentType = namedMarshallerType; + int totalArity = 0; + while (currentType is not null) + { + nestedTypes.Push(currentType); + totalArity += currentType.Arity; + currentType = currentType.ContainingType; + } + + if (totalArity != entryPointType.Arity) + { + //TODO: Report diagnostic + marshallerType = null; + return false; + } + + int currentArityOffset = 0; + currentType = null; + while (nestedTypes.Count > 0) + { + if (currentType is null) + { + currentType = nestedTypes.Pop(); + } + else + { + INamedTypeSymbol originalType = nestedTypes.Pop(); + currentType = currentType.GetTypeMembers(originalType.Name, originalType.Arity).First(); + } + + if (currentType.TypeParameters.Length > 0) + { + currentType = currentType.ConstructedFrom.Construct( + ImmutableArray.CreateRange(entryPointType.TypeArguments, currentArityOffset, currentType.TypeParameters.Length, x => x), + ImmutableArray.CreateRange(entryPointType.TypeArgumentNullableAnnotations, currentArityOffset, currentType.TypeParameters.Length, x => x)); + currentArityOffset += currentType.TypeParameters.Length; + } + } + + marshallerType = currentType; + return true; + } + /// /// Resolve a non- to the correct /// managed type if is generic and diff --git a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs index 1850321f96b42..3bcdf6bbf32c2 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/ShapeBreakingDiagnosticSuppressorTests.cs @@ -15,7 +15,7 @@ using Microsoft.Interop.Analyzers; using Xunit; -namespace LibraryImportGenerator.Unit.Tests +namespace LibraryImportGenerator.UnitTests { [ActiveIssue("https://github.com/dotnet/runtime/issues/60650", TestRuntimes.Mono)] public class ShapeBreakingDiagnosticSuppressorTests @@ -40,27 +40,15 @@ public struct ManagedToUnmanagedIn { public static int BufferSize { get; } = 1; - public void {|#0:FromManaged|}(S s) - { - } + public void {|#0:FromManaged|}(S s) {} - public void {|#1:FromManaged|}(S s, Span buffer) - { - } + public void {|#1:FromManaged|}(S s, Span buffer){} - public ManagedToUnmanagedIn {|#2:ToUnmanaged|}() - { - return default; - } + public ManagedToUnmanagedIn {|#2:ToUnmanaged|}() => default; - public void {|#3:FromUnmanaged|}(ManagedToUnmanagedIn unmanaged) - { - } + public void {|#3:FromUnmanaged|}(ManagedToUnmanagedIn unmanaged) {} - public S {|#4:ToManaged|}() - { - return default; - } + public S {|#4:ToManaged|}() => default; public void {|#5:Free|}() {} @@ -80,6 +68,56 @@ public struct ManagedToUnmanagedIn SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(7)); } + [Fact] + public async Task StatefulLinearCollectionMarshallerMethodsThatDoNotUseInstanceState_SuppressesDiagnostic() + { + await VerifySuppressorAsync(""" + using System; + using System.Runtime.CompilerServices; + using System.Runtime.InteropServices.Marshalling; + + struct S + { + public bool b; + }; + + [CustomMarshaller(typeof(S), MarshalMode.ManagedToUnmanagedIn, typeof(Marshaller<>.ManagedToUnmanagedIn))] + [ContiguousCollectionMarshaller] + static class Marshaller + { + public struct ManagedToUnmanagedIn + { + public void {|#0:FromManaged|}(S s) {} + + public void {|#1:FromManaged|}(S s, Span buffer){} + + public ManagedToUnmanagedIn {|#2:ToUnmanaged|}() => default; + + public void {|#3:FromUnmanaged|}(ManagedToUnmanagedIn unmanaged) {} + + public S {|#4:ToManaged|}() => default; + + public ReadOnlySpan {|#5:GetManagedValuesSource|}() => default; + + public Span {|#6:GetUnmanagedValuesDestination|}() => default; + + public ReadOnlySpan {|#7:GetUnmanagedValuesSource|}(int numElements) => default; + + public Span {|#8:GetManagedValuesDestination|}(int numElements) => default; + } + } + """, + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(0), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(1), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(2), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(3), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(4), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(5), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(6), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(7), + SuppressedDiagnostic(ShapeBreakingDiagnosticSuppressor.MarkMethodsAsStaticSuppression, DiagnosticSeverity.Info).WithLocation(8)); + } + [Fact] public async Task MethodWithShapeMatchingNameButDifferingSignature_DoesNotSuppressDiagnostic() { @@ -153,7 +191,7 @@ private static async Task VerifySuppressorAsync(string source, params Diagnostic await test.RunAsync(CancellationToken.None); } - class Test : CSharpCodeFixVerifier.Test + private class Test : CSharpCodeFixVerifier.Test { public Test() {