[Platform Compatibility Analyzer] Indicate why the current code violates the rule #4021

jeffhandley · 2020-08-15T01:38:37Z

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'

terrajobst · 2020-08-18T16:02:11Z

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

buyaa-n · 2020-09-15T22:37:08Z

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

jeffhandley · 2020-09-22T02:56:54Z

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.

List	Existing Message	Suggested Message
Allow list	'API' is supported on 'windows'	'API' is only supported on: 'windows'
Allow list	'API' is supported on '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', 'Linux'
Allow list	2 or more support warning with version	'API' is only supported on: 'windows' 10.0 and later, 'ios', 'Linux'
Allow list	'API' is unsupported on 'windows' 10.0 and later	'API' is unsupported on: 'windows' 10.0 and later
Allow list	2 warnings for support and unsupported	'API' is only supported on: 'windows' 11.0 to 12.0
Deny list	'API' is supported on 'windows' 10.0 and later	'API' is supported on: 'windows' 10.0 and later
Deny list	'API' is unsupported on 'windows'	'API' is unsupported on: 'windows'
Deny list	2 warnings for unsupported and supported	'API' is supported on: 'windows' 11.0 to 12.0

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

buyaa-n · 2020-10-09T21:06:01Z

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 · 2020-10-09T21:08:53Z

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

Evangelink · 2020-10-12T09:18:23Z

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.

buyaa-n · 2020-10-12T16:38:09Z

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 · 2020-10-12T22:57:42Z

@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

jeffhandley · 2020-10-14T05:34:59Z

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.

stephentoub · 2020-10-15T20:50:50Z

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'."

buyaa-n · 2020-10-16T22:06:48Z

'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?

jeffhandley · 2020-10-21T01:35:55Z

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?

buyaa-n · 2020-10-21T04:04:44Z

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"

jeffhandley · 2020-10-21T05:02:06Z

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.

stephentoub · 2020-10-21T14:16:51Z

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

"on all platforms"?

buyaa-n · 2020-12-09T18:38:27Z

Fixed with #4438

jeffhandley added Enhancement Area-Microsoft.CodeAnalysis.NetAnalyzers labels Aug 15, 2020

jeffhandley assigned buyaa-n Aug 15, 2020

jeffhandley mentioned this issue Aug 15, 2020

Platform compatibility analyzer #3917

Merged

buyaa-n mentioned this issue Aug 27, 2020

Platform names should be in quotes #4008

Closed

buyaa-n mentioned this issue Sep 24, 2020

Fix inconsistency bug, child cannot widen as support, can narrow #4208

Merged

buyaa-n mentioned this issue Oct 27, 2020

Improve messaging of Platform compat analyzer #4373

Closed

jeffhandley mentioned this issue Nov 19, 2020

Epic: Expand platform compat annotations to cover more configurations dotnet/runtime#43616

Closed

13 tasks

buyaa-n mentioned this issue Nov 19, 2020

Improve messaging for Platform Compat Analyzer #4438

Merged

jeffhandley added the Priority:0 Critical to the release label Nov 19, 2020

buyaa-n closed this as completed Dec 9, 2020

jeffhandley self-assigned this Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Platform Compatibility Analyzer] Indicate why the current code violates the rule #4021

[Platform Compatibility Analyzer] Indicate why the current code violates the rule #4021

jeffhandley commented Aug 15, 2020

terrajobst commented Aug 18, 2020 •

edited

Loading

buyaa-n commented Sep 15, 2020 •

edited

Loading

jeffhandley commented Sep 22, 2020

buyaa-n commented Oct 9, 2020

buyaa-n commented Oct 9, 2020

Evangelink commented Oct 12, 2020

buyaa-n commented Oct 12, 2020

buyaa-n commented Oct 12, 2020

jeffhandley commented Oct 14, 2020

stephentoub commented Oct 15, 2020

buyaa-n commented Oct 16, 2020

jeffhandley commented Oct 21, 2020

buyaa-n commented Oct 21, 2020

jeffhandley commented Oct 21, 2020

stephentoub commented Oct 21, 2020

buyaa-n commented Dec 9, 2020

[Platform Compatibility Analyzer] Indicate why the current code violates the rule #4021

[Platform Compatibility Analyzer] Indicate why the current code violates the rule #4021

Comments

jeffhandley commented Aug 15, 2020

Analyzer

Describe the improvement

terrajobst commented Aug 18, 2020 • edited Loading

buyaa-n commented Sep 15, 2020 • edited Loading

jeffhandley commented Sep 22, 2020

buyaa-n commented Oct 9, 2020

buyaa-n commented Oct 9, 2020

Evangelink commented Oct 12, 2020

buyaa-n commented Oct 12, 2020

buyaa-n commented Oct 12, 2020

jeffhandley commented Oct 14, 2020

stephentoub commented Oct 15, 2020

buyaa-n commented Oct 16, 2020

jeffhandley commented Oct 21, 2020

buyaa-n commented Oct 21, 2020

jeffhandley commented Oct 21, 2020

stephentoub commented Oct 21, 2020

buyaa-n commented Dec 9, 2020

terrajobst commented Aug 18, 2020 •

edited

Loading

buyaa-n commented Sep 15, 2020 •

edited

Loading