-
Notifications
You must be signed in to change notification settings - Fork 463
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
[New Analyzer/Fixer] Prefer static HashData method over ComputeHash #4797
[New Analyzer/Fixer] Prefer static HashData method over ComputeHash #4797
Conversation
b2cc096
to
af5ce67
Compare
I'm confused about the target branch. targeting master first for the CI |
8d2bce3
to
b97cc5a
Compare
Codecov Report
@@ Coverage Diff @@
## main #4797 +/- ##
==========================================
+ Coverage 95.52% 95.56% +0.03%
==========================================
Files 1275 1280 +5
Lines 292855 296596 +3741
Branches 17691 18014 +323
==========================================
+ Hits 279757 283432 +3675
- Misses 10663 10716 +53
- Partials 2435 2448 +13 |
This should target a release/6.0.1xx branch. But not sure if it should target preview1 or preview2 branch. @mavasani to confirm the correct branch. |
return CodeAction.Create( | ||
title: MicrosoftNetCoreAnalyzersResources.PreferHashDataOverComputeHashAnalyzerTitle, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mavasani @Evangelink Should the title for the codefix start with "Use" instead (i.e. different from analyzer title)?
|
||
return editor.GetChangedDocument(); | ||
}, | ||
equivalenceKey: MicrosoftNetCoreAnalyzersResources.PreferHashDataOverComputeHashAnalyzerTitle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, @sharwell said somewhere that equivalenceKey shouldn't be localized. So you should probably use nameof
here. (I'm not sure though what problems can this cause)
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Show resolved
Hide resolved
...ualBasic/Microsoft.NetCore.Analyzers/Performance/BasicPreferHashDataOverComputeHash.Fixer.vb
Outdated
Show resolved
Hide resolved
Namespace Microsoft.NetCore.VisualBasic.Analyzers.Performance | ||
<ExportCodeFixProvider(LanguageNames.VisualBasic)> | ||
Public Class BasicPreferHashDataOverComputeHashFixer : Inherits PreferHashDataOverComputeHashFixer | ||
Protected Overrides Function TryGetCodeFixer(computeHashNode As SyntaxNode, bufferArgNode As SyntaxNode, nodeToRemove As SyntaxNode, <NotNullWhen(True)> ByRef codeFixer As ApplyCodeFixAction) As Boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the NotNullWhen attribute is necessary in VB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure either. VS generated this
...zers/UnitTests/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHashTests.cs
Outdated
Show resolved
Hide resolved
...ualBasic/Microsoft.NetCore.Analyzers/Performance/BasicPreferHashDataOverComputeHash.Fixer.vb
Outdated
Show resolved
Hide resolved
{|#2:var sha256 = SHA256.Create();|} | ||
byte[] digest = {|#0:sha256.ComputeHash({|#1:buffer|})|}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little bit confused why there are 3 diagnostics here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm capturing AdditionalLocations for the fixer. Those show up in the diagnostics
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Outdated
Show resolved
Hide resolved
<value>Prefer static 'HashData' method over 'ComputeHash'</value> | ||
</data> | ||
<data name="PreferHashDataOverComputeHashAnalyzerDescription" xml:space="preserve"> | ||
<value>It is simpler to use the static 'HashData' method over 'ComputeHash'.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this description should also give a short explanation of why it is simpler.
@@ -1308,6 +1308,18 @@ Marshalling of 'StringBuilder' always creates a native buffer copy, resulting in | |||
|CodeFix|False| | |||
--- | |||
|
|||
## [CA1842](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1842): Prefer static 'HashData' method over 'ComputeHash' | |||
|
|||
It is simpler to use the static 'HashData' method over 'ComputeHash'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about expanding the description to explain why.
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Outdated
Show resolved
Hide resolved
Is it alright if I just squashed the commits into 1? It's getting messy imo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...nalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.Fixer.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.cs
Outdated
Show resolved
Hide resolved
...ualBasic/Microsoft.NetCore.Analyzers/Performance/BasicPreferHashDataOverComputeHash.Fixer.vb
Outdated
Show resolved
Hide resolved
it is case-insensitive
Hooray! Thanks for the contribution @wzchua. 👍 |
Resolves dotnet/runtime#40579