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.

Sign up for GitHub

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

Jump to bottom

Analyzer to detect RequiresPreviewFeatures attribute #5155

Merged
merged 2 commits into from
Jul 12, 2021
Merged

Analyzer to detect RequiresPreviewFeatures attribute #5155

merged 2 commits into from
Jul 12, 2021

Conversation

pgovind
Copy link
Contributor

@pgovind pgovind commented Jun 18, 2021 •
edited
Loading

Partly addresses dotnet/runtime#52726

I think this is a good point to get the implementation reviewed. I've borrowed ideas from the PlatformCompatilityAnalyzer implementation.

Pending work (in a follow up PR):

  1. Update unit tests to include VB once all the C# test cases have been defined comprehensively
  2. Another PR to start consuming the latest sdk in this repo so that we can do symbol level comparison of RequiresPreviewFeaturesAttribute and also implement logic to look for the attribute on dependencies.
    3 Replace UseFirstDescriptor with the DiagnosticID for each test case
  3. Fix the TODO comments in the analyzer

@pgovind pgovind requested a review from a team as a code owner June 18, 2021 00:21
@pgovind pgovind changed the title Preview feature 60 DO NOT REVIEW: Preview feature 60 Jun 18, 2021
@pgovind pgovind marked this pull request as draft June 18, 2021 00:21
pgovind
pgovind commented Jun 18, 2021
View reviewed changes
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureAnalyzer.cs
}
}

if (propertyOrMethodSymbol.IsImplementationOfAnyImplicitInterfaceMember(out ISymbol baseInterfaceMember))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new extension method. The existing method doesn't expose baseInterfaceMember. Instead it only returned a bool.

pgovind
pgovind commented Jun 18, 2021
View reviewed changes
src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureUnitTests.cs Outdated
csInput
}
},
MarkupOptions = MarkupOptions.UseFirstDescriptor,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured this out from running the unit tests with multiple diagnostics. Just out of curiosity, what is the right way to specify which diagnostic to test for with the markup syntax?

Copy link
Member

@Youssef1313 Youssef1313 Jun 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@pgovind I believe it's {|ID:code|}, example:

roslyn-analyzers/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/EnumWithFlagsAttributeTests.cs

Line 100 in c4bea7a

public enum {{|CA1027:SimpleFlagsEnumClass|}}

pgovind
pgovind commented Jun 18, 2021
View reviewed changes
src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureUnitTests.cs Outdated

}

// TODO: The following test cannot be activated yet because it requires preview roslyn bits
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you want this commented out portion removed.

@pgovind pgovind marked this pull request as ready for review June 18, 2021 01:08
@pgovind pgovind changed the title DO NOT REVIEW: Preview feature 60 Analyzer to detect ReqeuiresPreviewFeatures attribute Jun 18, 2021
@pgovind pgovind requested review from mavasani and buyaa-n June 18, 2021 01:08
@pgovind
Copy link
Contributor Author

pgovind commented Jun 18, 2021

I think this is ready to review now.

Youssef1313
Youssef1313 reviewed Jun 18, 2021
View reviewed changes
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureAnalyzer.cs
{
if (usageType == PreviewFeatureUsageType.StaticAbstract)
{
context.ReportDiagnostic(symbol.CreateDiagnostic(StaticAbstractIsPreviewFeatureRule));
Copy link
Member

Choose a reason for hiding this comment

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

Will that get reported for something like:

public abstract class C
{
    public static abstract M();
}

I think it shouldn't, since the above is an error anyway (static abstract is only allowed in interfaces), and this diagnostic will make it feel like this is allowed in preview.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will yea. However, I am unable to test it at the moment because we don't consume the latest compiler yet. Without testing, I'm not sure how the parent chain will look here. I've added a TODO for now. Once we started consuming the latest compiler here, I can fix this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you file a tracking issue for this and assign it to yourself?

mavasani
mavasani reviewed Jul 9, 2021
View reviewed changes
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureAnalyzer.cs
if (!requiresPreviewFeaturesSymbols.TryGetValue(symbol, out bool existing))
{
bool ret = false;
// TODO: Use symbol.HasAttribute(RequiresPreviewFeaturesAttribute) once we consume the latest sdk
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you file a tracking bug with all the TODOs and assigned it to yourself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5227

mavasani
mavasani reviewed Jul 9, 2021
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Mostly LGTM as well - will let @buyaa-n handle the final sign off for merge. I would certainly recommend doing performance measurements for this analyzer as it seems equally expensive as PlatformCompatAnalyzer.

@pgovind pgovind mentioned this pull request Jul 9, 2021
Open
4 tasks
Youssef1313
Youssef1313 reviewed Jul 9, 2021
View reviewed changes
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
<value>An assembly has to opt into preview features before using them.</value>
</data>
<data name="DetectPreviewFeaturesMessage" xml:space="preserve">
<value>Using '{0}' requires opting into preview features. See https://aka.ms/dotnet-warnings/preview-features for more information.</value>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the link should be dropped in favor of the help link (which will contain the rule doc at some point)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! This is still being discussed. It's minor enough that I'm fine with it going in as is and once the docs team decided which way to go, I'll change the error message

Youssef1313
Youssef1313 reviewed Jul 9, 2021
View reviewed changes
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated
Comment on lines 1689 to 1691
<data name="StaticAndAbstractRequiresPreviewFeatures" xml:space="preserve">
<value>Using both static and abstract modifiers requires opting into preview features. See https://aka.ms/dotnet-warnings/preview-features for more information.</value>
</data>
Copy link
Member

Choose a reason for hiding this comment

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

Consider locking "static" and "abstract" from localization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are single quotes the way to do this? I don't see many other examples in the resx file. Changed it to 'static' and 'abstract' now.

Youssef1313
Youssef1313 reviewed Jul 9, 2021
View reviewed changes
src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureUnitTests.cs
Comment on lines +55 to +82
var csInput = @"
using System.Runtime.Versioning; using System;
namespace Preview_Feature_Scratch
{" +
@"
public class Program
{
static void Main(string[] args)
{
var a = new Fraction();
var b = [|+a|];
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this is split into two strings being concatenated?

Copy link
Contributor Author

@pgovind pgovind Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There used to be another var after the +, but it's since been removed, so the + is now unnecessary. It's just painful to remove the + for every single test case right now, so I just left it in (since it's just unit tests)

@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #5155 (b25b8ed) into release/6.0.1xx (77c6f07) will decrease coverage by 0.01%.
The diff coverage is 92.26%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5155      +/-   ##
===================================================
- Coverage            95.62%   95.60%   -0.02%     
===================================================
  Files                 1231     1233       +2     
  Lines               282186   283130     +944     
  Branches             16900    16964      +64     
===================================================
+ Hits                269827   270681     +854     
- Misses               10074    10134      +60     
- Partials              2285     2315      +30

@pgovind
Copy link
Contributor Author

pgovind commented Jul 12, 2021

Thank you for the review everyone! @mavasani @buyaa-n @Youssef1313 @AraHaan

@pgovind pgovind merged commit 1e2ab23 into dotnet:release/6.0.1xx Jul 12, 2021
This was referenced Jul 26, 2021
Closed
Open
Merged
@jmarolf jmarolf mentioned this pull request Jul 28, 2021
Merged
@jmarolf jmarolf mentioned this pull request Aug 16, 2021
Merged
This was referenced Aug 30, 2021
Merged
Merged
Merged
This was referenced Sep 10, 2021
Merged
Merged
@jmarolf jmarolf mentioned this pull request Jan 31, 2022
Closed
@carlossanlop carlossanlop mentioned this pull request Mar 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Youssef1313 Youssef1313 Youssef1313 left review comments

@AraHaan AraHaan AraHaan left review comments

@mavasani mavasani mavasani left review comments

@stephentoub stephentoub stephentoub left review comments

@buyaa-n buyaa-n buyaa-n approved these changes

Assignees

@mavasani mavasani

@buyaa-n buyaa-n

Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pgovind @AraHaan @buyaa-n @stephentoub @mavasani @Youssef1313