Skip to content

[NETBEANS-4274] fix refactoring of class member when a new name is the same as the name of a local variable - #2612

Merged
neilcsmith-net merged 2 commits into
apache:masterfrom
jherkel:netbeans-4274
Jan 17, 2022
Merged

[NETBEANS-4274] fix refactoring of class member when a new name is the same as the name of a local variable#2612
neilcsmith-net merged 2 commits into
apache:masterfrom
jherkel:netbeans-4274

Conversation

@jherkel

@jherkel jherkel commented Dec 28, 2020

Copy link
Copy Markdown
Contributor

String useSuper = null;

if (elementToFind!=null && elementToFind.getKind().isField()) {
if (elementToFind!=null && elementToFind.getKind().isField() && tree.getKind() != Tree.Kind.MEMBER_SELECT) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add reasoning why you propose a change. In this case, the change looks wrong, as it contradicts the check in line 262. That branch can't be reached anymore with your change in place.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think the whole if statement started from line 262 can be deleted. Because this part of code (started from line 257) checks if a new name for refactored field doesn't have the same name as some local variables do. For this case it is adding an outer scope (this or class name for static fields). But I think if field is a member i.e. tree.getKind() == Tree.Kind.MEMBER_SELECT we don't need to do anything because we have a scope.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with @jherkel - the code on lines 262-267 does not make much sense to me. If a reference to a field is qualified, it cannot clash with a local variable (AFAIK), even if the qualifier is different from this. I agree we should delete the whole if starting at 262. Maybe instead of tree.getKind() != Tree.Kind.MEMBER_SELECT do && tree.getKind() == Tree.Kind.IDENTIFIER, because I believe that is the only case where the clash can occur.

@matthiasblaesing

Copy link
Copy Markdown
Contributor

@sarveshkesharwani @rtaneja1 @jlahoda could you please have a look at this?

@geertjanw

Copy link
Copy Markdown
Member

Any reason not to merge?

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) kind:bug Bug report or fix labels Jan 4, 2022
@mbien mbien added this to the NB13 milestone Jan 13, 2022
@mbien

mbien commented Jan 13, 2022

Copy link
Copy Markdown
Member

added to NB13 milestone to put it back on the radar (seems like reviewer remarks had been addressed)

edit: one job errors when i restart it. It might need a rebase on latest master to make the CI happy - not sure why this happens.

@neilcsmith-net

Copy link
Copy Markdown
Member

Thanks @mbien Let's merge before branch and check in rc for any issues.

@neilcsmith-net
neilcsmith-net merged commit b3f96a2 into apache:master Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) kind:bug Bug report or fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants