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.

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

Warn for platform specific type used as generic type parameter #4390

Closed
wants to merge 2 commits into from
Closed

Warn for platform specific type used as generic type parameter #4390

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5fab58
Warn for platform specific type used as generic type parameter
buyaa-n Oct 29, 2020
b86c1c1
Address feedback. add more scenario and test
buyaa-n Oct 31, 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
47 changes: 44 additions & 3 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 @@ -535,7 +535,7 @@ private static void ReportDiagnosticsForAll(PooledConcurrentDictionary<IOperatio
private static void ReportDiagnostics(IOperation operation, SmallDictionary<string, PlatformAttributes> attributes,
OperationBlockAnalysisContext context, ConcurrentDictionary<ISymbol, SmallDictionary<string, PlatformAttributes>?> platformSpecificMembers)
{
var symbol = operation is IObjectCreationOperation creation ? creation.Constructor.ContainingType : GetOperationSymbol(operation);
var symbol = GetOperationSymbol(operation);

if (symbol == null)
{
Expand All @@ -546,15 +546,39 @@ private static void ReportDiagnostics(IOperation operation, SmallDictionary<stri
{
symbol = GetAccessorMethod(platformSpecificMembers, symbol, GetPropertyAccessors(property, operation));
}

if (symbol is IEventSymbol iEvent)
else if (symbol is IEventSymbol iEvent)
{
var accessor = GetEventAccessor(iEvent, operation);
if (accessor != null)
{
symbol = accessor;
}
}
else if (symbol is IMethodSymbol method)
{
var typeArguments = method.TypeArguments;
if (!method.IsGenericMethod && method.ReceiverType is INamedTypeSymbol namedType && namedType.IsGenericType)
{
typeArguments = namedType.TypeArguments;
}

foreach (var typeArgument in typeArguments)
{
if (platformSpecificMembers.TryGetValue(typeArgument, out var foundAttributes) &&
foundAttributes != null &&
foundAttributes.Any())
{
symbol = typeArgument;
break;
}
}
}

if (operation is IObjectCreationOperation creation && symbol is not ITypeSymbol)
{
symbol = creation.Constructor.ContainingType;
}

var operationName = symbol.ToDisplayString(GetLanguageSpecificFormat(operation));

foreach (var platformName in attributes.Keys)
Expand Down Expand Up @@ -691,6 +715,23 @@ private static void AnalyzeOperation(IOperation operation, OperationAnalysisCont
else
{
CheckOperationAttributes(operation, context, platformSpecificOperations, platformSpecificMembers, msBuildPlatforms, symbol);
if (symbol is IMethodSymbol method)
{
if (method.IsGenericMethod)
{
foreach (var typeArgument in method.TypeArguments)
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
{
CheckOperationAttributes(operation, context, platformSpecificOperations, platformSpecificMembers, msBuildPlatforms, typeArgument);
}
}
else if (method.ReceiverType is INamedTypeSymbol namedType && namedType.IsGenericType)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method.IsGenericMethod could be used for generic method invocation, but for a constructor with a generic type parameter method.IsGenericMethod is somehow false and the method has no any info about the generic type, method.ReceiverType has the generic constructor info we need

{
foreach (var typeArgument in namedType.TypeArguments)
{
CheckOperationAttributes(operation, context, platformSpecificOperations, platformSpecificMembers, msBuildPlatforms, typeArgument);
}
}
}
}

static void CheckOperationAttributes(IOperation operation, OperationAnalysisContext context, PooledConcurrentDictionary<IOperation,
Expand Down
36 changes: 36 additions & 0 deletions ...NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.GuardedCallsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,42 @@ void Api()
await VerifyAnalyzerAsyncCs(source);
}

[Fact]
public async Task MethodsWithOsDependentTypeParameterGuarded()
{
var csSource = @"
using System;
using System.Runtime.Versioning;

[SupportedOSPlatform(""windows"")]
class WindowsOnlyType {}

class GenericClass<T> {}

public class Test
{
void GenericMethod<T>() {}
void GenericMethod2<T1, T2>() {}
void M1()
{
if (OperatingSystemHelper.IsWindows())
{
GenericMethod<WindowsOnlyType>();
GenericMethod2<Test, WindowsOnlyType>();
GenericClass<WindowsOnlyType> obj = new GenericClass<WindowsOnlyType>();
}
else
{
[|GenericMethod<WindowsOnlyType>()|];
[|GenericMethod2<Test, WindowsOnlyType>()|];
GenericClass<WindowsOnlyType> obj = [|new GenericClass<WindowsOnlyType>()|];
}
}
}
" + MockAttributesCsSource + MockOperatingSystemApiSource;
await VerifyAnalyzerAsyncCs(csSource, s_msBuildPlatforms);
}

[Fact]
public async Task SupportedUnsupportedRange_GuardedWithOr()
{
Expand Down
48 changes: 48 additions & 0 deletions ...itTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,54 @@ public static void WindowsOnlyMethod() { }
await VerifyAnalyzerAsyncCs(source, tfmAndOption);
}

[Fact]
public async Task MethodsWithOsDependentTypeParameterWarns()
{
var csSource = @"
using System.Runtime.Versioning;

[SupportedOSPlatform(""windows"")]
class WindowsOnlyType {}

public class Test
{
void GenericMethod<T>() {}
void GenericMethod2<T1, T2>() {}
void M1()
{
[|GenericMethod<WindowsOnlyType>()|];
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
[|GenericMethod2<Test, WindowsOnlyType>()|];
}
}
" + MockAttributesCsSource;
await VerifyAnalyzerAsyncCs(csSource);
}

[Fact]
public async Task ConstructorWithOsDependentTypeParameterWarns()
{
var csSource = @"
using System.Runtime.Versioning;

[SupportedOSPlatform(""windows"")]
class WindowsOnlyType {}

class GenericClass<T> {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test such as below:

using System.Runtime.Versioning;

class GenericClass<T>
{
}

[SupportedOSPlatform("windows")]
class WindowsOnlyType { }

class DerivedWindowsOnlyType : GenericClass<WindowsOnlyType> { }
class Test
{
    void M<T>()
    {
        var t = new DerivedWindowsOnlyType();  // Should be flagged
    }
}

Copy link
Contributor

@mavasani mavasani Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another test suggestion:

using System.Runtime.Versioning;

class GenericClass<T>
{
}

[SupportedOSPlatform("windows")]
class WindowsOnlyType { }

class Test<T>
    where T : WindowsOnlyType
{
    void M()
    {
        var t = new GenericClass<T>(); // Should be flagged.
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure it could be flagged, i will try

Copy link
Contributor Author

@buyaa-n buyaa-n Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these scenarios related to the attribute's inheritance and the platform attributes defined as [AttributeUsage(AttributeTargets.Assembly | ... , Inherited = false)] . We are not supporting inherited type even for non-generic case. So I don't think these scenarios should be covered/flagged correct me if i am wrong @terrajobst

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to warn in case the base constructor is doing any platform-specific operation. Created separate issue for discussing and further handling this scenario #4404


public class Test
{
void MethodWithGenericParameter(GenericClass<WindowsOnlyType> a) {}

void M1()
{
GenericClass<WindowsOnlyType> obj = [|new GenericClass<WindowsOnlyType>()|];
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
MethodWithGenericParameter([|new GenericClass<WindowsOnlyType>()|]);
}
}
" + MockAttributesCsSource;
await VerifyAnalyzerAsyncCs(csSource);
}

[Fact]
public async Task OsDependentMethodsCalledWarns()
{
Expand Down