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

Tiny Refactoring CA1824 #5124

Merged
merged 4 commits into from
Jun 28, 2021
Merged

Tiny Refactoring CA1824 #5124

merged 4 commits into from
Jun 28, 2021

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented May 23, 2021

Closes #5123

First commit:

Main idea is to move the line:

            INamedTypeSymbol? generatedCode = model.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCodeDomCompilerGeneratedCodeAttribute);

from CheckResxGeneratedFile to RegisterCompilationStartAction and bail-out early if needed.

Extra cleanups:

  • Avoid unneeded type checking and cast the node directly to AttributeSyntax. The cast can't fail because the registered node action is for SyntaxKind.Attribute.
  • Rename nc to context

Second commit:

Move TryCheckNeutralResourcesLanguageAttribute to compilation start to avoid unnecessary callbacks when we're sure there is no diagnostics to be produced.

@Youssef1313 Youssef1313 requested a review from a team as a code owner May 23, 2021 09:06
@codecov
Copy link

codecov bot commented May 23, 2021

Codecov Report

Merging #5124 (a35b306) into main (5968bec) will increase coverage by 0.00%.
The diff coverage is 84.84%.

@@           Coverage Diff           @@
##             main    #5124   +/-   ##
=======================================
  Coverage   95.53%   95.54%           
=======================================
  Files        1202     1202           
  Lines      275961   276035   +74     
  Branches    16641    16641           
=======================================
+ Hits       263648   263731   +83     
- Misses      10119    10124    +5     
+ Partials     2194     2180   -14     

@mavasani mavasani self-requested a review June 24, 2021 16:55
@mavasani mavasani self-assigned this Jun 24, 2021
{
if (!CheckAttribute(nc.Node))
var attributeSyntax = (AttributeSyntax)context.Node;
Copy link
Member

Choose a reason for hiding this comment

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

Previous code was having the possibility that the Node isn't a AttributeSyntax, it also doesn't look possible but I am wondering if that was done for a specific reason or just by "node is generic so let's work in the generic case".

Copy link
Member Author

Choose a reason for hiding this comment

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

@Evangelink It should always be AttributeSyntax since we do RegisterSyntaxNodeAction(...., SyntaxKind.Attribute)

@@ -53,9 +53,26 @@ public override void Initialize(AnalysisContext context)

context.RegisterCompilationStartAction(cc =>
{
var hasResource = false;
INamedTypeSymbol? generatedCode = cc.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCodeDomCompilerGeneratedCodeAttribute);
Copy link
Member

Choose a reason for hiding this comment

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

[cosmetic][minor] You could use the TryGetXXX instead of doing get then null check.

@@ -53,9 +53,26 @@ public override void Initialize(AnalysisContext context)

context.RegisterCompilationStartAction(cc =>
Copy link
Member

Choose a reason for hiding this comment

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

As you have done some extra refactorings, it might be worth also renaming cc into context.

@Youssef1313
Copy link
Member Author

Thanks for review @Evangelink. I addressed your feedback.

@mavasani mavasani merged commit 3ab8d26 into dotnet:main Jun 28, 2021
@Youssef1313 Youssef1313 deleted the refactor-ca1824 branch June 28, 2021 09:14
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.

GetOrCreateTypeByMetadataName is called on every attribute syntax callback
3 participants