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

NewellClark · 2021-02-02T17:10:58Z

Fixes issue #33777.
Issues in dotnet/runtime have been addressed.

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

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

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

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

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcatTests.cs

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

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

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

Youssef1313 · 2021-02-02T17:42:13Z

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcatTests.cs

+        public Task SingleViolationInOneBlock_ReportedAndFixed_CS(string testStatements, string fixedStatements)
+        {
+            var test = new VerifyCS.Test
+            {
+                TestCode = CSUsings + CSWithBody(testStatements),
+                FixedCode = CSUsings + CSWithBody(fixedStatements),
+                ReferenceAssemblies = ReferenceAssemblies.Net.Net50,
+                ExpectedDiagnostics = { VerifyCS.Diagnostic(Rule).WithLocation(0) }
+            };
+            return test.RunAsync();


Consider making the tests async, then await test.RunAsync().

public async Task TestMethod() => await test.RunAsync(); is semantically identical to
public Task TestMethod() => return test.RunAsync();, but potentially slower. If we needed to await multiple async calls, it would make sense to mark the method as async, but since we have just that one async call, it makes sense to simply return the Task produced by test.RunAsync().

Basically, we already have a Task. May as well just return it.

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcatTests.cs

Youssef1313

Consider having tests against null-conditional access operator (?.).

NewellClark · 2021-02-03T18:05:34Z

Consider having tests against null-conditional access operator (?.).

@Youssef1313 Do you mean for Substring invocations, as in foo?.Substring(1) + bar?

Youssef1313 · 2021-02-03T18:07:29Z

Consider having tests against null-conditional access operator (?.).

@Youssef1313 Do you mean for Substring invocations, as in foo?.Substring(1) + bar?

Yes.

NewellClark · 2021-02-03T18:15:44Z

Consider having tests against null-conditional access operator (?.).

@Youssef1313 Do you mean for Substring invocations, as in foo?.Substring(1) + bar?

Yes.

What should the fixer do for foo?.Substring(1)? Because the span-based equivalent is something like
foo is null ? default : foo.AsSpan(1), which seems kind of verbose for the fixer to be inserting. Should we ignore cases where Substring is accessed conditionally?

Youssef1313 · 2021-02-03T18:21:44Z

Consider having tests against null-conditional access operator (?.).

@Youssef1313 Do you mean for Substring invocations, as in foo?.Substring(1) + bar?

Yes.

What should the fixer do for foo?.Substring(1)? Because the span-based equivalent is something like
foo is null ? default : foo.AsSpan(1), which seems kind of verbose for the fixer to be inserting. Should we ignore cases where Substring is accessed conditionally?

I'm not sure whether the fixer should do something or not. But I think at least the analyzer should flag these.

@mavasani @bartonjs What do you think?

bartonjs · 2021-02-03T18:49:50Z

I'm not sure whether the fixer should do something or not. But I think at least the analyzer should flag these.

That's a reasonable starting position.

…d made it a static method. - Added tests for conditional substring invocation. We report conditional substring invocations, but do not fix them. - Fixed issue where `Imports System` was unnecessarily being added in projects that had `System` as an implicit global import, and added tests for VB projects with and without an implicit global `System` import. - Added null tests for both string and char type symbols, just in case. - Used exact values for expected iteration counts for nested violations tests. - Other minor style changes.

Some tests didn't explicitly specify net5.0, which caused the tests to fail due to lack of span-based string.Concat.

NewellClark · 2021-02-04T08:19:27Z

I've made the requested changes.

The fixer will now only add Imports System in VB if it is not specified as an implicit global import. System is imported by default in projects created in Visual Studio, but it can be toggled off, so I've added tests ensuring the fixer always does the right thing regardless of user configs.

Analyzer detects foo?.Substring(1) + whatever but fixer won't fix it, as previously discussed.

codecov · 2021-02-04T08:46:16Z

Codecov Report

Merging #4764 (092b95d) into release/6.0.1xx (d515202) will increase coverage by 0.00%.
The diff coverage is 93.62%.

@@                Coverage Diff                 @@
##           release/6.0.1xx    #4764     +/-   ##
==================================================
  Coverage            95.55%   95.56%             
==================================================
  Files                 1203     1210      +7     
  Lines               274337   275763   +1426     
  Branches             16632    16713     +81     
==================================================
+ Hits                262137   263526   +1389     
- Misses                9988    10005     +17     
- Partials              2212     2232     +20

NewellClark · 2021-02-04T22:30:15Z

How do we want to handle character arguments? I was thinking stackalloc char[] { charVar }, but that could potentially run into CA2014 if the concatenation is inside a loop.

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

mavasani · 2021-04-13T09:03:07Z

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

+                    //  If the current operation is a string-concatenation operation, walk up to the top-most concat
+                    //  operation and add it to the set.
+                    var binary = (IBinaryOperation)context.Operation;
+                    if (!TryGetTopMostConcatOperation(binary, out var topMostConcatOperation))


This approach seems fine, but seems you can do without topMostConcatOperations and reporting in the end action. Instead you can just have the following in IBinaryOperation callback:

if (IsTopMostConcatOperation(binary) && ShouldBeReported(binary)) { context.ReportDiagnostic(binary.CreateDiagnostic(Rule)); }

The problem with this approach is that we may end up reporting the same violation multiple times. I remember encountering this issue while writing the analyzer. I'll add a comment clarifying this.

mavasani · 2021-04-13T09:08:58Z

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

+            }
+        }
+
+        internal static ImmutableArray<IOperation> FlattenBinaryOperation(IBinaryOperation root)


Might be much simpler to just write a specialized OperationWalker here.

You're right. I've done that.

mavasani · 2021-04-13T09:10:28Z

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

+                case LanguageNames.CSharp when conversion.Type.SpecialType is SpecialType.System_Object:
+                case LanguageNames.VisualBasic when conversion.Type.SpecialType is SpecialType.System_Object or SpecialType.System_String:


Please move all language specific logic to language specific layers instead of checking language names - you already have similar abstract methods and language specific operation walking code, so please move this out as well.

Fixed. I did have to add two static methods with the VB and CS implementations, as the method is called from both the analyzer and the fixer, but I've taken care of it.

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

mavasani · 2021-04-13T09:13:48Z

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

+            if (root.FindNode(context.Span, getInnermostNodeForTie: true) is not SyntaxNode concatExpressionSyntax)
+                return;
+            //  OperatorKind will be BinaryOperatorKind.Concatenate, even when '+' is used instead of '&' in Visual Basic.
+            if (model.GetOperation(concatExpressionSyntax, cancellationToken) is not IBinaryOperation concatOperation || concatOperation.OperatorKind is not (BinaryOperatorKind.Add or BinaryOperatorKind.Concatenate))


Please break down the expression into separate lines for readability

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

mavasani

Overall LGTM. Thanks for extensive tests.

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

- Apply Mavasani's changes - Remove unused method from fixers

…alyzer

Youssef1313 · 2021-04-24T18:49:10Z

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

+                if (!IsSystemNamespaceImported(context.Document.Project, generator.GetNamespaceImports(newRoot)))
+                {
+                    SyntaxNode systemNamespaceImport = generator.NamespaceImportDeclaration(nameof(System));
+                    newRoot = generator.AddNamespaceImports(newRoot, systemNamespaceImport);
+                }


Would you be able to check if calling WithAddImportsAnnotation on concatMethodInvocationSyntax eliminate the need of IsSystemNamespaceImported?

If WithAddImportsAnnotation didn't work, consider doing a case-insensitive string comparison in the VB override.

WithAddImportsAnnotation only works for syntax nodes that represent types (just tested it).
I did change the VB fixer to use a case-insensitive comparison (good catch).

Use case-insensitive string comparison to import System namespace in the VB fixer.

mavasani · 2021-05-17T07:47:35Z

Same request and apologies as #4726 (comment). This needs to be targeted to release\6.0.1xx. Thanks!

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

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

NewellClark added 3 commits January 25, 2021 13:24

Add required files

ba6d78d

Common cases

42107f2

Analyzer and fixer working

0fe5cce

NewellClark requested a review from a team as a code owner February 2, 2021 17:10

NewellClark marked this pull request as draft February 2, 2021 17:12

mavasani reviewed Feb 2, 2021

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

mavasani reviewed Feb 2, 2021

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

NewellClark and others added 2 commits February 2, 2021 12:13

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

e98a55b

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

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

52652c9

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

Youssef1313 reviewed Feb 2, 2021

View reviewed changes

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

sharwell reviewed Feb 2, 2021

View reviewed changes

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcatTests.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Feb 2, 2021

View reviewed changes

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

Youssef1313 reviewed Feb 2, 2021

View reviewed changes

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

Youssef1313 reviewed Feb 2, 2021

View reviewed changes

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

Youssef1313 reviewed Feb 2, 2021

View reviewed changes

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcatTests.cs Show resolved Hide resolved

Youssef1313 reviewed Feb 2, 2021

View reviewed changes

NewellClark added 2 commits February 3, 2021 21:15

Fix tests on net472

c1baa81

Some tests didn't explicitly specify net5.0, which caused the tests to fail due to lack of span-based string.Concat.

Use Environment.NewLine

6d12d20

This was referenced Feb 4, 2021

Fix Use Span Based String Concat violations in System.Diagnostics.DiagnosticSource dotnet/runtime#47868

Closed

Fix violations system private xml dotnet/runtime#47874

Merged

mavasani reviewed Apr 13, 2021

View reviewed changes

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

mavasani reviewed Apr 13, 2021

View reviewed changes

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

mavasani reviewed Apr 13, 2021

View reviewed changes

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

mavasani reviewed Apr 13, 2021

View reviewed changes

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

mavasani reviewed Apr 13, 2021

View reviewed changes

NewellClark and others added 2 commits April 23, 2021 15:07

Apply formatting changes

822659c

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

Apply requested changes

760cd4a

- Apply Mavasani's changes - Remove unused method from fixers

NewellClark changed the base branch from release/6.0.1xx-preview1 to release/6.0.1xx-preview4 April 24, 2021 17:36

NewellClark changed the base branch from release/6.0.1xx-preview4 to release/6.0.1xx-preview1 April 24, 2021 17:59

Change ID to CA1845

49894c3

NewellClark changed the base branch from release/6.0.1xx-preview1 to release/6.0.1xx-preview4 April 24, 2021 18:13

Merge branch 'release/6.0.1xx-preview4' into use-span-based-concat-an…

1cd7b98

…alyzer

Youssef1313 reviewed Apr 24, 2021

View reviewed changes

NewellClark added 3 commits April 24, 2021 18:07

.

e4867b8

Rebuild xlf files to fix CI

2b71460

Import System namespace correctly

268acdc

Use case-insensitive string comparison to import System namespace in the VB fixer.

mavasani reviewed May 17, 2021

View reviewed changes

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md Outdated Show resolved Hide resolved

Update src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

092b95d

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

NewellClark changed the base branch from release/6.0.1xx-preview4 to release/6.0.1xx May 17, 2021 15:48

mavasani approved these changes May 18, 2021

View reviewed changes

mavasani merged commit 0f43809 into dotnet:release/6.0.1xx May 18, 2021

Youssef1313 mentioned this pull request May 18, 2021

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

Closed

22 tasks

This was referenced May 18, 2021

CA1845 dotnet/docs#24270

Merged

Use span-based string.Concat dotnet/runtime#33777

Closed

NewellClark deleted the use-span-based-concat-analyzer branch August 27, 2021 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NewellClark commented Feb 2, 2021 •

edited

Loading

Youssef1313 Feb 2, 2021

NewellClark Feb 2, 2021 •

edited

Loading

Youssef1313 left a comment

NewellClark commented Feb 3, 2021 •

edited

Loading

Youssef1313 commented Feb 3, 2021

NewellClark commented Feb 3, 2021

Youssef1313 commented Feb 3, 2021

bartonjs commented Feb 3, 2021

NewellClark commented Feb 4, 2021

codecov bot commented Feb 4, 2021 •

edited

Loading

NewellClark commented Feb 4, 2021

mavasani Apr 13, 2021

NewellClark Apr 23, 2021 •

edited

Loading

mavasani Apr 13, 2021

NewellClark Apr 24, 2021

mavasani Apr 13, 2021

NewellClark Apr 24, 2021 •

edited

Loading

mavasani Apr 13, 2021

NewellClark Apr 24, 2021

mavasani left a comment

Youssef1313 Apr 24, 2021 •

edited

Loading

NewellClark May 10, 2021

mavasani commented May 17, 2021

		case LanguageNames.CSharp when conversion.Type.SpecialType is SpecialType.System_Object:
		case LanguageNames.VisualBasic when conversion.Type.SpecialType is SpecialType.System_Object or SpecialType.System_String:

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

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

Conversation

NewellClark commented Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

NewellClark Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Youssef1313 left a comment

Choose a reason for hiding this comment

NewellClark commented Feb 3, 2021 • edited Loading

Youssef1313 commented Feb 3, 2021

NewellClark commented Feb 3, 2021

Youssef1313 commented Feb 3, 2021

bartonjs commented Feb 3, 2021

NewellClark commented Feb 4, 2021

codecov bot commented Feb 4, 2021 • edited Loading

Codecov Report

NewellClark commented Feb 4, 2021

Choose a reason for hiding this comment

NewellClark Apr 23, 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 Apr 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

Youssef1313 Apr 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented May 17, 2021

NewellClark commented Feb 2, 2021 •

edited

Loading

NewellClark Feb 2, 2021 •

edited

Loading

NewellClark commented Feb 3, 2021 •

edited

Loading

codecov bot commented Feb 4, 2021 •

edited

Loading

NewellClark Apr 23, 2021 •

edited

Loading

NewellClark Apr 24, 2021 •

edited

Loading

Youssef1313 Apr 24, 2021 •

edited

Loading