Support for Property getter/setter and event add/remove annotations

[buyaa-n]

Fixes #4071 Attributes are not respected on property getters/setters
Fixes #4072 Attributes are not respected on event add/remove methods

Copied IOperation Extensions method from roslyn and used it for determining if Read or Write operation taken place

CC @jmarolf @mavasani @jeffhandley @terrajobst

[jmarolf]

I would wait for @mavasani to review this before merging

[codecov]

Codecov Report

Merging #4074 into release/dotnet5-rc1 will decrease coverage by 0.02%.
The diff coverage is 77.45%.

@@                   Coverage Diff                   @@
##           release/dotnet5-rc1    #4074      +/-   ##
=======================================================
- Coverage                95.53%   95.51%   -0.03%     
=======================================================
  Files                     1159     1160       +1     
  Lines                   260997   261358     +361     
  Branches                 15649    15699      +50     
=======================================================
+ Hits                    249352   249635     +283     
- Misses                    9669     9743      +74     
- Partials                  1976     1980       +4     

[mavasani]

Some minor suggestions, but otherwise LGTM. I think you may want to tweak the nameof case to use the property symbol instead of the getter.

[jeffhandley]

What gets emitted in the diagnostic for the name of the API? Ideally, we would match the behavior of the Obsolete attribute's precedent. Here's what it does. Does the helper method we're using to get the display name already handle this by chance?

In C#

Test.WindowsOnlyPropertyGetter.get is obsolete

Test.WindowsOnlyPropertySetter.set is obsolete

In VB

'Get' accessor of 'Public Shared Overloads Property WindowsOnlyGetter as Boolean' is obsolete

'Set' accessor of 'Public Shared Overloads Property WindowsOnlySetter as Boolean' is obsolete

[buyaa-n]

What gets emitted in the diagnostic for the name of the API? Ideally, we would match the behavior of the Obsolete attribute's precedent. Here's what it does. Does the helper method we're using to get the display name already handle this by chance?

Display name is handling only C# similarly (well we are using c# option SymbolDisplayFormat.CSharpShortErrorMessageFormat), i guess we need to split that logic into a separate version for VB

[mavasani]

I actually prefer our current approach of using the similar display name for VB and C#, we strive very hard to not have different error messages between languages for same issue. I’d say it is in fact a compiler issue that messages are not similar for obsolete diagnostics between languages.

[jeffhandley]

Ok, cool. So long as it's deliberate and the property accessor names are friendly like Obsolete's messages. Thanks.

[mavasani]

Note that it is fine to use VB error message format for VB and C# error message format for C#. You can tweak that part easily in your ReportDiagnostics method by switching on IOperation.LanguageName. Let’s still try to use the same resources string for C# and VB so the message appears similar to user, but just with correct VB and C# terminology.

[buyaa-n]

JFYI: Messages looks in C# like:
'Test.WindowsOnlyPropertyGetter.get' is supported on 'windows'
'Test.WindowsOnlyPropertySetter.set' is supported on 'windows'

In VB:
'Test.WindowsOnlyPropertyGetter()' is supported on 'windows'
'Test.WindowsOnlyPropertySetter(Boolean)' is supported on 'windows'

[mavasani]

@buyaa-n Above messages are when both languages use ToDisplayString(CSharpErrorMessageFormat)? Or after change for each language to use different error message format?

[buyaa-n]

@buyaa-n Above messages are when both languages use ToDisplayString(CSharpErrorMessageFormat)? Or after change for each language to use different error message format?

we don't have any path for VB, so they both using C# SymbolDisplayFormat.CSharpShortErrorMessageFormat

[mavasani]

Thanks, can you try changing it to use C#/VB short error message format based on IOperation.Language and see the message?

[buyaa-n]

Thanks, can you try changing it to use C#/VB short error message format based on IOperation.Language and see the message?

Sure, about to do that, didn't know it was so easy 😛

[buyaa-n]

Now VB looks like
'Public Shared Property Get WindowsOnlyPropertyGetter() As Boolean' is supported on 'windows'
'Public Shared Property Set WindowsOnlyPropertySetter(value As Boolean)' is supported on 'windows'
Pretty close to the Obsolete's, thanks @mavasani, and @jeffhandley

[jeffhandley]

I recommend we go ahead and merge this in and then have @jmarolf proceed with getting the new build integrated into the SDK.

[buyaa-n]

I recommend we go ahead and merge this in and then have @jmarolf proceed with getting the new build integrated into the SDK.

The bug fix for Event accessor is coming, I am about to push i guess we want ti fixed, hope there is time for review

[buyaa-n]

Event accessors added, @mavasani final round review, please 🙏

[mavasani]

Small nits, but overall LGTM!

[jeffhandley]

I've done manual testing with this branch and property getters and setters as well as event add/remove methods are all working as expected. 💯