Skip to content

Fix inconsistency bug, child cannot widen as support, can narrow #4208

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.

Sign up for GitHub

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

Merged
buyaa-n merged 2 commits into from
Sep 24, 2020
Merged

Fix inconsistency bug, child cannot widen as support, can narrow #4208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
829de17
Fix inconsistency, cannot widen, can narrow
buyaa-n Sep 22, 2020
4ddca25
Address feedbacks
buyaa-n Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 92 additions & 97 deletions ...alyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,8 +763,7 @@ private static bool IsNotSuppressedByCallSite(SmallDictionary<string, PlatformAt
out SmallDictionary<string, PlatformAttributes> notSuppressedAttributes)
{
notSuppressedAttributes = new SmallDictionary<string, PlatformAttributes>(StringComparer.OrdinalIgnoreCase);
bool? supportedOnlyList = null;
bool mandatoryMatchFound = false;
bool? mandatorySupportFound = null;
using var supportedOnlyPlatforms = PooledHashSet<string>.GetInstance(StringComparer.OrdinalIgnoreCase);
foreach (var (platformName, attribute) in operationAttributes)
{
Expand All @@ -775,20 +774,15 @@ private static bool IsNotSuppressedByCallSite(SmallDictionary<string, PlatformAt
if (attribute.UnsupportedFirst == null || attribute.UnsupportedFirst > attribute.SupportedFirst)
{
// If only supported for current platform
if (supportedOnlyList.HasValue && !supportedOnlyList.Value)
{
return true; // invalid state, do not need to add this API to the list
}

supportedOnlyPlatforms.Add(platformName);
supportedOnlyList = true;
mandatorySupportFound ??= false;

if (callSiteAttributes.TryGetValue(platformName, out var callSiteAttribute))
{
var attributeToCheck = attribute.SupportedSecond ?? attribute.SupportedFirst;
if (MandatoryOsVersionsSuppressed(callSiteAttribute, attributeToCheck))
{
mandatoryMatchFound = true;
mandatorySupportFound = true;
}
else
{
Expand All @@ -803,15 +797,8 @@ private static bool IsNotSuppressedByCallSite(SmallDictionary<string, PlatformAt
}
}
}
else if (attribute.UnsupportedFirst != null) // also means Unsupported <= Supported, optional list
else if (attribute.UnsupportedFirst != null) // also means Unsupported < Supported, allow list
{
if (supportedOnlyList.HasValue && supportedOnlyList.Value)
{
return true; // do not need to add this API to the list
}

supportedOnlyList = false;

if (callSiteAttributes.TryGetValue(platformName, out var callSiteAttribute))
{
if (callSiteAttribute.SupportedFirst != null)
Expand Down Expand Up @@ -847,13 +834,6 @@ private static bool IsNotSuppressedByCallSite(SmallDictionary<string, PlatformAt
}
else
{
if (supportedOnlyList.HasValue && supportedOnlyList.Value)
{
return true; // do not need to add this API to the list
}

supportedOnlyList = false;

if (attribute.UnsupportedFirst != null) // Unsupported for this but supported all other
{
if (callSiteAttributes.TryGetValue(platformName, out var callSiteAttribute))
Expand Down Expand Up @@ -887,9 +867,9 @@ private static bool IsNotSuppressedByCallSite(SmallDictionary<string, PlatformAt
}
}

if (supportedOnlyList.HasValue && supportedOnlyList.Value)
if (mandatorySupportFound.HasValue)
{
if (!mandatoryMatchFound)
if (!mandatorySupportFound.Value)
{
foreach (var (name, attributes) in operationAttributes)
{
Expand Down Expand Up @@ -989,6 +969,12 @@ private static bool TryGetOrCreatePlatformAttributes(
{
var container = symbol.ContainingSymbol;

if (symbol is IMethodSymbol method && method.IsAccessorMethod())
{
// Add attributes for the associated Property
container = method.AssociatedSymbol;
}

// Namespaces do not have attributes
while (container is INamespaceSymbol)
{
Expand All @@ -1001,30 +987,90 @@ private static bool TryGetOrCreatePlatformAttributes(
attributes = CopyAttributes(containerAttributes);
}

AddPlatformAttributes(symbol.GetAttributes(), ref attributes);

if (symbol is IMethodSymbol method && method.IsAccessorMethod())
{
// Add attributes for the associated Property
AddPlatformAttributes(method.AssociatedSymbol.GetAttributes(), ref attributes);
}
MergePlatformAttributes(symbol.GetAttributes(), ref attributes);

attributes = platformSpecificMembers.GetOrAdd(symbol, attributes);
}

return attributes != null;

static bool AddPlatformAttributes(ImmutableArray<AttributeData> immediateAttributes, [NotNullWhen(true)] ref SmallDictionary<string, PlatformAttributes>? attributes)
static void MergePlatformAttributes(ImmutableArray<AttributeData> immediateAttributes, ref SmallDictionary<string, PlatformAttributes>? parentAttributes)
{
bool added = false;
SmallDictionary<string, PlatformAttributes>? childAttributes = null;
foreach (AttributeData attribute in immediateAttributes)
{
if (s_osPlatformAttributes.Contains(attribute.AttributeClass.Name))
{
added |= TryAddValidAttribute(ref attributes, attribute);
TryAddValidAttribute(ref childAttributes, attribute);
}
}

if (childAttributes == null)
{
return;
}

if (parentAttributes != null && parentAttributes.Any())
{
foreach (var (platform, attributes) in parentAttributes)
{
if (DenyList(attributes) &&
!parentAttributes.Any(ca => AllowList(ca.Value)))
{
// 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))
{
// but don't override existing unless narrowing the support
if (childAttribute.UnsupportedFirst != null &&
childAttribute.UnsupportedFirst < attributes.UnsupportedFirst)
{
attributes.UnsupportedFirst = childAttribute.UnsupportedFirst;
}
}
else
{
parentAttributes[name] = childAttribute;
}
}
// merged all attributes, no need to continue looping
return;
}
else if (AllowList(attributes))
{
// only attributes with same platform matter, could narrow the list
if (childAttributes.TryGetValue(platform, out var childAttribute))
{
// only later versions could narrow, other versions ignored
if (childAttribute.SupportedFirst > attributes.SupportedFirst)
{
attributes.SupportedSecond = childAttribute.SupportedFirst;
}

if (childAttribute.UnsupportedFirst != null)
{
if (childAttribute.UnsupportedFirst <= attributes.SupportedFirst)
{
attributes.SupportedFirst = null;
attributes.SupportedSecond = null;
}
else if (childAttribute.UnsupportedFirst <= attributes.SupportedSecond)
{
attributes.SupportedSecond = null;
}

attributes.UnsupportedFirst = childAttribute.UnsupportedFirst;
}
}
// other platform attributes are ignored as the list couldn't be extended
}
}
}
return added;
else
{
parentAttributes = childAttributes;
}
}
}

Expand All @@ -1039,13 +1085,12 @@ attribute.ConstructorArguments[0] is { } argument &&
TryParsePlatformNameAndVersion(argument.Value.ToString(), out string platformName, out Version? version))
{
attributes ??= new SmallDictionary<string, PlatformAttributes>(StringComparer.OrdinalIgnoreCase);

if (!attributes.TryGetValue(platformName, out var _))
{
attributes[platformName] = new PlatformAttributes();
}

AddAttribute(attribute.AttributeClass.Name, version, attributes, platformName);
AddAttribute(attribute.AttributeClass.Name, version, attributes[platformName]);
return true;
}

Expand Down Expand Up @@ -1076,20 +1121,19 @@ private static bool TryParsePlatformNameAndVersion(string osString, out string o
return true;
}

private static void AddAttribute(string name, Version version, SmallDictionary<string, PlatformAttributes> existingAttributes, string platformName)
private static void AddAttribute(string name, Version version, PlatformAttributes attributes)
{
if (name == SupportedOSPlatformAttribute)
{
AddOrUpdateSupportedAttribute(existingAttributes[platformName], version);
AddOrUpdateSupportedAttribute(attributes, version);
}
else
{
Debug.Assert(name == UnsupportedOSPlatformAttribute);
AddOrUpdateUnsupportedAttribute(platformName, existingAttributes[platformName], version, existingAttributes);
AddOrUpdateUnsupportedAttribute(attributes, version);
}

static void AddOrUpdateUnsupportedAttribute(string name, PlatformAttributes attributes,
Version version, SmallDictionary<string, PlatformAttributes> existingAttributes)
static void AddOrUpdateUnsupportedAttribute(PlatformAttributes attributes, Version version)
{
if (attributes.UnsupportedFirst != null)
{
Expand Down Expand Up @@ -1131,32 +1175,8 @@ static void AddOrUpdateUnsupportedAttribute(string name, PlatformAttributes attr
}
else
{
if (attributes.SupportedFirst != null && attributes.SupportedFirst >= version)
{
// Override needed
if (attributes.SupportedSecond != null)
{
attributes.SupportedFirst = attributes.SupportedSecond;
attributes.SupportedSecond = null;
}
else
{
attributes.SupportedFirst = null;
}
if (!HasAnySupportedOnlyAttribute(name, existingAttributes))
{
attributes.UnsupportedFirst = version;
}
}
else
{
attributes.UnsupportedFirst = version;
}
attributes.UnsupportedFirst = version;
}

static bool HasAnySupportedOnlyAttribute(string name, SmallDictionary<string, PlatformAttributes> existingAttributes) =>
existingAttributes.Any(a => !a.Key.Equals(name, StringComparison.OrdinalIgnoreCase) &&
AllowList(a.Value));
}

static void AddOrUpdateSupportedAttribute(PlatformAttributes attributes, Version version)
Expand All @@ -1165,34 +1185,9 @@ static void AddOrUpdateSupportedAttribute(PlatformAttributes attributes, Version
{
if (attributes.SupportedFirst > version)
{
if (attributes.SupportedSecond != null)
{
if (attributes.SupportedSecond < attributes.SupportedFirst)
{
attributes.SupportedSecond = attributes.SupportedFirst;
}
}
else
{
attributes.SupportedSecond = attributes.SupportedFirst;
}

attributes.SupportedFirst = version;
}
else
{
if (attributes.SupportedSecond != null)
{
if (attributes.SupportedSecond < version)
{
attributes.SupportedSecond = version;
}
}
else
{
attributes.SupportedSecond = version;
}
}
// only keep lowest version, ignore other versions
}
else
{
Expand All @@ -1208,7 +1203,7 @@ static void AddOrUpdateSupportedAttribute(PlatformAttributes attributes, Version
/// <returns>true if it is allow list</returns>
private static bool AllowList(PlatformAttributes attributes) =>
attributes.SupportedFirst != null &&
(attributes.UnsupportedFirst == null || attributes.SupportedFirst < attributes.UnsupportedFirst);
(attributes.UnsupportedFirst == null || attributes.SupportedFirst <= attributes.UnsupportedFirst);

/// <summary>
/// Determines if the attributes unsupported only for the platform (deny list)
Expand All @@ -1217,6 +1212,6 @@ private static bool AllowList(PlatformAttributes attributes) =>
/// <returns>true if it is deny list</returns>
private static bool DenyList(PlatformAttributes attributes) =>
attributes.UnsupportedFirst != null &&
(attributes.SupportedFirst == null || attributes.UnsupportedFirst <= attributes.SupportedFirst);
(attributes.SupportedFirst == null || attributes.UnsupportedFirst < attributes.SupportedFirst);
}
}
10 changes: 4 additions & 6 deletions ...NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.GuardedCallsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ public static void UnsupportedCombinations()
unsupportedOnWindows.TypeUnsupportedOnWindows_FunctionSupportedOnWindows11UnsupportedOnWindows12SupportedOnWindows13();

var unsupportedOnBrowser = [|new TypeUnsupportedOnBrowser()|];
unsupportedOnBrowser.TypeUnsupportedOnBrowser_FunctionSupportedOnBrowser();
[|unsupportedOnBrowser.TypeUnsupportedOnBrowser_FunctionSupportedOnBrowser()|]; // warn for unsupported browser type

var unsupportedOnWindowsSupportedOnWindows11 = new TypeUnsupportedOnWindowsSupportedOnWindows11();
unsupportedOnWindowsSupportedOnWindows11.TypeUnsupportedOnWindowsSupportedOnWindows11_FunctionUnsupportedOnWindows12();
Expand All @@ -260,7 +260,7 @@ public static void UnsupportedCombinations()
[|withoutAttributes.TypeWithoutAttributes_FunctionUnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12SupportedOnWindows13()|];

var unsupportedOnWindows = [|new TypeUnsupportedOnWindows()|];
[|unsupportedOnWindows.TypeUnsupportedOnWindows_FunctionSupportedOnWindows11()|];
[|unsupportedOnWindows.TypeUnsupportedOnWindows_FunctionSupportedOnWindows11()|]; // should only warn for unsupported type, function attribute ignored

var unsupportedOnBrowser = new TypeUnsupportedOnBrowser();
unsupportedOnBrowser.TypeUnsupportedOnBrowser_FunctionSupportedOnBrowser();
Expand All @@ -278,7 +278,6 @@ await VerifyAnalyzerAsyncCs(source, s_msBuildPlatforms,
VerifyCS.Diagnostic(PlatformCompatibilityAnalyzer.SupportedOsVersionRule).WithLocation(37, 17).WithMessage("'TypeWithoutAttributes.TypeWithoutAttributes_FunctionUnsupportedOnWindowsSupportedOnWindows11()' is unsupported on 'windows'"),
VerifyCS.Diagnostic(PlatformCompatibilityAnalyzer.SupportedOsVersionRule).WithLocation(38, 17).WithMessage("'TypeWithoutAttributes.TypeWithoutAttributes_FunctionUnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12()' is unsupported on 'windows'"),
VerifyCS.Diagnostic(PlatformCompatibilityAnalyzer.SupportedOsVersionRule).WithLocation(39, 17).WithMessage("'TypeWithoutAttributes.TypeWithoutAttributes_FunctionUnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12SupportedOnWindows13()' is unsupported on 'windows'"),
VerifyCS.Diagnostic(PlatformCompatibilityAnalyzer.SupportedOsVersionRule).WithLocation(42, 17).WithMessage("'TypeUnsupportedOnWindows.TypeUnsupportedOnWindows_FunctionSupportedOnWindows11()' is unsupported on 'windows'"),
VerifyCS.Diagnostic(PlatformCompatibilityAnalyzer.SupportedOsVersionRule).WithLocation(47, 64).WithMessage("'TypeUnsupportedOnWindowsSupportedOnWindows11' is unsupported on 'windows'"),
VerifyCS.Diagnostic(PlatformCompatibilityAnalyzer.SupportedOsVersionRule).WithLocation(48, 17).WithMessage("'TypeUnsupportedOnWindowsSupportedOnWindows11.TypeUnsupportedOnWindowsSupportedOnWindows11_FunctionUnsupportedOnWindows12SupportedOnWindows13()' is unsupported on 'windows'"),
VerifyCS.Diagnostic(PlatformCompatibilityAnalyzer.SupportedOsVersionRule).WithLocation(50, 86).WithMessage("'TypeUnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12' is unsupported on 'windows'"));
Expand Down Expand Up @@ -905,8 +904,7 @@ public async Task GuardedCalled_SimpleIfElse_VersionNotMatch_Warns()
using System.Runtime.Versioning;
using System;

[assembly:SupportedOSPlatform(""windows7.0"")]

[SupportedOSPlatform(""windows7.0"")]
static class Program
{
public static void Main()
Expand Down Expand Up @@ -938,7 +936,7 @@ public async Task ReintroducingApiSupport_Guarded_NotWarn()
using System;
using System.Runtime.Versioning;

[assembly:SupportedOSPlatform(""windows"")]
[SupportedOSPlatform(""windows"")]
static class Program
{
public static void Main()
Expand Down
Loading