Fix inconsistency bug, child cannot widen as support, can narrow #4208
Conversation
buyaa-n
requested review from
jeffhandley,
terrajobst,
jmarolf and
mavasani
Codecov Report
@@ Coverage Diff @@
## release/dotnet5-rc2 #4208 +/- ##
=======================================================
+ Coverage 95.77% 95.79% +0.01%
=======================================================
Files 1164 1164
Lines 263407 263723 +316
Branches 15901 15910 +9
=======================================================
+ Hits 252271 252626 +355
+ Misses 9137 9086 -51
- Partials 1999 2011 +12 |
|
Description sounds good to me. Code and tests look good, but I'll also appreciate @mavasani's review tomorrow. I'm going to do some local testing before I hit approve. |
|
My preliminary testing looks good. I specifically tested for the scenarios reported in #4168 as well as some other baseline tests, and they're behaving as expected. @buyaa-n, you mentioned that this change surfaced some new diagnostics in the |
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
|
|
||
| if (childAttributes != null) | ||
| { | ||
| if (parentAttributes != null && parentAttributes.Any()) |
There was a problem hiding this comment.
I am surprised that the rule doesn't raise but it'd be better to use Count instead of Any().
There was a problem hiding this comment.
I am actually getting diagnostic to use Any() over Count instead
| // if all are deny list then we can add the child attributes | ||
| foreach (var (name, childAttribute) in childAttributes) | ||
| { | ||
| if (parentAttributes.TryGetValue(name, out var existing)) |
There was a problem hiding this comment.
Revert condition and return to reduce nesting.
There was a problem hiding this comment.
It is not returning here nor in the revert
| attributes.SupportedSecond = childAttribute.SupportedFirst; | ||
| } | ||
|
|
||
| if (childAttribute.UnsupportedFirst != null) |
There was a problem hiding this comment.
Revert + return to reduce nesting.
There was a problem hiding this comment.
We cannot return on revert, need to continue to the next attribute for other platform
.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs
Show resolved
Hide resolved
.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs
Show resolved
Hide resolved
.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
Sure |
|
I find the error message a little weird: |
Not supported would say `'IOControlCode.KeepAliveValues' is unsupported on 'windows'. the above means the API si only supported in windows. We agree that the messaging is not perfect and we have an issue to improve it in the future #4021 |
Fixes #4168
Besides fixing the inconsistency issue this PR is updating out attributes handling logic:
Before: We were collecting all attributes for the invoked member and its containing types, after that were determining attributes inconsistency along with suppression from call site attributes when incosistency occurs bailout to avoid false positives. Also in case lowest version of supported and unsupported attributes are equal that counted as Deny list
Now: We are checking attributes consistency for each level, starting from the Parent, for merging each child member's attributes follow the rule suggested by @terrajobst:
child annotations can narrow support, but they cannot widen it, attributes not respecting the rule are ignored. Also in case, the lowest version of supported and unsupported attributes are equal that counted as Allow list.