RefactoringHelpers handle Attributes correctly + tests.

src/EditorFeatures/CSharpTest/PullMemberUp/CSharpPullMemberUpTests.cs

@@ -2276,8 +2276,7 @@ public async Task TestNoRefactoringCaretInArgs()
 namespace PushUpTest
 {
     public class A
-    {
-        
+    { 
     }
 
     public class B : A
@@ -2292,6 +2291,379 @@ void C([||])
             await TestQuickActionNotProvidedAsync(input);
         }
 
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestRefactoringCaretBeforeAttributes()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    class Test2Attribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [||][Test]
+        [Test2]
+        void C()
+        {
+        }
+    }
+}";
+            var expected = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    class Test2Attribute : Attribute { }
+    public class A
+    {
+        [Test]
+        [Test2]
+        void C()
+        {
+        }
+    }
+
+    public class B : A
+    {
+    }
+}";
+
+            await TestInRegularAndScriptAsync(testText, expected);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestMissingRefactoringCaretBetweenAttributes()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    class Test2Attribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [Test]
+        [||][Test2]
+        void C()
+        {
+        }
+    }
+}";
+            await TestQuickActionNotProvidedAsync(testText);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestRefactoringSelectionWithAttributes2()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [|[Test]
+        void C()
+        {
+        }|]
+    }
+}";
+            var expected = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    public class A
+    {
+        [Test]
+        void C()
+        {
+        }
+    }
+
+    public class B : A
+    {
+    }
+}";
+
+            await TestInRegularAndScriptAsync(testText, expected);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestMissingRefactoringInAttributeList()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [[||]Test]
+        void C()
+        {
+        }
+    }
+}";
+            await TestQuickActionNotProvidedAsync(testText);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestMissingRefactoringSelectionAttributeList()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    class Test2Attribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [|[Test]
+        [Test2]|]
+        void C()
+        {
+        }
+    }
+}";
+            await TestQuickActionNotProvidedAsync(testText);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestMissingRefactoringCaretInAttributeList()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    class Test2Attribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [[||]Test]
+        [Test2]
+        void C()
+        {
+        }
+    }
+}";
+            await TestQuickActionNotProvidedAsync(testText);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestMissingRefactoringCaretBetweenAttributeLists()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    class Test2Attribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [Test]
+        [||][Test2]
+        void C()
+        {
+        }
+    }
+}";
+            await TestQuickActionNotProvidedAsync(testText);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestMissingRefactoringSelectionAttributeList2()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    class Test2Attribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [|[Test]|]
+        [Test2]
+        void C()
+        {
+        }
+    }
+}";
+            await TestQuickActionNotProvidedAsync(testText);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestMissingRefactoringSelectAttributeList()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [|[Test]|]
+        void C()
+        {
+        }
+    }
+}";
+            await TestQuickActionNotProvidedAsync(testText);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestRefactoringCaretLocAfterAttributes1()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [Test]
+        [||]void C()
+        {
+        }
+    }
+}";
+            var expected = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    public class A
+    {
+        [Test]
+        void C()
+        {
+        }
+    }
+
+    public class B : A
+    {
+    }
+}";
+
+            await TestInRegularAndScriptAsync(testText, expected);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestRefactoringCaretLocAfterAttributes2()
+        {
+            var testText = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    class Test2Attribute : Attribute { }
+    public class A
+    {
+    }
+
+    public class B : A
+    {
+        [Test]
+        // Comment1
+        [Test2]
+        // Comment2
+        [||]void C()
+        {
+        }
+    }
+}";
+            var expected = @"
+using System;
+
+namespace PushUpTest
+{
+    class TestAttribute : Attribute { }
+    class Test2Attribute : Attribute { }
+    public class A
+    {
+        [Test]
+        // Comment1
+        [Test2]
+        // Comment2
+        void C()
+        {
+        }
+    }
+
+    public class B : A
+    {
+    }
+}";
+
+            await TestInRegularAndScriptAsync(testText, expected);
+        }
+
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
         [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
         public async Task TestRefactoringCaretLoc1()
@@ -2356,6 +2728,129 @@ void C()
         }
     }
 
+    public class B : A
+    {
+    }
+}";
+
+            await TestInRegularAndScriptAsync(testText, expected);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestRefactoringSelectionComments()
+        {
+            var testText = @"
+namespace PushUpTest
+{
+    public class A
+    {
+    }
+
+    public class B : A
+    {  [|
+        // Comment1
+        void C()
+        {
+        }|]
+    }
+}";
+            var expected = @"
+namespace PushUpTest
+{
+    public class A
+    {
+        // Comment1
+        void C()
+        {
+        }
+    }
+
+    public class B : A
+    {
+    }
+}";
+
+            await TestInRegularAndScriptAsync(testText, expected);
+        }
+
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestRefactoringSelectionComments2()
+        {
+            var testText = @"
+namespace PushUpTest
+{
+    public class A
+    {
+    }
+
+    public class B : A
+    {  
+        [|/// <summary>
+        /// Test
+        /// </summary>
+        void C()
+        {
+        }|]
+    }
+}";
+            var expected = @"
+namespace PushUpTest
+{
+    public class A
+    {
+        /// <summary>
+        /// Test
+        /// </summary>
+        void C()
+        {
+        }
+    }
+
+    public class B : A
+    {
+    }
+}";
+
+            await TestInRegularAndScriptAsync(testText, expected);
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsPullMemberUp)]
+        [WorkItem(35180, "https://github.com/dotnet/roslyn/issues/35180")]
+        public async Task TestRefactoringSelectionComments3()
+        {
+            var testText = @"
+namespace PushUpTest
+{
+    public class A
+    {
+    }
+
+    public class B : A
+    {  
+        /// <summary>
+        [|/// Test
+        /// </summary>
+        void C()
+        {
+        }|]
+    }
+}";
+            var expected = @"
+namespace PushUpTest
+{
+    public class A
+    {
+        /// <summary>
+        /// Test
+        /// </summary>
+        void C()
+        {
+        }
+    }
+
     public class B : A
     {
     }

src/Features/Core/Portable/CodeRefactorings/AbstractRefactoringHelpersService.cs

@@ -36,7 +36,7 @@ internal abstract class AbstractRefactoringHelpersService : IRefactoringHelpersS
         /// <para>
         /// Note: this function trims all whitespace from both the beginning and the end of given <paramref name="selection"/>.
         /// The trimmed version is then used to determine relevant <see cref="SyntaxNode"/>. It also handles incomplete selections
-        /// of tokens gracefully.
+        /// of tokens gracefully. Over-selection containing leading comments is also handled correctly. 
         /// </para>
         /// </summary>
         protected Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate<SyntaxNode> predicate, CancellationToken cancellationToken)
@@ -61,12 +61,12 @@ protected Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, TextSpan s
         /// potentially returned instead of current Node. 
         /// </para>
         /// <para>
-        /// Note: this function trims all whitespace from both the beginning and the end of given <paramref name="selection"/>.
+        /// Note: this function trims all whitespace from both the beginning and the end of given <paramref name="selectionRaw"/>.
         /// The trimmed version is then used to determine relevant <see cref="SyntaxNode"/>. It also handles incomplete selections
-        /// of tokens gracefully.
+        /// of tokens gracefully. Over-selection containing leading comments is also handled correctly. 
         /// </para>
         /// </summary>
-        protected async Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, TextSpan selection, Predicate<SyntaxNode> predicate, Func<SyntaxNode, ISyntaxFactsService, bool, SyntaxNode> extractNode, CancellationToken cancellationToken)
+        protected async Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, TextSpan selectionRaw, Predicate<SyntaxNode> predicate, Func<SyntaxNode, ISyntaxFactsService, bool, SyntaxNode> extractNode, CancellationToken cancellationToken)
         {
             // Given selection is trimmed first to enable over-selection that spans multiple lines. Since trailing whitespace ends
             // at newline boundary over-selection to e.g. a line after LocalFunctionStatement would cause FindNode to find enclosing
@@ -75,7 +75,7 @@ protected async Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, Text
 
             var syntaxFacts = document.GetLanguageService<ISyntaxFactsService>();
             var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
-            var selectionTrimmed = await CodeRefactoringHelpers.GetTrimmedTextSpan(document, selection, cancellationToken).ConfigureAwait(false);
+            var selectionTrimmed = await CodeRefactoringHelpers.GetTrimmedTextSpan(document, selectionRaw, cancellationToken).ConfigureAwait(false);
 
             // Every time a Node is considered by following algorithm (and tested with predicate) and the predicate fails
             // extractNode is called on the node and the result is tested with predicate again. If any of those succeed
@@ -94,24 +94,36 @@ protected async Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, Text
 
             // Handle selections:
             // - The smallest node whose FullSpan includes the whole (trimmed) selection
+            //   - Using FullSpan is important because it handles over-selection with comments
             // - Travels upwards through same-sized (FullSpan) nodes, extracting and testing predicate
             // - Handles situations where:
             //  - Token with wanted Node as direct parent is selected (e.g. IdentifierToken for LocalFunctionStatement: `C [|Fun|]() {}`) 
             //  - Most/the whole wanted Node is selected (e.g. `C [|Fun() {}|]`
-            var node = root.FindNode(selectionTrimmed, getInnermostNodeForTie: true);
-            SyntaxNode prevNode;
+            var selectionNode = root.FindNode(selectionTrimmed, getInnermostNodeForTie: true);
+            var prevNode = selectionNode;
             do
             {
-                var wantedNode = TryGetAcceptedNodeOrExtracted(node, predicate, extractNode, syntaxFacts, extractParentsOfHeader: false);
+                var wantedNode = TryGetAcceptedNodeOrExtracted(selectionNode, predicate, extractNode, syntaxFacts, extractParentsOfHeader: false);
                 if (wantedNode != null)
                 {
+                    // For selections we need to handle an edge case where only AttributeLists are within selection (e.g. `Func([|[in][out]|] arg1);`).
+                    // In that case the smallest encompassing node is still the whole argument node but it's hard to justify showing refactorings for it
+                    // if user selected only its attributes.
+
+                    // Selection contains only AttributeLists -> don't consider current Node
+                    var spanWithoutAttributes = GetSpanWithoutAttributes(wantedNode, root, syntaxFacts);
+                    if (!selectionTrimmed.IntersectsWith(spanWithoutAttributes))
+                    {
+                        break;
+                    }
+
                     return wantedNode;
                 }
 
-                prevNode = node;
-                node = node.Parent;
+                prevNode = selectionNode;
+                selectionNode = selectionNode.Parent;
             }
-            while (node != null && prevNode.FullWidth() == node.FullWidth());
+            while (selectionNode != null && prevNode.FullWidth() == selectionNode.FullWidth());
 
             // Handle what current selection is touching:
             //
@@ -134,7 +146,8 @@ protected async Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, Text
             // continue traveling upwards. The situation for right edge (`C methodName(){}[||]`) is analogical.
             // E.g. for right edge `C methodName(){}[||]`: CloseBraceToken -> BlockSyntax -> LocalFunctionStatement -> null (higher 
             // node doesn't end on position anymore)
-            if (!selection.IsEmpty)
+            // Note: left-edge climbing needs to handle AttributeLists explicitly, see below for more information. 
+            if (!selectionRaw.IsEmpty)
             {
                 return null;
             }
@@ -162,9 +175,29 @@ protected async Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, Text
                         return wantedNode;
                     }
 
-                    rightNode = rightNode?.Parent;
+                    rightNode = rightNode.Parent;
+                    if (rightNode == null)
+                    {
+                        break;
+                    }
+
+                    // The edge climbing for node to the right needs to handle Attributes e.g.:
+                    // [Test1]
+                    // //Comment1
+                    // [||]object Property1 { get; set; }
+                    // In essence:
+                    // - On the left edge of the node (-> left edge of first AttributeLists)
+                    // - On the left edge of the node sans AttributeLists (& as everywhere comments)
+                    if (rightNode.Span.Start != location)
+                    {
+                        var rightNodeSpanWithoutAttributes = GetSpanWithoutAttributes(rightNode, root, syntaxFacts);
+                        if (rightNodeSpanWithoutAttributes.Start != location)
+                        {
+                            break;
+                        }
+                    }
                 }
-                while (rightNode != null && rightNode.Span.Start == location);
+                while (true);
             }
 
             // if the location is inside tokenToRightOrIn -> no Token can be to Left (tokenToRightOrIn is also left from location, e.g: `tok[||]enITORightOrIn`)
@@ -195,9 +228,13 @@ protected async Task<SyntaxNode> TryGetSelectedNodeAsync(Document document, Text
                         return wantedNode;
                     }
 
-                    leftNode = leftNode?.Parent;
+                    leftNode = leftNode.Parent;
+                    if (leftNode != null || leftNode.Span.End != location)
+                    {
+                        break;
+                    }
                 }
-                while (leftNode != null && leftNode.Span.End == location);
+                while (true);
             }
 
             // nothing found -> return null
@@ -222,6 +259,28 @@ static SyntaxNode TryGetAcceptedNodeOrExtracted(SyntaxNode node, Predicate<Synta
             }
         }
 
+        private static TextSpan GetSpanWithoutAttributes(SyntaxNode node, SyntaxNode root, ISyntaxFactsService syntaxFacts)
+        {
+            // Span without AttributeLists
+            // - No AttributeLists -> original .Span
+            // - Some AttributeLists -> (first non-trivia/comment Token.Span.Begin, original.Span.End)
+            //   - We need to be mindful about comments due to:
+            //      // [Test1]
+            //      //Comment1
+            //      [||]object Property1 { get; set; }
+            //     the comment node being part of the next token's (`object`) leading trivia and not the AttributeList's node.
+            var attributeList = syntaxFacts.GetAttributeLists(node);
+            if (attributeList.Any())
+            {
+                var endOfAttributeLists = attributeList.Last().Span.End;
+                var afterAttributesToken = root.FindTokenOnRightOfPosition(endOfAttributeLists);
+
+                return TextSpan.FromBounds(afterAttributesToken.Span.Start, node.Span.End);
+            }
+
+            return node.Span;
+        }
+
         /// <summary>
         /// <para>
         /// Extractor function for <see cref="TryGetSelectedNodeAsync(Document, TextSpan, Predicate{SyntaxNode}, Func{SyntaxNode, ISyntaxFactsService, bool, SyntaxNode}, CancellationToken)"/> method 

src/Features/Core/Portable/CodeRefactorings/IRefactoringHelpersService.cs

@@ -22,13 +22,13 @@ internal interface IRefactoringHelpersService : ILanguageService
         /// - Whole node of a type <typeparamref name="TSyntaxNode"/> is selected.
         /// </para>
         /// <para>
-        /// Attempts extracting a Node of type <typeparamref name="TSyntaxNode"/> for each Node it consideres (see above).
-        /// By default extracts initializer expressions from declarations and assignments.
+        /// Attempts extracting a Node of type <typeparamref name="TSyntaxNode"/> for each Node it considers (see above).
+        /// E.g. extracts initializer expressions from declarations and assignments, Property declaration from any header node, etc.
         /// </para>
         /// <para>
         /// Note: this function trims all whitespace from both the beginning and the end of given <paramref name="selection"/>.
         /// The trimmed version is then used to determine relevant <see cref="SyntaxNode"/>. It also handles incomplete selections
-        /// of tokens gracefully.
+        /// of tokens gracefully. Over-selection containing leading comments is also handled correctly. 
         /// </para>
         /// </summary>
         Task<TSyntaxNode> TryGetSelectedNodeAsync<TSyntaxNode>(Document document, TextSpan selection, CancellationToken cancellationToken) where TSyntaxNode : SyntaxNode;

src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs

@@ -1136,7 +1136,7 @@ private static SyntaxNode WithAttributeArgumentList(SyntaxNode declaration, Attr
             return declaration;
         }
 
-        private static SyntaxList<AttributeListSyntax> GetAttributeLists(SyntaxNode declaration)
+        internal static SyntaxList<AttributeListSyntax> GetAttributeLists(SyntaxNode declaration)
         {
             switch (declaration)
             {

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs

@@ -1967,5 +1967,7 @@ public bool IsPartOfPropertyDeclarationHeader(SyntaxNode node)
         }
 
         public SyntaxNode GetContainingPropertyDeclaration(SyntaxNode node) => node.GetAncestor<PropertyDeclarationSyntax>();
+
+        public SyntaxList<SyntaxNode> GetAttributeLists(SyntaxNode node) => CSharpSyntaxGenerator.GetAttributeLists(node);
     }
 }

src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsService.cs

@@ -249,6 +249,7 @@ internal interface ISyntaxFactsService : ILanguageService
 
         bool IsAttribute(SyntaxNode node);
         bool IsAttributeName(SyntaxNode node);
+        SyntaxList<SyntaxNode> GetAttributeLists(SyntaxNode node);
 
         bool IsAttributeNamedArgumentIdentifier(SyntaxNode node);
         bool IsObjectInitializerNamedAssignmentIdentifier(SyntaxNode node);

src/Workspaces/VisualBasic/Portable/CodeGeneration/VisualBasicSyntaxGenerator.vb

@@ -1747,7 +1747,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeGeneration
             End Select
         End Function
 
-        Private Function GetAttributeLists(node As SyntaxNode) As SyntaxList(Of AttributeListSyntax)
+        Friend Shared Function GetAttributeLists(node As SyntaxNode) As SyntaxList(Of AttributeListSyntax)
             Select Case node.Kind
                 Case SyntaxKind.CompilationUnit
                     Return SyntaxFactory.List(DirectCast(node, CompilationUnitSyntax).Attributes.SelectMany(Function(s) s.AttributeLists))

src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSyntaxFactsService.vb

@@ -1960,5 +1960,9 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
         Public Function GetContainingPropertyDeclaration(node As SyntaxNode) As SyntaxNode Implements ISyntaxFactsService.GetContainingPropertyDeclaration
             Return node.GetAncestor(Of PropertyStatementSyntax)
         End Function
+
+        Public Function GetAttributeLists(node As SyntaxNode) As SyntaxList(Of SyntaxNode) Implements ISyntaxFactsService.GetAttributeLists
+            Return VisualBasicSyntaxGenerator.GetAttributeLists(node)
+        End Function
     End Class
 End Namespace