Extract Method Bug With Method Directly in Namespace #53031

akhera99 · 2021-04-29T20:40:50Z

Hello!

I worked on a bug in which, if you paste a method directly into a namespace and highlight an expression in the method, ExtractMethod crashes. To fix this, I look at the GetValidSelectionAsync methods in C# and VB. This calls GetInitialSelectionInfo() which returns valid or invalid selections. I added a check to see if the highlighted expression is contained within some block that methods could be contained in, Classes, Records, Structs, Modules, etc. I was going to make this change in UnderValidContext() but the existing code works to short-circuit if the higlighted code is an ExpressionSyntax.

Please let me know if there is a better way to design this that fits with the design of the existing extractmethod code better!

src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionValidator.vb

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionValidator.cs

CyrusNajmabadi · 2021-04-29T22:51:58Z

Bonjour!

src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs

CyrusNajmabadi · 2021-04-29T22:53:48Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionValidator.cs

+            {
+                return new SelectionInfo
+                {
+                    Status = new OperationStatus(OperationStatusFlag.None, CSharpFeaturesResources.No_valid_selection_to_perform_extraction),


i think we might want to condier a better string message here. SPecifically "Selection was not contained inside a type"

also, do we ahve tests for extracing from local functions inside the root namespace? we don't want to break that (if that works today. ) if it doesn't work prior to your change, tehn no worries.

"Selection was not contained inside a type"

This may not be a problem for top level statements right?

"Also, do we ahve tests for extracing from local functions inside the root namespace?"

Is that valid?

yup. top level functions are a thing :)

top level functions seem to have been added in C# 9? How can I test if it works?

There are no tests for extracting from the root namespace, and I tested it and it has the same error as the bug linked.

src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionValidator.vb

…hlighted expression

ryzngard · 2021-07-20T22:43:06Z

I should take a look at this soon

ryzngard · 2021-07-28T20:43:32Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionValidator.cs

+            /*var rootGlobalStatement = commonRoot.FirstAncestorOrSelf<SyntaxNode>(node => node is GlobalStatementSyntax);
+            var commonRootContainer = commonRoot.FirstAncestorOrSelf<SyntaxNode>(node => node is TypeDeclarationSyntax);
+
+            if (commonRootContainer is null && rootGlobalStatement is null)
+            {
+                return new SelectionInfo
+                {
+                    Status = new OperationStatus(OperationStatusFlag.None, FeaturesResources.Selection_not_contained_inside_a_type),
+                    OriginalSpan = adjustedSpan,
+                    FirstTokenInOriginalSpan = firstTokenInSelection,
+                    LastTokenInOriginalSpan = lastTokenInSelection
+                };
+            }*/


Remove this

ryzngard · 2021-07-28T20:44:35Z

src/Features/CSharp/Portable/ExtractMethod/Extensions.cs

-                    n is AccessorDeclarationSyntax ||
-                    n is BlockSyntax ||
-                    n is GlobalStatementSyntax)
+                if (n is BaseMethodDeclarationSyntax or AccessorDeclarationSyntax or BlockSyntax or GlobalStatementSyntax)


Suggested change

if (n is BaseMethodDeclarationSyntax or AccessorDeclarationSyntax or BlockSyntax or GlobalStatementSyntax)

if (n is BaseMethodDeclarationSyntax or

AccessorDeclarationSyntax or

BlockSyntax or

GlobalStatementSyntax)

CyrusNajmabadi · 2021-07-28T22:21:16Z

src/EditorFeatures/CSharpTest/ExtractMethod/ExtractMethodTests.cs

@@ -9188,13 +9188,13 @@ private static K NewMethod()
        public async Task Script_ArgumentException()
        {
            var code = @"using System;
-public static void GetNonVirtualMethod<TDelegate>( Type type, string name)
+static void GetNonVirtualMethod<TDelegate>( Type type, string name)


please don't update test cases. instead still record the change in behavior for this case, and add a new test for the differnet case you carea bout.

to be honest, I do not know why I even touched this method

CyrusNajmabadi · 2021-07-28T22:21:59Z

src/Features/CSharp/Portable/ExtractMethod/Extensions.cs

+            if (node.FromScript() || node.GetAncestor<TypeDeclarationSyntax>() != null)
+            {
+                return true;
+            }


these checks need comments.

CyrusNajmabadi · 2021-07-28T22:22:32Z

src/Features/CSharp/Portable/ExtractMethod/Extensions.cs

+        {
+            Contract.ThrowIfNull(node);
+
+            if (IsContainedDirectlyInNamespace(node))


Suggested change

if (IsContainedDirectlyInNamespace(node))

return !IsContainedDirectlyInNamespace(node);

CyrusNajmabadi · 2021-07-28T22:22:54Z

src/Features/CSharp/Portable/ExtractMethod/Extensions.cs

+                        immediateParent = true;
+                    }
+
+                    n = n.Parent!;


i do not know what is going on here. in english what are you trying to express :)

if I am a method, or a block, etc. and my immediate parent is a namespace, then bail and do not allow that as a valid selection. Still need to make sure I am handling everything that shouldn't be directly in a namespace

would it suffice to just ask: did i hit a namespace, but i did not hit a type? that seems simpler. or, put another way:

first look if we're contained by a type. if so, great.
then look to see if we're contained by a namespace. if so, bad.
otherwise, we must be global. if so, great.

CyrusNajmabadi · 2021-07-28T22:23:16Z

src/Features/VisualBasic/Portable/xlf/VBFeaturesResources.zh-Hant.xlf

@@ -332,21 +332,6 @@
        <target state="translated">選取範圍無效</target>
        <note />
      </trans-unit>
-      <trans-unit id="Selection_doesn_t_contain_any_valid_token">


can you make the loc change in a separate PR?

ryzngard · 2021-08-04T03:37:31Z

src/Features/CSharp/Portable/ExtractMethod/Extensions.cs

+        public static bool ContainedInValidType(this SyntaxNode node)
+        {
+            Contract.ThrowIfNull(node);
+            var typeContainer = node.FirstAncestorOrSelf<TypeDeclarationSyntax>();


Suggested change

var typeContainer = node.FirstAncestorOrSelf<TypeDeclarationSyntax>();

foreach (var ancestor in node.AncestorsAndSelf())

{

if (ancestor is TypeDeclarationSyntax)

return true;

if (ancestor is NamespaceDeclarationSyntax)

return false;

}

return true;

ryzngard · 2021-08-04T03:40:41Z

Current changes look good, pending test passing and conflict resolution

…/roslyn into bug/44214_extractmethod

akhera99 added 3 commits April 27, 2021 17:02

check to see if the expression is contained within a class

3ce9dc6

removed unused var

6af6f2d

added VB code

3abd5a8

akhera99 requested a review from CyrusNajmabadi April 29, 2021 20:40

akhera99 self-assigned this Apr 29, 2021

akhera99 requested a review from a team as a code owner April 29, 2021 20:40

dotnet-issue-labeler bot added the Area-IDE label Apr 29, 2021

akhera99 requested a review from ryzngard April 29, 2021 21:09

Youssef1313 reviewed Apr 29, 2021

View reviewed changes

src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionValidator.vb Outdated Show resolved Hide resolved

Youssef1313 reviewed Apr 29, 2021

View reviewed changes

src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionValidator.vb Outdated Show resolved Hide resolved

style fixes

cb1bc5e

davidwengier reviewed Apr 29, 2021

View reviewed changes

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionValidator.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 29, 2021

View reviewed changes

src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 29, 2021

View reviewed changes

src/Features/VisualBasic/Portable/ExtractMethod/VisualBasicSelectionValidator.vb Outdated Show resolved Hide resolved

akhera99 added 2 commits April 30, 2021 15:38

responded to feedback

69e3785

Fixed a bug where extraction in an interface would not update the hig…

f897ac9

…hlighted expression

akhera99 requested a review from CyrusNajmabadi May 3, 2021 19:25

akhera99 and others added 5 commits May 4, 2021 10:07

Added root namespace test

5f8643b

Merge branch 'main' into bug/44214_extractmethod

4575875

work for local function top level statements

4680e94

passes tests

5b77500

fixed issues in vb

154804c

akhera99 added 2 commits July 21, 2021 10:13

working on bug

984df0c

check for immediate parent being namespace

d4a4b49

ryzngard reviewed Jul 28, 2021

View reviewed changes

CyrusNajmabadi reviewed Jul 28, 2021

View reviewed changes

akhera99 added 5 commits July 29, 2021 09:01

pr feedback

c812304

simplify logic

8c5d2a0

pr feedback

836091e

changed logic

6a10c61

fixing vb side as well

79aa517

akhera99 requested review from ryzngard and CyrusNajmabadi August 3, 2021 22:30

ryzngard reviewed Aug 4, 2021

View reviewed changes

ryzngard approved these changes Aug 4, 2021

View reviewed changes

akhera99 and others added 9 commits August 4, 2021 09:46

pr feedback

2354aff

Merge branch 'main' into bug/44214_extractmethod

43cdacb

merge main

cce2d14

Merge branch 'bug/44214_extractmethod' of https://github.com/akhera99…

817e7e5

…/roslyn into bug/44214_extractmethod

fixing dupes

f341e0a

fixed bad logic

6868963

fix method

5486f2d

fixwarnign

1c1dcae

Merge branch 'main' into bug/44214_extractmethod

e7829be

akhera99 merged commit 4f2bc46 into dotnet:main Aug 11, 2021

ghost added this to the Next milestone Aug 11, 2021

akhera99 mentioned this pull request Aug 11, 2021

Version 16.11.0 Preview 2.0 - InvalidOperationException Unexpected false #54713

Closed

jaredpar mentioned this pull request Aug 17, 2021

Attempt to load System.Runtime reference assembly for execution causes test failure #53391

Open

dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract Method Bug With Method Directly in Namespace #53031

Extract Method Bug With Method Directly in Namespace #53031

akhera99 commented Apr 29, 2021

CyrusNajmabadi commented Apr 29, 2021

CyrusNajmabadi Apr 29, 2021

ryzngard Apr 30, 2021

CyrusNajmabadi Apr 30, 2021

akhera99 May 3, 2021

akhera99 May 4, 2021

ryzngard commented Jul 20, 2021

ryzngard Jul 28, 2021

ryzngard Jul 28, 2021

CyrusNajmabadi Jul 28, 2021

akhera99 Jul 28, 2021

CyrusNajmabadi Jul 28, 2021

CyrusNajmabadi Jul 28, 2021

CyrusNajmabadi Jul 28, 2021

akhera99 Jul 30, 2021

CyrusNajmabadi Jul 30, 2021

CyrusNajmabadi Jul 28, 2021

akhera99 Jul 30, 2021

ryzngard Aug 4, 2021

ryzngard commented Aug 4, 2021

	if (IsContainedDirectlyInNamespace(node))
	return !IsContainedDirectlyInNamespace(node);

-            var typeContainer = node.FirstAncestorOrSelf<TypeDeclarationSyntax>();
+           foreach (var ancestor in node.AncestorsAndSelf())
+           {
+               if (ancestor is TypeDeclarationSyntax)
+                   return true;
+               if (ancestor is NamespaceDeclarationSyntax)
+                   return false;
+           }
+           return true;

Extract Method Bug With Method Directly in Namespace #53031

Extract Method Bug With Method Directly in Namespace #53031

Conversation

akhera99 commented Apr 29, 2021

CyrusNajmabadi commented Apr 29, 2021

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

Choose a reason for hiding this comment

ryzngard commented Jul 20, 2021

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

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

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

ryzngard commented Aug 4, 2021