Fix LibraryImportDiagnosticsAnalyzer missing SYSLIB1051 for StringBuilder when StringMarshalling is set

[Copilot]

Description

LibraryImportDiagnosticsAnalyzer fails to report SYSLIB1051 for StringBuilder parameters when StringMarshalling = StringMarshalling.Utf16 is set on [LibraryImport]. Without StringMarshalling, the error is correctly reported. This is a regression introduced in preview.3 when diagnostic reporting was moved from LibraryImportGenerator to the separate LibraryImportDiagnosticsAnalyzer (PR #123780). When the diagnostic is silently dropped, the generator produces a forwarder [DllImport] stub that omits CharSet = CharSet.Unicode, causing incorrect runtime marshalling behavior.

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

• Investigated the LibraryImportDiagnosticsAnalyzer.CalculateDiagnostics code path to identify why the diagnostic is lost when StringMarshalling is set
• Root cause analysis of the resolver chain: StringBuilder receives NoMarshallingInfo regardless of StringMarshalling, and NotSupportedResolver should emit the diagnostic — the exact point of loss needs further pinpointing

Note: This PR captures the investigation. The fix and regression test are pending completion.

Testing

• Regression test in Diagnostics.cs using VerifyCS.VerifyAnalyzerAsync covering StringBuilder with StringMarshalling.Utf16 expecting ParameterTypeNotSupported (SYSLIB1051)

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

[Copilot]

The PR title/description indicates this change fixes a missing SYSLIB1051 diagnostic, but this diff only adds new expectations in tests. As written, these tests will fail until the analyzer behavior is updated; consider including the analyzer fix in this PR (or adjusting the PR scope/title and not landing failing tests).

[Copilot]

Please avoid embedding a GitHub issue reference in the test comment ("#126687"). Prefer a self-contained description of the scenario so the test remains stable even if issue trackers/links change.

Suggested change

	// StringBuilder with [Out] alongside a string parameter (repro from dotnet/runtime#126687)
	// StringBuilder with [Out] alongside a string parameter

[github-actions]

Note

This review was generated by Copilot.

🤖 Copilot Code Review — PR #126691

Holistic Assessment

Motivation: The PR adds regression tests for issue #126687, verifying that SYSLIB1051 (ParameterTypeNotSupported) is correctly reported for StringBuilder parameters in [LibraryImport] methods when StringMarshalling is configured. This is a valid gap in test coverage — StringBuilder was not previously tested in this analyzer test suite.

Approach: Adding [Theory]-based tests to the existing Diagnostics.cs test file is the right approach and follows the established patterns in the file. The two test methods cover both a standalone StringBuilder parameter and the more complex StringBuilder + string mixed-parameter scenario from the original issue.

Summary: ⚠️ Needs Human Review. The test code itself is well-structured and consistent with the codebase, but the branch name (copilot/fix-library-import-diagnostics-analyzer) implies a fix was intended alongside these tests, yet only test additions are present. A human reviewer should verify that these tests actually pass on main today — if the analyzer already handles these cases correctly, then this is good test coverage; if not, the PR may be incomplete (missing the accompanying fix).

---

Detailed Findings

⚠️ Completeness — Tests-only PR from a "fix" branch

The branch name is copilot/fix-library-import-diagnostics-analyzer, suggesting a fix to the analyzer was intended. However, the PR only adds 54 lines of new tests to Diagnostics.cs — no changes to the analyzer source (LibraryImportDiagnosticsAnalyzer.cs), generator, or marshalling infrastructure. The commit message describes these as "regression tests" for #126687.

This is fine if the tests pass on main today (confirming the analyzer already handles StringBuilder + StringMarshalling correctly). But if these tests fail without an accompanying fix, the PR is incomplete. A reviewer should confirm the tests pass in CI before merging.

✅ Test Quality — Well-structured and follows conventions

The two new test methods follow all established patterns in this file:

• [Theory] with [InlineData] — preferred over multiple [Fact] methods per the repo conventions
• VerifyCS.VerifyAnalyzerAsync — uses the standard Roslyn test verifier pattern
• $$""" raw string literal interpolation — matches the C# 11 syntax used by sibling tests (e.g., NonPartialParentType_Diagnostic)
• Diagnostic location markers ({|#0:sb|}) — correctly placed on the parameter name
• Expected diagnostic arguments — ("System.Text.StringBuilder", "sb") matches the fully-qualified type name pattern used elsewhere in the file

✅ Test Coverage — Good scenario selection

Test 1 (StringBuilderNotSupported_ReportsDiagnostic) covers three variations:

• StringMarshalling.Utf16
• StringMarshalling.Utf8
• No StringMarshalling (empty string)

Test 2 (StringBuilderNotSupported_WithStringParam_ReportsDiagnostic) covers the specific bug repro from #126687 — StringBuilder with [Out] alongside a string parameter. Correctly omits the "no StringMarshalling" case, since without StringMarshalling, string would also produce a diagnostic (ParameterTypeNotSupportedWithDetails), complicating the assertion.

The second test also correctly expects only one diagnostic (for StringBuilder) — when StringMarshalling is specified, string is a supported marshalled type and should not produce a diagnostic.

✅ No Public API Changes

No changes to ref/ assembly source files or new public members. API approval verification is not required.

💡 Suggestion — Consider testing StringBuilder as return type

The file has parallel test methods for parameter types and return types (e.g., ParameterTypeNotSupported_ReportsDiagnostic / ReturnTypeNotSupported_ReportsDiagnostic). If StringBuilder as a return type with StringMarshalling is also relevant to #126687 or could be a future gap, a ReturnTypeNotSupported variant for StringBuilder could be added in a follow-up.

Generated by Code Review for issue #126691 · ◷

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