Merge pull request #18372 from CyrusNajmabadi/codeCleanupErrors2

Don't perform 'commit cleanup' on regions of code with syntax errors *and* multi-line strings.

src/EditorFeatures/VisualBasicTest/CaseCorrecting/CaseCorrectionServiceTests.vb

@@ -1,9 +1,12 @@
 ' Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
+Imports System.Collections.Immutable
 Imports System.Threading
 Imports System.Xml.Linq
 Imports Microsoft.CodeAnalysis
 Imports Microsoft.CodeAnalysis.CaseCorrection
+Imports Microsoft.CodeAnalysis.CodeCleanup
+Imports Microsoft.CodeAnalysis.CodeCleanup.Providers
 Imports Microsoft.CodeAnalysis.Editor.UnitTests.Extensions
 Imports Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces
 
@@ -12,10 +15,8 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.CaseCorrecting
         Private Async Function TestAsync(input As XElement, expected As XElement, Optional interProject As Boolean = False) As Tasks.Task
             If (interProject) Then
                 Await TestAsync(input, expected.NormalizedValue)
-                Await TestAsync(input, expected.NormalizedValue)
             Else
                 Await TestAsync(input.NormalizedValue, expected.NormalizedValue)
-                Await TestAsync(input.NormalizedValue, expected.NormalizedValue)
             End If
         End Function
 
@@ -31,7 +32,12 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.CaseCorrecting
             Dim document = workspace.CurrentSolution.GetDocument(hostDocument.Id)
             Dim span = (Await document.GetSyntaxRootAsync()).FullSpan
 
-            Dim newDocument = Await CaseCorrector.CaseCorrectAsync(document, span, CancellationToken.None)
+            Dim service = document.GetLanguageService(Of ICodeCleanerService)
+            Dim newDocument = Await service.CleanupAsync(
+                document, ImmutableArray.Create(span),
+                ImmutableArray.Create(Of ICodeCleanupProvider)(New CaseCorrectionCodeCleanupProvider()),
+                CancellationToken.None)
+
             newDocument.Project.Solution.Workspace.ApplyDocumentChanges(newDocument, CancellationToken.None)
 
             Dim actual = buffer.CurrentSnapshot.GetText()
@@ -1169,6 +1175,44 @@ End Module
 Module M
     Dim s = NameOf(M)
 End Module
+</Code>
+
+            Await TestAsync(input, expected)
+        End Function
+
+        <WorkItem(397014, "https://devdiv.visualstudio.com/DevDiv/_workitems?id=397014")>
+        <WpfFact, Trait(Traits.Feature, Traits.Features.CaseCorrection)>
+        Public Async Function AvoidNodesWithSyntaxErrorsAndStringLiterals() As Task
+            Dim input = <Code>
+Class C
+    Private Sub Test() 
+        Dim sql As New System.Text.StringBuilder 
+
+        sql.AppendLine("	SELECT *") 
+        sql.AppendLine("	, Table.Column1" 
+        sql.AppendLine(", Table.Column2 
+        sql.AppendLine(", Table.Column3") 
+        sql.AppendLine("	, Table.Column4 AS ColumnAlias") 
+        sql.AppendLine("	, Table.Column5 AS ColumnAlias2") 
+        sql.AppendLine("FROM	Table('Parameter')") 
+    End Sub 
+End Class
+</Code>
+
+            Dim expected = <Code>
+Class C
+    Private Sub Test() 
+        Dim sql As New System.Text.StringBuilder 
+
+        sql.AppendLine("	SELECT *") 
+        sql.AppendLine("	, Table.Column1" 
+        sql.AppendLine(", Table.Column2 
+        sql.AppendLine(", Table.Column3") 
+        sql.AppendLine("	, Table.Column4 AS ColumnAlias") 
+        sql.AppendLine("	, Table.Column5 AS ColumnAlias2") 
+        sql.AppendLine("FROM	Table('Parameter')") 
+    End Sub 
+End Class
 </Code>
 
             Await TestAsync(input, expected)

src/Workspaces/CSharp/Portable/CodeCleanup/CSharpCodeCleanerService.cs

@@ -1,8 +1,10 @@
 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
+using System;
 using System.Collections.Immutable;
 using Microsoft.CodeAnalysis.CodeCleanup;
 using Microsoft.CodeAnalysis.CodeCleanup.Providers;
+using Microsoft.CodeAnalysis.Text;
 
 namespace Microsoft.CodeAnalysis.CSharp.CodeCleanup
 {
@@ -14,5 +16,8 @@ internal class CSharpCodeCleanerService : AbstractCodeCleanerService
 
         public override ImmutableArray<ICodeCleanupProvider> GetDefaultProviders()
             => s_defaultProviders;
+
+        protected override ImmutableArray<TextSpan> GetSpansToAvoid(SyntaxNode root)
+            => ImmutableArray<TextSpan>.Empty;
     }
 }

src/Workspaces/Core/Portable/CodeCleanup/AbstractCodeCleanerService.cs

@@ -9,6 +9,7 @@
 using Microsoft.CodeAnalysis.CodeCleanup.Providers;
 using Microsoft.CodeAnalysis.Internal.Log;
 using Microsoft.CodeAnalysis.LanguageServices;
+using Microsoft.CodeAnalysis.Shared;
 using Microsoft.CodeAnalysis.Shared.Collections;
 using Microsoft.CodeAnalysis.Shared.Extensions;
 using Microsoft.CodeAnalysis.Text;
@@ -19,13 +20,12 @@ namespace Microsoft.CodeAnalysis.CodeCleanup
     internal abstract class AbstractCodeCleanerService : ICodeCleanerService
     {
         public abstract ImmutableArray<ICodeCleanupProvider> GetDefaultProviders();
+        protected abstract ImmutableArray<TextSpan> GetSpansToAvoid(SyntaxNode root);
 
         public async Task<Document> CleanupAsync(Document document, ImmutableArray<TextSpan> spans, ImmutableArray<ICodeCleanupProvider> providers, CancellationToken cancellationToken)
         {
             using (Logger.LogBlock(FunctionId.CodeCleanup_CleanupAsync, cancellationToken))
             {
-                var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
-
                 // If there is no span to format...
                 if (!spans.Any())
                 {
@@ -35,6 +35,8 @@ public async Task<Document> CleanupAsync(Document document, ImmutableArray<TextS
 
                 var codeCleaners = providers.IsDefault ? GetDefaultProviders() : providers;
 
+                var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
+
                 var normalizedSpan = spans.ToNormalizedSpans();
                 if (CleanupWholeNode(root.FullSpan, normalizedSpan))
                 {
@@ -55,8 +57,6 @@ public async Task<Document> CleanupAsync(Document document, ImmutableArray<TextS
                     return await IterateAllCodeCleanupProvidersAsync(document, document, n => ImmutableArray.Create(n.FullSpan), codeCleaners, cancellationToken).ConfigureAwait(false);
                 }
 
-                var model = await document.GetSemanticModelForSpanAsync(spans.Collapse(), cancellationToken).ConfigureAwait(false);
-
                 // Replace the initial node and document with the annotated node.
                 var annotatedRoot = newNodeAndAnnotations.newNode;
                 var annotatedDocument = document.WithSyntaxRoot(annotatedRoot);
@@ -481,7 +481,7 @@ private async Task<Document> IterateAllCodeCleanupProvidersAsync(
                         // Document was changed by the previous code cleaner, compute new spans.
                         var root = await currentDocument.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
                         previousDocument = currentDocument;
-                        spans = spanGetter(root);
+                        spans = GetSpans(root, spanGetter);;
                     }
 
                     // If we are at the end and there were no changes to the document, use the original document for the cleanup.
@@ -516,6 +516,21 @@ private async Task<Document> IterateAllCodeCleanupProvidersAsync(
             }
         }
 
+        private ImmutableArray<TextSpan> GetSpans(
+            SyntaxNode root, Func<SyntaxNode, ImmutableArray<TextSpan>> spanGetter)
+        {
+            // Get all the spans we've been requested to clean up.
+            var requestedSpans = new NormalizedTextSpanCollection(spanGetter(root));
+
+            // See if there are any spans we should not touch.
+            var spansToAvoid = new NormalizedTextSpanCollection(GetSpansToAvoid(root));
+
+            // Remove the spans we should not touch from the requested spans and return that final set.
+            var result = NormalizedTextSpanCollection.Difference(requestedSpans, spansToAvoid);
+
+            return result.ToImmutableArray();
+        }
+
         private async Task<SyntaxNode> IterateAllCodeCleanupProvidersAsync(
             SyntaxNode originalRoot,
             SyntaxNode annotatedRoot,
@@ -542,7 +557,7 @@ private async Task<SyntaxNode> IterateAllCodeCleanupProvidersAsync(
                     {
                         // The root was changed by the previous code cleaner, compute new spans.
                         previousRoot = currentRoot;
-                        spans = spanGetter(currentRoot);
+                        spans = GetSpans(currentRoot, spanGetter);
                     }
 
                     // If we are at the end and there were no changes to the document, use the original document for the cleanup.

src/Workspaces/VisualBasic/Portable/CodeCleanup/VisualBasicCodeCleanerService.vb

@@ -3,23 +3,106 @@
 Imports System.Collections.Immutable
 Imports Microsoft.CodeAnalysis.CodeCleanup
 Imports Microsoft.CodeAnalysis.CodeCleanup.Providers
+Imports Microsoft.CodeAnalysis.Text
 
 Namespace Microsoft.CodeAnalysis.VisualBasic.CodeCleanup
     Partial Friend Class VisualBasicCodeCleanerService
         Inherits AbstractCodeCleanerService
 
         Private Shared ReadOnly s_defaultProviders As ImmutableArray(Of ICodeCleanupProvider) = ImmutableArray.Create(Of ICodeCleanupProvider)(
-                New AddMissingTokensCodeCleanupProvider(),
-                New FixIncorrectTokensCodeCleanupProvider(),
-                New ReduceTokensCodeCleanupProvider(),
-                New NormalizeModifiersOrOperatorsCodeCleanupProvider(),
-                New RemoveUnnecessaryLineContinuationCodeCleanupProvider(),
-                New CaseCorrectionCodeCleanupProvider(),
-                New SimplificationCodeCleanupProvider(),
-                New FormatCodeCleanupProvider())
+            New AddMissingTokensCodeCleanupProvider(),
+            New FixIncorrectTokensCodeCleanupProvider(),
+            New ReduceTokensCodeCleanupProvider(),
+            New NormalizeModifiersOrOperatorsCodeCleanupProvider(),
+            New RemoveUnnecessaryLineContinuationCodeCleanupProvider(),
+            New CaseCorrectionCodeCleanupProvider(),
+            New SimplificationCodeCleanupProvider(),
+            New FormatCodeCleanupProvider())
 
         Public Overrides Function GetDefaultProviders() As ImmutableArray(Of ICodeCleanupProvider)
             Return s_defaultProviders
         End Function
+
+        Protected Overrides Function GetSpansToAvoid(root As SyntaxNode) As ImmutableArray(Of TextSpan)
+            ' We don't want to touch nodes in the document that have syntax errors on them and which
+            ' contain multi-line string literals.  It's quite possible that there is some string 
+            ' literal on a previous line that was intended to be terminated on that line, but which
+            ' wasn't.  The string may then have terminated on this line (because of the start of 
+            ' another literal) causing the literal contents to then be considered code.  We don't 
+            ' want to cleanup 'code' that the user intends to be the content of a string literal.
+
+            Dim result = ArrayBuilder(Of TextSpan).GetInstance()
+
+            ProcessNode(root, result)
+
+            Return result.ToImmutableAndFree()
+        End Function
+
+        Private Sub ProcessNode(node As SyntaxNode, result As ArrayBuilder(Of TextSpan))
+            If SkipProcessing(node, result) Then
+                Return
+            End If
+
+            For Each child In node.ChildNodesAndTokens()
+                If child.IsNode Then
+                    ProcessNode(child.AsNode(), result)
+                Else
+                    ProcessToken(child.AsToken(), result)
+                End If
+            Next
+        End Sub
+
+        Private Sub ProcessToken(token As SyntaxToken, result As ArrayBuilder(Of TextSpan))
+            If SkipProcessing(token, result) Then
+                Return
+            End If
+
+            Dim parentMultiLineNode = GetMultiLineContainer(token.Parent)
+            If parentMultiLineNode IsNot Nothing Then
+                If ContainsMultiLineStringLiteral(parentMultiLineNode) Then
+                    result.Add(parentMultiLineNode.FullSpan)
+                End If
+            End If
+        End Sub
+
+        Private Function SkipProcessing(nodeOrToken As SyntaxNodeOrToken, result As ArrayBuilder(Of TextSpan)) As Boolean
+            ' Don't bother looking at nodes or token that don't have any syntax errors in them.
+            If Not nodeOrToken.ContainsDiagnostics Then
+                Return True
+            End If
+
+            If result.Count > 0 AndAlso result.Last.Contains(nodeOrToken.Span) Then
+                ' Don't bother looking at nodes or token that are contained within a span we've already 
+                ' marked as something to avoid. We would only ever produce mark the same (or smaller) 
+                ' span again.
+                Return True
+            End If
+
+            Return False
+        End Function
+
+        Private Function ContainsMultiLineStringLiteral(node As SyntaxNode) As Boolean
+            Return node.DescendantTokens().Any(
+                Function(t)
+                    If t.Kind() = SyntaxKind.StringLiteralToken OrElse
+                       t.Kind() = SyntaxKind.InterpolatedStringTextToken Then
+                        Return Not VisualBasicSyntaxFactsService.Instance.IsOnSingleLine(t.Parent, fullSpan:=False)
+                    End If
+
+                    Return False
+                End Function)
+        End Function
+
+        Private Function GetMultiLineContainer(node As SyntaxNode) As SyntaxNode
+            If node Is Nothing Then
+                Return Nothing
+            End If
+
+            If Not VisualBasicSyntaxFactsService.Instance.IsOnSingleLine(node, fullSpan:=False) Then
+                Return node
+            End If
+
+            Return GetMultiLineContainer(node.Parent)
+        End Function
     End Class
 End Namespace