Suppress InaccessibleClassProblem if the class reference is unused in the code attribute#1676
Suppress InaccessibleClassProblem if the class reference is unused in the code attribute#1676
Conversation
I'll need to find a good test case of InaccessibleClassProblem |
|
I tried to find an artifact that contains a class that became private from public, and another artifact containing a class that referenced the class. No luck. Maybe I'll commit a JAR file that contains such bad class files and add instruction to generate one. |
|
@elharo PTAL. I updated the tests. |
| // JVM Instruction Set: anewarray, checkcast, instanceof, ldc, ldc2_w, multianewarray, | ||
| // and new. |
There was a problem hiding this comment.
They cover the ones listed in http://go/jdd-class-reference-validation
There was a problem hiding this comment.
I think you mean ldc_w, not ldc2_w. ldc2_w is for longs or doubles.
|
|
||
| SymbolReferences.Builder builder = new SymbolReferences.Builder(); | ||
| builder.addClassReference( | ||
| new ClassFile(dummySource, LinkageCheckerTest.class.getName()), |
There was a problem hiding this comment.
It was using a dummy pair. The dummySource jar does not contain LinkageCheckerTest class.
With this PR, the test cases require real class files that have constant pool section containing the target class.
| Instruction instruction = instructionHandle.getInstruction(); | ||
| if (instruction instanceof CPInstruction) { |
There was a problem hiding this comment.
@netdpb FYI, BCEL has an abstract class "CPInstruction" that represents an instruction that takes a constant pool index, and we already had this function to determine whether a constant pool index is unused by JVM instructions. (Therefore this function does not list "safe" operations)
There was a problem hiding this comment.
That's fine, as long as it doesn't list "dangerous" operations either.
|
|
||
| if (!isClassAccessibleFrom(targetClass, sourceClassName)) { | ||
| return Optional.of(new InaccessibleClassProblem(sourceClassFile, targetClassFile, symbol)); | ||
| if (classDumper.isUnusedClassSymbolReference(sourceClassName, symbol)) { |
There was a problem hiding this comment.
What about:
if (!isClassAccessibleFrom(targetClass, sourceClassName)
&& !classDumper.isUnusedClassSymbolReference(sourceClassName, symbol)) {
return Optional.of(new InaccessibleClassProblem(sourceClassFile, targetClassFile, symbol));
}
return Optional.empty();There was a problem hiding this comment.
I tried that and I was confused. Maybe this is more clear:
if (isClassAccessibleFrom(targetClass, sourceClassName)
|| classDumper.isUnusedClassSymbolReference(sourceClassName, symbol)) {
return Optional.empty();
} else {
return Optional.of(new InaccessibleClassProblem(sourceClassFile, targetClassFile, symbol));
}
There was a problem hiding this comment.
Maybe, but it does seem like an inversion of the test.
If the class is inaccessible and it's not unused... What if you inverted isUnusedClassSymbolReference?
if (!isClassAccessibleFrom(targetClass, sourceClassname)
&& classDumpter.isClassSymbolReferenceUsed(sourceClassName, symbol)) {
return Optional.of(…);
}
return Optional.empty();There was a problem hiding this comment.
Added isClassSymbolReferenceUsed to ClassDumper.
| // io.grpc.grpclb.GrpclbLoadBalancer in grpc-grpclb 0.12.0 had a reference to | ||
| // io.grpc.internal.SingleTransportChannel. The SingleTransportChannel became non-public in | ||
| // grpc-core 0.15.0. | ||
| ClassPathBuilder classPathBuilder = new ClassPathBuilder(); |
There was a problem hiding this comment.
You only use this once. Inline?
| } | ||
|
|
||
| boolean isClassSymbolReferenceUsed(String sourceClassName, ClassSymbol classSymbol) { | ||
| return !isUnusedClassSymbolReference(sourceClassName, classSymbol); |
There was a problem hiding this comment.
Oh. Do we need both? I think positive method names are usually better than negative ones. Where else is isUnusedClassSymbolReference used, and can it be switched?
There was a problem hiding this comment.
Updated.
positive method names are usually better than negative ones
Yes, especially when we negate the result.
|
|
||
| /** | ||
| * Returns true if the class symbol reference is unused in the source class file. It checks | ||
| * Returns true if the class symbol reference is used in the source class file. It checks |
Fixes #1608.