RFR: 8015831: Add lint check for calling overridable methods from a constructor [v9]
Archie L. Cobbs
duke at openjdk.org
Mon Jan 16 16:32:25 UTC 2023
On Mon, 16 Jan 2023 12:51:49 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:
>> The number of times around any single loop is bounded by the number of new references that can possibly be created during the analysis of that loop.
>>
>> That number is at most 2 * (1 + 1 + 1 + 1 + N) where N is the number of parameters and/or variables declared in that scope (the 2 is for direct or indirect, and the 1's are for each of the singleton reference types `ThisRef`, `OuterRef`, `ExprRef`, and `ReturnRef`).
>>
>> If you have nested scopes A, B, C each with Na, Nb, and Nc variables declared therein (respectively), then the bound would be something like 2 * (1 + 1 + 1 + 1 + Na + (Na * Nb) + (Na * Nb * Nc)) worst case (waving hands here).
>
> I have played a bit with the patch, trying to disable certain features. The main reason to deal with loops and recursive calls the way the patch does is to eliminate false positives. If we see a loop, and we can't perform the analysis multiple times (as the PR does), then we'd have to conclude that the loop can be potentially escaping (as we might have missed an local var update). Same goes for recursive calls (e.g. a recursive call would be treated as escaping). Same goes for analyzing invoked methods that fall in the same compilation unit. If we don't do that, more methods might look as "escaping".
>
> So, here's what I found:
>
> * compiling the JDK with the current patch produces 2376 warnings
> * disabling support for recursive calls produces 2427 warnings
> * treating all method calls inside a loop to be escaping produces 2464 warnings
> * disabling scanning of methods in same compilation unit produces 4317 warnings
>
> (Note: the patches I used to do this analysis are a bit blunt, and perhaps can be made a bit less conservative, which might result in less false positives added - I just went for the simplest possible approach, just to test the contribute of each analysis).
>
> This seems to suggest that even a blunt approach to deal with recursion and loop does not result in a significant increase of false positives (~2% more). That said, disabling scanning of methods in the same compilation unit results in a big impact in terms of false positive (~100% increase).
>
> So, I'm pretty confident that, should performance problems arise, we could probably dial back the analysis not to do loops (or to do them in a bounded way, as Vicente suggest, which is much better of what I tried here) - and that will probably give us the same results we have today (or a very very minor increase of false positives). But scanning of dependent methods in same compilation unit seems to be more or less a must-have.
Thanks for doing that analysis - very interesting.
It looks like you might be counting the "here via invocation" lines as separate warnings. These are really part of the previous `possible 'this' escape` line, e.g.:
.../java/awt/Frame.java:429: Note: possible 'this' escape before subclass is fully initialized
init(title, null);
^
.../java/awt/Frame.java:460: Note: previous possible 'this' escape happens here via invocation
SunToolkit.checkAndSetPolicy(this);
^
Semi-related... I was also curious what would happen if we changed the semantics of the warning from "subclass must be in a separate compilation unit" to "subclass must be in a separate package".
I'm not proposing we change this definition, and obviously there are trade-offs in where you draw this boundary, but was curious anywan (at one point I thought it might be worth having an option for this, e.g., with variants like `-Xlint:this-escape` vs. `-Xlint:this-escape:package`, or even `-Xlint:this-escape:module`, etc.).
In any case, here are the results:
* Warnings for subclass in separate compilation unit: 2093
* Warnings for subclass in separate package: 1334
So a 36% reduction - not too surprising since the JDK includes a bunch of non-public implementation classes.
FWIW this is the patch I used:
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
index e81df22b017..f309a4742aa 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
@@ -216,24 +217,24 @@ class ThisEscapeAnalyzer extends TreeScanner {
private Lint lint = ThisEscapeAnalyzer.this.lint;
private JCClassDecl currentClass;
- private boolean privateOuter;
+ private boolean nonPublicOuter;
@Override
public void visitClassDef(JCClassDecl tree) {
JCClassDecl currentClassPrev = currentClass;
- boolean privateOuterPrev = privateOuter;
+ boolean nonPublicOuterPrev = nonPublicOuter;
Lint lintPrev = lint;
lint = lint.augment(tree.sym);
try {
currentClass = tree;
- privateOuter |= tree.sym.isAnonymous();
- privateOuter |= (tree.mods.flags & Flags.PRIVATE) != 0;
+ nonPublicOuter |= tree.sym.isAnonymous();
+ nonPublicOuter |= (tree.mods.flags & Flags.PUBLIC) == 0;
// Recurse
super.visitClassDef(tree);
} finally {
currentClass = currentClassPrev;
- privateOuter = privateOuterPrev;
+ nonPublicOuter = nonPublicOuterPrev;
lint = lintPrev;
}
}
@@ -268,10 +269,10 @@ class ThisEscapeAnalyzer extends TreeScanner {
// Determine if this is a constructor we should analyze
boolean analyzable = currentClassIsExternallyExtendable() &&
TreeInfo.isConstructor(tree) &&
- !tree.sym.isPrivate() &&
+ (tree.sym.isPublic() || (tree.sym.flags() & Flags.PROTECTED) != 0) &&
!suppressed.contains(tree.sym);
- // Determine if this method is "invokable" in an analysis (can't be overridden)
+ // Determine if this method is "invokable" in an analysis (can't be overridden outside package)
boolean invokable = !currentClassIsExternallyExtendable() ||
TreeInfo.isConstructor(tree) ||
(tree.mods.flags & (Flags.STATIC | Flags.PRIVATE | Flags.FINAL)) != 0;
@@ -286,12 +287,13 @@ class ThisEscapeAnalyzer extends TreeScanner {
}
}
- // Determines if the current class could be extended in some external compilation unit
+ // Determines if the current class could be extended in some other package
private boolean currentClassIsExternallyExtendable() {
return !currentClass.sym.isFinal() &&
+ currentClass.sym.isPublic() &&
!(currentClass.sym.isSealed() && currentClass.permitting.isEmpty()) &&
!(currentClass.sym.owner.kind == MTH) &&
- !privateOuter;
+ !nonPublicOuter;
}
}.scan(env.tree);
-------------
PR: https://git.openjdk.org/jdk/pull/11874
More information about the build-dev
mailing list