Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Analyzer for MarshalAs usage #34831

Closed
AaronRobinsonMSFT opened this issue Apr 10, 2020 · 6 comments
Closed

Analyzer for MarshalAs usage #34831

AaronRobinsonMSFT opened this issue Apr 10, 2020 · 6 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Interop-coreclr code-analyzer Marks an issue that suggests a Roslyn analyzer
Milestone

Comments

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Apr 10, 2020

Defining how types should be marshaled via a P/Invoke or COM interop is difficult to get correct and many bad practices have been developed over the years. The Native interoperability best practices page should be converted into an analyzer to help developers do the right thing.

Related to #33808

Additional thoughts:

/cc @terrajobst @jkoritzinsky @elinor-fung

@AaronRobinsonMSFT AaronRobinsonMSFT added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Interop-coreclr code-analyzer Marks an issue that suggests a Roslyn analyzer labels Apr 10, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Apr 10, 2020
@danmoseley
Copy link
Member

There may be more in
https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/interop-guidelines.md
https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/interop-pinvokes.md (which I'll merge into the former)

Of course, anything unique in those would be candidates for adding to the official doc as well.

@elinor-fung
Copy link
Member

There is an existing P/Invoke analyzer, which could be a fitting home for some of this.

Some marshalling-related items that could be reasonable rules from the guidance doc:

  • Specify CharSet on DllImport when strings/characters are present
    • CA2101 seems to partly cover this (does string and StringBuilder, not char), although it is under Globalization(?) and the doc talks about security / partial trust
  • Prefer ExactSpelling=true on DllImport (Performance)
  • Do not use [Out] string (Interoperability)
  • Avoid StringBuilder parameters (Performance)
  • Specify marshalling for booleans (Interoperability)
    • CA1414 exists, but is disabled for Core (also only for p/invokes, so won't hit fields on types used in p/invokes)
  • Specify SizeConst when marshalling field as ByValArray (Interoperability)
  • Do not use Delegate or MulticastDelegate fields in marshalled structs (Interoperability).

@AaronRobinsonMSFT
Copy link
Member Author

@elinor-fung Here is another issue that should get a rule: #9944

@JeremyKuhne
Copy link
Member

Some initial thoughts:

  • Having both [ComVisible] and [ComImport] on an interface should be incorrect, right?
  • For COM interfaces we should consider always requiring [MarshalAs] for string and object as they are often not BSTR and VARIANT

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label May 11, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 5.0 milestone May 11, 2020
@AaronRobinsonMSFT
Copy link
Member Author

@elinor-fung Is this aggregated issue being handled by the various other issues being filed or should we keep this one open?

@elinor-fung
Copy link
Member

Closing this and using #37039 as the tracking issue that will point to the separate issues we file for each rule.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Interop-coreclr code-analyzer Marks an issue that suggests a Roslyn analyzer
Projects
None yet
Development

No branches or pull requests

5 participants