Platform compatibility analyzer not warning for platform specific type parameter

[buyaa-n]

Platform compatibility analyzer not warning for platform-specific type parameter, seems we could warn for any type parameter usage of platform-specific API. Related to PR review comment :

To cover both could we warn for any type of parameter usages? If that could cause any false positives?

My understanding of the intention here:

1. The whole class/type is marked as [UnsupportedOSPlatform("browser")], meaning that "this class is not going to work on that platform".
2. The method call SomeGenericMethod<AboveType>() should warn about using this type. It is not supported on this platform.

I guess I don't see how a false positive could occur here. You are using a type that is not supported on the platform. It seems like it should warn.

Originally posted by @eerhardt in dotnet/runtime#43363 (comment)

[mavasani]

@buyaa-n Seems like a valid bug. However, note that fixing this means now violations can be raised outside of executable code (i.e. in symbol signatures, base list, etc.). These usages cannot be scanned from operation callbacks, which are only for executable code. You will need to add symbol (or syntax?) analyzer actions to the analyzer to identify these. I'd recommend starting with a symbol action (which is still languge agnostic), and if that is not sufficient, then go to a syntax action (which would need separate C# and VB implementations).

[buyaa-n]

However, note that fixing this means now violations can be raised outside of executable code (i.e. in symbol signatures, base list, etc.). These usages cannot be scanned from operation callbacks, which are only for executable code. You will need to add symbol (or syntax?) analyzer actions to the analyzer to identify these. I'd recommend starting with a symbol action (which is still language agnostic), and if that is not sufficient, then go to a syntax action (which would need separate C# and VB implementations).

Thanks, for now i think only diagnosing within the operation block will be enough, no need to diagnose outside of executable code (i.e. in symbol signatures, base list, etc.), but will see how it goes

[mavasani]

no need to diagnose outside of executable code (i.e. in symbol signatures, base list, etc.)

I thought that was one one of the request from @eerhardt? If you have a type marked as unsupported on a platform, then using that type as a type parameter inside the base list or method signatures within another type supports that platform should be flagged. Isn't that the scenario?

[Unsupported("Platform1")]
class A { }

class B<T> { }

[Supported("Platform1")]
class C : A, B<A>   // Both uses of 'A' should be flagged here?
{
   void M(A p1, B<A> p2) // Both uses of 'A' should be flagged here?
   {
   }
}

[eerhardt]

@mavasani - that is one example scenario, yes.

The other example scenario is using the type in a generic method call directly:

[Unsupported("Platform1")]
class A {}

class B
{
    static void Method<T> {}
}

[Supported("Platform1")]
class C
{
    void M()
    {
        B.Method<A>();  // should be flagged
    }
}

If we can fix this scenario, I think that is a good start. But yes, to completely fix this I think we would need to fix the scenario you wrote as well.

[buyaa-n]

cc @jeffhandley

[buyaa-n]

If you have a type marked as unsupported on a platform, then using that type as a type parameter inside the base list or method signatures within another type supports that platform should be flagged. Isn't that the scenario?

@mavasani i think the question/scenario you mentioned is the same as should we flag all reference of platform-specific type? I would say no, as you mentioned, with the existing design we only flag them when they are used/invoked, and i don't think we need to flag on type declaration. let's see a similar/simpler example:

[SupportedOSPlatform("windows")]
class A{ }
class B<T> { }
class C : A, B<A>   // Both uses of 'A' should be flagged here? No
{
   A instance; //  should be flagged here? - No
   void M1(A p1) { } // 'A' should be flagged here? - No
   void M2(A p1, B<A> p2) // Both uses of 'A' should be flagged here? No
   {
   }
   void InvokeThem()
   {
        instance = new A(); // We are flagging here with constructor invocation
        M1(new A()); // same here, with constructor invocation
        M2(new A(), new B<A>()); // first argument already flagged, but not the 2nd, it should be fixed with this issue
   }
}

Why we should not warn for the type/method declaration or other cases you mentioned?

• i think it is how we designed the feature cc @terrajobst
• It will be flagged on the usage anyways
• instance/method declarations cannot be guarded with guard methods, if we flag them anyway, the types using them cannot handle them as efficiently as designed

[mavasani]

Thanks @buyaa-n - that seems a reasonable design stance. We also took a very similar approach for the BannedApiAnalyzer, which flags uses of banned symbols in operation blocks/executable code, but we explicitly don't flag/detect uses of banned symbols in declarations. It is also based on the assumption that eventually users will use such a type/method built upon banned symbols in executable code and they will see violations flagged there.

[buyaa-n]

If we can fix this scenario, I think that is a good start. But yes, to completely fix this I think we would need to fix the scenario you wrote as well.

@eerhardt the scenario from your below example will be covered with the fix, but we are not gonna flag it on the type/API declarations please let me know if you are OK with that

[Unsupported("Platform1")]
class A {}

class B
{
    static void Method<T> {}
}

[Supported("Platform1")]
class C
{
    void M()
    {
        B.Method<A>();  // will be flagged
    }
}

[eerhardt]

please let me know if you are OK with that

Yes, I am OK with moving forward with just the method calls being flagged for now.