Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evil Test Case 2 for Parser/Resolver #973

Closed
ThunderFrame opened this issue Jan 27, 2016 · 6 comments
Closed

Evil Test Case 2 for Parser/Resolver #973

ThunderFrame opened this issue Jan 27, 2016 · 6 comments
Assignees
Labels
parse-tree-processing technical-debt This makes development harder or is leftover from a PullRequest. Needs to be adressed at some point.
Milestone

Comments

@ThunderFrame
Copy link
Member

1 Module and 2 Classes

There's an Interface, so The Locals Window will not help you!

'================================================================
'Project Name : MyProject
'Module Name  : MyModule

Option Explicit

Public Const MyProject As String = "String"
Public Const MyModule As String = MyProject
Public Const MyConst As String = MyModule

Public MyProc As Interface

Type MyProc
  MyModule As String
  MyProject As String
End Type

Type MyModule
  MyProject As MyProc
End Type

Type MyProject
  MyModule As MyModule
End Type

Sub MyProc1()
  Dim MyProject As Interface
  Dim MyModule
  Dim MyCircular
  Dim o
  Set MyProject = New Interface
  Set MyProject.MyModule = MyProject
  Set MyModule = New Interface
  Set MyProject.MyModule = MyModule
  With MyProject
    With MyModule
      Set MyProject = New Class
      Set MyProject.MyModule = New MyProject.Class
      Set MyProject.MyModule.MyProc = MyProject
      Set o = MyProject.MyModule
      Set MyModule.MyProc = MyProject.MyModule
      Set .MyProc = MyProject.MyModule
      On Error Resume Next
      Set .MyModule.MyProject.MyModule.MyProject = .MyModule.MyProject
      On Error GoTo 0
    End With
    Set .MyProc = .MyModule
  End With
End Sub
'================================================================
'Project Name : MyProject
'Class Module Name  : Interface
Option Explicit

Public Property Get MyProject() As Interface
End Property

Public Property Set MyProject(MyModule As Interface)
End Property

Public Property Get MyModule() As Interface
End Property

Public Property Set MyModule(MyProject As Interface)
End Property

Public Property Get MyProc() As Interface
End Property

Public Property Set MyProc(MyProject As Interface)
End Property


'================================================================
'Project Name : MyProject
'Class Module Name  : Class
Option Explicit

Implements Interface

Private MyProject As MyModule

Private Type MyModule
  MyProject As Interface
  MyModule As Interface
  MyProc As Interface
End Type

Private Property Set Interface_MyModule(MyModule As Interface)
  Set MyProject.MyModule = MyModule
End Property

Private Property Get Interface_MyModule() As Interface
  Set Interface_MyModule = MyProject.MyModule
End Property

Private Property Set Interface_MyProc(MyModule As Interface)
  Set MyProject.MyProject = MyModule
End Property

Private Property Get Interface_MyProc() As Interface
  Set MyProject.MyModule = MyProject.MyProc
End Property

Private Property Set Interface_MyProject(MyModule As Interface)
  Set MyProject.MyProc = MyModule
End Property

Private Property Get Interface_MyProject() As Interface
  Set Interface = New Interface
End Property
@retailcoder retailcoder added this to the Version 2.0 milestone Jan 27, 2016
@retailcoder retailcoder self-assigned this Jan 27, 2016
@retailcoder
Copy link
Member

Ooh nested With blocks! I like that one! 😄

@retailcoder
Copy link
Member

Okay this is one interesting test for refactor/rename 2.0 - renaming the interface in 1.4.3 wrecks the code, by renaming all references to the class name, but not the class itself, and not the implementation members.

Renaming the private MyProject variable with 1.4.3 breaks the code by renaming nothing other than the variable itself; "Find all references" finds no references to it.

Renaming the target of the last assignment in the nested With, set .MyProc = .MyModule, also wrecks the code. It correctly renames the actual interface member (both Get and Set accessors):

Public Property Get MyRenamedProc() As Interface
End Property

Public Property Set MyRenamedProc(MyProject As Interface)
End Property

But wrecks the implementations:

Private Property Set Interface_MyRenamedProcmedProc(MyModule As Interface)
  Set MyProject.MyProject = MyModule
End Property

Private Property Get Interface_MyRenamedProcmedProc() As Interface
  Set MyProject.MyModule = MyProject.MyProc
End Property

...and best of all, doesn't rename the call site:

    Set .MyProc = .MyModule

In other words, this code reveals all of 1.4.3's major flaws.

I'll test it against the 2.0 resolver and rename refactoring tonight.

@retailcoder
Copy link
Member

Ok.

This bug is still present in 2.0 - renaming an interface member wrecks the implementations:

Private Property Set Interface_MyRenamedProcmedProc(MyModule As Interface)
  Set MyProject.MyProject = MyModule
End Property

Private Property Get Interface_MyRenamedProcmedProc() As Interface
  Set MyProject.MyModule = MyProject.MyProc
End Property

The .MyProc bug observed in the 1.4.3 build is also present; the interface members get renamed correctly, the implementations are renamed in a broken way (to Interface_MyRenamedProcmedProc), and the call site that's selected, keeps its original name. clearly that's a bug in the rename refactoring, ...but not in the resolver.

I was able to rename the Class class from this line:

  Set MyProject = New Class

But not from this one:

  Set MyProject.MyModule = New MyProject.Class

The refactoring correctly renamed the actual class module, but then there was this:

  Set MyProject = New RenamedClass
  Set MyProject.MyModule = New MyProject.Class

...which looks very much like a resolver bug. In fact, MyProject in New MyProject.Class resolves to the MyProject local variable. Renaming that variable to foo gives me this:

Sub MyProc1()
  Dim foo As Interface
  Dim MyModule
  Dim MyCircular
  Dim o
  Set foo = New Interface
  Set foo.MyModule = foo
  Set MyModule = New Interface
  Set foo.MyModule = MyModule
  With foo
    With MyModule
      Set foo = New Class
      Set foo.MyModule = New foo.Class
      Set foo.MyModule.MyProc = foo
      Set o = foo.MyModule
      Set MyModule.MyProc = foo.MyModule
      Set .MyProc = foo.MyModule
      On Error Resume Next
      Set .MyModule.foo.MyModule.MyProject = .MyModule.MyProject
      On Error GoTo 0
    End With
    Set .MyProc = .MyModule
  End With
End Sub

...which reveals further issues, notably here:

      Set .MyModule.foo.MyModule.MyProject = .MyModule.MyProject

Where .MyModule stands for Interface.MyModule, which is of type Interface, so the MyProject should have resolved to Interface.MyProject (which is also of type Interface), but instead resolved to the local variable... which is completely nuts.

In other words... you've just sent me back to the drawing board with this one.

@retailcoder
Copy link
Member

Okay, so, tackling this elephant... one bite at a time.

Set foo = New {Type} is not entering the AsTypeClause pipeline because it's not an AsTypeClause, but a ValueStmt... where the "value" is a new object reference, not a type.

Take this instruction:

Set MyProject.MyModule = New MyProject.Class

The assignment target on the left side, is correctly resolved. The right-side member call however, doesn't take the New keyword into consideration, and that's why MyProject in MyProject.Class resolves to the local variable MyProject - I need to take the context and verify if the parent of the parent of its parent is a VsNewContext, in which case I'll be armed to switch to a different strategy to resolve it as a project or module declaration (well, project in this particular case).

I'll fix that, and then I'll implement the find all references XAML UI before continuing, because find all references is a much better tool than the rename refactoring to assist with this... especially given the bugs in the rename refactoring that this code highlights.

@retailcoder
Copy link
Member

Okay. One down. I've refactored the logic I had implemented to resolve ComplexTypeContext, into its own method working with an array of AmbiguousIdentifierContext objects, and now when the resolver encounters a ICS_S_MembersCallContext where the parent of the parent of its parent is a VsNewContext, I'm passing it to this method:

    private void ResolveType(VBAParser.ICS_S_MembersCallContext context)
    {
        var first = context.iCS_S_VariableOrProcedureCall().ambiguousIdentifier();
        var identifiers = new[] {first}.Concat(context.iCS_S_MemberCall()
                    .Select(member => member.iCS_S_VariableOrProcedureCall().ambiguousIdentifier()))
                    .ToList();
        ResolveType(identifiers);
    }

As a result, = New MyProject.Class now resolves correctly.

retailcoder added a commit to retailcoder/Rubberduck that referenced this issue Jan 28, 2016
@ThunderFrame
Copy link
Member Author

I'll have throw some events into test case 3, passing some globals and some interfaces as arguments. Maybe some as byref and some as byval.... :-)

@Vogel612 Vogel612 modified the milestones: Version 2.0, v2.1 Aug 29, 2016
@Vogel612 Vogel612 added technical-debt This makes development harder or is leftover from a PullRequest. Needs to be adressed at some point. and removed quality-control labels Nov 16, 2017
@Vogel612 Vogel612 modified the milestones: Version 2.1.x, 2.3.1 Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parse-tree-processing technical-debt This makes development harder or is leftover from a PullRequest. Needs to be adressed at some point.
Projects
None yet
Development

No branches or pull requests

3 participants