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

Add CA1872: Prefer 'Convert.ToHexString' over 'BitConverter.ToString' #6967

Merged
merged 13 commits into from Mar 19, 2024

Conversation

mpidash
Copy link
Contributor

@mpidash mpidash commented Sep 30, 2023

Fixes dotnet/runtime#81796.

This analyzer detects the usage of the pattern BitConverter.ToString(bytes).Replace("-", "") to convert an array of bytes to an uppercase hex string (without hyphens in between) and replaces it with a call to Convert.ToHexString(bytes).
The analyzer will also try to preserve chaining ToLower* in between (before and after string.Replace) for a lowercase hex string.

I have found 2 warnings in dotnet/runtime, 1 warning in dotnet/roslyn and no warnings in dotnet/aspnetcore and dotnet/roslyn-analyzers:

  1. runtime\src\coreclr\tools\aot\ILCompiler.Compiler\Compiler\XmlObjectDumper.cs(73,20): info CA1872: Prefer 'System.Convert.ToHexString(byte[])' over 'System.BitConverter.ToString(byte[])'
  2. runtime\src\coreclr\tools\Common\Compiler\NativeAotNameMangler.cs(125,38): warning CA1872: Prefer 'System.Convert.ToHexString(byte[])' over 'System.BitConverter.ToString(byte[])'
  3. roslyn\src\Tools\BuildValidator\CompilationDiff.cs(404,46): info CA1872: Prefer 'System.Convert.ToHexString(byte[])' over 'System.BitConverter.ToString(byte[])

This analyzer detects the usage of the pattern
`BitConverter.ToString(bytes).Replace("-", "")` to convert an array of
bytes to an uppercase hex string (without hyphens in between) and
replaces it with a call to `Convert.ToHexString(bytes)`.
The analyzer will also try to preserve chaining `ToLower*` in between
for a lowercase hex string.
@mpidash mpidash requested a review from a team as a code owner September 30, 2023 13:56
@codecov
Copy link

codecov bot commented Sep 30, 2023

Codecov Report

Attention: Patch coverage is 98.25386% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 96.47%. Comparing base (de3a920) to head (f3fee4a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6967      +/-   ##
==========================================
- Coverage   96.47%   96.47%   -0.01%     
==========================================
  Files        1436     1439       +3     
  Lines      342881   344370    +1489     
  Branches    11292    11324      +32     
==========================================
+ Hits       330797   332230    +1433     
- Misses       9230     9272      +42     
- Partials     2854     2868      +14     

@mpidash
Copy link
Contributor Author

mpidash commented Mar 8, 2024

@buyaa-n I've addressed your feedback and also tried to improve the messages, PTAL 😸.

Here are the findings with the new message template:

W:\roslyn\src\Tools\BuildValidator\CompilationDiff.cs(404,46): info CA1872: Prefer 'System.Convert.ToHexString(byte[])' over call chains based on 'System.BitConverter.ToString(byte[])'

W:\runtime\src\coreclr\tools\Common\Compiler\NativeAotNameMangler.cs(111,38): warning CA1872: Prefer 'System.Convert.ToHexString(byte[])' over call chains based on 'System.BitConverter.ToString(byte[])' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1872) [W:\runtime\src\coreclr\tools\aot\ILCompiler.Compiler\ILCompiler.Compiler.csproj]
W:\runtime\src\coreclr\tools\aot\ILCompiler.Compiler\Compiler\XmlObjectDumper.cs(73,20): warning CA1872: Prefer 'System.Convert.ToHexStringLower(byte[])' over call chains based on 'System.BitConverter.ToString(byte[])' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1872) [W:\runtime\src\coreclr\tools\aot\ILCompiler.Compiler\ILCompiler.Compiler.csproj]
W:\runtime\src\libraries\System.Private.Xml\tests\XmlNodeReader\System.Xml.XmlNodeReader.Tests\XmlNodeReaderReadTests.cs(183,41): warning CA1872: Prefer 'System.Convert.ToHexString(byte[])' over call chains based on 'System.BitConverter.ToString(byte[])' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1872) [W:\runtime\src\libraries\System.Private.Xml\tests\System.Private.Xml.Tests.csproj]
W:\runtime\src\libraries\System.Private.Xml\tests\XmlNodeReader\System.Xml.XmlNodeReader.Tests\XmlNodeReaderReadTests.cs(233,46): warning CA1872: Prefer 'System.Convert.ToHexString(byte[])' over call chains based on 'System.BitConverter.ToString(byte[])' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1872) [W:\runtime\src\libraries\System.Private.Xml\tests\System.Private.Xml.Tests.csproj]

And an example of an applied code fix:

image
image
image

@gewarren: It would be great if you could also take a look at the resource strings, thanks!

@mpidash mpidash requested a review from buyaa-n March 8, 2024 22:57
Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

I left some feedback on the resource strings. Thanks for tagging me!

@buyaa-n
Copy link
Member

buyaa-n commented Mar 15, 2024

@buyaa-n I've addressed your feedback and also tried to improve the messages, PTAL 😸.

Thank you @mpidash, the messages and test coverage look great to me, functional testing worked well too, this time I looked into the code more and left few more feedback.

@mpidash
Copy link
Contributor Author

mpidash commented Mar 16, 2024

Thanks for the review @buyaa-n! I've addressed your feedback, PTAL.

Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Thank you @mpidash, LGTM.

It's good to go after the #6967 (comment) done

@buyaa-n buyaa-n merged commit 0c86a45 into dotnet:main Mar 19, 2024
11 checks passed
@mpidash mpidash deleted the issue-81796 branch March 19, 2024 20:47
arkalyanms pushed a commit that referenced this pull request Mar 22, 2024
…#6967)

* Add CA1872: Prefer 'Convert.ToHexString' over 'BitConverter.ToString'

This analyzer detects the usage of the pattern
`BitConverter.ToString(bytes).Replace("-", "")` to convert an array of
bytes to an uppercase hex string (without hyphens in between) and
replaces it with a call to `Convert.ToHexString(bytes)`.
The analyzer will also try to preserve chaining `ToLower*` in between
for a lowercase hex string.

* Use span overload when replacing call with two arguments

* Use Convert.ToHexStringLower if available

* Improve resource strings

* Improve description resource string

* Remove redundant helper methods

* Change invocation analyze order and remove duplicate work in fixer

* Remove temporary Net90 reference assembly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Analyzer] BitConverter.ToString(bytes).Replace("-", "") to Convert.ToHexString
3 participants