change signature: add visibility conflicts in hierarchy (IDEA-84645)

make method '...': use change signature to perform modification in the hierarchy (IDEA-119015)
This commit is contained in:
Anna.Kozlova
2017-06-30 19:54:36 +02:00
parent be7d40349a
commit 36eee39712
4 changed files with 115 additions and 67 deletions
@@ -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<UsageInfo> 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<PsiElement, String> 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<PsiElement, String> conflicts) {
String newMethodName = myChangeInfo.getNewName();
@@ -0,0 +1,13 @@
class X {
void m() {}
public void f() {}
}
class Y extends X {
@Override
public void f<caret>() {}
@Override
void m() {
super.m();
}
}
@@ -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);
@@ -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<PsiElement, String> 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();