redhat-developer / vscode-java

Java Language Support for Visual Studio Code
Eclipse Public License 2.0
2.08k stars 441 forks source link

Using the "Extract as a local variable (replacing all occurrences)" feature changes only one to a variable #3860

Open khpark0203 opened 1 week ago

khpark0203 commented 1 week ago

There seems to be no difference between the functionality of "Extract as a local variable" and the functionality of "Extract as a local variable (replacing all occurrences)." If you use this feature in a common way as a variable, only one selected location will be changed. I hope all the places written in common will be changed collectively. I love vscode, but I hope this feature works well in intelij works well in vscode java as well.

rgrunber commented 6 days ago

I have seen the behaviour you're referring to just trying things out on a simple examples, but when I reverse engineered what you posted in the screenshots, it does seem to work as expected :

https://github.com/user-attachments/assets/4279efb7-1140-4633-8321-0d19ea6124c4

There might be some cases that aren't being properly handled so I'll have to look deeper.

khpark0203 commented 4 days ago

It’s really amazing. I also confirmed that my other code for testing does not work. Is it irrelevant to JDK's version? I'm using version 21.

https://github.com/user-attachments/assets/422c3eaa-4834-4e4c-95aa-68142ea157be

snjeza commented 4 days ago

This is an upstream Eclipse jdt.ui/Lombok issue. I can reproduce it in Eclipse, too.

Test class

package com.example;
import lombok.AllArgsConstructor;
import lombok.Data;
public class Main {
    public void test() {
        Person p = new Person(10, "abc");
        System.out.println(p.getAge() + p.getName());
        System.out.println(p.getAge() + p.getName());
        System.out.println(p.getAge() + p.getName());
    }
    @Data
    @AllArgsConstructor
    class Person {
        private Integer age;
        private String name;
    }
}

https://github.com/user-attachments/assets/cffc62df-5e5e-427b-ab38-d26416691123

Related jdt ui code: https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/2886912fd00e7d4f0dc50af8ff17cbde0007a382/org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/util/SideEffectChecker.java#L160-L169

khpark0203 commented 4 days ago

Oh thank you!! As a result of the test, this is what happens when converting classes with @Getter from Lombok.

When implementing Getter methods directly, the entire variable is well transformed.

Can you modify this so that it can work the same even if you have a Getter in Lombok?

snjeza commented 4 days ago

@khpark0203 Could you create a new Lombok issue? - https://github.com/projectlombok/lombok/issues/new/

khpark0203 commented 4 days ago

@snjeza, I posted the issue on the GitHub you told me about. https://github.com/projectlombok/lombok/issues/3783

khpark0203 commented 4 days ago

@rgrunber I found the cause of this issue. I would appreciate it if you could help me solve it. (Lombok @Getter Issue)

rgrunber commented 3 days ago

Is there some piece of information that Lombok should provide that is lacking for JDT to do this correctly ? Why is Lombok to blame here ?

snjeza commented 3 days ago

Why is Lombok to blame here ?

Lombok doesn't intercept NodeFinder.perform at https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/2886912fd00e7d4f0dc50af8ff17cbde0007a382/org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/util/SideEffectChecker.java#L160