Warn CA5392 on LibraryImport #6493
Conversation
|
cc @elinor-fung @AaronRobinsonMSFT I can't add reviewers. |
...alyzers/Core/Microsoft.NetCore.Analyzers/Security/UseDefaultDllImportSearchPathsAttribute.cs
Outdated
Show resolved
Hide resolved
...alyzers/Core/Microsoft.NetCore.Analyzers/Security/UseDefaultDllImportSearchPathsAttribute.cs
Outdated
Show resolved
Hide resolved
...t.NetCore.Analyzers/Security/UseDefaultDllImportSearchPathsAttributeForLibraryImportTests.cs
Outdated
Show resolved
Hide resolved
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6493 +/- ##
==========================================
+ Coverage 96.40% 96.41% +0.01%
==========================================
Files 1371 1372 +1
Lines 318980 319657 +677
Branches 10265 10273 +8
==========================================
+ Hits 307524 308212 +688
+ Misses 8999 8987 -12
- Partials 2457 2458 +1 |
jtschuster
requested review from
AaronRobinsonMSFT and
elinor-fung
and removed request for
AaronRobinsonMSFT and
elinor-fung
| @@ -79,21 +90,25 @@ public override void Initialize(AnalysisContext context) | |||
| defaultValue: UnsafeBits); | |||
| var defaultDllImportSearchPathsAttributeOnAssembly = compilation.Assembly.GetAttributes().FirstOrDefault(o => o.AttributeClass.Equals(defaultDllImportSearchPathsAttributeTypeSymbol)); | |||
|
|
|||
| // Does not analyze local functions. To analyze local functions, we'll need to use RegisterSyntaxAction. | |||
There was a problem hiding this comment.
I'm okay with this decision. We should update the doc to mention that local functions will not be analyzed if others are okay with this.
There was a problem hiding this comment.
Just to be clear, you mean this doc, right? https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca5392
There was a problem hiding this comment.
Probably both that and https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca5393
|
|
||
| if (!symbol.IsExtern || !symbol.IsStatic) | ||
| // LibraryImport methods will be partial, and the generated implementation will have a non-null PartialDefinitionPart property | ||
| if (!symbol.IsStatic || symbol.PartialDefinitionPart != null) |
There was a problem hiding this comment.
Functionally, this looks fine, but I think we should still check for the method being either IsExtern or IsPartialDefinition for performance. Otherwise, we're doing the more costly analysis on every static method that isn't a partial implementation.
|
@dotnet/roslyn-analysis Can you take a look at this? |
|
@dotnet/roslyn-analysis Can you take a look and approve? |
related issue: dotnet/runtime#81939 related pr: dotnet/roslyn-analyzers#6493
Addresses part of dotnet/runtime#81939
Using DllImport without DllImportSearchPaths warns with CA5392. With generated LibraryImports that need to marshal to and from a local P/Invoke method, there is no warning because there is no DllImport on the partial method. This change warns on LibraryImports as well as DllImports, and only the Definition part of a partial method with both attributes are present.
The second part of the fix for this issue is to enforce the warning on local methods, and propagate any suppressions on the definition to the generated local P/Invoke. This would require looking at syntax nodes instead of symbols and would add a lot more complexity. If we do not warn on local methods, we wouldn't need to propagate suppression attributes to the generated local function either. I think there will be very few cases where someone would hand write a local method P/Invoke and want this warning to appear, so I'm not sure it's worth the effort of changing the analysis to use Syntax instead of Symbols, but let me know your thoughts.