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

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

Closed
jhiston opened this issue Jun 4, 2018 · 2 comments · Fixed by #4092
Assignees
Labels
bug Identifies work items for known bugs feature-refactorings refactoring-rename regression This used to work, but something broke it. Fixes should involve a regression test, if possible

Comments

@jhiston
Copy link

jhiston commented Jun 4, 2018

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
@retailcoder retailcoder added bug Identifies work items for known bugs feature-refactorings refactoring-rename labels Jun 4, 2018
@Vogel612 Vogel612 added the regression This used to work, but something broke it. Fixes should involve a regression test, if possible label Jun 4, 2018
@MDoerner MDoerner self-assigned this Jun 9, 2018
@MDoerner
Copy link
Contributor

MDoerner commented Jun 9, 2018

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
Copy link
Contributor

MDoerner commented Jun 9, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identifies work items for known bugs feature-refactorings refactoring-rename regression This used to work, but something broke it. Fixes should involve a regression test, if possible
Projects
Development

Successfully merging a pull request may close this issue.

4 participants