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

Enable string comparison FxCop rules #24567

Closed
wants to merge 1 commit into from

Conversation

Pilchie
Copy link
Member

@Pilchie Pilchie commented Aug 4, 2020

No description provided.

@Pilchie
Copy link
Member Author

Pilchie commented Aug 4, 2020

Tagging @dotnet/aspdoi

[*.cs]
dotnet_diagnostic.CA1307.severity = none
dotnet_diagnostic.CA1308.severity = none
dotnet_diagnostic.CA1309.severity = none
Copy link
Member Author

Choose a reason for hiding this comment

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

We should eventually fix the 3 violations in this directory and remove this, but I figured I'd at least push something that runs the "restore" step.

@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Aug 5, 2020
@pranavkm
Copy link
Contributor

pranavkm commented Aug 6, 2020

I pinged @GrabYourPitchforks about this. It looks like there are some open issues with the analyzer: dotnet/roslyn-analyzers#2581. The build error we're seeing is also incorrect:

The behavior of 'string.GetHashCode()' could vary based on the current user's locale settings.

Should we hold off on this?

@Pilchie
Copy link
Member Author

Pilchie commented Aug 6, 2020

That linked issue does seem pretty significant to me. We probably should wait for it to be fixed before enabling that rule.

@mavasani - any idea of timelines for fixing dotnet/roslyn-analyzers#2581.

@mavasani
Copy link

mavasani commented Aug 6, 2020

@Pilchie I'll try to pick it up early next week.

@mavasani
Copy link

mavasani commented Aug 18, 2020

@@ -42,3 +42,8 @@ end_of_line = lf

[*.{razor,cshtml}]
charset = utf-8-bom

[*.cs]
dotnet_diagnostic.CA1307.severity = error

Choose a reason for hiding this comment

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

I think you want CA1310 here instead of CA1307 from the latest package.
CA1307 will continue to have its old behavior of being a readability/maintainability rule which provides the recommendation regardless of the default string comparison used by the flagged methid. It has been disabled by default as it can be noisy if you only care about correctness. CA1310 is the new rule which only flags string methods which are known to default to culture specific string comparisons, and is the correctness rule which will not be noisy.

@Pilchie
Copy link
Member Author

Pilchie commented Sep 15, 2020

Is that fix only in 3.3.1? Do we know when that will be on nuget?

@mavasani
Copy link

Is that fix only in 3.3.1? Do we know when that will be on nuget?

Yes, that is correct. No timelines known for 3.3.1. We are even considering completely deprecating FxCopAnalyzers NuGet in favor of equivalent CA analyzers that are now inserted into the .NET SDK: https://docs.microsoft.com/dotnet/fundamentals/productivity/code-analysis

@Pilchie
Copy link
Member Author

Pilchie commented Oct 15, 2020

@pranavkm is this completed superseded by #26656? Should I just close it?

@pranavkm
Copy link
Contributor

Yup. Closing this

@pranavkm pranavkm closed this Oct 15, 2020
@Pilchie
Copy link
Member Author

Pilchie commented Oct 15, 2020

One fewer stale PR! Thanks @pranavkm - and thanks for following through on enabling the rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants