rubberduck-vba / Rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).
https://rubberduckvba.com
GNU General Public License v3.0
1.91k stars 299 forks source link

Refactor/rename of type within a class doesn't change the identifier of the type definition itself #4056

Closed jhiston closed 6 years ago

jhiston commented 6 years ago

This may be similar/rehash of #3468 / #3469 (please let me know if I should be just commenting on that to re-open?). I believe my installed RD version should incorporate the fix referenced there:

Version 2.2.6720.27013 OS: Microsoft Windows NT 10.0.16299.0, x64 Host Product: Microsoft Office 2016 x86 Host Version: 16.0.9330.2087 Host Executable: MSACCESS.EXE

However, the following code in a blank database and class module did not result in a rename of the UDT type name itself:

Private Type t_ToBeRenamed  'Cursor here, RD-->Refactor-->Rename, t_ToBeRenamed stays the same
    Blank as object
End Type

Private Sub ASub(aParam as t_ToBeRenamed) ' does get renamed to the new name
End Sub
MDoerner commented 6 years ago

Thanks for reporting this.

For some reason, tests for renaming UDTs have been missing. After adding them based on the tests for enumerations, I can see them fail on the reported issue.

MDoerner commented 6 years ago

The basic problem here is that we save a different level of parser context on UDT declarations than for all other declarations. This throws off the refactoring. More precisely, for UDTs we have a separate level encoding the accessability of the type and we save that on the declarations, which save the accessablility on a separate field, too.

There are basically three options how to fix this issue:

  1. Special case UDT declarations in the rename refactoring.
  2. Save the context one level deeper, i.e. the one without the accessability, on the declarations for UDTs.
  3. Merge the accessability into the deeper level and adapt the resolver accordingly.

The first option is the easiest but least clean one. Going that way, we might stuble over the same problem again for a future refactoring. The second option should still be relatively easy to do, but it would cause issues with the logic determining the selected declaration: the accesssability would no longer belong to the declaration. The third option seems to me to be the cleanest one, because, afterwards, the handling of accessability would be consistent across the grammar. It would certainly be the most work, but still doable.