Fix LibraryImportDiagnosticsAnalyzer and DownlevelLibraryImportDiagnosticsAnalyzer missing diagnostics when StringMarshalling is set

[Copilot]

Description

LibraryImportDiagnosticsAnalyzer and DownlevelLibraryImportDiagnosticsAnalyzer fail to report diagnostics (e.g. SYSLIB1051) for [LibraryImport] methods when StringMarshalling is set. This is a regression introduced in preview.3 when diagnostic reporting was moved from the generators to separate analyzer classes. Additionally, the generated inner [DllImport] stub omits CharSet = CharSet.Unicode when StringMarshalling.Utf16 is set, causing incorrect runtime marshalling of forwarded types (e.g. StringBuilder).

Root cause: The source generator marks its generated stub output with [GeneratedCode]. Roslyn's generated-code heuristics then also classify the user's partial method declaration as generated code. With ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None), both analyzers silently skipped all [LibraryImport] methods.

Reproduction:

// SYSLIB1051 should be reported but is NOT (with StringMarshalling.Utf16)
[LibraryImport("kernel32.dll", StringMarshalling = StringMarshalling.Utf16)]
internal static partial int GetVolumeNameForVolumeMountPointW(
    string volumeMountPoint,
    [Out] StringBuilder volumeName,  // No error — silent bad codegen
    int bufferLength);

Changes

• LibraryImportDiagnosticsAnalyzer: Changed GeneratedCodeAnalysisFlags.None → Analyze so the analyzer runs on methods whose partial declaration is classified as generated code.
• DownlevelLibraryImportDiagnosticsAnalyzer: Applied the same GeneratedCodeAnalysisFlags.Analyze fix.
• LibraryImportGenerator.CreateTargetDllImportAsLocalStatement: Now forwards CharSet = CharSet.Unicode to the inner [DllImport] when StringMarshalling.Utf16 is set, ensuring forwarded types use the correct encoding.
• DownlevelLibraryImportGenerator.CreateTargetDllImportAsLocalStatement: Applied the same CharSet = CharSet.Unicode forwarding fix.
• Tests (Diagnostics.cs): Added StringBuilderNotSupported_ReportsDiagnostic and StringBuilderNotSupported_WithStringParam_ReportsDiagnostic regression tests covering StringBuilder with and without StringMarshalling variants.
• Tests (Compiles.cs): Added ForwardedTypesWithStringMarshalling_InnerDllImportHasCharSet test verifying that the inner [DllImport] has CharSet.Unicode when StringMarshalling.Utf16 is set and no CharSet argument when other values are used. Fixed test assertion to use EndsWith to handle the global:: qualified name emitted by the generator.

Testing

• Both generator projects (LibraryImportGenerator, DownlevelLibraryImportGenerator) build successfully with zero errors and warnings.
• Regression tests added for the diagnostic and code-generation fixes.
• Code review and CodeQL scan passed with no issues.

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[danmoseley]

It seems Copilot could not make a fix before timing out.

[jkoritzinsky]

@danmoseley I had copilot crank locally for quite a while and it couldn't figure it out but it did add some regression tests that should cover your scenario and pass without any product changes. Can you get me a complog of your build that didn't report the diagnostic?

Is it possible that you have RunAnalyzers set to false or that VS set it to false for one run?

[Copilot]

Pull request overview

This PR targets a regression in LibraryImportDiagnosticsAnalyzer where SYSLIB1051 is not reported for StringBuilder parameters when LibraryImportAttribute.StringMarshalling is specified, leading to silently incorrect forwarder stub generation.

Changes:

• Added analyzer unit tests asserting SYSLIB1051 (ParameterTypeNotSupported) is reported for StringBuilder parameters with StringMarshalling set (Utf16/Utf8) and without it.
• Added a reproduction-style test case for string + [Out] StringBuilder parameter combinations.

[danmoseley]

@danmoseley I had copilot crank locally for quite a while and it couldn't figure it out but it did add some regression tests that should cover your scenario and pass without any product changes. Can you get me a complog of your build that didn't report the diagnostic?

Is it possible that you have RunAnalyzers set to false or that VS set it to false for one run?

@jkoritzinsky I updated the repro (top post #126687) to include a global.json -- can you still not repro with that?

[danmoseley]

@jkoritzinsky also added the install command for preview 3. Using preview 2, it doesn't repro. I verified this by hand again, and it does repro for me with global.json pointing to 11.0.100-preview.3.26170.106

C:\temp\repro>type global.json | find /i "ver"
    "version": "11.0.100-preview.3.26170.106",

C:\temp\repro>type program.cs
using System;
using System.Runtime.InteropServices;
using System.Text;

Console.WriteLine("If you see this, SYSLIB1051 was NOT reported.");

static partial class NativeMethods
{
    [LibraryImport("kernel32.dll", StringMarshalling = StringMarshalling.Utf16)]
    internal static partial int GetVolumeNameForVolumeMountPointW(
        string volumeMountPoint,
        [Out] StringBuilder volumeName,
        int bufferLength);
}
C:\temp\repro>dotnet build
Restore complete (0.3s)
    info NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  doit net11.0 succeeded (0.1s) → bin\Debug\net11.0\doit.dll

Build succeeded in 0.9s

and

C:\temp\repro>set emitcompilergeneratedfiles=true

C:\temp\repro>dotnet build --no-incremental
Restore complete (0.3s)
    info NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  doit net11.0 succeeded (1.5s) → bin\Debug\net11.0\doit.dll

Build succeeded in 2.3s

C:\temp\repro>type obj\Debug\net11.0\generated\Microsoft.Interop.LibraryImportGenerator\Microsoft.Interop.LibraryImportGenerator\LibraryImports.g.cs
// <auto-generated/>
static unsafe partial class NativeMethods
{
    [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.LibraryImportGenerator", "11.0.14.17106")]
    [global::System.Runtime.CompilerServices.SkipLocalsInitAttribute]
    internal static partial int GetVolumeNameForVolumeMountPointW(string volumeMountPoint, global::System.Text.StringBuilder volumeName, int bufferLength)
    {
        int __retVal;
        // Pin - Pin data in preparation for calling the P/Invoke.
        fixed (void* __volumeMountPoint_native = &global::System.Runtime.InteropServices.Marshalling.Utf16StringMarshaller.GetPinnableReference(volumeMountPoint))
        {
            __retVal = __PInvoke((ushort*)__volumeMountPoint_native, volumeName, bufferLength);
        }

        return __retVal;
        // Local P/Invoke
        [global::System.Runtime.InteropServices.DllImportAttribute("kernel32.dll", EntryPoint = "GetVolumeNameForVolumeMountPointW", ExactSpelling = true)]
        static extern unsafe int __PInvoke(ushort* __volumeMountPoint_native, [System.Runtime.InteropServices.OutAttribute] global::System.Text.StringBuilder volumeName, int __bufferLength_native);
    }
}

[danmoseley]

The idea is that you get an IDE error, then you either fix the error or copy the generated code into your source and remove the LibraryImportAttribute. Then the error goes away as you aren't using the generator any more.

If I understand right then there are two bugs -- the error no longer firing, and the generated code not working (as I discovered in my original PR)

the issue with the generated code pasted above (I think) is that it doesn't have CharSet = CharSet.Unicode so I guess it defaulted to CharSet.None for volumeName which treats it as Ansi on Windows. (Aside, maybe generated code should always be explicit about CharSet anyway).

[jkoritzinsky]

@danmoseley I found the bug. Apparently the analyzer wasn't firing because the source-generated code was marking the user part of the LibraryImport as generated and skipping it. I've updated the analyzer to catch this case.

I've also added logic to forward CharSet.Unicode when needed in the forwarder.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[danmoseley]

BTW, if you have .Analyze do you also want | CodeAnalysisFlags.ReportDiagnostics

[jkoritzinsky]

We don't want ReportDiagnostics as we aren't reporting diagnostics in generated source files. We're reporting in user source.

To my understanding, what's blocking us here is the concept of "generated source symbols" not locations.

[danmoseley]

@jeffhandley do you own the code review agent nowadays? this is not the first time I've seen it time out:

Error: The action 'Execute GitHub Copilot CLI' has timed out after 20 minutes.

That blocks the PR being merged BTW

edit: I merged some changes to fix/improve this

[jtschuster]

This change looks good, but do we have a similar issue in the ComInterfaceGenerator?

[github-actions]

Note

This review was generated by Copilot (Claude Opus 4.6), with cross-validation from Claude Sonnet 4.5.

🤖 Copilot Code Review — PR #126691

Holistic Assessment

Motivation: The PR addresses a real bug where LibraryImportGenerator failed to forward CharSet=Unicode to the inner [DllImport] for non-forwarder stubs when StringMarshalling.Utf16 was specified. This caused runtime-marshalled types (e.g. StringBuilder) to default to ANSI encoding—a genuine correctness issue. The PR is justified.

Approach: The core generator fix is correct and mirrors the existing forwarder path. The analyzer change (GeneratedCodeAnalysisFlags.Analyze) is well-motivated—[GeneratedCode] on the generated stub causes Roslyn to treat the user's partial declaration as generated code, silently suppressing diagnostics. However, there are cross-cutting concerns with incomplete fixes in sibling generators and a test assertion that appears incorrect.

Summary: ⚠️ Needs Human Review. The core generator fix (CharSet forwarding) is correct. However: (1) a test assertion likely does not match the generated code's actual text form, (2) the identical bug exists unfixed in DownlevelLibraryImportGenerator, and (3) the analyzer flag change is inconsistent with all sibling analyzers. A maintainer should verify the test actually passes and decide on the scope of cross-cutting fixes.

---

Detailed Findings

⚠️ Test Assertion Likely Incorrect — Compiles.cs line 741

The test InnerDllImportCharSetTest.VerifyFinalCompilation asserts:

Assert.Equal($"{nameof(CharSet)}.{nameof(CharSet.Unicode)}", charSetArgument.Expression.ToString());
// i.e. Assert.Equal("CharSet.Unicode", ...)

However, the generator emits the expression via AliasQualifiedName("global", IdentifierName(typeof(CharSet).FullName)) (line 406), which produces the text global::System.Runtime.InteropServices.CharSet.Unicode. The generated source is emitted as text via NormalizeWhitespace().ToFullString() (IncrementalGeneratorInitializationContextExtensions.cs line 54), then re-parsed by the compiler into the compilation's syntax trees. When the test reads compilation.SyntaxTrees.Last() and calls .Expression.ToString(), the result should be "global::System.Runtime.InteropServices.CharSet.Unicode", not "CharSet.Unicode".

This assertion was added in the last commit ("Potential fix for pull request finding", authored by Copilot Autofix). The simpler presence/absence check from the prior version of the test was correct; this stronger assertion may cause the test to fail when actually run.

Suggestion: Either revert to the simpler presence check, or update the expected value:

Assert.Equal("global::System.Runtime.InteropServices.CharSet.Unicode", charSetArgument.Expression.ToString());

⚠️ Same Bug Exists in DownlevelLibraryImportGenerator — Not Fixed

DownlevelLibraryImportGenerator.CreateTargetDllImportAsLocalStatement (lines 317–358) has the identical bug: it creates the inner DllImport with only ModuleName, EntryPoint, and ExactSpelling—no CharSet forwarding. Its forwarder path (CreateForwarderDllImport, lines 378–387) already has the fix, confirming the non-forwarder path was just missed.

This is the same pattern as the bug being fixed here. Consider applying the same fix to the downlevel generator in this PR or a follow-up.

⚠️ Analyzer Configuration Inconsistency — Only This Analyzer Changed

The change from GeneratedCodeAnalysisFlags.None to GeneratedCodeAnalysisFlags.Analyze is well-motivated (the [GeneratedCode] attribute on generated stubs prevents user-code diagnostics). However, all sibling diagnostics analyzers still use None:

• DownlevelLibraryImportDiagnosticsAnalyzer — None
• ComInterfaceGeneratorDiagnosticsAnalyzer — None
• VtableIndexStubDiagnosticsAnalyzer — None
• ComClassGeneratorDiagnosticsAnalyzer — None

Since all these generators apply [GeneratedCode] to generated stubs via SignatureContext.cs, the DownlevelLibraryImportDiagnosticsAnalyzer likely has the same diagnostic suppression problem. The ComInterface analyzers may also be affected depending on how they target user-written code. Worth investigating whether the same fix is needed for the downlevel analyzer at minimum.

💡 Code Duplication — Consider Extracting Helper

The new CharSet expression construction (lines 401–407) manually inlines the same pattern as CreateEnumExpressionSyntax<T> (lines 484–490), which is a local function inside CreateForwarderDllImport and therefore inaccessible. Consider extracting it to a file-level private static method so both call sites can reuse it.

✅ Core Fix Correctness — CharSet Forwarding Logic

The fix correctly mirrors the existing forwarder behavior:

• Checks IsUserDefined.HasFlag(InteropAttributeMember.StringMarshalling) to ensure the user explicitly set the value
• Only forwards for StringMarshalling.Utf16 (not Utf8 or Custom), since CharSet only maps to Utf16
• Uses the same AliasQualifiedName + MemberAccessExpression pattern as the forwarder path

This ensures types forwarded to the CLR runtime marshaller (like StringBuilder) use the correct encoding. (Flagged as correct by both review models.)

✅ Diagnostic Tests — Well-Structured

The new tests in Diagnostics.cs are well-structured:

• StringBuilderNotSupported_ReportsDiagnostic uses [Theory] with three inline data cases (Utf16, Utf8, empty)
• StringBuilderNotSupported_WithStringParam_ReportsDiagnostic tests a more realistic scenario matching the linked issue
• Both correctly verify ParameterTypeNotSupported (SYSLIB1051) diagnostic location and arguments

✅ Analyzer Change Rationale — Correct for This Analyzer

The commit message (69a3b5c) explains: "Since we put the GeneratedCodeAttribute on the stub implementation, the user part of the partial is also marked as generated code." Using Analyze (without ReportDiagnostics) is the right choice—it allows the analyzer to inspect user-written partial declarations while diagnostics are reported at user code locations, not on generated stubs.

Generated by Code Review for issue #126691 · ◷

[jkoritzinsky]

The trigger attribute isn't applied to a member that we put the GeneratedCodeAttribute on with the COM source generator, so this isn't a problem there.

[jkoritzinsky]

@copilot please fix for the DownlevelLibraryImportGenerator and its corresponding analyzer as well

[Copilot]

@copilot please fix for the DownlevelLibraryImportGenerator and its corresponding analyzer as well

Applied the same two fixes to the DownlevelLibraryImportGenerator in commit d52c93e:

1. DownlevelLibraryImportDiagnosticsAnalyzer: changed GeneratedCodeAnalysisFlags.None → Analyze (same root-cause fix as the main analyzer)
2. DownlevelLibraryImportGenerator.CreateTargetDllImportAsLocalStatement: now forwards CharSet = Unicode when StringMarshalling.Utf16 is set, consistent with what CreateForwarderDllImport already did

Also fixed a test assertion (Assert.Equal → Assert.EndsWith) in the InnerDllImportCharSetTest to handle the global::System.Runtime.InteropServices.CharSet.Unicode qualified name emitted by the generator.