Roslyn Code analyzers for interop · Issue #37039 · dotnet/runtime (original) (raw)

This tracking issue for a small backlog of rules around interop to add to the officially recommended set of analyzers. The rules are grouped around runtime functionality/features, but would represent separate analyzers in implementation. Since many issues concerning interop are rooted in a discrepancy between the user's intent and implementation, it is expected that only a small number of rules would be useful and applicable to all users. Official documentation will remain the main source of guidance for more nuanced behaviour that requires clear understanding of intent.

The upcoming plan to provide source generators for p/invokes should not directly affect the rules here, as that functionality would be using a different attribute for declaring p/invokes. However, there is the opportunity to provide an analyzer to help users migrate to the source generator approach. Once a source generator exists, the rules here could also be updated to consider p/invokes which use that generator or, depending on their severity, warnings within the generator itself.

The intent is that as the runtime interop team works through a backlog of analyzers around existing features and functionality, the concept of considering and adding analyzers for any new features will become natural and simply be part of that feature work. The P/Invokes and Marshalling rules are expected to be the starting points for working through this backlog.

P/Invokes

These rules inspect the actual declaration and invocation of p/invokes.

• Do not use [Out] string for P/Invokes: Do not use [Out] string for P/Invokes #35692
• Avoid StringBuilder parameters for P/Invokes: Avoid StringBuilder parameters for P/Invokes #35693
• Prefer ExactSpelling=true in [DllImport] for known APIs: Prefer ExactSpelling=true on [DllImport] for known APIs #35695
• Remove redundant configuration from [DllImport] declaration Remove redundant configuration from [DllImport] declaration #33808
• CA1404 port: Port FxCop rule CA1404: CallGetLastErrorImmediatelyAfterPInvoke roslyn-analyzers#420
• Prefer SafeHandle over IntPtr for known APIs: Analyzer suggestion: Prefer SafeHandle over IntPtr #42404
• Get the last error after calls to P/Invokes with SetLastError=true
  • Category: Interoperability
  • Default: Enabled
  • After calling a p/invoke that sets the last error, Marshal.GetLastWin32Error should be called to retrieve the last error.
    [DllImport("MyLibrary", SetLastError = true)]
    private static extern void Foo();
    public static void Bar()
    {
    Foo(); // Flag last error not retrieved
    }
    public static void Baz()
    {
    Foo(); // OK
    if (Marshal.GetLastWin32Error() == 0) { ... }
    }
• Specify SetLastError=true in [DllImport] for known APIs
  • Category: Interoperability
  • Default: Enabled
  • For APIs that are known to set last error, SetLastError=true should be specified. This would require having/building a database of APIs to compare against.
    // Flag SetLastError=false for known API
    [DllImport("kernel32")]
    public static extern bool CloseHandle(IntPtr hObject);
    // OK - known API does not set last error
    [DllImport("kernel32")]
    public static extern int GetCurrentProcessId();
  • Recommend:
    [DllImport("kernel32.dll", SetLastError = true)]
    public static extern bool CloseHandle(IntPtr hObject);
  • If the user does not intend to check the error, this can be skipped, but the general guidance is to both define the p/invoke to set last error (for APIs that do so) and check the error.

Marshalling

These rules inspect how data is marshalled. They would apply to the parameters on p/invokes and any types (e.g. marshalled structs, delegates) used by p/invokes.

• Specify SizeConst when marshalling as ByValArray: Specify SizeConst when marshalling field as ByValArray #36134
• CA1414 port: Port FxCop rule CA1414: MarkBooleanPInvokeArgumentsWithMarshalAs roslyn-analyzers#430
• Avoid Delegate or MulticastDelegate fields in marshalled structs
  • Category: Interoperability
  • Default: Enabled
  • Delegate and MulticastDelegate do not have a required signature, so they do not guarantee that the delegate passed in will match the signature the native code expects. Marshalling a struct containing a Delegate or MulticastDelegate from its native representation to a managed object can destabilize the runtime if the value of the field in the native representation is not a function pointer that wraps a managed delegate. Use a specific delegate type instead of Delegate or MulticastDelegate.
    [DllImport("MyLibrary")]
    private static extern void Foo(MyStruct s);
    struct MyStruct
    {
    Delegate Bar; // Flag Delegate in marshalled struct
    }
• Rules for implementing ICustomMarshaler
  • GetInstance() requirement - Analyzer proposal: Missing GetInstance for ICustomMarshalers #46521

COM

These rules inspect COM-related functionality. Many of the existing rules that have not yet been ported are around COM.

• Types should not be both [ComImport] and [ComVisible(true)]
  • Types should not be marked as both imported from COM and made visible to COM.
• COM-visible types should be accessible and creatable
  • Types marked ComVisible(true) should be public, non-abstract, and have a public parameterless constructor.
  • Port FxCop rule CA1409: ComVisibleTypesShouldBeCreatable roslyn-analyzers#425
    * CA1409 looks to have the same intent, but only specifically flagged the public parameterless constructor
• Partial interface definitions with ComImport – Warning for partial ComImport types with members in more than one type part #59013.

Low-level interop APIs

These rules are related to APIs that provide low-level interaction/integration with the runtime's interop system.

• Interfaces marked with DynamicInterfaceCastableImplementationAttribute should provide a default implementation of all inherited interface methods: All inherited interface members should be implemented on an interface marked with DynamicInterfaceCastableImplementationAttribute #41529

Cross-platform

There is an existing proposal for an analyzer that detects the use of platform-specific APIs where the API might not be available. Many interop-related APIs are platform-specific and would be given the appropriate attribute such that the proposed analyzer would flag their use.

The logic that will be used by the proposed analyzer for platform-specific APIs could be leveraged for platform-specifc interop behaviour that is not tied to a specifid API. There are some types or MarshalAs values for which marshalling is not supported on all platforms. In these cases, it is not a known API that is platform-specific, but a user-defined p/invoke that is platform-specific due to the way it is defined. An analyzer would detect those p/invokes and treat those as platform-specific calls, following the same logic as that for the platform-specific APIs for determining the platform context and checking for platform guards around the call site.

Rules:

• Marshalling of <Type> requires <OS>
• Marshalling as <UnmanagedType.*> requires <OS>

Existing rules

There are a number of rules around interop and marshalling that go through legacy (static) analysis. They have all previously been deprecated or slated to be ported as time allows. See FxCop rule port status for a list of all ported, tracked, and deprecated rules.