-
Notifications
You must be signed in to change notification settings - Fork 0
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
Platform compatibility analyzer draft #3
Conversation
…nto plat_spec_an
overload of guard method
.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzerTests.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzerTests.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
[Fact] | ||
public async Task WrongPlatformStringsShouldHandledGracefully() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand this test: Why do invalid strings still trigger the rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, the purpose of this analyzer is not reporting the wrong string values, the purpose of this test is to check if the analyzer would crush or throw any unhandled exceptions with any different/wrong OS platform string values
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
{ | ||
var typeName = WellKnownTypeNames.SystemRuntimeInteropServicesRuntimeInformation; | ||
|
||
if (!context.Compilation.TryGetOrCreateTypeByMetadataName(typeName + "Helper", out var runtimeInformationType)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we are able to consume the new APIs this mock class load will be removed
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
return; | ||
} | ||
|
||
if (guardMethods.IsEmpty || !(context.OperationBlocks.GetControlFlowGraph() is { } cfg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guardMethods are new API so if it does not exist in the current compilation no need to do flow analysis, just report diagnostic for all methods in platformSpecificOperations dictionary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guardMethods are new API so if it does not exist in the current compilation
Right, but we could have bailed much earlier, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to warn on builds not having those methods?
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
I've pulled down your PR and played with the analyzer. My feedback is below. I can file them as individual bugs as well. Constructors aren't warnedRepro: class Program
{
static void Main(string[] args)
{
var x = new WindowsOnlyAPI();
Console.WriteLine("Hello World!");
}
}
[MinimumOSPlatform("win7.0")]
class WindowsOnlyAPI
{
} Actual: No diagnostic Expected:
Analyzer should also handle IsOSPlatformWhile Repro: class Program
{
static void Main(string[] args)
{
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
return;
WindowsOnlyAPI.Test();
}
}
[MinimumOSPlatform("win7.0")]
static class WindowsOnlyAPI
{
public static void Test()
{
}
} Actual:
Expected: No diagnostic Incorrect version numbers are ignoredRepro: class Program
{
static void Main(string[] args)
{
if (!RuntimeInformation.IsOSPlatformOrLater("win7"))
return;
WindowsOnlyApi();
}
[MinimumOSPlatform("win7.0")]
static void WindowsOnlyApi()
{
}
} Actual:
Expected: Either infer that "win7" is "win7.0" or warn that "win7" isn't valid. Logical or isn't handled properlyRepro: class Program
{
static void Main(string[] args)
{
if (RuntimeInformation.IsOSPlatformOrLater("win7.0") ||
RuntimeInformation.IsOSPlatformOrLater("win7.1"))
{
WindowsOnlyApi();
}
}
[MinimumOSPlatform("win7.0")]
static void WindowsOnlyApi()
{
}
} Actual: CA1416: 'WindowsOnlyApi' requires win 7.0 version or later. Expected: No diagnostic Feature: Handle OSVersion for WindowsWe should consider special casing Windows (and maybe Unix) to also allow platform checks via the old Repro: class Program
{
static void Main(string[] args)
{
if (Environment.OSVersion.Platform == PlatformID.Win32NT &&
Environment.OSVersion.Version >= new Version(7, 0))
{
WindowsOnlyApi();
}
}
[MinimumOSPlatform("win7.0")]
static void WindowsOnlyApi()
{
}
} Actual:
Expected: No diagnostic Local functions aren't processedRepro: class Program
{
static void Main(string[] args)
{
void Test()
{
WindowsOnlyApi();
}
Test();
}
[MinimumOSPlatform("win7.0")]
static void WindowsOnlyApi()
{
}
} Actual: No diagnostics Expected:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove the TFM logic. The rest is just minor suggestions.
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
return; | ||
} | ||
|
||
if (guardMethods.IsEmpty || !(context.OperationBlocks.GetControlFlowGraph() is { } cfg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guardMethods are new API so if it does not exist in the current compilation
Right, but we could have bailed much earlier, no?
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
Those are really interesting/good scenarios, thanks! |
description: s_localizableObsoleteMessage, | ||
isPortedFxCopRule: false, | ||
isDataflowRule: false); | ||
internal static DiagnosticDescriptor RemovedRule = DiagnosticDescriptorHelper.Create(RuleId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@terrajobst @jeffhandley as i mentioned in the morning the Obsolete and Removed rules are work same as MinimumOS rule, i.e. if Obsolete attribute defined for an API then that API is restricted to only for that Platform. For now, i am gonna leave this implementation as is as we haven no any Obsolete/Removed attributes added into any API. The fix will come with the functionality update for the Unsupported attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads-up, @buyaa-n. For Preview 8, that sounds good to me. We'll then change the behavior in RC1 with the other changes.
Related design doc https://github.com/dotnet/designs/blob/master/accepted/2020/platform-checks/platform-checks.md
Related issue dotnet/runtime#37359 (It need to be updated with latest API review changes)