Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VB support for the virtual methods in DetectPreviewFeatureAnalyzer #5566

Merged
merged 29 commits into from
Oct 18, 2021

Conversation

pgovind
Copy link
Contributor

@pgovind pgovind commented Sep 30, 2021

EDIT: The original description moved to below comment

Customer Impact

With #5425, we have improved the diagnostics locations for fields/events, inheritance from preview types, method parameters, generic type parameters etc as proposed in dotnet/runtime#57224. But the implementation done only for C#, this PR has the VB implementation. By taking the VB improvement as well, we’ll align the VB and C# behavior such that the reference to the preview API will be flagged instead of flagging the symbol that consumes that reference.

With this PR we have added

Without this change,

  • We will have no any test for VB for this analyzer in 6.0
  • Some diagnostic locations will be different than C# versions, therefore the location in VB will be confusing for the user as it will not squiggle in the expected location

Lets assume we have following class, interfaces

    <RequiresPreviewFeatures>
    Interface PreviewInterface
    End Interface

    Interface NonPreviewInterface
    End Interface

    <RequiresPreviewFeatures>
    Public MustInherit Class PreviewType
    End Class

Example of current diagnostic location:

    Public Partial Class [|Zoo|] 'reports diagnostic on the class not the preview interface
        Implements NonPreviewInterface, PreviewInterface

        Private [|_field|] As List(Of PreviewType) 'reports diagnostic on the field not in the preview type parameter 
    End Class

    Public Partial Class [|Zoo|] ' same, squiggle around `Zoo` 
        Inherits PreviewType
    End Class

Desired/updated location:

    Public Partial Class Zoo
        Implements NonPreviewInterface, [|PreviewInterface|] ' only flag the preview type/interface

        Private _field As List(Of [|PreviewType|]) ' flag preview type parameter instead of the field 
    End Class

    Public Partial Class Zoo
        Inherits [|PreviewType|] ' flag inherited preview type only
    End Class

Testing

Unit tests added for VB, not only for the related code change, also added smoke tests for overall functionality

Risk

Minimal. This PR only adding test for VB plus improving the diagnostics locations. It will not introduce new warnings. There is very little chance that it could bread the build when the diagnostics are suppressed in the old location and with this change they could see the warning in different location.

@pgovind pgovind requested a review from a team as a code owner September 30, 2021 19:57
@pgovind pgovind marked this pull request as draft September 30, 2021 19:57
@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #5566 (73adb2e) into release/6.0.1xx (06ec01d) will decrease coverage by 0.00%.
The diff coverage is 94.81%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5566      +/-   ##
===================================================
- Coverage            95.53%   95.53%   -0.01%     
===================================================
  Files                 1275     1275              
  Lines               291033   292698    +1665     
  Branches             17569    17701     +132     
===================================================
+ Hits                278046   279624    +1578     
- Misses               10599    10649      +50     
- Partials              2388     2425      +37     

@buyaa-n buyaa-n marked this pull request as ready for review October 15, 2021 19:13
@buyaa-n
Copy link
Member

buyaa-n commented Oct 15, 2021

The original description by @pgovind:

Just leaving this PR up for now. This builds on top of #5502.

  1. Once Support Url and Message parameters in RequiresPreviewFeaturesAttribute #5502 goes in, this PR needs to be rebased on the latest release/6.0.1xx. Until then, please only look at the latest commit for changes specific to this PR

What this PR does: It overrides all the virtual methods in DetectPreviewFeaturesAnalyzer EXCEPT GetConstraintSyntaxNodeForTypeConstrainedByPreviewTypes.

Pending work:

  1. Implement GetConstraintSyntaxNodeForTypeConstrainedByPreviewTypes (can be it's own PR)
  2. I've only added smoke tests to this PR. While all new code that's been added is being tested in the smoke tests, they are by no means exhaustive. One suggestion here is to create an issue to improve test coverage for VB, and mark it as up for grabs? Or, we could roll with just basic smoke tests for VB considering it's usage.
  1. There is no API for implementing the VB version of GetConstraintSyntaxNodeForTypeConstrainedByPreviewTypes filed an issue
  2. I have added more tests for interesting scenarios, overall coverage looks good, I think it is enough as we do not require 100% coverage for VB

Now the PR is ready for review @jeffhandley

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching the C# location issue while doing this. I left a couple comments but I think they may be related to the issue you filed.

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This has been approved by tactics for 6.0 GA as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants