(4/4) Prefer 'AsSpan' over 'Substring' analyzer #4806

NewellClark · 2021-02-09T05:44:38Z

Fixes issue #33784.
Violations found in dotnet/runtime:

System.Private.DataContractSerialization (PR)
System.Private.Xml (PR)

Edit: ambiguous cases are now reported, but not fixed.

I'm marking this as a draft for now because I wanted to ask what we should do when only some of the Substring calls can be converted to AsSpan (currently it only reports when all Substring invocations in the call can be converted).

using Roschar = System.ReadOnlySpan<char>
// No overload takes 3 roschars
public void Consume(string a, string b, string c) { }
public void Consume(Roschar a, string b, string c) { }
public void Consume(string a, Roschar b, Roschar c) { }

public void NotAmbiguous(string foo)
{
    Consume(foo.Substring(1), foo.Substring(2), foo.Substring(3));

    // Candidates
    Consume(foo.AsSpan(1), foo.Substring(2), foo.Substring(3));    // Option 1
    Consume(foo.Substring(1), foo.AsSpan(2), foo.AsSpan(3));       // Option 2
}

Should we mirror overload resolution and have the fixer choose the "best" option here? Seems like the right thing to do. But what about when it's ambiguous?

using Roschar = System.ReadOnlySpan<char>;

public void Consume(string left, string right) { }
public void Consume(Roschar left, string right) { }
public void Consume(string left, Roschar right) { }

public void Ambiguous(string foo)
{
    Consume(foo.Substring(1), foo.Substring(2));

    // Candidates
    Consume(foo.AsSpan(1), foo.Substring(2));    // Option 1
    Consume(foo.Substring(1), foo.AsSpan(2));    // Option 2
 }

When it's ambiguous, should we not report at all? Or should we report, but not fix?

Add 'GetFirstOrDefaultMemberWithParameterTypes' helper method

…refer-AsSpan-over-Substring-analyzer

codecov · 2021-02-09T06:11:11Z

Codecov Report

Merging #4806 (34dc2d6) into release/6.0.1xx (92a247f) will increase coverage by 0.01%.
The diff coverage is 97.14%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #4806      +/-   ##
===================================================
+ Coverage            95.58%   95.59%   +0.01%     
===================================================
  Files                 1215     1220       +5     
  Lines               278104   279856    +1752     
  Branches             16768    16815      +47     
===================================================
+ Hits                265824   267529    +1705     
- Misses               10037    10077      +40     
- Partials              2243     2250       +7

sharwell · 2021-02-09T15:08:33Z

Is this a common case?

I would typically prefer one of the following:

Report a diagnostic and provide two possible fixes
Report a diagnostic but do not provide a fix

NewellClark · 2021-02-09T19:26:45Z

Report a diagnostic but do not provide a fix

Sounds reasonable. I don't think it's super common, so I'll do this for now, unless we find evidence that it's a common scenario.

Previously, fixer would only fix cases where all Substring calls could be converted. Now, it fixes all cases where there is a single unambiguous best overload.

- Handle cases where not every Substring invocation can be converted to AsSpan - Add tests for inaccessible overloads

NewellClark · 2021-02-13T21:56:39Z

@sharwell I've implemented it so that ambiguous cases are reported, but not fixed.

Fix issue where 'System' namespace would be imported unnecessarily when violations were inside a System namespace declaration, or where the System import was inside a namespace declaration. This issue was encountered in dotnet/runtime

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/PreferAsSpanOverSubstring.Fixer.cs

Evangelink · 2021-02-18T21:34:06Z

...nalyzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpPreferAsSpanOverSubstring.Fixer.cs

+        private protected override void ReplaceInvocationMethodName(SyntaxEditor editor, SyntaxNode memberInvocation, string newName)
+        {
+            var cast = (InvocationExpressionSyntax)memberInvocation;
+            var memberAccessSyntax = (MemberAccessExpressionSyntax)cast.Expression;


Are we sure we can't be on something else than MemberAccessExpresionSyntax here? Maybe a member binding or something like this.

The only way this could be a MemberBindingExpressionSyntax is if the substring invocation is conditional, foo?.Substring(1).
This can't possibly be true because the analyzer detects IArgumentOperations that are IInvocationOperations (after stripping implicit conversions), and the syntax of foo?.Substring(1) is a ConditionalAccessExpressionSyntax, and not an InvocationExpressionSyntax.
I'll add some tests ensuring the analyzer does not report conditional substring invocations, and I'll add some assertions and comments to document this invariant.
I'll also rename the method to ReplaceNonConditionalInvocationMethodName

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

Evangelink · 2021-02-18T21:36:40Z

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

+
+            context.RegisterOperationBlockStartAction(OnOperationBlockStart);
+
+            void OnOperationBlockStart(OperationBlockStartAnalysisContext context)


[minor] Method could be made static.

It captures symbols (used on line 63).

My bad, it's not easy to see captured variables in github.

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

Evangelink · 2021-02-18T21:40:53Z

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

+            }
+        }
+
+        internal static IOperation WalkDownImplicitConversions(IOperation operation)


I am wondering if we should update the WalkDownConversions helper to have an optional predicate function Func<IConversionOperation, bool>.

I think that would be a good idea. I've wanted that function on several other analyzers. While we're talking about upgrading helpers, I've also wanted a GetFirstOrDefaultWithParamater overload that takes ITypeSymbols instead of ParameterInfos.

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

-Add tests ensuring conditional substring invocations are not reported -Add separate resource for code fix title -Remove period from method name

-Inline diagnostic descriptors -Add WalkDownConversion overload that takes a predicate. -Use more descriptive names for RequiredSymbols members.

Evangelink · 2021-02-19T18:45:29Z

src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt

@@ -12,7 +12,7 @@
 Design: CA2210, CA1000-CA1070
 Globalization: CA2101, CA1300-CA1310
 Mobility: CA1600-CA1601
-Performance: HA, CA1800-CA1838
+Performance: HA, CA1800-CA1842


Why the jump from 38 to 42?

Because my other analyzer PRs use CA1839-CA1841. I wasn't sure how it's typically handled when there are multiple outstanding PRs that both want to add an analyzer to the same diagnostic category.

Makes sense, maybe you could add a [X/Y] prefix on your PRs so that we merge them in the right order.

I've added prefixes here (1/4), here (2/4), here (3/4), and this one is 4/4. I think some of them may be in different milestones. Do you want me to change the IDs around so that earlier milestones get merged first?

src/Utilities/Compiler/Extensions/IEnumerableOfIMethodSymbolExtensions.cs

Evangelink · 2021-02-19T18:47:31Z

src/Utilities/Compiler/Extensions/IOperationExtensions.cs

+        /// <param name="operation">The starting operation.</param>
+        /// <param name="predicate">A predicate to filter conversion operations.</param>
+        /// <returns>The first operation that either isn't a conversion or doesn't satisfy <paramref name="predicate"/>.</returns>
+        public static IOperation WalkDownConversion(this IOperation operation, Func<IConversionOperation, bool> predicate)


Maybe this could be merged with the previous and allow nullable Func. Note that I am not convinced whether it brings much.

I did end up needing it on one of my other PRs. I can see it being useful in the future, especially for dealing with arguments where you don't want to skip through explicit casts.

Evangelink · 2021-02-19T18:48:53Z

@sharwell @mavasani Could one of you review this PR? Thank you.

Co-authored-by: Amaury Levé <amaury.leve@gmail.com>

…refer-AsSpan-over-Substring-analyzer

…ring-analyzer

…CoreAnalyzersResources.resx Co-authored-by: Manish Vasani <mavasani@microsoft.com>

…yzer

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

mavasani · 2021-05-19T06:27:01Z

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

+            }
+        }
+
+        private static IEnumerable<IMethodSymbol> GetAllAccessibleOverloadsIncludingSelf(IInvocationOperation invocation, CancellationToken cancellationToken)


Can we use/enhance/update this extension method?

roslyn-analyzers/src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

Line 441 in 1fa83fe

public static IEnumerable<IMethodSymbol> GetOverloads(this IMethodSymbol? method)

That method is poorly named. Basically, it looks at a particular invocation operation and finds all overloads of the invoked method that would also be legal at that location, taking into account the type of the receiver (for instance methods) or type (for static methods). It's pretty specific to this analyzer; I can't make a version that uses an IMethodSymbol instead of an IInvocationOperation because it depends on the receiver of the invocation. I'll rename it to something more descriptive to make this clearer.

If we did want to add an extension method that does this, it couldn't go in IMethodSymbolExtensions because the extended receiver would need to be IInvocationOperation.

Edit: I've renamed the method to GetAllAccessibleOverloadsAtInvocationCallSite to reduce confusion.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/PreferAsSpanOverSubstring.Fixer.cs

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

mavasani · 2021-05-20T05:26:04Z

...NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/PreferAsSpanOverSubstringTests.cs

+        [MemberData(nameof(Data_NestedViolations))]
+        public Task NestedViolations_AreAllReportedAndFixed_CS(
+            string receiverClass, string testExpression, string fixedExpression, int[] locations,
+            int? incrementalIterations = null, int? fixAllInDocumentIterations = null, int? fixAllIterations = null)


All the yield return values in Data_NestedViolations seems to provide non-null values for these 3 parameters. Can these parameters be marked non-optional?

Also it seems fixAllInDocumentIterations and fixAllIterations is always 1. Do we need a parameter for these as all?

I think at one point I had different values for these parameters. I'll mark them non-optional and remove fixAllInDocumentIterations and fixAllIterations, as they are always 1.

mavasani · 2021-05-20T05:32:32Z

...NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/PreferAsSpanOverSubstringTests.cs

+public void Consume(string a, Roschar b) { }";
+                yield return new[]
+                {
+                    CS.WithBody(WithKey(@"Consume(foo.Substring(1), foo.Substring(2))", 0) + ';', members)


We can potentially consider showing 2 code fixes here, one for each best candidate, but given this would be a rare case, current approach of not showing code fix is also reasonable.

@mavasani I'm happy to implement that functionality. How do I write tests that expect two code fixes for one violation? Is there another analyzer that does this that I could look at as an example?

@sharwell Might have suggestions on this - I don't believe we need this as part of this PR. Feel free to file a separate issue or open a follow-up PR for it.

mavasani

Overall looks great to me and extensive unit tests give a lot of confidence, thanks!

Have some minor comments - I can merge once they are resolved.

Co-authored-by: Manish Vasani <mavasani@microsoft.com>

mavasani · 2021-05-20T16:52:34Z

@NewellClark Can you please resolve merge conflicts?

…yzer

NewellClark added 4 commits February 6, 2021 12:31

Add files and resources

29037e8

Add basic tests

8a8e73f

Add 'GetFirstOrDefaultMemberWithParameterTypes' helper method

Add more tests

b02bfbf

Merge remote-tracking branch 'dotnet/release/6.0.1xx-preview2' into p…

a82eb49

…refer-AsSpan-over-Substring-analyzer

NewellClark requested a review from a team as a code owner February 9, 2021 05:44

NewellClark marked this pull request as draft February 9, 2021 05:44

NewellClark added 2 commits February 11, 2021 11:44

Fixer fixes all unambiguous cases

2a16ad0

Previously, fixer would only fix cases where all Substring calls could be converted. Now, it fixes all cases where there is a single unambiguous best overload.

Handle incomplete cases

0bd3228

- Handle cases where not every Substring invocation can be converted to AsSpan - Add tests for inaccessible overloads

NewellClark marked this pull request as ready for review February 13, 2021 21:59

This was referenced Feb 14, 2021

Use 'AsSpan' over 'Substring' in 'System.Private.DataContractSerialization' dotnet/runtime#48274

Merged

Use 'AsSpan' over 'Substring' in 'System.Private.Xml' dotnet/runtime#48277

Merged

Fix superfluous System imports

94889ec

Fix issue where 'System' namespace would be imported unnecessarily when violations were inside a System namespace declaration, or where the System import was inside a namespace declaration. This issue was encountered in dotnet/runtime

Evangelink reviewed Feb 18, 2021

View reviewed changes

NewellClark added 2 commits February 19, 2021 12:06

Add tests

8fcaaa4

-Add tests ensuring conditional substring invocations are not reported -Add separate resource for code fix title -Remove period from method name

Apply suggested changes

ba5c872

-Inline diagnostic descriptors -Add WalkDownConversion overload that takes a predicate. -Use more descriptive names for RequiredSymbols members.

NewellClark mentioned this pull request Feb 19, 2021

(3/4) Use Span-Based String Concat Analyzer and Fixer #4764

Merged

Evangelink reviewed Feb 19, 2021

View reviewed changes

Apply suggestions from code review

d0eeafd

Co-authored-by: Amaury Levé <amaury.leve@gmail.com>

NewellClark changed the title ~~Prefer 'AsSpan' over 'Substring' analyzer~~ (D) Prefer 'AsSpan' over 'Substring' analyzer Feb 19, 2021

NewellClark changed the title ~~(D) Prefer 'AsSpan' over 'Substring' analyzer~~ (4/4) Prefer 'AsSpan' over 'Substring' analyzer Feb 19, 2021

Merge remote-tracking branch 'dotnet/release/6.0.1xx-preview2' into p…

cd202c6

…refer-AsSpan-over-Substring-analyzer

NewellClark changed the base branch from release/6.0.1xx-preview2 to release/6.0.1xx-preview4 May 8, 2021 13:44

NewellClark added 3 commits May 8, 2021 10:01

Change ID to CA1846

bf3e977

Merge branch 'release/6.0.1xx-preview4' into prefer-AsSpan-over-Subst…

b79616d

…ring-analyzer

Rebuild resource files

4cc4704

NewellClark and others added 4 commits May 18, 2021 13:57

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNet…

a8e5a51

…CoreAnalyzersResources.resx Co-authored-by: Manish Vasani <mavasani@microsoft.com>

Apply suggested changes

74bfc25

Merge branch 'release/6.0.1xx' into prefer-AsSpan-over-Substring-anal…

053b2c4

…yzer

Run msbuild to fix CI

9574d85

mavasani reviewed May 19, 2021

View reviewed changes

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

mavasani reviewed May 19, 2021

View reviewed changes

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

mavasani reviewed May 19, 2021

View reviewed changes

Apply suggested changes

3f76529