Move diagnostic generation from LibraryImportGenerator to LibraryImportDiagnosticsAnalyzer

[Copilot]

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

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

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

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

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Mis-clicked approve

[jkoritzinsky]

@copilot address this review.

[Copilot]

@copilot Please adjust CompilationExtensions.GetEnvironmentFlags to check for DisableRuntimeMarshallingAttribute on the source assembly instead of the module. Also, add handling for the rest of the di...

Addressed in commits f33ac74 and 9f85c66:

1. CompilationExtensions.GetEnvironmentFlags - Updated to check compilation.Assembly.GetAttributes() instead of compilation.SourceModule.GetAttributes() for DisableRuntimeMarshallingAttribute.

2. Removed Diagnostics field - Removed from IncrementalStubGenerationContext record and updated all downstream usages. The generator now uses discarding diagnostics bags and no longer returns diagnostics.

3. Diagnostic edge cases - Fixed AddDisableRuntimeMarshallingAttributeFixerTests to use LibraryImportDiagnosticsAnalyzer. 28 tests still fail - the analyzer isn't reporting diagnostics for CompileFails.ValidateSnippets tests when run together with the generator. This appears to be related to how the analyzer's RegisterSymbolAction interacts with methods that have compile errors. Investigating further.

[jkoritzinsky]

@copilot all of the test failures look like they're due to diagnostics reported by a marshalling generator resolver not being reported from the analyzers.

[Copilot]

@copilot all of the test failures look like they're due to diagnostics reported by a marshalling generator resolver not being reported from the analyzers.

Identified and partially fixed. The issue was that CSharpSourceGeneratorTest doesn't properly run analyzer diagnostics verification. Updated tests to use CSharpAnalyzerVerifier instead.

Commits eb6466c and cd49aed:

• Changed CompileFails.ValidateSnippets to use analyzer verifier
• Changed ByValueContentsMarshalling to use analyzer verifier
• Removed CS8795 expectations (not relevant for analyzer-only tests)

Test status: 688/703 pass (14 failing). The 13 array marshalling tests are still failing because the analyzer returns 0 diagnostics for "Marshalling an array from unmanaged to managed requires..." cases. The ManagedToNativeStubGenerator in the analyzer may not be producing these diagnostics correctly. Investigating the marshalling generator resolver path.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

[jkoritzinsky]

@copilot please address this feedback.

[Copilot]

Pull request overview

This pull request refactors diagnostic generation for LibraryImport by moving it from the LibraryImportGenerator source generator to a new LibraryImportDiagnosticsAnalyzer, following Roslyn team recommendations to separate diagnostic reporting from code generation.

Changes:

• Created LibraryImportDiagnosticsAnalyzer that runs diagnostic logic independently of the generator
• Updated LibraryImportGenerator to filter invalid methods using the analyzer's validation logic instead of emitting diagnostics
• Corrected CompilationExtensions.GetEnvironmentFlags to check for DisableRuntimeMarshallingAttribute on the assembly instead of the source module (aligning with the attribute's AttributeTargets.Assembly usage)

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
LibraryImportDiagnosticsAnalyzer.cs	New analyzer that reports LibraryImport diagnostics independently from code generation
LibraryImportGenerator.cs	Removed diagnostic emission; now calls analyzer's validation method to filter methods
Comparers.cs	Removed Comparers static class wrapper (no longer needed without diagnostics in pipeline)
CompilationExtensions.cs	Fixed to check DisableRuntimeMarshallingAttribute on assembly (correct) vs module (incorrect)
CSharpSourceGeneratorVerifier.cs	Added TAnalyzer type parameter to support running both generator and analyzer in tests
CSharpAnalyzerVerifier.cs	Added constructor that disables compiler diagnostics and SYSLIB1092 by default
Diagnostics.cs, CompileFails.cs, etc.	Updated to use analyzer-only testing for diagnostic verification; removed compiler error expectations
ByValueContentsMarshalling.cs	Uses analyzer verifier with SYSLIB1092 re-enabled for specific tests
ComInterfaceGenerator test files	Updated type parameters to include analyzer types

[Copilot]

The foundLibraryImportMethod variable is accessed and modified from concurrent RegisterSymbolAction callbacks without synchronization. This creates a race condition since EnableConcurrentExecution() is called on line 51. The variable should be an int field using Interlocked.CompareExchange or wrapped with proper synchronization to ensure thread-safe access across concurrent symbol analysis callbacks.

Suggested change

	bool foundLibraryImportMethod = false;
	bool unsafeEnabled = context.Compilation.Options is CSharpCompilationOptions { AllowUnsafe: true };
	context.RegisterSymbolAction(symbolContext =>
	{
	if (AnalyzeMethod(symbolContext, env, libraryImportAttrType, options))
	{
	foundLibraryImportMethod = true;
	}
	}, SymbolKind.Method);
	// Report RequiresAllowUnsafeBlocks once per compilation if there are LibraryImport methods and unsafe is not enabled
	context.RegisterCompilationEndAction(endContext =>
	{
	if (foundLibraryImportMethod && !unsafeEnabled)
	int foundLibraryImportMethod = 0;
	bool unsafeEnabled = context.Compilation.Options is CSharpCompilationOptions { AllowUnsafe: true };
	context.RegisterSymbolAction(symbolContext =>
	{
	if (AnalyzeMethod(symbolContext, env, libraryImportAttrType, options))
	{
	Interlocked.CompareExchange(ref foundLibraryImportMethod, 1, 0);
	}
	}, SymbolKind.Method);
	// Report RequiresAllowUnsafeBlocks once per compilation if there are LibraryImport methods and unsafe is not enabled
	context.RegisterCompilationEndAction(endContext =>
	{
	if (Interlocked.CompareExchange(ref foundLibraryImportMethod, 0, 0) == 1 && !unsafeEnabled)

[jtschuster]

Is there a way to only disable a set of compiler diagnostics? I worry a typo in a new test case would raise a compiler diagnostic, but cause an expected analyzer diagnostic to not show up, and I could see myself spending a lot of time debugging the analyzer without realizing that it's just a typo error.

[jtschuster]

I'm not sure about how analyzers work in VS, is there any caching / incrementality that we might set this to true, then the LibraryImport is deleted, and we still report the message later?

[jtschuster]

Did we hit a failure that made this change necessary?

[stephentoub]

🤖 Copilot Code Review — PR #123780

Holistic Assessment

Motivation: Moving diagnostic generation from the LibraryImportGenerator source generator to a dedicated LibraryImportDiagnosticsAnalyzer aligns with Roslyn team recommendations. Separating diagnostics from code generation improves incremental build performance (analyzer diagnostics don't trigger recompilation when only diagnostics change) and allows diagnostic-only testing.

Approach: The implementation correctly reuses the existing diagnostic infrastructure (StubEnvironment, GeneratorDiagnosticsBag, ManagedToNativeStubGenerator) in the analyzer while having the generator call the analyzer's validation logic to filter invalid methods. This prevents logic drift between components.

Summary: ⚠️ Needs Human Review. The core architectural change is sound and tests have been appropriately updated. However, there is one thread-safety concern flagged by multiple models that should be verified, and a few minor items worth confirming before merge.

---

Detailed Findings

⚠️ Thread-Safety — foundLibraryImportMethod flag access (flagged by 2 models)

Location: LibraryImportDiagnosticsAnalyzer.cs lines 66-85

Both Gemini and GPT-5.1 flagged that the foundLibraryImportMethod boolean is captured in closures and accessed without synchronization:

bool foundLibraryImportMethod = false;
// ...
context.RegisterSymbolAction(symbolContext =>
{
    if (AnalyzeMethod(...))
    {
        foundLibraryImportMethod = true;  // Write from concurrent threads
    }
}, SymbolKind.Method);

context.RegisterCompilationEndAction(endContext =>
{
    if (foundLibraryImportMethod && !unsafeEnabled)  // Read
    {
        // Report diagnostic
    }
});

Since EnableConcurrentExecution() is called at line 51, symbol actions can execute concurrently. While boolean writes are atomic and the pattern acts as a latch (only ever set to true), there's no guarantee the compilation end action sees the updated value due to lack of a memory barrier.

Assessment: In practice, this is likely benign since the compilation end action runs after all symbol actions complete, and the Roslyn host typically provides proper synchronization at those boundaries. However, this depends on implementation details that could change. A defensive fix would be minimal overhead:

int foundLibraryImportMethod = 0;
// In symbol action:
Interlocked.Exchange(ref foundLibraryImportMethod, 1);
// In compilation end action:
if (Volatile.Read(ref foundLibraryImportMethod) != 0 && !unsafeEnabled)

Recommendation: Verify with the Roslyn team whether this is a real concern or if host guarantees make it safe. If uncertain, apply the defensive fix.

---

✅ Correctness — DisableRuntimeMarshallingAttribute detection fix

Location: CompilationExtensions.cs line 21

The change from compilation.SourceModule.GetAttributes() to compilation.Assembly.GetAttributes() is correct. DisableRuntimeMarshallingAttribute is an assembly-level attribute ([AttributeUsage(AttributeTargets.Assembly)]), so checking the assembly is more accurate than checking the source module.

This appears to be a bug fix rather than a behavior change.

---

✅ API Design — Shared validation logic via internal static method

Location: LibraryImportDiagnosticsAnalyzer.GetDiagnosticIfInvalidMethodForGeneration (lines 269-298)

The generator calling the analyzer's internal static method to share validation logic is a reasonable design choice. It ensures both components use identical checks for invalid methods (generic types, non-static/partial, ref returns, non-partial containing types).

---

💡 Code Duplication — ProcessLibraryImportAttribute duplicated

Locations:

• LibraryImportDiagnosticsAnalyzer.cs lines 237-267
• LibraryImportGenerator.cs lines 164-194

The ProcessLibraryImportAttribute method is duplicated in both files with nearly identical logic. This creates a maintenance risk where future changes to attribute parsing could diverge between analyzer and generator.

Recommendation: Consider extracting this to a shared helper method in a subsequent PR to reduce duplication.

---

✅ Test Coverage — Adequate for the architectural change

The test changes appropriately:

1. Added TAnalyzer type parameter to CSharpSourceGeneratorVerifier to run both generator and analyzer
2. Switched diagnostic-only tests to use CSharpAnalyzerVerifier
3. Removed CS8795 (partial method without implementation) expectations from analyzer-only tests since the generator isn't running
4. Added default disabling of SYSLIB1092 in analyzer verifier matching generator test behavior

The existing ValidateRequireAllowUnsafeBlocksDiagnostic test at CompileFails.cs:907-920 covers the RequiresAllowUnsafeBlocks diagnostic using the combined generator+analyzer verifier.

---

💡 Performance Consideration — Duplicate stub analysis

Locations:

• Analyzer: LibraryImportDiagnosticsAnalyzer.CalculateDiagnostics lines 208-232
• Generator: LibraryImportGenerator.GenerateSource lines 295-303

For non-forwarder configurations, both analyzer and generator now construct ManagedToNativeStubGenerator instances. The analyzer does this for diagnostics; the generator does this for codegen with a discarding diagnostics bag. This effectively doubles the stub analysis work.

Assessment: This is an accepted cost of the architectural split. For typical projects with modest P/Invoke counts, this should be negligible. If perf becomes a concern in large projects, consider incremental data sharing in a follow-up.

---

Cross-Cutting Analysis

The changes are consistent across the codebase:

• All LibraryImport tests using diagnostics now use the appropriate analyzer verifier
• ComInterfaceGenerator tests correctly use EmptyDiagnosticAnalyzer since they don't have a dedicated diagnostics analyzer
• The Comparers.GeneratedSyntax static field was correctly removed since diagnostics are no longer part of the generator pipeline output

---

Verdict

The PR is architecturally sound and follows Roslyn team recommendations. The main item requiring human judgment is whether the thread-safety concern around foundLibraryImportMethod needs a defensive fix or is covered by Roslyn's host guarantees. A maintainer familiar with Roslyn analyzer threading semantics should confirm.

The duplicated ProcessLibraryImportAttribute logic is a minor code quality concern that can be addressed in a follow-up PR.

---

Review generated by multi-model analysis (Claude Sonnet, Gemini 3 Pro, GPT-5.1)