dotnet / platform-compat

Roslyn analyzer that finds usages of APIs that will throw PlatformNotSupportedException on certain platforms.
MIT License
278 stars 43 forks source link

False positive for Activator.CreateInstance #32

Closed bording closed 7 years ago

bording commented 7 years ago

I'm seeing the following warning:

PC001 Activator.CreateInstance(Type, params object[]) isn't supported on Linux, MacOSX, Windows 

But when testing this out with the following code, it works correctly on all platforms without throwing an exception:

using System;

namespace TestPlatform
{
    class Program
    {
        static void Main(string[] args)
        {
            var instance = Activator.CreateInstance(typeof(Test), 100, "42");
        }
    }

    class Test
    {
        int first;
        string second;

        public Test(int one, string two)
        {
            first = one;
            second = two;
        }
    }
}
ghost commented 7 years ago

I'm getting similar false positives with Activator.CreateInstance.

I'm also getting a false positive with AutoResetEvent evnt = new AutoResetEvent(false). Seeing as AutoResetEvent is used in dotnet/corefx, and is listed in the .NET Core 2.0 API browser I assume this is a false positive too?

Same goes for HMACSHA1 - generates PC001 warning, but listed in .NET Core 2.0 api browser.

pjanotti commented 7 years ago

As you saw the automated scan has some false positives I am working on a manually curated list of PNSE exceptions, then some work to track this manual exclusions.

pjanotti commented 7 years ago

/cc @bording @elevate-andrewlock

I am adding the cases of Activator.CreateInstance and sync primitives via #35. Some of them are simply incorrect and added to a exclusion list while some of them are being moved to deprecation since it is not possible to use a given parameter, so the recommendation becomes to use alternative overloads.

bording commented 7 years ago

@pjanotti That's good news!

I've come across a number of other false positives for other APIs as well. Should I file another issue for them?

pjanotti commented 7 years ago

@bording Yes, please. It will be very helpful.

pjanotti commented 7 years ago

@elevate-andrewlock

... generates PC001 warning, but listed in .NET Core 2.0 api browser.

Those listed in your comment are false-positives but notice that is possible for some method/property to be in .NET Core 2.0 and for it to throw PlatformNotSupportedException . This tool is to try to reduce such bad surprises and point the real ones.

ghost commented 7 years ago

@pjanotti A fair point :)

Personally, I feel like the decision to make PlatformNotSupportedExceptions an acceptable way of "implementing" .NET Standard completely undermines the whole premise of portable, cross-platform code :( Without this analyzer, we lose the compile time guarantees that our code will actually work at all when you come to run it.

Ah well (end rant)

Appreciate your efforts in updating this! :)

terrajobst commented 7 years ago

@elevate-andrewlock

Personally, I feel like the decision to make PlatformNotSupportedExceptions an acceptable way of "implementing" .NET Standard completely undermines the whole premise of portable, cross-platform code :(

It's a trade-off. See this entry in our FAQ.

ghost commented 7 years ago

Yep, totally understand the decision, I guess I just don't necessarily agree with it due to the ambiguity it introduces :) This analyzer should go a long way to fixing that though 👍

pjanotti commented 7 years ago

@elevate-andrewlock @bording - the APIs mentioned here were covered in #35, I will close this item but please keep raising issues for any false positives that you may find, I did some scan of that but it is a large surface...