Extract Method places new method call in incorrect line position for *some* extracts, causes parser/resolver/logical errors

[ThunderFrame]

Extract Method seems to cause a syntax error when the extracted statements are near the top bottom of the procedure, and possibly other situations.

Before:

Sub foo()
  Dim x As String
  x = "foo"
End Sub

After:

Sub foo()
End Sub
NewMethod
Private Sub NewMethod()
  Dim x As String
  x = "foo"
End Sub

[ThunderFrame]

I've added some line numbers as comments to help identify the bug.

Option Explicit '1
'2
Sub foo() '3
'4
  Dim x As String  ' 5
  x = "foo"        ' 6
'7
End Sub '8
'9
Sub bar() '10
'11
End Sub '12

I trigger the Extract Method command by right clicking anywhere in Line 6, and get this result:

Option Explicit '1
'2
Sub foo() '3
'4
'7
NewMethod
End Sub '8
Private Sub NewMethod()
  Dim x As String  ' 5
  x = "foo"        ' 6
End Sub
'9
Sub bar() '10
'11
End Sub '12

Note that the call to the new method, above, should be inserted between the lines commented with 4 and 7, but is instead inserted between lines commented with 7 and 8.

That can lead to changed execution paths/outcomes, like this, where the refactored code is never executed.

Before

Option Explicit '1
'2
Sub foo() '3
'4
  Dim x As String  ' 5
  x = "foo"        ' 6
  Exit Sub         ' 7
'8
End Sub '9

After

Option Explicit '1
'2
Sub foo() '3
'4
  Exit Sub         ' 7
NewMethod
'8
End Sub '9
Private Sub NewMethod()
  Dim x As String  ' 5
  x = "foo"        ' 6
End Sub

[ThunderFrame]

see #2285 for another example

[Vogel612]

This should be simple to fix. The problem is that the "newMethodCall" is inserted at the SelectionStart here

            var newMethod = constructLinesOfProc(codeModule, model);
            codeModule.InsertLines(positionToInsertNewMethod.StartLine, newMethod);
            removeSelection(codeModule, selectionToRemove);
            codeModule.InsertLines(selection.StartLine, newMethodCall);

This needs to be looked at a bit better. It might be possible to just insert one line earlier (but that could be a problem). Alternatively removeSelection should just leave a single line instead of completely removing every line selected.

[retailcoder]

Probably the same underlying issue as #2202

[Falthazar]

This is doing the same thing for me.

This:

    With asheet
        For Each colToDel In delCols:
            .Range("SiteTable[" & colToDel & "]").Delete
        Next colToDel
    End With

Turns into this:

Private Sub NewMethod(ByRef delCols As Variant, ByRef asheet As Worksheet)
        For Each colToDel In delCols:
    With asheet
            .Range("SiteTable[" & colToDel & "]").Delete
        Next colToDel
    End With
End Sub

[retailcoder]

@Falthazar aye, the extract method refactoring, as crucially important as it is, is currently considered broken. I'd recommend avoiding its use for now. I need to make a note to remove it from the refactorings menu for the 2.0.12 release.

[Vogel612]

Since the Extract Method refactoring is in dire need of a rewrite from scratch, all [bug] issues pertaining to it, are in the process of becoming irrelevant. Closing.