Fixer for https://github.com/dotnet/runtime/issues/33785 by pgovind · Pull Request #3610 · dotnet/roslyn-analyzers

pgovind · 2020-05-07T18:03:57Z

Offers a code-fix for the following cases when they appear inside a binary expression such as if (string.IndexOf() == -1):

String.IndexOf(char) -> string.Contains(char)
String.IndexOf(string) -> string.Contains(string, StringComparison.CurrentCulture)
String.IndexOf(string/char, StringComparison.Ordinal) -> string.Contains(string/char)
String.IndexOf(string/char, NON StringComparison.Ordinal) -> string.Contains(string/char, NON StringComparison.Ordinal)

The analyzer also tags the following case, but @mavasani recommended against offering a code-fix here(since it involves more data flow analysis than roslyn provides currently). Note: The analyzer will still fire:

int index = str.IndexOf("AString");
if (index == -1)

Cases tagged by dotnet/roslyn-analyzers#3610

codecov · 2020-05-11T20:07:09Z

Codecov Report

Merging #3610 into master will decrease coverage by 0.02%.
The diff coverage is 89.94%.

@@            Coverage Diff             @@
##           master    #3610      +/-   ##
==========================================
- Coverage   95.18%   95.15%   -0.03%     
==========================================
  Files        1073     1075       +2     
  Lines      243974   244377     +403     
  Branches    15867    16011     +144     
==========================================
+ Hits       232218   232537     +319     
- Misses      10003    10019      +16     
- Partials     1753     1821      +68

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/PreferStringContainsOverIndexOf.Fixer.cs

mavasani · 2020-05-13T15:16:55Z

...lyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/PreferStringContainsOverIndexOfTests.cs

Can we add tests with named arguments with different combination/ordering of arguments in input:

if ([|str.IndexOf(value: ""This"", comparison)" + operatorKind + value + @"|])

if ([|str.IndexOf(""This"", comparisonType: comparison)" + operatorKind + value + @"|])

if ([|str.IndexOf(value: ""This"", comparisonType: comparison)" + operatorKind + value + @"|])

if ([|str.IndexOf(comparisonType: comparison, value: ""This"")" + operatorKind + value + @"|])

Done. The only issue was for case 3 in VB. I've commented in the unit test

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/PreferStringContainsOverIndexOf.Fixer.cs

mavasani · 2020-05-13T15:47:05Z

int index = str.IndexOf("AString");
if (index == -1)

Can we file a follow-up issue to handle the simplest case here where index computation is immediately followed by an index check in the next statement? I think it maybe useful, even if it is implemented only for C#. We can certainly wait for feedback before implementing it.

codecov · 2020-05-20T02:43:55Z

Codecov Report

Merging #3610 into master will increase coverage by 0.48%.
The diff coverage is 89.94%.

@@            Coverage Diff             @@
##           master    #3610      +/-   ##
==========================================
+ Coverage   94.67%   95.15%   +0.48%     
==========================================
  Files        1073     1075       +2     
  Lines      244477   244377     -100     
  Branches    14771    16011    +1240     
==========================================
+ Hits       231449   232537    +1088     
+ Misses      10999    10019     -980     
+ Partials     2029     1821     -208

pgovind · 2020-05-20T05:41:47Z

...lyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/PreferStringContainsOverIndexOfTests.cs

+    Class TestClass
+        Public Sub Main()
+            Dim Str As String = ""This is a statement""
+            If Not Str.Contains(value:= " + startQuote + input + startQuote + vbCharLiteral + @", comparisonType:= System.StringComparison.OrdinalIgnoreCase) Then


Note that the arguments are reversed here. I don't understand why this happens, but in the debugger I can see that invocationOperation.SyntaxNode (in the fixer code) has its arguments in the (comparisonType, value) order, but invocationOperation.Arguments returns it as (value, comparisonType). It seems to work fine for C# though

@pgovind Argument order in the IOperation world is in the order of runtime evaluation, does not depend on source ordering, see https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/Generated/Operations.Generated.cs#L639-L646

Sounds good - feel free to file a tracking issue for VB. We can take a look at it based on customer feedback.

codecov · 2020-05-20T20:39:09Z

Codecov Report

Merging #3610 into master will increase coverage by 0.48%.
The diff coverage is 89.94%.

@@            Coverage Diff             @@
##           master    #3610      +/-   ##
==========================================
+ Coverage   94.67%   95.15%   +0.48%     
==========================================
  Files        1073     1075       +2     
  Lines      244504   244377     -127     
  Branches    14773    16011    +1238     
==========================================
+ Hits       231480   232537    +1057     
+ Misses      11000    10019     -981     
+ Partials     2024     1821     -203

codecov · 2020-05-20T21:24:49Z

Codecov Report

Merging #3610 into master will increase coverage by 0.00%.
The diff coverage is 98.21%.

@@           Coverage Diff            @@
##           master    #3610    +/-   ##
========================================
  Coverage   94.67%   94.67%            
========================================
  Files        1073     1074     +1     
  Lines      244504   244939   +435     
  Branches    14773    14787    +14     
========================================
+ Hits       231480   231896   +416     
- Misses      11000    11010    +10     
- Partials     2024     2033     +9

pgovind · 2020-05-20T21:29:21Z

Thanks for the review @mavasani

pgovind requested a review from a team as a code owner May 7, 2020 18:03

pgovind requested review from carlossanlop, jeffhandley, mavasani and stephentoub May 7, 2020 18:04

pgovind pushed a commit to pgovind/runtime that referenced this pull request May 7, 2020

Apply code-fixers

c83ade0

Cases tagged by dotnet/roslyn-analyzers#3610

pgovind mentioned this pull request May 11, 2020

Apply code fixes from recently added analyzers dotnet/runtime#36097

Merged

mavasani reviewed May 13, 2020

View reviewed changes

This was referenced May 17, 2020

Prefer string.Contains() over string.IndexOf() != -1 dotnet/runtime#33785

Closed

Add logic to properly honor naming policy when serializing flag enums dotnet/runtime#36726

Merged

Prashanth Govindarajan added 4 commits May 19, 2020 19:41

Fixer and unit tests

73cb73e

sq

707f439

Preserve trivia

1bb68f6

Address feedback

57d5440

pgovind force-pushed the StringContainsFixer branch from dd6d97d to 57d5440 Compare May 20, 2020 02:42

pgovind commented May 20, 2020

View reviewed changes

mavasani approved these changes May 20, 2020

View reviewed changes

Prashanth Govindarajan added 2 commits May 20, 2020 12:30

sq

69f8d64

Merge remote-tracking branch 'upstream/master' into StringContainsFixer

8b1d33f

Update the xlf files

cc92882

pgovind merged commit 1bc66b4 into dotnet:master May 20, 2020

This was referenced May 20, 2020

Consider extending the PreferStringContainsOverIndexOf code fixer #3657

Open

Named arguments are reversed when string.IndexOf is replaced by string.Contains #3655

Open

carlossanlop mentioned this pull request Apr 14, 2023

CA2249 (prefer Contains instead of IndexOf) does not trigger when IndexOf comparison operator is != #6587

Closed

Conversation

pgovind commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 11, 2020

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mavasani May 13, 2020

Choose a reason for hiding this comment

Uh oh!

pgovind May 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mavasani commented May 13, 2020

Uh oh!

codecov bot commented May 20, 2020

Codecov Report

Uh oh!

pgovind May 20, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani May 20, 2020

Choose a reason for hiding this comment

Uh oh!

pgovind May 20, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 20, 2020

Codecov Report

Uh oh!

codecov bot commented May 20, 2020

Codecov Report

Uh oh!

pgovind commented May 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pgovind commented May 7, 2020 •

edited

Loading

mavasani May 20, 2020 •

edited

Loading