From 32696d9926adbf56305b6081dac7c4a8da4de296 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 22 May 2025 01:35:36 +0000 Subject: [PATCH 1/2] Refactor and test AddFinalIntention This commit refactors the AddFinalIntention action for improved behavior and adds comprehensive tests. Key changes: - The `isAvailable` method is now more context-aware, only enabling the intention when the caret is on a variable (parameter, local variable) or within a method scope suitable for adding 'final' modifiers. - The `invoke` method has been rewritten to: - Prioritize making a specific variable final if the caret is on it. - Fall back to making all eligible parameters and local variables final if the caret is within a method but not on a specific variable. - Avoid adding the 'final' modifier if it's already present on an element. - A comprehensive test suite (`AddFinalIntentionTest.java`) has been added, utilizing `LightJavaCodeInsightFixtureTestCase`. The tests cover: - Adding 'final' to single parameters and local variables. - Adding 'final' to all applicable elements in a method scope. - Ensuring no changes occur for already 'final' elements. - Verifying the intention is not available in irrelevant contexts (e.g., class declarations, import statements). - Basic checks for field variables (though full field support might be a future enhancement). These changes make the intention more robust, predictable, and maintainable. --- .../plugin/intention/AddFinalIntention.java | 62 +++++--- .../intention/AddFinalIntentionTest.java | 146 ++++++++++++++++++ 2 files changed, 190 insertions(+), 18 deletions(-) create mode 100644 src/test/java/lwm/plugin/intention/AddFinalIntentionTest.java diff --git a/src/main/java/lwm/plugin/intention/AddFinalIntention.java b/src/main/java/lwm/plugin/intention/AddFinalIntention.java index a8d9c67..92b7a17 100644 --- a/src/main/java/lwm/plugin/intention/AddFinalIntention.java +++ b/src/main/java/lwm/plugin/intention/AddFinalIntention.java @@ -12,6 +12,7 @@ import org.jetbrains.annotations.NotNull; import java.util.Arrays; +import java.util.Collection; /** * @author longwm @@ -29,35 +30,60 @@ public class AddFinalIntention implements IntentionAction { @Override public boolean isAvailable(@NotNull Project project, Editor editor, PsiFile file) { - return file instanceof PsiJavaFile; + if (!(file instanceof PsiJavaFile)) { + return false; + } + final PsiElement element = file.findElementAt(editor.getCaretModel().getOffset()); + if (element == null) { + return false; + } + + if (element instanceof PsiVariable) { + return true; + } + + return PsiTreeUtil.getParentOfType(element, PsiMethod.class) != null; } @Override public void invoke(@NotNull Project project, Editor editor, PsiFile file) throws IncorrectOperationException { final PsiElement element = file.findElementAt(editor.getCaretModel().getOffset()); + if (element == null) { + return; + } - final PsiMethod method = PsiTreeUtil.getParentOfType(element, PsiMethod.class); - if (method != null) { - final PsiParameterList parameterList = method.getParameterList(); - Arrays.stream(parameterList.getParameters()) - .forEach(psiParameter -> PsiUtil.setModifierProperty(psiParameter, PsiModifier.FINAL, true)); - final PsiCodeBlock methodBody = method.getBody(); + PsiVariable specificVariable = PsiTreeUtil.getParentOfType(element, PsiVariable.class, false); + if (specificVariable != null) { + addFinalModifierIfNotPresent(specificVariable); + return; + } + + final PsiMethod containingMethod = PsiTreeUtil.getParentOfType(element, PsiMethod.class); + if (containingMethod != null) { + // Add final to parameters + final PsiParameterList parameterList = containingMethod.getParameterList(); + for (PsiParameter parameter : parameterList.getParameters()) { + addFinalModifierIfNotPresent(parameter); + } + + // Add final to local variables + final PsiCodeBlock methodBody = containingMethod.getBody(); if (methodBody != null) { - final PsiStatement[] statements = methodBody.getStatements(); - Arrays.stream(statements) - .filter(PsiDeclarationStatement.class::isInstance) - .map(psiStatement -> ((PsiDeclarationStatement) psiStatement).getDeclaredElements()) - .flatMap(Arrays::stream) - .filter(PsiLocalVariable.class::isInstance) - .forEach(psiElement -> PsiUtil.setModifierProperty((PsiLocalVariable) psiElement, PsiModifier.FINAL, true)); + Collection localVariables = PsiTreeUtil.collectElementsOfType(methodBody, PsiLocalVariable.class); + for (PsiLocalVariable localVariable : localVariables) { + addFinalModifierIfNotPresent(localVariable); + } } } + } - final PsiVariable psiVariable = PsiTreeUtil.getParentOfType(element, PsiVariable.class); - if (psiVariable != null) { - PsiUtil.setModifierProperty(psiVariable, PsiModifier.FINAL, true); + private void addFinalModifierIfNotPresent(PsiModifierListOwner element) { + if (element != null) { + PsiModifierList modifierList = element.getModifierList(); + if (modifierList != null && !modifierList.hasExplicitModifier(PsiModifier.FINAL)) { + PsiUtil.setModifierProperty(element, PsiModifier.FINAL, true); + } } - } @Override diff --git a/src/test/java/lwm/plugin/intention/AddFinalIntentionTest.java b/src/test/java/lwm/plugin/intention/AddFinalIntentionTest.java new file mode 100644 index 0000000..8340232 --- /dev/null +++ b/src/test/java/lwm/plugin/intention/AddFinalIntentionTest.java @@ -0,0 +1,146 @@ +package lwm.plugin.intention; + +import com.intellij.codeInsight.intention.IntentionAction; +import com.intellij.testFramework.fixtures.LightJavaCodeInsightFixtureTestCase; +import java.util.List; +import java.util.stream.Collectors; + +public class AddFinalIntentionTest extends LightJavaCodeInsightFixtureTestCase { + + private static final String INTENTION_TEXT = "Add final modifier(添加final修饰)"; + + private void doTest(String before, String after) { + myFixture.configureByText("Test.java", before.replace("", "")); // Ensure caret is processed + final IntentionAction intention = myFixture.findSingleIntention(INTENTION_TEXT); + assertNotNull("Intention '" + INTENTION_TEXT + "' not found", intention); + myFixture.launchAction(intention); + myFixture.checkResult(after); + } + + private void doTestNotAvailable(String content) { + myFixture.configureByText("Test.java", content.replace("", "")); // Ensure caret is processed + List intentions = myFixture.getAvailableIntentions(); + List intentionTexts = intentions.stream() + .map(IntentionAction::getText) + .collect(Collectors.toList()); + assertFalse("Intention '" + INTENTION_TEXT + "' should not be available. Available: " + intentionTexts, + intentionTexts.contains(INTENTION_TEXT)); + } + + private void doTestNoChange(String contentWithCaret) { + String contentWithoutCaret = contentWithCaret.replace("", ""); + myFixture.configureByText("Test.java", contentWithCaret); + // Attempt to find the intention. It should be available. + final IntentionAction intention = myFixture.findSingleIntention(INTENTION_TEXT); + assertNotNull("Intention '" + INTENTION_TEXT + "' should be available but was not found.", intention); + myFixture.launchAction(intention); + myFixture.checkResult(contentWithoutCaret); // Ensure no change happened + } + + public void testAddFinalToSingleParameter() { + String before = "class Test {\n" + + " void method(String param1, int param2) {}\n" + + "}"; + String after = "class Test {\n" + + " void method(final String param1, int param2) {}\n" + + "}"; + doTest(before, after); + } + + public void testAddFinalToSingleLocalVariable() { + String before = "class Test {\n" + + " void method() {\n" + + " String local1 = \"hello\";\n" + + " int local2 = 10;\n" + + " }\n" + + "}"; + String after = "class Test {\n" + + " void method() {\n" + + " final String local1 = \"hello\";\n" + + " int local2 = 10;\n" + + " }\n" + + "}"; + doTest(before, after); + } + + public void testAddFinalToAllInMethodScope() { + // Caret is removed in 'after' string by checkResult implicitly if not present. + String before = "class Test {\n" + + " void method(String param1, int param2) {\n" + + " \n" + + " String local1 = \"hello\";\n" + + " int local2 = 10;\n" + + " final String alreadyFinal = \"done\";\n" + + " }\n" + + "}"; + String after = "class Test {\n" + + " void method(final String param1, final int param2) {\n" + + " \n" + + " final String local1 = \"hello\";\n" + + " final int local2 = 10;\n" + + " final String alreadyFinal = \"done\";\n" + + " }\n" + + "}"; + doTest(before, after); + } + + public void testNotAvailableOnAlreadyFinalParameter() { + String content = "class Test {\n" + + " void method(final String param1) {}\n" + + "}"; + // As per current isAvailable & invoke logic, intention is available but makes no change. + doTestNoChange(content); + } + + public void testNotAvailableOnAlreadyFinalLocalVariable() { + String content = "class Test {\n" + + " void method() {\n" + + " final String local1 = \"hello\";\n" + + " }\n" + + "}"; + // As per current isAvailable & invoke logic, intention is available but makes no change. + doTestNoChange(content); + } + + public void testNotAvailableOnClassDeclaration() { + String content = "class Test {\n" + + " void method(String param1) {}\n" + + "}"; + doTestNotAvailable(content); + } + + public void testNotAvailableInImportStatement() { + String content = "import java.util.List;\n" + + "class Test {\n" + + " void method(String param1) {}\n" + + "}"; + doTestNotAvailable(content); + } + + public void testNotAvailableOnFieldName() { + // The current isAvailable will make it available for fields. + // The invoke method will make the field final. + String before = "class Test {\n" + + " String myField = \"value\";\n" + + "}"; + String after = "class Test {\n" + + " final String myField = \"value\";\n" + + "}"; + doTest(before, after); + } + + public void testNotAvailableOnAlreadyFinalFieldName() { + String content = "class Test {\n" + + " final String myField = \"value\";\n" + + "}"; + // Intention should be available (based on isAvailable) but do nothing (based on invoke) + doTestNoChange(content); + } + + + @Override + protected String getTestDataPath() { + // Not using testData files for these tests, so path can be empty or root. + return ""; + } +} From 2ddace602e3e53ca63b32add988446c01bcb4580 Mon Sep 17 00:00:00 2001 From: longwm Date: Thu, 22 May 2025 22:00:40 +0800 Subject: [PATCH 2/2] Refactor "Add Final Intention" logic and tests for clarity. Updated the intention logic to better handle final modifier additions for fields, variables, and parameters. Improved test coverage and added assertions for caret presence to ensure test reliability. Enhanced code readability with consistent use of `final` in method parameters and local variables. --- .../plugin/intention/AddFinalIntention.java | 38 ++++++++---- .../intention/AddFinalIntentionTest.java | 59 ++++++++++--------- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/src/main/java/lwm/plugin/intention/AddFinalIntention.java b/src/main/java/lwm/plugin/intention/AddFinalIntention.java index 92b7a17..f83fddf 100644 --- a/src/main/java/lwm/plugin/intention/AddFinalIntention.java +++ b/src/main/java/lwm/plugin/intention/AddFinalIntention.java @@ -5,13 +5,24 @@ import com.intellij.codeInspection.util.IntentionName; import com.intellij.openapi.editor.Editor; import com.intellij.openapi.project.Project; -import com.intellij.psi.*; +import com.intellij.psi.PsiCodeBlock; +import com.intellij.psi.PsiElement; +import com.intellij.psi.PsiField; +import com.intellij.psi.PsiFile; +import com.intellij.psi.PsiJavaFile; +import com.intellij.psi.PsiLocalVariable; +import com.intellij.psi.PsiMethod; +import com.intellij.psi.PsiModifier; +import com.intellij.psi.PsiModifierList; +import com.intellij.psi.PsiModifierListOwner; +import com.intellij.psi.PsiParameter; +import com.intellij.psi.PsiParameterList; +import com.intellij.psi.PsiVariable; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiUtil; import com.intellij.util.IncorrectOperationException; import org.jetbrains.annotations.NotNull; -import java.util.Arrays; import java.util.Collection; /** @@ -29,7 +40,7 @@ public class AddFinalIntention implements IntentionAction { } @Override - public boolean isAvailable(@NotNull Project project, Editor editor, PsiFile file) { + public boolean isAvailable(@NotNull final Project project, final Editor editor, final PsiFile file) { if (!(file instanceof PsiJavaFile)) { return false; } @@ -37,7 +48,10 @@ public boolean isAvailable(@NotNull Project project, Editor editor, PsiFile file if (element == null) { return false; } - + final PsiField field = PsiTreeUtil.getParentOfType(element, PsiField.class, false); + if (field != null) { + return true; + } if (element instanceof PsiVariable) { return true; } @@ -46,40 +60,40 @@ public boolean isAvailable(@NotNull Project project, Editor editor, PsiFile file } @Override - public void invoke(@NotNull Project project, Editor editor, PsiFile file) throws IncorrectOperationException { + public void invoke(@NotNull final Project project, final Editor editor, final PsiFile file) throws IncorrectOperationException { final PsiElement element = file.findElementAt(editor.getCaretModel().getOffset()); if (element == null) { return; } - PsiVariable specificVariable = PsiTreeUtil.getParentOfType(element, PsiVariable.class, false); + final PsiVariable specificVariable = PsiTreeUtil.getParentOfType(element, PsiVariable.class, false); if (specificVariable != null) { addFinalModifierIfNotPresent(specificVariable); - return; + return; } final PsiMethod containingMethod = PsiTreeUtil.getParentOfType(element, PsiMethod.class); if (containingMethod != null) { // Add final to parameters final PsiParameterList parameterList = containingMethod.getParameterList(); - for (PsiParameter parameter : parameterList.getParameters()) { + for (final PsiParameter parameter : parameterList.getParameters()) { addFinalModifierIfNotPresent(parameter); } // Add final to local variables final PsiCodeBlock methodBody = containingMethod.getBody(); if (methodBody != null) { - Collection localVariables = PsiTreeUtil.collectElementsOfType(methodBody, PsiLocalVariable.class); - for (PsiLocalVariable localVariable : localVariables) { + final Collection localVariables = PsiTreeUtil.collectElementsOfType(methodBody, PsiLocalVariable.class); + for (final PsiLocalVariable localVariable : localVariables) { addFinalModifierIfNotPresent(localVariable); } } } } - private void addFinalModifierIfNotPresent(PsiModifierListOwner element) { + private void addFinalModifierIfNotPresent(final PsiModifierListOwner element) { if (element != null) { - PsiModifierList modifierList = element.getModifierList(); + final PsiModifierList modifierList = element.getModifierList(); if (modifierList != null && !modifierList.hasExplicitModifier(PsiModifier.FINAL)) { PsiUtil.setModifierProperty(element, PsiModifier.FINAL, true); } diff --git a/src/test/java/lwm/plugin/intention/AddFinalIntentionTest.java b/src/test/java/lwm/plugin/intention/AddFinalIntentionTest.java index 8340232..311a915 100644 --- a/src/test/java/lwm/plugin/intention/AddFinalIntentionTest.java +++ b/src/test/java/lwm/plugin/intention/AddFinalIntentionTest.java @@ -9,26 +9,29 @@ public class AddFinalIntentionTest extends LightJavaCodeInsightFixtureTestCase { private static final String INTENTION_TEXT = "Add final modifier(添加final修饰)"; - private void doTest(String before, String after) { - myFixture.configureByText("Test.java", before.replace("", "")); // Ensure caret is processed + private void doTest(final String before, final String after) { + assertTrue("Test code must contain ", before.contains("")); + myFixture.configureByText("Test.java", before); final IntentionAction intention = myFixture.findSingleIntention(INTENTION_TEXT); assertNotNull("Intention '" + INTENTION_TEXT + "' not found", intention); myFixture.launchAction(intention); myFixture.checkResult(after); } - private void doTestNotAvailable(String content) { - myFixture.configureByText("Test.java", content.replace("", "")); // Ensure caret is processed - List intentions = myFixture.getAvailableIntentions(); - List intentionTexts = intentions.stream() - .map(IntentionAction::getText) - .collect(Collectors.toList()); + private void doTestNotAvailable(final String content) { + assertTrue("Test code must contain ", content.contains("")); + myFixture.configureByText("Test.java", content); + final List intentions = myFixture.getAvailableIntentions(); + final List intentionTexts = intentions.stream() + .map(IntentionAction::getText) + .collect(Collectors.toList()); assertFalse("Intention '" + INTENTION_TEXT + "' should not be available. Available: " + intentionTexts, intentionTexts.contains(INTENTION_TEXT)); } - - private void doTestNoChange(String contentWithCaret) { - String contentWithoutCaret = contentWithCaret.replace("", ""); + + private void doTestNoChange(final String contentWithCaret) { + assertTrue("Test code must contain ", contentWithCaret.contains("")); + final String contentWithoutCaret = contentWithCaret.replace("", ""); myFixture.configureByText("Test.java", contentWithCaret); // Attempt to find the intention. It should be available. final IntentionAction intention = myFixture.findSingleIntention(INTENTION_TEXT); @@ -38,23 +41,23 @@ private void doTestNoChange(String contentWithCaret) { } public void testAddFinalToSingleParameter() { - String before = "class Test {\n" + + final String before = "class Test {\n" + " void method(String param1, int param2) {}\n" + "}"; - String after = "class Test {\n" + + final String after = "class Test {\n" + " void method(final String param1, int param2) {}\n" + "}"; doTest(before, after); } public void testAddFinalToSingleLocalVariable() { - String before = "class Test {\n" + + final String before = "class Test {\n" + " void method() {\n" + " String local1 = \"hello\";\n" + " int local2 = 10;\n" + " }\n" + "}"; - String after = "class Test {\n" + + final String after = "class Test {\n" + " void method() {\n" + " final String local1 = \"hello\";\n" + " int local2 = 10;\n" + @@ -65,7 +68,7 @@ public void testAddFinalToSingleLocalVariable() { public void testAddFinalToAllInMethodScope() { // Caret is removed in 'after' string by checkResult implicitly if not present. - String before = "class Test {\n" + + final String before = "class Test {\n" + " void method(String param1, int param2) {\n" + " \n" + " String local1 = \"hello\";\n" + @@ -73,9 +76,9 @@ public void testAddFinalToAllInMethodScope() { " final String alreadyFinal = \"done\";\n" + " }\n" + "}"; - String after = "class Test {\n" + + final String after = "class Test {\n" + " void method(final String param1, final int param2) {\n" + - " \n" + + " \n" + " final String local1 = \"hello\";\n" + " final int local2 = 10;\n" + " final String alreadyFinal = \"done\";\n" + @@ -85,7 +88,7 @@ public void testAddFinalToAllInMethodScope() { } public void testNotAvailableOnAlreadyFinalParameter() { - String content = "class Test {\n" + + final String content = "class Test {\n" + " void method(final String param1) {}\n" + "}"; // As per current isAvailable & invoke logic, intention is available but makes no change. @@ -93,7 +96,7 @@ public void testNotAvailableOnAlreadyFinalParameter() { } public void testNotAvailableOnAlreadyFinalLocalVariable() { - String content = "class Test {\n" + + final String content = "class Test {\n" + " void method() {\n" + " final String local1 = \"hello\";\n" + " }\n" + @@ -101,36 +104,36 @@ public void testNotAvailableOnAlreadyFinalLocalVariable() { // As per current isAvailable & invoke logic, intention is available but makes no change. doTestNoChange(content); } - + public void testNotAvailableOnClassDeclaration() { - String content = "class Test {\n" + + final String content = "class Test {\n" + " void method(String param1) {}\n" + "}"; doTestNotAvailable(content); } public void testNotAvailableInImportStatement() { - String content = "import java.util.List;\n" + + final String content = "import java.util.List;\n" + "class Test {\n" + " void method(String param1) {}\n" + "}"; doTestNotAvailable(content); } - + public void testNotAvailableOnFieldName() { // The current isAvailable will make it available for fields. // The invoke method will make the field final. - String before = "class Test {\n" + + final String before = "class Test {\n" + " String myField = \"value\";\n" + "}"; - String after = "class Test {\n" + + final String after = "class Test {\n" + " final String myField = \"value\";\n" + "}"; doTest(before, after); } public void testNotAvailableOnAlreadyFinalFieldName() { - String content = "class Test {\n" + + final String content = "class Test {\n" + " final String myField = \"value\";\n" + "}"; // Intention should be available (based on isAvailable) but do nothing (based on invoke) @@ -141,6 +144,6 @@ public void testNotAvailableOnAlreadyFinalFieldName() { @Override protected String getTestDataPath() { // Not using testData files for these tests, so path can be empty or root. - return ""; + return ""; } }