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
• 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

---

✨ 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.

[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

[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)

[jkoritzinsky]

/ba-g browser timeouts and Android failures unrelated.