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.

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

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Oct 29, 2020

Warn when platform-specific type used as generic type parameter, useful when a type created using reflections

Fixes #4362

@buyaa-n buyaa-n requested a review from a team as a code owner October 29, 2020 18:46
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

[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

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

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

@mavasani
Copy link
Contributor

@buyaa-n I think you want to target this PR to release\5.0.2xx branch?

@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

Merging #4390 into release/5.0.2xx will increase coverage by 0.06%.
The diff coverage is 97.77%.

@@                 Coverage Diff                 @@
##           release/5.0.2xx    #4390      +/-   ##
===================================================
+ Coverage            95.71%   95.77%   +0.06%     
===================================================
  Files                 1182     1174       -8     
  Lines               274766   269538    -5228     
  Branches             18787    16826    -1961     
===================================================
- Hits                262996   258154    -4842     
+ Misses                9648     9273     -375     
+ Partials              2122     2111      -11     

@buyaa-n buyaa-n changed the base branch from master to release/5.0.2xx November 2, 2020 17:48
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Nov 2, 2020

@buyaa-n I think you want to target this PR to release\5.0.2xx branch?

That is right, changing target, but seems i need a new branch based on release\5.0.2xx branch

@buyaa-n buyaa-n changed the base branch from release/5.0.2xx to master November 2, 2020 19:53
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Nov 2, 2020

Closing this PR as its need to target release\5.0.2xx branch, created new PR #4403

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Platform compatibility analyzer not warning for platform specific type parameter
2 participants