#45552 use string equals over string compare #5116

NewellClark · 2021-05-19T01:53:34Z

Note: I've gone ahead and added a GetFirstOrDefaultMemberWithParameterTypes extension method. I've found the existing GetFirstOrDefaultMemberWithParameterInfos method to be clunky while implementing other analyzers. Also, the ParameterInfo class that the older method uses only accepts INamedTypeSymbol, and not ITypeSymbol, which has caused problems on a couple of my other analyzers.

Add GetFirstOrDefaultMemberWithParameterTypes to utilities

…-string-equals-over-string-compare

Remove all references to SyntaxGenerator from analyzer class

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs

Youssef1313 · 2021-05-19T06:44:38Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs

+ if (!RequiredSymbols.TryGetSymbols(semanticModel.Compilation, out var symbols))
+ return;


How can this fail? This was already checked in the analyzer. Consider an assertion

Do we know for a fact that it is impossible for this to fail in the fixer after it succeeded in the analyzer? I figured better safe than sorry.

Agree this should be an assertion. If the symbols somehow disappear between the analyzer and the fixer something has gone horribly wrong.

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs

Youssef1313 · 2021-05-19T06:59:21Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs

+ var replacementNode = selector.GetReplacementExpression(operation, editor.Generator);
+ editor.ReplaceNode(operation.Syntax, replacementNode);
+ return editor.GetChangedDocument();
+ }, Resx.UseStringEqualsOverStringCompareCodeFixTitle);


Suggested change

}, Resx.UseStringEqualsOverStringCompareCodeFixTitle);

}, nameof(Resx.UseStringEqualsOverStringCompareCodeFixTitle));

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs

Analyzer and fixer were intertwined in unhealthy ways that also caused CI failures.

NewellClark · 2021-05-19T21:01:27Z

I ended up having to completely rewrite the analyzer because I wasn't aware that analyzers are not supposed to have any reference whatsoever to certain APIs, and originally both the analyzer and fixer had access to a type that used SyntaxGenerator which kept triggeringRS1022. I rewrote the analyzer so it no longer has any dependencies on the fixer.

codecov · 2021-05-19T21:17:00Z

Codecov Report

Merging #5116 (6562d14) into release/6.0.1xx (5ee4a10) will increase coverage by 0.00%.
The diff coverage is 94.45%.

@@                Coverage Diff                @@
##           release/6.0.1xx    #5116    +/-   ##
=================================================
  Coverage            95.59%   95.59%            
=================================================
  Files                 1220     1223     +3     
  Lines               279887   280320   +433     
  Branches             16818    16841    +23     
=================================================
+ Hits                267548   267965   +417     
- Misses               10077    10092    +15     
- Partials              2262     2263     +1

…-string-equals-over-string-compare

mavasani · 2021-05-21T04:30:49Z

Tagging @ryzngard for review.

…ing-compare

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs

ryzngard · 2021-05-21T19:03:09Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs

+ CreateChangedDocument,
+ nameof(Resx.UseStringEqualsOverStringCompareCodeFixTitle));
+ context.RegisterCodeFix(codeAction, context.Diagnostics);
+ return;


return is not needed here

@ryzngard It's a convention used in the repository (and dotnet/roslyn as well) to put an explicit return just before local functions

o.o who decided that? Because I definitely haven't been

Apparently this is @CyrusNajmabadi preferrence... I won't block a PR on style preference that isn't documented. You can keep if you like, and I'll just grumble in my corner alone :P

I've added https://github.com/dotnet/roslyn-analyzers/issues/5122 for further discussion

@mavasani has asked me to have a return before the local function definitions for clarity.

Yea, I followed up offline with him and a few others. I was unaware that was a practice we started. Hopefully we can get an analyzer to help the underlying issue, but for now this is correct.

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs

ryzngard · 2021-05-21T19:08:04Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs

+
+ // We know that the 'ignoreCase' argument in 'string.Compare(string, string, bool)' is a boolean literal
+ // because we've asserted that 'IsMatch' returns true.
+ var ignoreCaseLiteral = (ILiteralOperation)compareInvocation.Arguments.GetArgumentForParameterAtIndex(2).Value;


Is this also true for cases where a boolean variable is used? I haven't checked UseStringEqualsOverStringCompare.IsStringStringBoolCase to know if it handles that as well

bool CompareNames(Person p, bool ignoreCase) => string.Compare(Name, p.Name, ignoreCase)

The analyzer only flags cases where the boolean argument is a literal.

ryzngard · 2021-05-21T19:10:15Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs

+ // replace it with a call to 'string.Equals(x, y, StringComparison.CurrentCultureIgnoreCase)'.
+ // If the violation contains a call to 'string.Compare(x, y, false)' then we
+ // replace it with a call to 'string.Equals(x, y, StringComparison.CurrentCulture)'. 
+ var stringComparisonEnumMemberName = (bool)ignoreCaseLiteral.ConstantValue.Value ?


This should likely be Ordinal and OrdinalIgnoreCase based on guidelines

@ryzngard string.Compare is using CurrentCulture by default.

https://source.dot.net/#System.Private.CoreLib/String.Comparison.cs,200

So probably the codefix would want to keep the same semantics.

I don't fully agree. In general we don't want to change explicit semantics for code, but there's no way to know if the user intended to use CurrentCulture or not. When it's ambiguous, I think suggesting the public documented guidelines would be a good thing.

The examples in the issue specified that we use CurrentCulture and CurrentCultureIgnoreCase so we don't change semantics.

I don't see any explicit discussion in the issue about whether the replacement for string.Compare(s1, s2) should use CurrentCulture or Ordinal. That said, I'm not strong enough of the opinion that we should change the functional meaning for potential correction. Especially if others think it should be exact parity.

I will expand on my thought process for discussion, but consider this non-blocking:

https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings specifies

"Use overloads that explicitly specify the string comparison rules for string operations. Typically, this involves calling a method overload that has a parameter of type StringComparison."

If I were a user trying to update code to follow that guideline, how would I consider doing it? I think most cases I would add a Ordinal or OrdinalIgnoreCase as the intent for string comparison unless context dictates I should be using culture aware comparison. That also follows the guidance of

"Use StringComparison.Ordinal or StringComparison.OrdinalIgnoreCase for comparisons as your safe default for culture-agnostic string matching."

and

"Use comparisons with StringComparison.Ordinal or StringComparison.OrdinalIgnoreCase for better performance."

This fix is listed as Usage, but the key findings in the issue are centered around performance. Probably because nobody disagreed that string.Equals(...) is more readable than string.Compare(...) == 0.

If we are encouraging users to have mostly correct and performant code, or even for what I would consider to give as guidance as "probably correct" in most cases, we would use Ordinal or OrdinalIgnoreCase.

In my opinion, a code fix should only change the issue that its analyzer is reporting on. This analyzer is not flagging incorrect uses of CurrentCulture, it is flagging uses of a total-order comparison when a simple equality check was desired. I would not be opposed to an analyzer that flags usages of any Compare or Equals overload that doesn't specify the kind of comparison, but I don't think this analyzer should be changing semantics.

If analyzers that appear to be purely about style or performance start suggesting code fixes that quietly change semantics, as a developer I'm going to be much more wary of using code fixes.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs

ryzngard · 2021-05-21T19:14:48Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs

+ if (!RequiredSymbols.TryGetSymbols(context.Compilation, out var symbols))
+ return;
+ context.RegisterOperationAction(AnalyzeOperation, OperationKind.Binary);
+ return;


Suggested change

return;

Feel free to ignore per #5116 (comment)

Same as above. @mavasani has asked that I return before local function declarations.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs

ryzngard · 2021-05-21T19:21:11Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs

+ }
+
+ // No IOperation instances are being stored here.
+#pragma warning disable RS1008 // Avoid storing per-compilation data into the fields of a diagnostic analyzer


Is there a bug for this false positive on RS1008?

ryzngard · 2021-05-21T19:24:37Z

...yzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs

+
+ [Theory]
+ [MemberData(nameof(CS_StringCompareExpressionsTestData))]
+ public Task StringCompareResult_CompareToNonLiteralZero_NoDiagnostic_CS(string expression)


For const values we could also provide a diagnostic. I'd consider this an area for future improvement and not required for this PR

If I write an expression with a constant in it, I expect that changing the value initially assigned to the constant will change the value of the expression. The fixer removes the constant from the expression entirely, which could break if the constant is changed.

While true, it's an indicator of what I would consider hard to read code and potentially bad design. I don't know of a strong use case where we would want to encourage that, and analyzers are opinionated by design.

ryzngard

Looks good to me other than discussion points, but nothing blocking. Thanks for adding this!

NewellClark added 3 commits May 17, 2021 19:18

Create files and tests

78d78e9

Implement analyzer and fixer

c3408c7

Add GetFirstOrDefaultMemberWithParameterTypes to utilities

Run msbuild

6e503b6

NewellClark requested a review from a team as a code owner May 19, 2021 01:53

NewellClark added 3 commits May 18, 2021 22:04

Merge remote-tracking branch 'dotnet/release/6.0.1xx' into #45552-use…

13ff0a4

…-string-equals-over-string-compare

Fix build warnings

1963ab1

Fix more build warnings

db69ec2

Remove all references to SyntaxGenerator from analyzer class

Youssef1313 reviewed May 19, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed May 19, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed May 19, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed May 19, 2021

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed May 19, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs Outdated Show resolved Hide resolved

NewellClark marked this pull request as draft May 19, 2021 14:40

Restructure analyzer and fixer

7883135

Analyzer and fixer were intertwined in unhealthy ways that also caused CI failures.

NewellClark marked this pull request as ready for review May 19, 2021 20:43

Run msbuild

0c0d788

Merge remote-tracking branch 'dotnet/release/6.0.1xx' into #45552-use…

5f5b92d

…-string-equals-over-string-compare

mavasani assigned jmarolf and buyaa-n May 20, 2021

NewellClark added 2 commits May 20, 2021 13:27

Change diagnostic id to CA2251

579f683

Merge remote-tracking branch 'dotnet/release/6.0.1xx' into #45552-use…

390f6fa

…-string-equals-over-string-compare

mavasani assigned ryzngard May 21, 2021

Merge branch 'release/6.0.1xx' into #45552-use-string-equals-over-str…

4f0881f

…ing-compare

ryzngard reviewed May 21, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs Outdated Show resolved Hide resolved

ryzngard reviewed May 21, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs Outdated Show resolved Hide resolved

ryzngard reviewed May 21, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs Outdated Show resolved Hide resolved

ryzngard reviewed May 21, 2021

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs Outdated Show resolved Hide resolved

ryzngard reviewed May 21, 2021

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs Outdated Show resolved Hide resolved

ryzngard reviewed May 21, 2021

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs Outdated Show resolved Hide resolved

ryzngard reviewed May 21, 2021

View reviewed changes

Apply suggested changes

2371844

ryzngard approved these changes May 21, 2021

View reviewed changes

Assert if symbols can't be loaded in fixer

6562d14

ryzngard merged commit d464b5f into dotnet:release/6.0.1xx May 23, 2021

Youssef1313 mentioned this pull request May 24, 2021

Tracking issue for missing documentation of .NET6 CAxxxx rules dotnet/docs#23900

Closed

22 tasks

NewellClark mentioned this pull request May 31, 2021

Ca2251 dotnet/docs#24463

Merged

jeffhandley mentioned this pull request Aug 20, 2021

User Story: .NET 6.0 developers can verify more APIs for their correct usage to speed up their development dotnet/runtime#43617

Closed

NewellClark deleted the #45552-use-string-equals-over-string-compare branch August 27, 2021 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#45552 use string equals over string compare #5116

#45552 use string equals over string compare #5116

NewellClark commented May 19, 2021

Youssef1313 May 19, 2021 •

edited

Loading

NewellClark May 19, 2021

ryzngard May 21, 2021

Youssef1313 May 19, 2021

NewellClark commented May 19, 2021

codecov bot commented May 19, 2021 •

edited

Loading

mavasani commented May 21, 2021

ryzngard May 21, 2021

Youssef1313 May 21, 2021

ryzngard May 21, 2021

ryzngard May 21, 2021

ryzngard May 21, 2021

NewellClark May 21, 2021 •

edited

Loading

ryzngard May 21, 2021

ryzngard May 21, 2021

NewellClark May 21, 2021

ryzngard May 21, 2021

Youssef1313 May 21, 2021

ryzngard May 21, 2021

NewellClark May 21, 2021

ryzngard May 21, 2021

NewellClark May 21, 2021 •

edited

Loading

ryzngard May 21, 2021

ryzngard May 21, 2021

NewellClark May 21, 2021

ryzngard May 21, 2021

NewellClark May 21, 2021

ryzngard May 21, 2021

NewellClark May 21, 2021 •

edited

Loading

ryzngard May 21, 2021

ryzngard left a comment

		if (!RequiredSymbols.TryGetSymbols(semanticModel.Compilation, out var symbols))
		return;

	}, Resx.UseStringEqualsOverStringCompareCodeFixTitle);
	}, nameof(Resx.UseStringEqualsOverStringCompareCodeFixTitle));

#45552 use string equals over string compare #5116

#45552 use string equals over string compare #5116

Conversation

NewellClark commented May 19, 2021

Youssef1313 May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewellClark commented May 19, 2021

codecov bot commented May 19, 2021 • edited Loading

Codecov Report

mavasani commented May 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewellClark May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewellClark May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewellClark May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzngard left a comment

Choose a reason for hiding this comment

Youssef1313 May 19, 2021 •

edited

Loading

codecov bot commented May 19, 2021 •

edited

Loading

NewellClark May 21, 2021 •

edited

Loading

NewellClark May 21, 2021 •

edited

Loading

NewellClark May 21, 2021 •

edited

Loading