Skip to content

Commit d97a9c1

Browse files
Copilotjkoritzinsky
Copilot
and
jkoritzinsky
authored
Move diagnostic generation from LibraryImportGenerator to LibraryImportDiagnosticsAnalyzer (#123780)
## Description Moves diagnostic generation/emit from the `LibraryImportGenerator` source generator to a new `LibraryImportDiagnosticsAnalyzer` in the same assembly, per Roslyn team recommendation. ### Changes Made #### 1. Created `LibraryImportDiagnosticsAnalyzer` - Runs the same diagnostic logic as the generator - Uses `SymbolEqualityComparer` to compare attribute types instead of string matching - Creates `LibraryImportGeneratorOptions` once per compilation in `RegisterCompilationStartAction` - `GetDiagnosticIfInvalidMethodForGeneration` is `internal static` so the generator can call it directly - Uses thread-safe access for `foundLibraryImportMethod` flag with `Interlocked.Exchange` and `Volatile.Read` #### 2. Updated `LibraryImportGenerator` - Removed diagnostic reporting from the pipeline - Calls `LibraryImportDiagnosticsAnalyzer.GetDiagnosticIfInvalidMethodForGeneration` directly to filter methods - No longer uses `DiagnosticOr` in the pipeline - Removed `Diagnostics` field from `IncrementalStubGenerationContext` - Inlined `Comparers.GeneratedSyntax` to use `SyntaxEquivalentComparer.Instance` directly - Removed the `Comparers` class static field wrapper #### 3. Updated `CompilationExtensions.GetEnvironmentFlags` - Changed to check for `DisableRuntimeMarshallingAttribute` on the source assembly instead of the module (aligning with the attribute's `AttributeTargets.Assembly` usage) #### 4. Updated Test Infrastructure - Added `TAnalyzer` type parameter to `CSharpSourceGeneratorVerifier` - Added `DisabledDiagnostics.Add(GeneratorDiagnostics.Ids.NotRecommendedGeneratedComInterfaceUsage)` to `CSharpAnalyzerVerifier` - Tests using `LibraryImportGenerator` use `LibraryImportDiagnosticsAnalyzer` - Tests using other generators use `EmptyDiagnosticAnalyzer` - `CSharpAnalyzerVerifier` uses targeted compiler diagnostic exclusion (CS8795, CS0751) instead of disabling all compiler diagnostics #### 5. Updated Test Files - `Diagnostics.cs`: Uses `CSharpAnalyzerVerifier` (analyzer-only testing) - `CompileFails.cs`: Uses `CSharpAnalyzerVerifier` for diagnostic tests - `ByValueContentsMarshalling.cs`: Uses analyzer verifier with SYSLIB1092 re-enabled for specific tests ### Test Results - **LibraryImportGenerator.Unit.Tests**: 702/703 pass (1 skipped) - **ComInterfaceGenerator.Unit.Tests**: 839/839 pass ### Key Architectural Decisions 1. **Analyzer runs independently**: The analyzer can run without the generator, making diagnostic-only tests faster 2. **Generator uses analyzer logic**: The generator calls the analyzer's `GetDiagnosticIfInvalidMethodForGeneration` method to share validation logic 3. **Test separation**: Tests that verify diagnostics run only the analyzer; tests that verify code generation run both 4. **Thread-safety**: The `foundLibraryImportMethod` flag uses `Interlocked`/`Volatile` for thread-safe access since `EnableConcurrentExecution()` is called 5. **Targeted diagnostic exclusion**: Analyzer-only tests exclude only CS8795 and CS0751 compiler diagnostics rather than all compiler diagnostics, ensuring typos in tests are still caught <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com> Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
1 parent 8f2cee4 commit d97a9c1

21 files changed

Lines changed: 616 additions & 417 deletions

‎src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/LibraryImportDiagnosticsAnalyzer.cs‎

Lines changed: 300 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,300 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Collections.Immutable;
6+
using System.Threading;
7+
using Microsoft.CodeAnalysis;
8+
using Microsoft.CodeAnalysis.CSharp;
9+
using Microsoft.CodeAnalysis.CSharp.Syntax;
10+
using Microsoft.CodeAnalysis.Diagnostics;
11+
using Microsoft.CodeAnalysis.DotnetRuntime.Extensions;
12+
using Microsoft.Interop;
13+
14+
namespace Microsoft.Interop.Analyzers
15+
{
16+
/// <summary>
17+
/// Analyzer that reports diagnostics for LibraryImport methods.
18+
/// This analyzer runs the same diagnostic logic as LibraryImportGenerator
19+
/// but reports diagnostics separately from the source generator.
20+
/// </summary>
21+
[DiagnosticAnalyzer(LanguageNames.CSharp)]
22+
public class LibraryImportDiagnosticsAnalyzer : DiagnosticAnalyzer
23+
{
24+
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } =
25+
ImmutableArray.Create(
26+
GeneratorDiagnostics.InvalidAttributedMethodSignature,
27+
GeneratorDiagnostics.InvalidAttributedMethodContainingTypeMissingModifiers,
28+
GeneratorDiagnostics.InvalidStringMarshallingConfiguration,
29+
GeneratorDiagnostics.ParameterTypeNotSupported,
30+
GeneratorDiagnostics.ReturnTypeNotSupported,
31+
GeneratorDiagnostics.ParameterTypeNotSupportedWithDetails,
32+
GeneratorDiagnostics.ReturnTypeNotSupportedWithDetails,
33+
GeneratorDiagnostics.ParameterConfigurationNotSupported,
34+
GeneratorDiagnostics.ReturnConfigurationNotSupported,
35+
GeneratorDiagnostics.MarshalAsParameterConfigurationNotSupported,
36+
GeneratorDiagnostics.MarshalAsReturnConfigurationNotSupported,
37+
GeneratorDiagnostics.ConfigurationNotSupported,
38+
GeneratorDiagnostics.ConfigurationValueNotSupported,
39+
GeneratorDiagnostics.MarshallingAttributeConfigurationNotSupported,
40+
GeneratorDiagnostics.CannotForwardToDllImport,
41+
GeneratorDiagnostics.RequiresAllowUnsafeBlocks,
42+
GeneratorDiagnostics.UnnecessaryParameterMarshallingInfo,
43+
GeneratorDiagnostics.UnnecessaryReturnMarshallingInfo,
44+
GeneratorDiagnostics.SizeOfInCollectionMustBeDefinedAtCallOutParam,
45+
GeneratorDiagnostics.SizeOfInCollectionMustBeDefinedAtCallReturnValue,
46+
GeneratorDiagnostics.LibraryImportUsageDoesNotFollowBestPractices);
47+
48+
public override void Initialize(AnalysisContext context)
49+
{
50+
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
51+
context.EnableConcurrentExecution();
52+
context.RegisterCompilationStartAction(context =>
53+
{
54+
// Nothing to do if the LibraryImportAttribute is not in the compilation
55+
INamedTypeSymbol? libraryImportAttrType = context.Compilation.GetBestTypeByMetadataName(TypeNames.LibraryImportAttribute);
56+
if (libraryImportAttrType is null)
57+
return;
58+
59+
StubEnvironment env = new StubEnvironment(
60+
context.Compilation,
61+
context.Compilation.GetEnvironmentFlags());
62+
63+
// Get generator options once per compilation
64+
LibraryImportGeneratorOptions options = new(context.Options.AnalyzerConfigOptionsProvider.GlobalOptions);
65+
66+
// Track if we found any LibraryImport methods to report RequiresAllowUnsafeBlocks once
67+
int foundLibraryImportMethod = 0;
68+
bool unsafeEnabled = context.Compilation.Options is CSharpCompilationOptions { AllowUnsafe: true };
69+
70+
context.RegisterSymbolAction(symbolContext =>
71+
{
72+
if (AnalyzeMethod(symbolContext, env, libraryImportAttrType, options))
73+
{
74+
Interlocked.Exchange(ref foundLibraryImportMethod, 1);
75+
}
76+
}, SymbolKind.Method);
77+
78+
// Report RequiresAllowUnsafeBlocks once per compilation if there are LibraryImport methods and unsafe is not enabled
79+
context.RegisterCompilationEndAction(endContext =>
80+
{
81+
if (Volatile.Read(ref foundLibraryImportMethod) != 0 && !unsafeEnabled)
82+
{
83+
endContext.ReportDiagnostic(DiagnosticInfo.Create(GeneratorDiagnostics.RequiresAllowUnsafeBlocks, null).ToDiagnostic());
84+
}
85+
});
86+
});
87+
}
88+
89+
/// <summary>
90+
/// Analyzes a method for LibraryImport diagnostics.
91+
/// </summary>
92+
/// <returns>True if the method has LibraryImportAttribute, false otherwise.</returns>
93+
private static bool AnalyzeMethod(SymbolAnalysisContext context, StubEnvironment env, INamedTypeSymbol libraryImportAttrType, LibraryImportGeneratorOptions options)
94+
{
95+
IMethodSymbol method = (IMethodSymbol)context.Symbol;
96+
97+
// Only analyze methods with LibraryImportAttribute
98+
AttributeData? libraryImportAttr = null;
99+
foreach (AttributeData attr in method.GetAttributes())
100+
{
101+
if (SymbolEqualityComparer.Default.Equals(attr.AttributeClass, libraryImportAttrType))
102+
{
103+
libraryImportAttr = attr;
104+
break;
105+
}
106+
}
107+
108+
if (libraryImportAttr is null)
109+
return false;
110+
111+
// Find the method syntax
112+
foreach (SyntaxReference syntaxRef in method.DeclaringSyntaxReferences)
113+
{
114+
if (syntaxRef.GetSyntax(context.CancellationToken) is MethodDeclarationSyntax methodSyntax)
115+
{
116+
AnalyzeMethodSyntax(context, methodSyntax, method, libraryImportAttr, env, options);
117+
break;
118+
}
119+
}
120+
121+
return true;
122+
}
123+
124+
private static void AnalyzeMethodSyntax(
125+
SymbolAnalysisContext context,
126+
MethodDeclarationSyntax methodSyntax,
127+
IMethodSymbol method,
128+
AttributeData libraryImportAttr,
129+
StubEnvironment env,
130+
LibraryImportGeneratorOptions options)
131+
{
132+
// Check for invalid method signature
133+
DiagnosticInfo? invalidMethodDiagnostic = GetDiagnosticIfInvalidMethodForGeneration(methodSyntax, method);
134+
if (invalidMethodDiagnostic is not null)
135+
{
136+
context.ReportDiagnostic(invalidMethodDiagnostic.ToDiagnostic());
137+
return; // Don't continue analysis if the method is invalid
138+
}
139+
140+
// Note: RequiresAllowUnsafeBlocks is reported once per compilation in Initialize method
141+
142+
// Calculate stub information and collect diagnostics
143+
var diagnostics = CalculateDiagnostics(methodSyntax, method, libraryImportAttr, env, options, context.CancellationToken);
144+
145+
foreach (DiagnosticInfo diagnostic in diagnostics)
146+
{
147+
context.ReportDiagnostic(diagnostic.ToDiagnostic());
148+
}
149+
}
150+
151+
private static ImmutableArray<DiagnosticInfo> CalculateDiagnostics(
152+
MethodDeclarationSyntax originalSyntax,
153+
IMethodSymbol symbol,
154+
AttributeData libraryImportAttr,
155+
StubEnvironment environment,
156+
LibraryImportGeneratorOptions options,
157+
CancellationToken ct)
158+
{
159+
ct.ThrowIfCancellationRequested();
160+
161+
var locations = new MethodSignatureDiagnosticLocations(originalSyntax);
162+
var generatorDiagnostics = new GeneratorDiagnosticsBag(
163+
new DiagnosticDescriptorProvider(),
164+
locations,
165+
SR.ResourceManager,
166+
typeof(FxResources.Microsoft.Interop.LibraryImportGenerator.SR));
167+
168+
// Process the LibraryImport attribute
169+
LibraryImportCompilationData? libraryImportData = ProcessLibraryImportAttribute(libraryImportAttr);
170+
171+
// If we can't parse the attribute, we have an invalid compilation - stop processing
172+
if (libraryImportData is null)
173+
{
174+
return generatorDiagnostics.Diagnostics.ToImmutableArray();
175+
}
176+
177+
if (libraryImportData.IsUserDefined.HasFlag(InteropAttributeMember.StringMarshalling))
178+
{
179+
// User specified StringMarshalling.Custom without specifying StringMarshallingCustomType
180+
if (libraryImportData.StringMarshalling == StringMarshalling.Custom && libraryImportData.StringMarshallingCustomType is null)
181+
{
182+
generatorDiagnostics.ReportInvalidStringMarshallingConfiguration(
183+
libraryImportAttr, symbol.Name, SR.InvalidStringMarshallingConfigurationMissingCustomType);
184+
}
185+
186+
// User specified something other than StringMarshalling.Custom while specifying StringMarshallingCustomType
187+
if (libraryImportData.StringMarshalling != StringMarshalling.Custom && libraryImportData.StringMarshallingCustomType is not null)
188+
{
189+
generatorDiagnostics.ReportInvalidStringMarshallingConfiguration(
190+
libraryImportAttr, symbol.Name, SR.InvalidStringMarshallingConfigurationNotCustom);
191+
}
192+
}
193+
194+
// Check for unsupported LCIDConversion attribute
195+
INamedTypeSymbol? lcidConversionAttrType = environment.LcidConversionAttrType;
196+
if (lcidConversionAttrType is not null)
197+
{
198+
foreach (AttributeData attr in symbol.GetAttributes())
199+
{
200+
if (SymbolEqualityComparer.Default.Equals(attr.AttributeClass, lcidConversionAttrType))
201+
{
202+
generatorDiagnostics.ReportConfigurationNotSupported(attr, nameof(TypeNames.LCIDConversionAttribute));
203+
break;
204+
}
205+
}
206+
}
207+
208+
// Create the signature context to collect marshalling-related diagnostics
209+
var signatureContext = SignatureContext.Create(
210+
symbol,
211+
DefaultMarshallingInfoParser.Create(environment, generatorDiagnostics, symbol, libraryImportData, libraryImportAttr),
212+
environment,
213+
new CodeEmitOptions(SkipInit: true),
214+
typeof(LibraryImportGenerator).Assembly);
215+
216+
// If forwarders are not being generated, we need to run stub generation logic to get those diagnostics too
217+
if (!options.GenerateForwarders)
218+
{
219+
IMarshallingGeneratorResolver resolver = DefaultMarshallingGeneratorResolver.Create(
220+
environment.EnvironmentFlags,
221+
MarshalDirection.ManagedToUnmanaged,
222+
TypeNames.LibraryImportAttribute_ShortName,
223+
[]);
224+
225+
// Check marshalling generators - this collects diagnostics for marshalling issues
226+
_ = new ManagedToNativeStubGenerator(
227+
signatureContext.ElementTypeInformation,
228+
LibraryImportData.From(libraryImportData).SetLastError,
229+
generatorDiagnostics,
230+
resolver,
231+
new CodeEmitOptions(SkipInit: true));
232+
}
233+
234+
return generatorDiagnostics.Diagnostics.ToImmutableArray();
235+
}
236+
237+
private static LibraryImportCompilationData? ProcessLibraryImportAttribute(AttributeData attrData)
238+
{
239+
// Found the LibraryImport, but it has an error so report the error.
240+
// This is most likely an issue with targeting an incorrect TFM.
241+
if (attrData.AttributeClass?.TypeKind is null or TypeKind.Error)
242+
{
243+
return null;
244+
}
245+
246+
if (attrData.ConstructorArguments.Length == 0)
247+
{
248+
return null;
249+
}
250+
251+
ImmutableDictionary<string, TypedConstant> namedArguments = ImmutableDictionary.CreateRange(attrData.NamedArguments);
252+
253+
string? entryPoint = null;
254+
if (namedArguments.TryGetValue(nameof(LibraryImportCompilationData.EntryPoint), out TypedConstant entryPointValue))
255+
{
256+
if (entryPointValue.Value is not string)
257+
{
258+
return null;
259+
}
260+
entryPoint = (string)entryPointValue.Value!;
261+
}
262+
263+
return new LibraryImportCompilationData(attrData.ConstructorArguments[0].Value!.ToString())
264+
{
265+
EntryPoint = entryPoint,
266+
}.WithValuesFromNamedArguments(namedArguments);
267+
}
268+
269+
/// <summary>
270+
/// Checks if a method is invalid for generation and returns a diagnostic if so.
271+
/// </summary>
272+
/// <returns>A diagnostic if the method is invalid, null otherwise.</returns>
273+
internal static DiagnosticInfo? GetDiagnosticIfInvalidMethodForGeneration(MethodDeclarationSyntax methodSyntax, IMethodSymbol method)
274+
{
275+
// Verify the method has no generic types or defined implementation
276+
// and is marked static and partial.
277+
if (methodSyntax.TypeParameterList is not null
278+
|| methodSyntax.Body is not null
279+
|| !methodSyntax.Modifiers.Any(SyntaxKind.StaticKeyword)
280+
|| !methodSyntax.Modifiers.Any(SyntaxKind.PartialKeyword))
281+
{
282+
return DiagnosticInfo.Create(GeneratorDiagnostics.InvalidAttributedMethodSignature, methodSyntax.Identifier.GetLocation(), method.Name);
283+
}
284+
285+
// Verify that the types the method is declared in are marked partial.
286+
if (methodSyntax.Parent is TypeDeclarationSyntax typeDecl && !typeDecl.IsInPartialContext(out var nonPartialIdentifier))
287+
{
288+
return DiagnosticInfo.Create(GeneratorDiagnostics.InvalidAttributedMethodContainingTypeMissingModifiers, methodSyntax.Identifier.GetLocation(), method.Name, nonPartialIdentifier);
289+
}
290+
291+
// Verify the method does not have a ref return
292+
if (method.ReturnsByRef || method.ReturnsByRefReadonly)
293+
{
294+
return DiagnosticInfo.Create(GeneratorDiagnostics.ReturnConfigurationNotSupported, methodSyntax.Identifier.GetLocation(), "ref return", method.ToDisplayString());
295+
}
296+
297+
return null;
298+
}
299+
}
300+
}

‎src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Comparers.cs‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,6 @@
1010

1111
namespace Microsoft.Interop
1212
{
13-
internal static class Comparers
14-
{
15-
/// <summary>
16-
/// Comparer for an individual generated stub source as a syntax tree and the generated diagnostics for the stub.
17-
/// </summary>
18-
public static readonly IEqualityComparer<(MemberDeclarationSyntax Syntax, ImmutableArray<DiagnosticInfo> Diagnostics)> GeneratedSyntax = new CustomValueTupleElementComparer<MemberDeclarationSyntax, ImmutableArray<DiagnosticInfo>>(SyntaxEquivalentComparer.Instance, new ImmutableArraySequenceEqualComparer<DiagnosticInfo>(EqualityComparer<DiagnosticInfo>.Default));
19-
}
20-
2113
/// <summary>
2214
/// Generic comparer to compare two <see cref="ImmutableArray{T}"/> instances element by element.
2315
/// </summary>

0 commit comments

Comments
 (0)