Roslyn-analyzers: [Platform Compatibility Analyzer] Indicate why the current code violates the rule

Created on 15 Aug 2020 · 16Comments · Source: dotnet/roslyn-analyzers

Analyzer

Diagnostic ID: CA1416 Platform Compatibility Analyzer

Describe the improvement

When a diagnostic is raised, the message includes information about the target method, such as:

'API' requires 'windows' 10.1.2.3 or later
'API' is not supported or has been removed from 'browser'

But the diagnostic doesn't indicate what platform support was determined for the call site. We could guide users to resolve issues more easily by including in the message something indicating what support the current code has.

'API' requires 'windows' 10.1.2.3 or later; this code is reachable with 'windows' 10.0
'API' requires 'windows' 10.1.2.3 or later; this code is reachable without 'windows'
'API' is not supported or has been removed from 'browser'; this code is reachable with 'browser'

Area-Microsoft.NetCore.Analyzers Enhancement 0

Source

jeffhandley

👍1

Most helpful comment

@gewarren do you have any suggestions for the below messages?

'API' is supported on 'windows' 10.1.2.3 and later; this code is reachable with 'windows' 10.0
'API' is supported on 'windows' 10.1.2.3 and later; this code is reachable without 'windows'
'API' is unsupported on 'browser'; this code is reachable with 'browser'

1st one trying to express that the invoked API is supported on 'windows' 10.1.2.3 and later but the call site is supported on earlier version (Windows 10.0 and later)
2nd one saying that the invoked API is supported on 'windows' 10.1.2.3 and later but the call site is unsupported on windows
3rd one saying that the invoked API is unsupported on 'browser' but the call site is supported on browser

cc @terrajobst @jeffhandley

buyaa-n on 13 Oct 2020

👍2

All 16 comments

Maybe. I'm concerned that they might not understand what this means.

terrajobst on 18 Aug 2020

The final messaging updated by @jeffhandley's suggestion
Existing Message | Suggested Message
-- | --
'API' requires 'windows' | 'API' is supported in 'windows'
'API' requires 'windows' 10.0 or later | 'API' is supported in 'windows' 10.0 and later
'API' has been deprecated since 'windows' 0.0 | 'API' is deprecated in 'windows'
'API' has been deprecated since 'windows' 10.0 | 'API' is deprecated in 'windows' 10.0 and later
'API' is not supported or has been removed from 'windows' | 'API' is unsupported in 'windows'
'API' is not supported or has been removed since 'windows' 10.0 | 'API' is unsupported in 'windows' 10.0 and later

Now we are receiving feedbacks that 'API' is supported in 'windows' is not that obvious what the problem is @stephentoub suggested to change it to 'API' is only supported in 'windows'. Further when there is multiple supported OSs devs gonna see messages for each:

  [SupportedOSPlatform("Windows")]
  [SupportedOSPlatform("Linux")]
  public void SupportedOnWindowsAndLinuxOnly() { }

  public void Caller()
  {
      SupportedOnWindowsAndLinuxOnly(); // warns: 'SupportedOnWindowsAndLinuxOnly' is supported on 'Windows'  
                                        // warns: 'SupportedOnWindowsAndLinuxOnly' is supported on 'Linux'
  }

Another examples of having two warnings:

  [UnsupportedOSPlatform("windows")]
  [SupportedOSPlatform("10.0.19041.0")]
  public void StartWindowsSupportFromCertainVersion() { }

  public void Caller()
  {
      StartWindowsSupportFromCertainVersion(); 
      // warns :'StartWindowsSupportFromCertainVersion' is unsupported on 'windows'  
      // warns :'StartWindowsSupportFromCertainVersion' is supported on 'windows' 10.0.1903 and later
  }

  [SupportedOSPlatform("windows")]
  [UnsupportedOSPlatform("windows11.0")]
  public void SupportedOnWindowsUnsupportedOnWindows11() { }

  public void Caller2()
  {
      SupportedOnWindowsUnsupportedOnWindows11(); 
      // warns :'SupportedOnWindowsUnsupportedOnWindows11' is supported on 'windows'  
      // warns :'SupportedOnWindowsUnsupportedOnWindows11' is unsupported on 'windows' 11 and later
  }

  [UnsupportedOSPlatform("windows")] 
  [SupportedOSPlatform("windows11.0")]
  [UnsupportedOSPlatform("windows12.0")]
  public void UnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12() { }

  public void Caller3()
  {
      UnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12(); 
      // warns :'UnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12' is supported on 'windows'  
      // warns :'UnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12' is unsupported on 'windows' 11 and later
  }

Which also not looks meaningful. Based on the feedbacks I propose the following messaging:

List | Existing Message | Suggested Message
-- | -- | --
Allow list | 'API' is supported in 'windows' | 'API' is only supported on: 'windows'
Allow list | 'API' is supported in 'windows' 10.0 and later | 'API' is only supported on: 'windows' 10.0 and later
Allow list | 2 or more support warning | 'API' is only supported on: 'windows', 'ios' and 'Linux'
Allow list | 2 or more support warning with version | 'API' is only supported on: 'windows' 10.0 and later, 'ios', and 'Linux'
Allow list | 'API' is unsupported in 'windows' 10.0 and later | 'API' is unsupported on: 'windows' from version 10.0 and later ()
Allow list | 2 warnings for support and unsupported | 'API' is only supported on: 'windows' from version 11.0 to 12.0 ()
Deny list | 'API' is supported in 'windows' 10.0 and later | 'API' is supported on: 'windows' only from version 10.0 and later
Deny list | 'API' is unsupported in 'windows' | 'API' is unsupported on: 'windows' (same)
Deny list | 2 warnings for unsupported and supported | 'API' is supported on: 'windows' from version 11.0 to 12.0

(*) Any other support could be warned with a separate message

  [SupportedOSPlatform("Windows")]
 [SupportedOSPlatform("ios")]
  [SupportedOSPlatform("Linux")]
  public void SupportedOnWindowsAndLinuxOnly() { }

  public void Caller()
  {
     // warns: 'SupportedOnWindowsAndLinuxOnly' is only supported on 'Windows', `ios` and 'Linux'
      SupportedOnWindowsAndLinuxOnly(); 
  }

  [UnsupportedOSPlatform("windows")]
  [SupportedOSPlatform("10.0.19041.0")]
  public void StartWindowsSupportFromCertainVersion() { }

  public void Caller()
  {
      // warns :'StartWindowsSupportFromCertainVersion' is supported on 'windows' only from version 10.0.19041.0 and later
      StartWindowsSupportFromCertainVersion(); 
  }

  [SupportedOSPlatform("windows")]
  [UnsupportedOSPlatform("windows11.0")]
  public void SupportedOnWindowsUnsupportedOnWindows11() { }

  public void Caller2()
  {
      // warns :'SupportedOnWindowsUnsupportedOnWindows11' is unsupported on 'windows' from version 11 and later
      SupportedOnWindowsUnsupportedOnWindows11(); 
  }

  [UnsupportedOSPlatform("windows")] 
  [SupportedOSPlatform("windows11.0")]
  [UnsupportedOSPlatform("windows12.0")]
  public void UnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12() { }

  public void Caller3()
  {
     // warns :'UnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12' is supported on 'windows' from version 11.0 to 12.0
      UnsupportedOnWindowsSupportedOnWindows11UnsupportedOnWindows12(); 
  }

  [SupportedOSPlatform("windows11.0")]
  [UnsupportedOSPlatform("windows12.0")]
  public void SupportedOnWindows11UnsupportedOnWindows12() { }

  public void Caller4()
  {
     // warns :'SupportedOnWindows11UnsupportedOnWindows12' is only supported on 'windows' from version 11.0 to 12.0
      SupportedOnWindows11UnsupportedOnWindows12(); 
  }

CC @jeffhandley @terrajobst

buyaa-n on 16 Sep 2020

I like where this is going. I suggest a couple of tweaks. We changed "in 'windows'" to "on 'windows'" previously, which I've reflected here too.

Here are some notes:

I'd still like to see the current code's supported platforms included in the message somehow
- It isn't always obvious why "'API' is only supported on: 'windows'" or "'API' is unsupported on: 'ios'" matters?
- Developers might not realize their code is assumed to be platform-neutral or recognize which platforms are marked as supported for their current context
I think we can use bulleted lists inside the messages; the different platforms could be bullets
- We should also consider localization here -- having "'windows', 'ios', and 'Linux'" could be tough to capture in a localizable way; doing just a bulleted list, or comma-separated list will be easier, especially if there's a ":" before the list

jeffhandley on 22 Sep 2020

👍1

I'd still like to see the current code's supported platforms included in the message somehow

It isn't always obvious why "'API' is only supported on: 'windows'" or "'API' is unsupported on: 'ios'" matters?

Developers might not realize their code is assumed to be platform-neutral or recognize which platforms are marked as supported for their current context

@jeffhandley are you suggesting include the call site targets/attributes in the message? Could you write some examples of how the wording could be?

buyaa-n on 9 Oct 2020

I like where this is going. I suggest a couple of tweaks. We changed "in 'windows'" to "on 'windows'" previously, which I've reflected here too.

Yep, copied from old messaging, thanks

I think we can use bulleted lists inside the messages; the different platforms could be bullets

We should also consider localization here -- having "'windows', 'ios', and 'Linux'" could be tough to capture in a localizable way; doing just a bulleted list, or comma-separated list will be easier, especially if there's a ":" before the list

Sounds good

buyaa-n on 9 Oct 2020

It feels weird not to have the versions also quoted: 'windows' 10.0 and later -> 'windows 10.0 and later' (or maybe simply adding quotes around versions).

Besides, I think that having the list of what is currently recognized, as suggested on the first comment, would be helpful for debugging/understanding the problem.

Evangelink on 12 Oct 2020

It feels weird not to have the versions also quoted: 'windows' 10.0 and later -> 'windows 10.0 and later' (or maybe simply adding quotes around versions).

We cannot make 'windows 10.0 and later' as and later need to be localized, quoting it separately 'windows' '10.0' and later looks kind of ugly 😛 to me, quoting only platform and version 'windows 10.0' and later might also have localization issue, also i guess version not quoted because it is counted as a number

Besides, I think that having the list of what is currently recognized, as suggested on the first comment, would be helpful for debugging/understanding the problem.

Are you referring to the suggestion in the original description about including the call site like below?

'API' requires 'windows' 10.1.2.3 or later; this code is reachable with 'windows' 10.0
'API' requires 'windows' 10.1.2.3 or later; this code is reachable without 'windows'
'API' is not supported or has been removed from 'browser'; this code is reachable with 'browser'

For me this messages not that clear, i think its need some improvement

buyaa-n on 12 Oct 2020

@gewarren do you have any suggestions for the below messages?

'API' is supported on 'windows' 10.1.2.3 and later; this code is reachable with 'windows' 10.0
'API' is supported on 'windows' 10.1.2.3 and later; this code is reachable without 'windows'
'API' is unsupported on 'browser'; this code is reachable with 'browser'

1st one trying to express that the invoked API is supported on 'windows' 10.1.2.3 and later but the call site is supported on earlier version (Windows 10.0 and later)
2nd one saying that the invoked API is supported on 'windows' 10.1.2.3 and later but the call site is unsupported on windows
3rd one saying that the invoked API is unsupported on 'browser' but the call site is supported on browser

cc @terrajobst @jeffhandley

buyaa-n on 13 Oct 2020

👍2

I'm coming up blank trying to think of a good way to communicate a scenario such as this:

API called is annotated with multiple platforms
The call site supports several platforms (even the default set)
There's a mismatch in the support

We could ostensibly want to produce a message like this:

'API' is supported on:

windows

macos

ios

ipados

watchos

android

This call site is reachable on:

unix

linux

browser

I presume we wouldn't want messages _that_ long. But it's hard to express all of the data in a succinct and helpful way.

jeffhandley on 14 Oct 2020

👍1

I presume we wouldn't want messages that long

That doesn't seem prohibitively long to me:
"'Foo' is supported only on 'windows', 'macos', 'ios', 'ipados', 'watchos', and 'android', but this call site is reachable on 'unix', 'linux', and 'browser'."

stephentoub on 15 Oct 2020

👍1

'API' is supported on 'windows' 10.1.2.3 and later; this code is reachable with 'windows' 10.0
'API' is supported on 'windows' 10.1.2.3 and later; this code is reachable without 'windows'
'API' is unsupported on 'browser'; this code is reachable with 'browser'

So the example above will look like:

'API' is only supported on 'windows' 10.1.2.3 and later, but this call site is reachable on 'windows' 10.0
'API' is only supported on 'windows' 10.1.2.3 and later, but this call site is unreachable on 'windows'
'API' is unsupported on 'browser', but this call site is reachable with 'browser'

Sounds good to me 👍

For many os support/unsupport case platforms with or without a version looks a little odd, but i think it is fine: 'Foo' is only supported on 'windows' 10.1.2.3 and later, 'macos', 'ios' 14.0 and later, 'ipados', 'watchos', and 'android', but this call site is reachable on 'unix', 'linux' 4.8 and later, and 'browser'.

Plus there could be a multitargeted call site, in that case, it could be something like this:

'API' is only supported on 'windows' 10.1.2.3 and later, but this call site is multitargeted
'API' is unsupported on 'browser', but this call site is multitargeted

How is it sound?

buyaa-n on 17 Oct 2020

That's looking good to me. Under what condition would we hit "but this call site is multitargeted"? Depending on that answer, I think we might want to pick a different word instead of "multitargeted."

Instead of "this call site" do folks think "this code" would be OK?

jeffhandley on 21 Oct 2020

👍1

Under what condition would we hit "but this call site is multitargeted"? Depending on that answer, I think we might want to pick a different word instead of "multitargeted."

It means the call site has no any OSPlatform attribute. An unattributed API is considered to work on all OS platforms. When the call site will have no any attribute? - when the code has no target platform, that is why i said it multi-targeted

Instead of "this call site" do folks think "this code" would be OK?

For me, call site sounds more meaningful, but i am OK if we decide to go with "code"

buyaa-n on 21 Oct 2020

Thanks. Multi-targeting usually makes me think of having multiple target frameworks in the project file. https://devblogs.microsoft.com/dotnet/the-future-of-net-standard/ doesn't really define a term for net5.0 where no platform is specified, other than "portable," but I think that term is too loaded (from history).

@terrajobst @stephentoub do you have a suggestion for what word we should use to describe an API that is supported everywhere?

I'm fine with "call site" too.

jeffhandley on 21 Oct 2020

👍1