Roslyn-analyzers: Inconsistency in CA1307 warnings when a project targets multiple-frameworks

[kartheekp-ms]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v2.9.8

Diagnostic ID

Example: CA1307

Repro steps

In NuGet.Client repo, NuGet.Packaging project targets multiple target frameworks (net472 and netstandard2.0). When we added a new tfm netstandard2.1 in a feature branch then we have noticed a new warning raised by CA1307 analyzer.
I am unable to reproduce this issue in a sample class library project that targets net472, netstandard2.0 and netstandard2.1 frameworks There could be a possibility that we are missing something but nothing was obvious.

Expected behavior

CA1307 raises warning consistently across various tfms.

Actual behavior

For e.g. In dev branch with net472 and netstandard2.0 the following line of code doesn't report any violation.
https://github.com/NuGet/NuGet.Client/blob/788bc01a1b063a37841cdd6d035feb320e90e475/src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PhysicalPackageFile.cs#L123

Where as in a feature branch with 3 tfms (net472, netstandard2.0 and netstandard2.1) raises a new warning for only netstandard2.1 project as highlighted below.

https://github.com/NuGet/NuGet.Client/blob/dev-feature-xplatVerification/src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PhysicalPackageFile.cs#L123

We suppressed all the existing warnings in the dev branch but this warning was not included in the GlobalSupression.cs because the analyzer did not flag this problem initially. We are still using 2.9.8 version in the feature branch where new tfm was added and started seeing these warnings.

Our repo has TreatWarningsAsErrors to true. These warnings are causing build failures locally and in CI/CD. We have changed the severity of this rule to Info to unblock our builds.

We have also set the rule severity of another 7 analyzers due to another issue tracked here

[mavasani]

@kartheekp-ms This analyzer works by first collecting the available set of APIs on string type in the target TFM. If it finds that you are calling a string API without StringComparison parameter, but the TFM has an overload for that API with exactly the same parameters + an additional StringComparison parameter, then it reports a CA1307. It is possible that for a multi-TFM project, the additional overload with StringComparison parameter is present on only a subset of target TFMs. However, the analyzer executes once per-TFM, so is not aware about this and reports the diagnostic only on the applicable TFM. I think that is the case with GetHashCode API as GetHashCode(StringComparison) is only present on netstandard2.1. This is by design for the analyzer. You have couple of options here:

1. Add an inline pragma/SuppressMessageAttribute based source suppression for such violations that do not apply to all target TFMs.
2. Add a conditional directive such as follows to your sources:

#if NETCOREAPP2_1
   hash = SourcePath.GetHashCode(StringComparison.Ordinal);
#else
   hash = SourcePath.GetHashCode();
#endif

[zivkan]

@mavasani I don't know if this is the correct place, as technically my question is now out of scope of the title of this issue.

However, it seems to me like CA1307 is just wrong for string.GetHashCode(). I'd expect it to use StringComparison.Ordinal by default. Unfortunately I can't find the implementation of it in dotnet/runtime, as there are over 1800 results for GetHashCode, and my attempt at finding the source for the string class/struct wasn't more successful.

I thought the behaviour of string.GetHashCode was always culture-independent in the past, so while it's possible that changed in .NET 5, or whatever TFM added the string comparator overload, I would be very surprised.

[mavasani]

@zivkan I think your concern is well captured in https://github.com/dotnet/roslyn-analyzers/issues/2581.

[mavasani]

Closing this issue by design.

[Evangelink]

In addition to @mavasani comment regarding the pragma if you could also create an helper class/method to ensure the if is done at only one place.

[mavasani]

That's a great suggestion @Evangelink!