JPS: handle method visibility changes that might lead to ambiguous call issues (IDEA-286565)

GitOrigin-RevId: 7ecd81c5786d33573c39116a3e16d82bef3a752d
This commit is contained in:
Eugene Zhuravlev
2022-01-13 08:57:24 +01:00
committed by intellij-monorepo-bot
parent 09cda97111
commit 370339d76e
50 changed files with 642 additions and 14 deletions

View File

@@ -0,0 +1,12 @@
Cleaning output files:
out/production/ChangePackagePrivateToProtected/ppp/Service.class
End of files
Compiling files:
src/ppp/Service.java
End of files
Cleaning output files:
out/production/ChangePackagePrivateToProtected/qqq/DerivedService.class
End of files
Compiling files:
src/qqq/DerivedService.java
End of files

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
void method(String param, boolean flag) {
}
void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
protected void method(String param, boolean flag) {
}
protected void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,9 @@
package qqq;
import ppp.Service;
public class Client {
public void perform(Service service) {
service.method(null, true);
}
}

View File

@@ -0,0 +1,7 @@
package qqq;
public class ClientDerived {
public void perform(DerivedService service) {
service.method2(null, true);
}
}

View File

@@ -0,0 +1,14 @@
package qqq;
import ppp.Service;
public class DerivedService extends Service{
public void method2(Long param, boolean flag) {
}
public void compute() {
method(null, true);
}
}

View File

@@ -0,0 +1,16 @@
Cleaning output files:
out/production/ChangePackagePrivateToPublic/ppp/Service.class
End of files
Compiling files:
src/ppp/Service.java
End of files
Cleaning output files:
out/production/ChangePackagePrivateToPublic/qqq/Client.class
out/production/ChangePackagePrivateToPublic/qqq/ClientDerived.class
out/production/ChangePackagePrivateToPublic/qqq/DerivedService.class
End of files
Compiling files:
src/qqq/Client.java
src/qqq/ClientDerived.java
src/qqq/DerivedService.java
End of files

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
void method(String param, boolean flag) {
}
void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
public void method(String param, boolean flag) {
}
public void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,9 @@
package qqq;
import ppp.Service;
public class Client {
public void perform(Service service) {
service.method(null, true);
}
}

View File

@@ -0,0 +1,7 @@
package qqq;
public class ClientDerived {
public void perform(DerivedService service) {
service.method2(null, true);
}
}

View File

@@ -0,0 +1,14 @@
package qqq;
import ppp.Service;
public class DerivedService extends Service{
public void method2(Long param, boolean flag) {
}
public void compute() {
method(null, true);
}
}

View File

@@ -0,0 +1,16 @@
Cleaning output files:
out/production/ChangePrivateToPackagePrivate/ppp/Service.class
End of files
Compiling files:
src/ppp/Service.java
End of files
Cleaning output files:
out/production/ChangePrivateToPackagePrivate/ppp/Client.class
out/production/ChangePrivateToPackagePrivate/ppp/ClientDerived.class
out/production/ChangePrivateToPackagePrivate/ppp/DerivedService.class
End of files
Compiling files:
src/ppp/Client.java
src/ppp/ClientDerived.java
src/ppp/DerivedService.java
End of files

View File

@@ -0,0 +1,7 @@
package ppp;
public class Client {
public void perform(Service service) {
service.method(null, true);
}
}

View File

@@ -0,0 +1,7 @@
package ppp;
public class ClientDerived {
public void perform(DerivedService service) {
service.method2(null, true);
}
}

View File

@@ -0,0 +1,12 @@
package ppp;
public class DerivedService extends Service{
public void method2(Long param, boolean flag) {
}
public void compute() {
method(null, true);
}
}

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
private void method(String param, boolean flag) {
}
private void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
void method(String param, boolean flag) {
}
void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,9 @@
package qqq;
import ppp.Service;
public class Client {
public void perform(Service service) {
service.method(null, true);
}
}

View File

@@ -0,0 +1,9 @@
package qqq;
import ppp.DerivedService;
public class ClientDerived {
public void perform(DerivedService service) {
service.method2(null, true);
}
}

View File

@@ -0,0 +1,14 @@
package qqq;
import ppp.Service;
public class DerivedService extends Service{
public void method2(Long param, boolean flag) {
}
public void compute() {
method(null, true);
}
}

View File

@@ -0,0 +1,18 @@
Cleaning output files:
out/production/ChangePrivateToProtected/ppp/Service.class
End of files
Compiling files:
src/ppp/Service.java
End of files
Cleaning output files:
out/production/ChangePrivateToProtected/ppp/Client.class
out/production/ChangePrivateToProtected/ppp/ClientDerived.class
out/production/ChangePrivateToProtected/ppp/DerivedService.class
out/production/ChangePrivateToProtected/qqq/DerivedService.class
End of files
Compiling files:
src/ppp/Client.java
src/ppp/ClientDerived.java
src/ppp/DerivedService.java
src/qqq/DerivedService.java
End of files

View File

@@ -0,0 +1,7 @@
package ppp;
public class Client {
public void perform(Service service) {
service.method(null, true);
}
}

View File

@@ -0,0 +1,7 @@
package ppp;
public class ClientDerived {
public void perform(DerivedService service) {
service.method2(null, true);
}
}

View File

@@ -0,0 +1,12 @@
package ppp;
public class DerivedService extends Service{
public void method2(Long param, boolean flag) {
}
public void compute() {
method(null, true);
}
}

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
private void method(String param, boolean flag) {
}
private void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
protected void method(String param, boolean flag) {
}
protected void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,9 @@
package qqq;
import ppp.Service;
public class Client {
public void perform(Service service) {
service.method(null, true);
}
}

View File

@@ -0,0 +1,9 @@
package qqq;
import ppp.DerivedService;
public class ClientDerived {
public void perform(DerivedService service) {
service.method2(null, true);
}
}

View File

@@ -0,0 +1,14 @@
package qqq;
import ppp.Service;
public class DerivedService extends Service{
public void method2(Long param, boolean flag) {
}
public void compute() {
method(null, true);
}
}

View File

@@ -0,0 +1,22 @@
Cleaning output files:
out/production/ChangePrivateToPublic/ppp/Service.class
End of files
Compiling files:
src/ppp/Service.java
End of files
Cleaning output files:
out/production/ChangePrivateToPublic/ppp/Client.class
out/production/ChangePrivateToPublic/ppp/ClientDerived.class
out/production/ChangePrivateToPublic/ppp/DerivedService.class
out/production/ChangePrivateToPublic/qqq/Client.class
out/production/ChangePrivateToPublic/qqq/ClientDerived.class
out/production/ChangePrivateToPublic/qqq/DerivedService.class
End of files
Compiling files:
src/ppp/Client.java
src/ppp/ClientDerived.java
src/ppp/DerivedService.java
src/qqq/Client.java
src/qqq/ClientDerived.java
src/qqq/DerivedService.java
End of files

View File

@@ -0,0 +1,7 @@
package ppp;
public class Client {
public void perform(Service service) {
service.method(null, true);
}
}

View File

@@ -0,0 +1,7 @@
package ppp;
public class ClientDerived {
public void perform(DerivedService service) {
service.method2(null, true);
}
}

View File

@@ -0,0 +1,12 @@
package ppp;
public class DerivedService extends Service{
public void method2(Long param, boolean flag) {
}
public void compute() {
method(null, true);
}
}

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
private void method(String param, boolean flag) {
}
private void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
public void method(String param, boolean flag) {
}
public void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,9 @@
package qqq;
import ppp.Service;
public class Client {
public void perform(Service service) {
service.method(null, true);
}
}

View File

@@ -0,0 +1,9 @@
package qqq;
import ppp.DerivedService;
public class ClientDerived {
public void perform(DerivedService service) {
service.method2(null, true);
}
}

View File

@@ -0,0 +1,14 @@
package qqq;
import ppp.Service;
public class DerivedService extends Service{
public void method2(Long param, boolean flag) {
}
public void compute() {
method(null, true);
}
}

View File

@@ -0,0 +1,16 @@
Cleaning output files:
out/production/ChangeProtectedToPublic/ppp/Service.class
End of files
Compiling files:
src/ppp/Service.java
End of files
Cleaning output files:
out/production/ChangeProtectedToPublic/qqq/Client.class
out/production/ChangeProtectedToPublic/qqq/ClientDerived.class
out/production/ChangeProtectedToPublic/qqq/DerivedService.class
End of files
Compiling files:
src/qqq/Client.java
src/qqq/ClientDerived.java
src/qqq/DerivedService.java
End of files

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
protected void method(String param, boolean flag) {
}
protected void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,13 @@
package ppp;
public class Service {
public void method(String param, boolean flag) {
}
public void method2(String param, boolean flag) {
}
public void method(Integer param, boolean flag) {
}
}

View File

@@ -0,0 +1,9 @@
package qqq;
import ppp.Service;
public class Client {
public void perform(Service service) {
service.method(null, true);
}
}

View File

@@ -0,0 +1,7 @@
package qqq;
public class ClientDerived {
public void perform(DerivedService service) {
service.method2(null, true);
}
}

View File

@@ -0,0 +1,9 @@
package qqq;
import ppp.Service;
public class DerivedService extends Service{
public void method2(Long param, boolean flag) {
}
}

View File

@@ -12,12 +12,22 @@ import java.util.*;
public abstract class Difference {
public static boolean weakerAccess(final int me, final int than) {
return ((me & Opcodes.ACC_PRIVATE) > 0 && (than & Opcodes.ACC_PRIVATE) == 0) ||
((me & Opcodes.ACC_PROTECTED) > 0 && (than & Opcodes.ACC_PUBLIC) > 0) ||
(isPackageLocal(me) && (than & (Opcodes.ACC_PROTECTED | Opcodes.ACC_PUBLIC)) > 0);
return (isPrivate(me) && !isPrivate(than)) || (isProtected(me) && isPublic(than)) || (isPackageLocal(me) && (than & (Opcodes.ACC_PROTECTED | Opcodes.ACC_PUBLIC)) != 0);
}
private static boolean isPackageLocal(final int access) {
public static boolean isPrivate(int access) {
return (access & Opcodes.ACC_PRIVATE) != 0;
}
public static boolean isPublic(int access) {
return (access & Opcodes.ACC_PUBLIC) != 0;
}
public static boolean isProtected(int access) {
return (access & Opcodes.ACC_PROTECTED) != 0;
}
public static boolean isPackageLocal(final int access) {
return (access & (Opcodes.ACC_PRIVATE | Opcodes.ACC_PROTECTED | Opcodes.ACC_PUBLIC)) == 0;
}
@@ -162,6 +172,8 @@ public abstract class Difference {
public abstract boolean accessRestricted();
public abstract boolean accessExpanded();
public abstract int addedModifiers();
public abstract int removedModifiers();

View File

@@ -29,6 +29,11 @@ class DifferenceImpl extends Difference{
return myDelegate.accessRestricted();
}
@Override
public boolean accessExpanded() {
return myDelegate.accessExpanded();
}
@Override
public int addedModifiers() {
return myDelegate.addedModifiers();

View File

@@ -30,6 +30,7 @@ import java.io.PrintStream;
import java.lang.annotation.RetentionPolicy;
import java.util.*;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.function.Supplier;
@@ -572,9 +573,25 @@ public final class Mappings {
}
private Iterable<MethodRepr> allMethodsRecursively(ClassRepr cls) {
return Iterators.flat(cls.getMethods(), Iterators.flat(Iterators.map(cls.getSuperTypes(), st -> {
return Iterators.flat(collectRecursively(cls, c-> c.getMethods()));
}
private Iterable<OverloadDescriptor> findAllOverloads(final ClassRepr cls, Function<MethodRepr, Integer> correspondenceFinder) {
Function<ClassRepr, Iterable<OverloadDescriptor>> converter = c -> c == null? Collections.emptyList() : Iterators.filter(Iterators.map(c.getMethods(), m -> {
Integer accessScope = correspondenceFinder.apply(m);
return accessScope != null? new OverloadDescriptor(accessScope, m, c) : null;
}), Objects::nonNull);
return Iterators.flat(Iterators.flat(
collectRecursively(cls, converter),
Iterators.map(getAllSubclasses(cls.name), subName -> converter.apply(subName != cls.name? classReprByName(subName) : null))
));
}
private <T> Iterable<T> collectRecursively(ClassRepr cls, Function<ClassRepr, T> mapper) {
return Iterators.flat(Iterators.asIterable(mapper.apply(cls)), Iterators.flat(Iterators.map(cls.getSuperTypes(), st -> {
final ClassRepr cr = classReprByName(st.className);
return cr != null ? allMethodsRecursively(cr) : Collections.emptyList();
return cr != null ? collectRecursively(cr, mapper) : Collections.emptyList();
})));
}
@@ -886,7 +903,8 @@ public final class Mappings {
@Override
public boolean checkResidence(final int residence) {
return !ClassRepr.getPackageName(myContext.getValue(residence)).equals(packageName);
final String className = myContext.getValue(residence);
return className == null || !ClassRepr.getPackageName(className).equals(packageName);
}
}
@@ -1434,6 +1452,8 @@ public final class Mappings {
}
}
final List<Pair<MethodRepr, MethodRepr.Diff>> moreAccessible = new ArrayList<>();
for (final Pair<MethodRepr, MethodRepr.Diff> mr : changed) {
final MethodRepr m = mr.first;
final MethodRepr.Diff d = mr.second;
@@ -1441,6 +1461,10 @@ public final class Mappings {
debug("Method: ", m.name);
if (d.accessExpanded()) {
moreAccessible.add(mr);
}
if (it.isAnnotation()) {
if (d.defaultRemoved()) {
debug("Class is annotation, default value is removed => adding annotation query");
@@ -1530,9 +1554,7 @@ public final class Mappings {
}
}
else {
if ((d.addedModifiers() & Opcodes.ACC_FINAL) != 0 ||
(d.addedModifiers() & Opcodes.ACC_PUBLIC) != 0 ||
(d.addedModifiers() & Opcodes.ACC_ABSTRACT) != 0) {
if ((d.addedModifiers() & (Opcodes.ACC_FINAL | Opcodes.ACC_PUBLIC | Opcodes.ACC_ABSTRACT)) != 0) {
debug("Added final, public or abstract specifier --- affecting subclasses");
myFuture.affectSubclasses(it.name, myAffectedFiles, state.myAffectedUsages, state.myDependants, false, myCompiledFiles, null);
if (it.isInterface() && (d.addedModifiers() & Opcodes.ACC_ABSTRACT) != 0) {
@@ -1591,6 +1613,41 @@ public final class Mappings {
}
}
}
if (!moreAccessible.isEmpty()) {
final Iterable<OverloadDescriptor> allOverloads = myFuture.findAllOverloads(it, mr -> {
Integer result = null;
for (Pair<MethodRepr, MethodRepr.Diff> pair : moreAccessible) {
MethodRepr m = pair.first;
MethodRepr.Diff d = pair.second;
if (mr.name == m.name && !m.equals(mr)) {
int newAccess = m.access & (~d.removedModifiers()) | d.addedModifiers();
if (result == null || Difference.weakerAccess(result, newAccess)) {
result = newAccess;
}
}
}
return result;
});
for (OverloadDescriptor descr : allOverloads) {
debug("Method became more accessible --- affect usages of overloading methods: ", descr.overloadMethod.name);
final Set<UsageRepr.Usage> overloadsUsages = new HashSet<>();
myFuture.affectMethodUsages(
descr.overloadMethod, myFuture.propagateMethodAccess(descr.overloadMethod, descr.overloadMethodOwner.name), descr.overloadMethod.createUsage(myContext, descr.overloadMethodOwner.name), overloadsUsages, state.myDependants
);
state.myAffectedUsages.addAll(overloadsUsages);
final UsageConstraint constr = Difference.isPackageLocal(descr.accessScope)? myFuture.new PackageConstraint(it.getPackageName()).negate() :
Difference.isProtected(descr.accessScope)? myFuture.new InheritanceConstraint(it).negate() : null;
if (constr != null) {
for (final UsageRepr.Usage usage : overloadsUsages) {
state.myUsageConstraints.put(usage, constr);
}
}
}
}
debug("End of changed methods processing");
}
@@ -1739,11 +1796,9 @@ public final class Mappings {
if (!field.isPrivate() && (field.access & INLINABLE_FIELD_MODIFIERS_MASK) == INLINABLE_FIELD_MODIFIERS_MASK && d.hadValue()) {
final int changedModifiers = d.addedModifiers() | d.removedModifiers();
final boolean harmful = (changedModifiers & (Opcodes.ACC_STATIC | Opcodes.ACC_FINAL)) != 0;
final boolean accessChanged = (changedModifiers & (Opcodes.ACC_PUBLIC | Opcodes.ACC_PRIVATE | Opcodes.ACC_PROTECTED)) != 0;
final boolean becameLessAccessible = accessChanged && d.accessRestricted();
final boolean valueChanged = (d.base() & Difference.VALUE) != 0;
if (harmful || valueChanged || becameLessAccessible) {
if (harmful || valueChanged || d.accessRestricted()) {
if (myProcessConstantsIncrementally) {
debug("Potentially inlined field changed its access or value => affecting field usages and static member import usages");
myFuture.affectFieldUsages(field, propagated.get(), field.createUsage(myContext, it.name), state.myAffectedUsages, state.myDependants);
@@ -3055,6 +3110,18 @@ public final class Mappings {
return myChangedFiles;
}
private static class OverloadDescriptor {
final int accessScope;
final MethodRepr overloadMethod;
final ClassRepr overloadMethodOwner;
OverloadDescriptor(int accessScope, MethodRepr overloadMethod, ClassRepr overloadMethodOwner) {
this.accessScope = accessScope;
this.overloadMethod = overloadMethod;
this.overloadMethodOwner = overloadMethodOwner;
}
}
private static void debug(final String s) {
LOG.debug(s);
}

View File

@@ -156,7 +156,7 @@ class Proto implements RW.Savable, Streamable, ProtoEntity {
@Override
public boolean packageLocalOn() {
return (past.isPrivate() || past.isPublic() || past.isProtected()) && Proto.this.isPackageLocal();
return !past.isPackageLocal() && Proto.this.isPackageLocal();
}
@Override
@@ -169,6 +169,11 @@ class Proto implements RW.Savable, Streamable, ProtoEntity {
return Difference.weakerAccess(access, past.access);
}
@Override
public boolean accessExpanded() {
return Difference.weakerAccess(past.access, access);
}
@Override
public Specifier<TypeRepr.ClassType, Difference> annotations() {
return ann;

View File

@@ -31,6 +31,30 @@ public class MethodModifierTest extends IncrementalTestCase {
doTest();
}
public void testChangePrivateToPackagePrivate() {
doTest().assertFailed();
}
public void testChangePrivateToProtected() {
doTest().assertFailed();
}
public void testChangePrivateToPublic() {
doTest().assertFailed();
}
public void testChangePackagePrivateToProtected() {
doTest().assertFailed();
}
public void testChangePackagePrivateToPublic() {
doTest().assertFailed();
}
public void testChangeProtectedToPublic() {
doTest().assertFailed();
}
public void testSetAbstract() {
doTest();
}