CA1419 is firing for cases that aren't related to P/Invokes

[stephentoub]

Analyzer

Diagnostic ID: CA1419: Provide a public parameterless constructor for concrete types derived from 'System.Runtime.InteropServices.SafeHandle

Analyzer source

SDK: Built-in CA analyzers in .NET 6 SDK or later

Describe the bug

The purpose of CA1419 is to help make SafeHandles friendly for a source generator, which requires access to the ctor to avoid using reflection as the runtime would. If there's no ctor the runtime could use for marshaling, the rule shouldn't fire. It should only fire in cases where there's a ctor that the runtime could have used but where that ctor won't be visible for a source generator.

Steps To Reproduce

Example:
https://github.com/dotnet/runtime/blob/004feb873271a7e5d9862b4b0dd1edb8e1db9543/src/libraries/System.Private.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/DataView.cs#L12

Expected behavior

No warning

Actual behavior

CA1419 fires.

cc: @jkoritzinsky

[jkoritzinsky]

I disagree with limiting this to scenarios where a SafeHandle would have an inaccessible constructor instead of no constructor because a lack of a constructor doesn't stop a SafeHandle-derived type from being used in interop scenarios. The parameterless constructor is only required in return-value or by-ref scenarios. As a result, types derived from SafeHandle can be safely used in P/Invokes today when passed as by value parameters even when they don't have parameterless constructors.

Additionally, the source generator can function with a SafeHandle-type having a private parameterless constructor. It uses the Activator.CreateInstance<T>(nonPublic:true) path to call the private parameterless constructor if needed, which if I remember correctly technically doesn't use reflection after the initial lookup. The purpose of the CA1419 analyzer is to push developers towards our best practices for SafeHandles that provide the best performance in source-generated scenarios, especially since this guidance changed recently (for source-generated interop as you mentioned).

We can't also limit the analyzer to SafeHandles used in marshalling scenarios. That doesn't help for the scenario where a SafeHandle-derived type is public or when it is internal and its assembly has InternalsVisibleTo attributes. For the cases where it's exposed outside of the declaring assembly, we'd have to fire the diagnostic to ensure that the type would have the public parameterless constructor in case a consuming assembly uses it in an P/Invoke scenario.

Additionally, this won't work when we move to source-generated P/Invokes and future source-generated interop scenarios.

Theoretically, we could hard-code in "is this type used in a method signature of a P/Invoke", but that wouldn't handle source-generated P/Invoke stubs, which are technically not P/Invokes. We could hard-code checking for GeneratedDllImportAttribute-attributed methods, but then we'd have to update the analyzer to understand each interop source generator trigger mechanism.

Additionally, we've built our interop source-generation model to be sharable between different interop source generators, some of which might not be owned by Microsoft. We've specifically designed the "marshaller generator" mechanism to enable developers to reuse our marshalling logic for various types, SafeHandles included. As a result, we could never have an exhaustive list of "marshalling scenarios" to opt into the checking.

[stephentoub]

because a lack of a constructor doesn't stop a SafeHandle-derived type from being used in interop scenarios. The parameterless constructor is only required in return-value or by-ref scenarios. As a result, types derived from SafeHandle can be safely used in P/Invokes today when passed as by value parameters even when they don't have parameterless constructors.

I don't understand how this is relevant to this issue. If a parameterless constructor isn't needed, then we shouldn't be warning about one being needed.

The purpose of the CA1419 analyzer is to push developers towards our best practices for SafeHandles that provide the best performance in source-generated scenarios

Exactly. The only reason we created this was to avoid the reflection when using source generators. If it wasn't for that, we wouldn't have changed any of the SafeHandle ctor visibility in the core libraries, we wouldn't have updated the SafeHandle best practices guidance, and we wouldn't have added this rule. We should update the guidance to clarify that such a public SafeHandle ctor is only required if the SafeHandle can/should be used with P/Invokes or you'd want one for other reasons.

I don't understand the pushback. I had to suppress this rule in dozens of places in dotnet/runtime because of false positives. That's a bad experience.

[jkoritzinsky]

I don't understand how this is relevant to this issue. If a parameterless constructor isn't needed, then we shouldn't be warning about one being needed.

The problem I have is that we don't have a good way to detect if one is needed. We can't use "if it isn't there today then it isn't needed" because that doesn't help new developers to create SafeHandle-derived types that follow the best practices guidance. That also doesn't help a scenario where a SafeHandle-derived type is initially used in a scenario where the constructor is not required and later is used in a scenario where it is required. Even before the guidance of a public parameterless constructor, the guidance to add a parameterless constructor that was a bit of a hidden land mine since there was no analyzer to help developers add one if they forgot. We can't use "if this is used in a P/Invoke then it is needed" because we can't determine every scenario where the constructor might be needed for source-generated interop.

I don't understand the pushback. I had to suppress this rule in dozens of places in dotnet/runtime because of false positives. That's a bad experience.

My pushback is around the fact that I don't see a good way to limit how often the analyzer triggers without effectively making it useless for developers outside of dotnet/runtime. Additionally, the large majority of SafeHandle-derived types outside of dotnet/runtime are used in P/Invoke or COM scenarios, the exact scenarios where this analyzer is useful. I view dotnet/runtime (specifically all of the JavaScript support SafeHandles) as an anomaly.

If we can figure out a good way to identify "SafeHandle is used in interop" without making the analyzer useless for developers who write new SafeHandle-derived types, I'd be 100% on board with adding that check. If we can't come up with a good check for that, then I'd also be okay with lowering the severity of this analyzer due to the false positives.

[stephentoub]

because that doesn't help new developers to create SafeHandle-derived types that follow the best practices guidance

That's not what this analyzer is for. If you need a SafeHandle for use with P/Invokes that need to instantiate one, and you don't have an appropriate constructor, you'll find out right quick when you crash with a very explicit exception that says exactly what the problem is. The purpose of this analyzer was to address the case of you having a working constructor but that required reflection to access rather than being directly callable.

[jkoritzinsky]

When we proposed this analyzer, I viewed that part of its job was to move the SafeHandle requirements for P/Invokes to compile time instead of at runtime to make it easier to diagnose failures in interop code and make writing interop code more understandable, though I'm not sure how the rest of the interop team viewed it. Since the source generated interop supports back-compat with non-accessible constructors and ref assemblies have limited information, the generated interop can't tell the difference between a private constructor that's not present in a ref assembly (so at runtime the reflection path will work) vs a nonexistent constructor (where the source-generated interop will fail at runtime).

For example, one of the issues that was the impetus for the analyzer had a scenario very similar to this, where we shipped a SafeHandle-derived type that didn't have a parameterless constructor that was meant for interop usage and users hit crashes they couldn't fix (though this case was due to the linker stripping it out in the library-linking mode since it was unused, not since it was forgotten in source): dotnet/runtime#45633

@AaronRobinsonMSFT @elinor-fung what did the two of you view as the goal behind the SafeHandle constructor analyzer?

If we agree that the goal of the analyzer was to help developers update their already-existing SafeHandle-derived types to follow the new guidance and not to help developers write new SafeHandle-derived types from scratch, then here's my proposal for what we should do:

1. Split the current analyzer to report a different diagnostic ID for when a SafeHandle type doesn't have a parameterless constructor.
2. Make the new diagnostic off-by-default because it is too noisy (though this loses most of the gain of having this analyzer since new developers won't know how to turn it on)
3. Downgrade the current diagnostic about making the current parameterless constructor public to a note instead of a warning since it is no longer a correctness issue but a performance one (maybe recategorize in the Performance category).
4. Keep the code fixes for the two different diagnostics.

[jkoritzinsky]

After discussing this issue offline, here's what we've decided to do:

1. Make the CA1419 only trigger when a parameterless constructor already exists.
2. Have the marshalling infrastructure in source-generated interop emit a diagnostic when a parameterless constructor is not present.

[mavasani]

Fixed in release/6.0.1xx