Merge pull request #59385 from CyrusNajmabadi/wrappingError

Support wrapping constructs even in the presence of some recoverable errors

src/EditorFeatures/CSharpTest/Wrapping/ParameterWrappingTests.cs

@@ -9,6 +9,7 @@
 using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
 using Microsoft.CodeAnalysis.CSharp.Wrapping;
 using Microsoft.CodeAnalysis.Test.Utilities;
+using Roslyn.Test.Utilities;
 using Xunit;
 
 namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Wrapping
@@ -810,6 +811,22 @@ public C(int i,
 }");
         }
 
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsWrapping)]
+        [WorkItem(38986, "https://github.com/dotnet/roslyn/issues/38986")]
+        public async Task TestInConstructorWithSyntaxErrorAfter()
+        {
+            await TestInRegularAndScript1Async(
+@"class C {
+    public [||]C(int i, int j) : base(,) {
+    }
+}",
+@"class C {
+    public C(int i,
+             int j) : base(,) {
+    }
+}");
+        }
+
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsWrapping)]
         public async Task TestInIndexer()
         {

src/EditorFeatures/VisualBasicTest/Wrapping/ParameterWrappingTests.vb

@@ -22,6 +22,20 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.Wrapping
 end class")
         End Function
 
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsWrapping)>
+        Public Async Function TestAvailableWithSyntaxErrorAfter() As Task
+            Await TestInRegularAndScript1Async(
+"class C
+    function Goobar([||]i as integer, j as integer) as
+    end function
+end class",
+"class C
+    function Goobar(i as integer,
+                    j as integer) as
+    end function
+end class")
+        End Function
+
         <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsWrapping)>
         Public Async Function TestMissingWithSelection() As Task
             Await TestMissingAsync(

src/Features/CSharp/Portable/Wrapping/SeparatedSyntaxList/CSharpArgumentWrapper.cs

@@ -2,11 +2,10 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-#nullable disable
-
 using System.Linq;
 using Microsoft.CodeAnalysis.CSharp.LanguageServices;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Shared.Extensions;
 using Microsoft.CodeAnalysis.Text;
 using Roslyn.Utilities;
 
@@ -27,7 +26,7 @@ internal partial class CSharpArgumentWrapper
         protected override SeparatedSyntaxList<ArgumentSyntax> GetListItems(BaseArgumentListSyntax listSyntax)
             => listSyntax.Arguments;
 
-        protected override BaseArgumentListSyntax TryGetApplicableList(SyntaxNode node)
+        protected override BaseArgumentListSyntax? TryGetApplicableList(SyntaxNode node)
             => node switch
             {
                 InvocationExpressionSyntax invocationExpression => invocationExpression.ArgumentList,
@@ -38,18 +37,23 @@ protected override BaseArgumentListSyntax TryGetApplicableList(SyntaxNode node)
             };
 
         protected override bool PositionIsApplicable(
-            SyntaxNode root, int position,
-            SyntaxNode declaration, BaseArgumentListSyntax listSyntax)
+            SyntaxNode root,
+            int position,
+            SyntaxNode declaration,
+            bool containsSyntaxError,
+            BaseArgumentListSyntax listSyntax)
         {
+            if (containsSyntaxError)
+                return false;
+
             var startToken = listSyntax.GetFirstToken();
 
-            if (declaration is InvocationExpressionSyntax or
-                ElementAccessExpressionSyntax)
+            if (declaration is InvocationExpressionSyntax or ElementAccessExpressionSyntax)
             {
                 // If we have something like  Foo(...)  or  this.Foo(...)  allow anywhere in the Foo(...)
                 // section.
                 var expr = (declaration as InvocationExpressionSyntax)?.Expression ??
-                           (declaration as ElementAccessExpressionSyntax).Expression;
+                           ((ElementAccessExpressionSyntax)declaration).Expression;
                 var name = TryGetInvokedName(expr);
 
                 startToken = name == null ? listSyntax.GetFirstToken() : name.GetFirstToken();
@@ -68,40 +72,32 @@ protected override bool PositionIsApplicable(
             var endToken = listSyntax.GetLastToken();
             var span = TextSpan.FromBounds(startToken.SpanStart, endToken.Span.End);
             if (!span.IntersectsWith(position))
-            {
                 return false;
-            }
 
             // allow anywhere in the arg list, as long we don't end up walking through something
             // complex like a lambda/anonymous function.
             var token = root.FindToken(position);
-            if (token.Parent.Ancestors().Contains(listSyntax))
+            if (token.GetRequiredParent().Ancestors().Contains(listSyntax))
             {
-                for (var current = token.Parent; current != listSyntax; current = current.Parent)
+                for (var current = token.Parent; current != listSyntax; current = current?.Parent)
                 {
                     if (CSharpSyntaxFacts.Instance.IsAnonymousFunctionExpression(current))
-                    {
                         return false;
-                    }
                 }
             }
 
             return true;
         }
 
-        private static ExpressionSyntax TryGetInvokedName(ExpressionSyntax expr)
+        private static ExpressionSyntax? TryGetInvokedName(ExpressionSyntax expr)
         {
             // `Foo(...)`.  Allow up through the 'Foo' portion
             if (expr is NameSyntax name)
-            {
                 return name;
-            }
 
             // `this[...]`. Allow up through the 'this' token.
             if (expr is ThisExpressionSyntax or BaseExpressionSyntax)
-            {
                 return expr;
-            }
 
             // expr.Foo(...) or expr?.Foo(...)
             // All up through the 'Foo' portion.

src/Features/CSharp/Portable/Wrapping/SeparatedSyntaxList/CSharpParameterWrapper.cs

@@ -31,14 +31,12 @@ protected override SeparatedSyntaxList<ParameterSyntax> GetListItems(BaseParamet
             => node.GetParameterList();
 
         protected override bool PositionIsApplicable(
-            SyntaxNode root, int position, SyntaxNode declaration, BaseParameterListSyntax listSyntax)
+            SyntaxNode root, int position, SyntaxNode declaration, bool containsSyntaxError, BaseParameterListSyntax listSyntax)
         {
             // CSharpSyntaxGenerator.GetParameterList synthesizes a parameter list for simple-lambdas.
             // In that case, we're not applicable in that list.
             if (declaration.Kind() == SyntaxKind.SimpleLambdaExpression)
-            {
                 return false;
-            }
 
             var generator = CSharpSyntaxGenerator.Instance;
             var attributes = generator.GetAttributes(declaration);
@@ -52,7 +50,13 @@ protected override bool PositionIsApplicable(
             var lastToken = listSyntax.GetLastToken();
 
             var headerSpan = TextSpan.FromBounds(firstToken.SpanStart, lastToken.Span.End);
-            return headerSpan.IntersectsWith(position);
+            if (!headerSpan.IntersectsWith(position))
+                return false;
+
+            if (containsSyntaxError && ContainsOverlappingSyntaxErrror(declaration, headerSpan))
+                return false;
+
+            return true;
         }
     }
 }

src/Features/Core/Portable/Wrapping/AbstractWrapper.cs

@@ -2,16 +2,16 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-#nullable disable
-
 using System.Collections.Generic;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Shared.Extensions;
 
 namespace Microsoft.CodeAnalysis.Wrapping
 {
+    using System.Linq;
     using Microsoft.CodeAnalysis.Indentation;
+    using Microsoft.CodeAnalysis.Text;
 
     /// <summary>
     /// Common implementation of all <see cref="ISyntaxWrapper"/>.  This type takes care of a lot of common logic for
@@ -32,7 +32,8 @@ internal abstract partial class AbstractSyntaxWrapper : ISyntaxWrapper
         protected AbstractSyntaxWrapper(IIndentationService indentationService)
             => IndentationService = indentationService;
 
-        public abstract Task<ICodeActionComputer> TryCreateComputerAsync(Document document, int position, SyntaxNode node, CancellationToken cancellationToken);
+        public abstract Task<ICodeActionComputer?> TryCreateComputerAsync(
+            Document document, int position, SyntaxNode node, bool containsSyntaxError, CancellationToken cancellationToken);
 
         protected static async Task<bool> ContainsUnformattableContentAsync(
             Document document, IEnumerable<SyntaxNodeOrToken> nodesAndTokens, CancellationToken cancellationToken)
@@ -46,25 +47,23 @@ protected static async Task<bool> ContainsUnformattableContentAsync(
             var sourceText = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
             foreach (var item in nodesAndTokens)
             {
-                if (item == null ||
-                    item.Span.IsEmpty)
-                {
+                if (item == null || item.Span.IsEmpty)
                     return true;
-                }
 
-                var firstToken = item.IsToken ? item.AsToken() : item.AsNode().GetFirstToken();
-                var lastToken = item.IsToken ? item.AsToken() : item.AsNode().GetLastToken();
+                var firstToken = item.IsToken ? item.AsToken() : item.AsNode()!.GetFirstToken();
+                var lastToken = item.IsToken ? item.AsToken() : item.AsNode()!.GetLastToken();
 
                 // Note: we check if things are on the same line, even in the case of a single token.
                 // This is so that we don't try to wrap multiline tokens either (like a multi-line 
                 // string).
                 if (!sourceText.AreOnSameLine(firstToken, lastToken))
-                {
                     return true;
-                }
             }
 
             return false;
         }
+
+        protected static bool ContainsOverlappingSyntaxErrror(SyntaxNode declaration, TextSpan headerSpan)
+            => declaration.GetDiagnostics().Any(d => d.Severity == DiagnosticSeverity.Error && d.Location.SourceSpan.OverlapsWith(headerSpan));
     }
 }

src/Features/Core/Portable/Wrapping/AbstractWrappingCodeRefactoringProvider.cs

@@ -2,12 +2,11 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-#nullable disable
-
 using System.Collections.Immutable;
 using System.Linq;
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.CodeRefactorings;
+using Microsoft.CodeAnalysis.Shared.Extensions;
 
 namespace Microsoft.CodeAnalysis.Wrapping
 {
@@ -34,22 +33,15 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte
         {
             var (document, span, cancellationToken) = context;
             if (!span.IsEmpty)
-            {
                 return;
-            }
 
             var position = span.Start;
-            var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
+            var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
             var token = root.FindToken(position);
 
-            foreach (var node in token.Parent.AncestorsAndSelf())
+            foreach (var node in token.GetRequiredParent().AncestorsAndSelf())
             {
-                // Make sure we don't have any syntax errors here.  Don't want to format if we don't
-                // really understand what's going on.
-                if (node.GetDiagnostics().Any(d => d.Severity == DiagnosticSeverity.Error))
-                {
-                    return;
-                }
+                var containsSyntaxError = node.GetDiagnostics().Any(d => d.Severity == DiagnosticSeverity.Error);
 
                 // Check if any wrapper can handle this node.  If so, then we're done, otherwise
                 // keep walking up.
@@ -58,22 +50,23 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte
                     cancellationToken.ThrowIfCancellationRequested();
 
                     var computer = await wrapper.TryCreateComputerAsync(
-                        document, position, node, cancellationToken).ConfigureAwait(false);
+                        document, position, node, containsSyntaxError, cancellationToken).ConfigureAwait(false);
 
                     if (computer == null)
-                    {
                         continue;
-                    }
 
                     var actions = await computer.GetTopLevelCodeActionsAsync().ConfigureAwait(false);
                     if (actions.IsDefaultOrEmpty)
-                    {
                         continue;
-                    }
 
                     context.RegisterRefactorings(actions);
                     return;
                 }
+
+                // if we hit a syntax error and the computer couldn't handle it, then bail out.  Don't want to format if
+                // we don't really understand what's going on.
+                if (containsSyntaxError)
+                    return;
             }
         }
     }

src/Features/Core/Portable/Wrapping/BinaryExpression/AbstractBinaryExpressionWrapper.cs

@@ -2,8 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-#nullable disable
-
 using System.Collections.Immutable;
 using System.Threading;
 using System.Threading.Tasks;
@@ -41,19 +39,18 @@ protected AbstractBinaryExpressionWrapper(
         /// </summary>
         protected abstract SyntaxTriviaList GetNewLineBeforeOperatorTrivia(SyntaxTriviaList newLine);
 
-        public sealed override async Task<ICodeActionComputer> TryCreateComputerAsync(
-            Document document, int position, SyntaxNode node, CancellationToken cancellationToken)
+        public sealed override async Task<ICodeActionComputer?> TryCreateComputerAsync(
+            Document document, int position, SyntaxNode node, bool containsSyntaxError, CancellationToken cancellationToken)
         {
+            if (containsSyntaxError)
+                return null;
+
             if (node is not TBinaryExpressionSyntax binaryExpr)
-            {
                 return null;
-            }
 
             var precedence = _precedenceService.GetPrecedenceKind(binaryExpr);
             if (precedence == PrecedenceKind.Other)
-            {
                 return null;
-            }
 
             // Don't process this binary expression if it's in a parent binary expr of the same or
             // lower precedence.  We'll just allow our caller to walk up to that and call back into
@@ -87,9 +84,7 @@ public sealed override async Task<ICodeActionComputer> TryCreateComputerAsync(
                 document, exprsAndOperators, cancellationToken).ConfigureAwait(false);
 
             if (containsUnformattableContent)
-            {
                 return null;
-            }
 
             var sourceText = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
             var options = await document.GetOptionsAsync(cancellationToken).ConfigureAwait(false);