From 36eee397123e554601e948b601003692d55420fe Mon Sep 17 00:00:00 2001 From: "Anna.Kozlova" Date: Fri, 30 Jun 2017 19:43:40 +0200 Subject: [PATCH] change signature: add visibility conflicts in hierarchy (IDEA-84645) make method '...': use change signature to perform modification in the hierarchy (IDEA-119015) --- .../JavaChangeSignatureUsageProcessor.java | 75 +++++++++++++++- ...nAboutAssigningWeakerAccessPrivileges.java | 13 +++ .../java/refactoring/ChangeSignatureTest.java | 8 ++ .../ipp/modifiers/ModifierIntention.java | 86 +++++-------------- 4 files changed, 115 insertions(+), 67 deletions(-) create mode 100644 java/java-tests/testData/refactoring/changeSignature/WarnAboutAssigningWeakerAccessPrivileges.java diff --git a/java/java-impl/src/com/intellij/refactoring/changeSignature/JavaChangeSignatureUsageProcessor.java b/java/java-impl/src/com/intellij/refactoring/changeSignature/JavaChangeSignatureUsageProcessor.java index 0854d515fbf9..577ec002e58c 100644 --- a/java/java-impl/src/com/intellij/refactoring/changeSignature/JavaChangeSignatureUsageProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/changeSignature/JavaChangeSignatureUsageProcessor.java @@ -33,10 +33,13 @@ import com.intellij.psi.codeStyle.CodeStyleManager; import com.intellij.psi.codeStyle.CodeStyleSettingsManager; import com.intellij.psi.codeStyle.JavaCodeStyleManager; import com.intellij.psi.codeStyle.VariableKind; +import com.intellij.psi.impl.source.resolve.JavaResolveUtil; import com.intellij.psi.scope.processor.VariablesProcessor; import com.intellij.psi.scope.util.PsiScopesUtil; import com.intellij.psi.search.LocalSearchScope; +import com.intellij.psi.search.searches.OverridingMethodsSearch; import com.intellij.psi.search.searches.ReferencesSearch; +import com.intellij.psi.search.searches.SuperMethodsSearch; import com.intellij.psi.util.*; import com.intellij.refactoring.RefactoringBundle; import com.intellij.refactoring.rename.RenameUtil; @@ -51,6 +54,7 @@ import com.intellij.util.VisibilityUtil; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.HashSet; import com.intellij.util.containers.MultiMap; +import com.siyeh.IntentionPowerPackBundle; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -1002,7 +1006,7 @@ public class JavaChangeSignatureUsageProcessor implements ChangeSignatureUsagePr } } - private static class ConflictSearcher { + public static class ConflictSearcher { private final JavaChangeInfo myChangeInfo; private ConflictSearcher(@NotNull JavaChangeInfo changeInfo) { @@ -1104,7 +1108,10 @@ public class JavaChangeSignatureUsageProcessor implements ChangeSignatureUsagePr throws IncorrectOperationException { PsiMethod method = myChangeInfo.getMethod(); PsiModifierList modifierList = (PsiModifierList)method.getModifierList().copy(); - VisibilityUtil.setVisibility(modifierList, myChangeInfo.getNewVisibility()); + String visibility = myChangeInfo.getNewVisibility(); + VisibilityUtil.setVisibility(modifierList, visibility); + + searchForHierarchyConflicts(method, conflictDescriptions, visibility); for (Iterator iterator = usages.iterator(); iterator.hasNext();) { UsageInfo usageInfo = iterator.next(); @@ -1122,7 +1129,7 @@ public class JavaChangeSignatureUsageProcessor implements ChangeSignatureUsagePr String message = RefactoringBundle.message("0.with.1.visibility.is.not.accessible.from.2", RefactoringUIUtil.getDescription(method, true), - VisibilityUtil.toPresentableText(myChangeInfo.getNewVisibility()), + VisibilityUtil.toPresentableText(visibility), RefactoringUIUtil.getDescription(ConflictsUtil.getContainer(element), true)); conflictDescriptions.putValue(method, message); if (!needToChangeCalls()) { @@ -1134,6 +1141,68 @@ public class JavaChangeSignatureUsageProcessor implements ChangeSignatureUsagePr } } + public static void searchForHierarchyConflicts(PsiMethod method, MultiMap conflicts, final String modifier) { + SuperMethodsSearch.search(method, method.getContainingClass(), true, false).forEach( + methodSignature -> { + final PsiMethod superMethod = methodSignature.getMethod(); + if (!hasCompatibleVisibility(superMethod, true, modifier)) { + conflicts.putValue(superMethod, IntentionPowerPackBundle.message( + "0.will.have.incompatible.access.privileges.with.super.1", + RefactoringUIUtil.getDescription(method, false), + RefactoringUIUtil.getDescription(superMethod, true))); + } + return true; + }); + OverridingMethodsSearch.search(method).forEach(overridingMethod -> { + if (!isVisibleFromOverridingMethod(method, overridingMethod, modifier)) { + conflicts.putValue(overridingMethod, IntentionPowerPackBundle.message( + "0.will.no.longer.be.visible.from.overriding.1", + RefactoringUIUtil.getDescription(method, false), + RefactoringUIUtil.getDescription(overridingMethod, true))); + } + else if (!hasCompatibleVisibility(overridingMethod, false, modifier)) { + conflicts.putValue(overridingMethod, IntentionPowerPackBundle.message( + "0.will.have.incompatible.access.privileges.with.overriding.1", + RefactoringUIUtil.getDescription(method, false), + RefactoringUIUtil.getDescription(overridingMethod, true))); + } + return false; + }); + } + + private static boolean hasCompatibleVisibility(PsiMethod method, boolean isSuper, final String modifier) { + if (modifier.equals(PsiModifier.PRIVATE)) { + return false; + } + else if (modifier.equals(PsiModifier.PACKAGE_LOCAL)) { + if (isSuper) { + return !(method.hasModifierProperty(PsiModifier.PUBLIC) || method.hasModifierProperty(PsiModifier.PROTECTED)); + } + return true; + } + else if (modifier.equals(PsiModifier.PROTECTED)) { + if (isSuper) { + return !method.hasModifierProperty(PsiModifier.PUBLIC); + } + else { + return method.hasModifierProperty(PsiModifier.PROTECTED) || method.hasModifierProperty(PsiModifier.PUBLIC); + } + } + else if (modifier.equals(PsiModifier.PUBLIC)) { + if (!isSuper) { + return method.hasModifierProperty(PsiModifier.PUBLIC); + } + return true; + } + throw new AssertionError(); + } + + private static boolean isVisibleFromOverridingMethod(PsiMethod method, PsiMethod overridingMethod, final String modifier) { + final PsiModifierList modifierListCopy = (PsiModifierList)method.getModifierList().copy(); + modifierListCopy.setModifierProperty(modifier, true); + return JavaResolveUtil.isAccessible(method, method.getContainingClass(), modifierListCopy, overridingMethod, null, null); + } + private PsiMethod addMethodConflicts(MultiMap conflicts) { String newMethodName = myChangeInfo.getNewName(); diff --git a/java/java-tests/testData/refactoring/changeSignature/WarnAboutAssigningWeakerAccessPrivileges.java b/java/java-tests/testData/refactoring/changeSignature/WarnAboutAssigningWeakerAccessPrivileges.java new file mode 100644 index 000000000000..f8ef18fa7094 --- /dev/null +++ b/java/java-tests/testData/refactoring/changeSignature/WarnAboutAssigningWeakerAccessPrivileges.java @@ -0,0 +1,13 @@ +class X { + void m() {} + public void f() {} +} +class Y extends X { + @Override + public void f() {} + + @Override + void m() { + super.m(); + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/refactoring/ChangeSignatureTest.java b/java/java-tests/testSrc/com/intellij/java/refactoring/ChangeSignatureTest.java index 6f31ba429959..2e3d882359d1 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/ChangeSignatureTest.java +++ b/java/java-tests/testSrc/com/intellij/java/refactoring/ChangeSignatureTest.java @@ -53,6 +53,14 @@ public class ChangeSignatureTest extends ChangeSignatureBaseTest { catch (BaseRefactoringProcessor.ConflictsInTestsException ignored) { } } + public void testWarnAboutAssigningWeakerAccessPrivileges() { + try { + doTest(PsiModifier.PRIVATE,null, null, new ParameterInfoImpl[0], new ThrownExceptionInfo[0], false); + fail("Conflict expected"); + } + catch (BaseRefactoringProcessor.ConflictsInTestsException ignored) { } + } + public void testDelegateWithoutChangesWarnAboutSameMethodInClass() throws Exception { try { doTest(null, new ParameterInfoImpl[0], true); diff --git a/plugins/IntentionPowerPak/src/com/siyeh/ipp/modifiers/ModifierIntention.java b/plugins/IntentionPowerPak/src/com/siyeh/ipp/modifiers/ModifierIntention.java index 570bcb076cd4..cdde9d8be406 100644 --- a/plugins/IntentionPowerPak/src/com/siyeh/ipp/modifiers/ModifierIntention.java +++ b/plugins/IntentionPowerPak/src/com/siyeh/ipp/modifiers/ModifierIntention.java @@ -22,11 +22,13 @@ import com.intellij.openapi.util.io.FileUtil; import com.intellij.psi.*; import com.intellij.psi.codeStyle.CodeStyleManager; import com.intellij.psi.impl.source.resolve.JavaResolveUtil; -import com.intellij.psi.search.searches.OverridingMethodsSearch; import com.intellij.psi.search.searches.ReferencesSearch; -import com.intellij.psi.search.searches.SuperMethodsSearch; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.refactoring.RefactoringBundle; +import com.intellij.refactoring.changeSignature.ChangeSignatureProcessor; +import com.intellij.refactoring.changeSignature.JavaChangeSignatureUsageProcessor; +import com.intellij.refactoring.changeSignature.JavaThrownExceptionInfo; +import com.intellij.refactoring.changeSignature.ParameterInfoImpl; import com.intellij.refactoring.ui.ConflictsDialog; import com.intellij.refactoring.util.CommonRefactoringUtil; import com.intellij.refactoring.util.RefactoringUIUtil; @@ -83,8 +85,23 @@ abstract class ModifierIntention extends Intention implements LowPriorityAction } private void changeModifier(PsiModifierList modifierList) { + PsiElement parent = modifierList.getParent(); + @VisibilityConstant final String modifier = getModifier(); + if (parent instanceof PsiMethod) { + PsiMethod method = (PsiMethod)parent; + //no myPrepareSuccessfulSwingThreadCallback means that the conflicts when any, won't be shown again + new ChangeSignatureProcessor(parent.getProject(), + method, + false, + modifier, + method.getName(), + method.getReturnType(), + ParameterInfoImpl.fromMethod(method), + JavaThrownExceptionInfo.extractExceptions(method)) + .run(); + return; + } WriteAction.run(() -> { - final String modifier = getModifier(); modifierList.setModifierProperty(modifier, true); if (!PsiModifier.PACKAGE_LOCAL.equals(modifier)) { final Project project = modifierList.getProject(); @@ -92,7 +109,7 @@ abstract class ModifierIntention extends Intention implements LowPriorityAction final PsiElement sibling = modifierList.getNextSibling(); if (sibling instanceof PsiWhiteSpace) { sibling.replace(whitespace); - CodeStyleManager.getInstance(project).reformatRange(modifierList.getParent(), modifierList.getTextOffset(), + CodeStyleManager.getInstance(project).reformatRange(parent, modifierList.getTextOffset(), modifierList.getNextSibling().getTextOffset()); } } @@ -126,33 +143,7 @@ abstract class ModifierIntention extends Intention implements LowPriorityAction } final MultiMap conflicts = new MultiMap<>(); if (member instanceof PsiMethod) { - final PsiMethod method = (PsiMethod)member; - SuperMethodsSearch.search(method, method.getContainingClass(), true, false).forEach( - methodSignature -> { - final PsiMethod superMethod = methodSignature.getMethod(); - if (!hasCompatibleVisibility(superMethod, true)) { - conflicts.putValue(superMethod, IntentionPowerPackBundle.message( - "0.will.have.incompatible.access.privileges.with.super.1", - RefactoringUIUtil.getDescription(method, false), - RefactoringUIUtil.getDescription(superMethod, true))); - } - return true; - }); - OverridingMethodsSearch.search(method).forEach(overridingMethod -> { - if (!isVisibleFromOverridingMethod(method, overridingMethod)) { - conflicts.putValue(overridingMethod, IntentionPowerPackBundle.message( - "0.will.no.longer.be.visible.from.overriding.1", - RefactoringUIUtil.getDescription(method, false), - RefactoringUIUtil.getDescription(overridingMethod, true))); - } - else if (!hasCompatibleVisibility(overridingMethod, false)) { - conflicts.putValue(overridingMethod, IntentionPowerPackBundle.message( - "0.will.have.incompatible.access.privileges.with.overriding.1", - RefactoringUIUtil.getDescription(method, false), - RefactoringUIUtil.getDescription(overridingMethod, true))); - } - return false; - }); + JavaChangeSignatureUsageProcessor.ConflictSearcher.searchForHierarchyConflicts((PsiMethod)member, conflicts, getModifier()); } final PsiModifierList modifierListCopy = (PsiModifierList)modifierList.copy(); modifierListCopy.setModifierProperty(getModifier(), true); @@ -175,39 +166,6 @@ abstract class ModifierIntention extends Intention implements LowPriorityAction return conflicts; } - private boolean hasCompatibleVisibility(PsiMethod method, boolean isSuper) { - if (getModifier().equals(PsiModifier.PRIVATE)) { - return false; - } - else if (getModifier().equals(PsiModifier.PACKAGE_LOCAL)) { - if (isSuper) { - return !(method.hasModifierProperty(PsiModifier.PUBLIC) || method.hasModifierProperty(PsiModifier.PROTECTED)); - } - return true; - } - else if (getModifier().equals(PsiModifier.PROTECTED)) { - if (isSuper) { - return !method.hasModifierProperty(PsiModifier.PUBLIC); - } - else { - return method.hasModifierProperty(PsiModifier.PROTECTED) || method.hasModifierProperty(PsiModifier.PUBLIC); - } - } - else if (getModifier().equals(PsiModifier.PUBLIC)) { - if (!isSuper) { - return method.hasModifierProperty(PsiModifier.PUBLIC); - } - return true; - } - throw new AssertionError(); - } - - private boolean isVisibleFromOverridingMethod(PsiMethod method, PsiMethod overridingMethod) { - final PsiModifierList modifierListCopy = (PsiModifierList)method.getModifierList().copy(); - modifierListCopy.setModifierProperty(getModifier(), true); - return JavaResolveUtil.isAccessible(method, method.getContainingClass(), modifierListCopy, overridingMethod, null, null); - } - @VisibilityConstant protected abstract String getModifier();