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 77ddd5c6583..8710e22ab5a 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 @@ -25,9 +25,7 @@ package com.sun.tools.javac.comp; -import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.Comparator; import java.util.LinkedHashMap; import java.util.EnumSet; import java.util.HashSet; @@ -35,12 +33,13 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; -import java.util.function.BiPredicate; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Collector; import java.util.stream.Collectors; +import java.util.stream.IntStream; import java.util.stream.Stream; import com.sun.tools.javac.code.Directive; @@ -52,21 +51,20 @@ import com.sun.tools.javac.code.Symtab; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.resources.CompilerProperties.LintWarnings; -import com.sun.tools.javac.resources.CompilerProperties.Warnings; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree.*; import com.sun.tools.javac.tree.TreeInfo; import com.sun.tools.javac.tree.TreeScanner; import com.sun.tools.javac.util.Assert; import com.sun.tools.javac.util.Context; -import com.sun.tools.javac.util.JCDiagnostic; -import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; +import com.sun.tools.javac.util.JCDiagnostic.LintWarning; import com.sun.tools.javac.util.List; import com.sun.tools.javac.util.Log; import com.sun.tools.javac.util.Names; import com.sun.tools.javac.util.Pair; import static com.sun.tools.javac.code.Kinds.Kind.*; +import static com.sun.tools.javac.code.Lint.LintCategory.THIS_ESCAPE; import static com.sun.tools.javac.code.TypeTag.*; import static com.sun.tools.javac.tree.JCTree.Tag.*; @@ -164,7 +162,7 @@ public class ThisEscapeAnalyzer extends TreeScanner { /** Environment for symbol lookup. */ - private Env attrEnv; + private Env topLevelEnv; /** Maps symbols of all methods to their corresponding declarations. */ @@ -185,30 +183,25 @@ public class ThisEscapeAnalyzer extends TreeScanner { /** Snapshots of {@link #callStack} where possible 'this' escapes occur. */ - private final ArrayList warningList = new ArrayList<>(); + private final ArrayList warningList = new ArrayList<>(); // These fields are scoped to the constructor being analyzed - /** The declaring class of the "invoked" method we're currently analyzing. + /** The method we're currently analyzing. * This is either the analyzed constructor or some method it invokes. */ - private JCClassDecl methodClass; + private MethodInfo currentMethod; - /** The current "call stack" during our analysis. The first entry is some method - * invoked from the target constructor; if empty, we're still in the constructor. + /** The current "call stack" during our analysis. The first entry is the initial + * constructor we started with, and subsequent entries correspond to invoked methods. + * If we're still in the initial constructor, the list will be empty. */ - private final ArrayDeque callStack = new ArrayDeque<>(); + private final ArrayList callStack = new ArrayList<>(); /** Used to terminate recursion in {@link #invokeInvokable invokeInvokable()}. */ private final Set>> invocations = new HashSet<>(); - /** Snapshot of {@link #callStack} where a possible 'this' escape occurs. - * If non-null, a 'this' escape warning has been found in the current - * constructor statement, initialization block statement, or field initializer. - */ - private DiagnosticPosition[] pendingWarning; - // These fields are scoped to the constructor or invoked method being analyzed /** Current lexical scope depth in the constructor or method we're currently analyzing. @@ -246,18 +239,18 @@ public class ThisEscapeAnalyzer extends TreeScanner { // public void analyzeTree(Env env) { + topLevelEnv = env; try { doAnalyzeTree(env); } finally { - attrEnv = null; + topLevelEnv = null; methodMap.clear(); nonPublicOuters.clear(); targetClass = null; warningList.clear(); - methodClass = null; + currentMethod = null; callStack.clear(); invocations.clear(); - pendingWarning = null; depth = -1; refs = null; } @@ -270,7 +263,7 @@ public class ThisEscapeAnalyzer extends TreeScanner { Assert.check(methodMap.isEmpty()); // we are not prepared to be used more than once // Short circuit if warnings are totally disabled - if (!lint.isEnabled(Lint.LintCategory.THIS_ESCAPE)) + if (!lint.isEnabled(THIS_ESCAPE)) return; // Determine which packages are exported by the containing module, if any. @@ -324,7 +317,7 @@ public class ThisEscapeAnalyzer extends TreeScanner { try { // Track warning suppression of fields - if (tree.sym.owner.kind == TYP && !lint.isEnabled(Lint.LintCategory.THIS_ESCAPE)) + if (tree.sym.owner.kind == TYP && !lint.isEnabled(THIS_ESCAPE)) suppressed.add(tree.sym); // Recurse @@ -341,23 +334,23 @@ public class ThisEscapeAnalyzer extends TreeScanner { try { // Track warning suppression of constructors - if (TreeInfo.isConstructor(tree) && !lint.isEnabled(Lint.LintCategory.THIS_ESCAPE)) + if (TreeInfo.isConstructor(tree) && !lint.isEnabled(THIS_ESCAPE)) suppressed.add(tree.sym); + // Gather some useful info + boolean constructor = TreeInfo.isConstructor(tree); + boolean extendableClass = currentClassIsExternallyExtendable(); + boolean nonPrivate = (tree.sym.flags() & (Flags.PUBLIC | Flags.PROTECTED)) != 0; + boolean finalish = (tree.mods.flags & (Flags.STATIC | Flags.PRIVATE | Flags.FINAL)) != 0; + // Determine if this is a constructor we should analyze - boolean extendable = currentClassIsExternallyExtendable(); - boolean analyzable = extendable && - TreeInfo.isConstructor(tree) && - (tree.sym.flags() & (Flags.PUBLIC | Flags.PROTECTED)) != 0 && - !suppressed.contains(tree.sym); + boolean analyzable = extendableClass && constructor && nonPrivate; - // Determine if this method is "invokable" in an analysis (can't be overridden) - boolean invokable = !extendable || - TreeInfo.isConstructor(tree) || - (tree.mods.flags & (Flags.STATIC | Flags.PRIVATE | Flags.FINAL)) != 0; + // Determine if it's safe to "invoke" the method in an analysis (i.e., it can't be overridden) + boolean invokable = !extendableClass || constructor || finalish; - // Add method or constructor to map - methodMap.put(tree.sym, new MethodInfo(currentClass, tree, analyzable, invokable)); + // Add this method or constructor to our map + methodMap.put(tree.sym, new MethodInfo(currentClass, tree, constructor, analyzable, invokable)); // Recurse super.visitMethodDef(tree); @@ -377,106 +370,54 @@ public class ThisEscapeAnalyzer extends TreeScanner { } }.scan(env.tree); - // Analyze non-static field initializers and initialization blocks, - // but only for classes having at least one analyzable constructor. + // Analyze the analyzable constructors we found methodMap.values().stream() - .filter(MethodInfo::analyzable) - .map(MethodInfo::declaringClass) - .distinct() - .forEach(klass -> { - for (List defs = klass.defs; defs.nonEmpty(); defs = defs.tail) { + .filter(MethodInfo::analyzable) + .forEach(this::analyzeConstructor); - // Ignore static stuff - if ((TreeInfo.flags(defs.head) & Flags.STATIC) != 0) - continue; + // Manually apply any Lint suppression + filterWarnings(warning -> !warning.isSuppressed()); - // Handle field initializers - if (defs.head instanceof JCVariableDecl vardef) { - visitTopLevel(env, klass, () -> { - scan(vardef); - copyPendingWarning(); - }); - continue; - } + // Field intitializers and initialization blocks will generate a separate warning for each primary constructor. + // Trim off stack frames up through the super() call so these will have identical stacks and get de-duplicated below. + warningList.forEach(Warning::trimInitializerFrames); - // Handle initialization blocks - if (defs.head instanceof JCBlock block) { - visitTopLevel(env, klass, () -> analyzeStatements(block.stats)); - continue; - } - } - }); - - // Analyze all of the analyzable constructors we found - methodMap.values().stream() - .filter(MethodInfo::analyzable) - .forEach(methodInfo -> { - visitTopLevel(env, methodInfo.declaringClass(), - () -> analyzeStatements(methodInfo.declaration().body.stats)); - }); - - // Eliminate duplicate warnings. Warning B duplicates warning A if the stack trace of A is a prefix - // of the stack trace of B. For example, if constructor Foo(int x) has a leak, and constructor - // Foo() invokes this(0), then emitting a warning for Foo() would be redundant. - BiPredicate extendsAsPrefix = (warning1, warning2) -> { - if (warning2.length < warning1.length) + // Sort warnings so redundant warnings immediately follow whatever they are redundant for, then remove them + warningList.sort(Warning::sortByStackFrames); + AtomicReference previousRef = new AtomicReference<>(); + filterWarnings(warning -> { + Warning previous = previousRef.get(); + if (previous != null && warning.isRedundantWith(previous)) return false; - for (int index = 0; index < warning1.length; index++) { - if (warning2[index].getPreferredPosition() != warning1[index].getPreferredPosition()) - return false; - } + previousRef.set(warning); return true; - }; + }); - // Stack traces are ordered top to bottom, and so duplicates always have the same first element(s). - // Sort the stack traces lexicographically, so that duplicates immediately follow what they duplicate. - Comparator ordering = (warning1, warning2) -> { - for (int index1 = 0, index2 = 0; true; index1++, index2++) { - boolean end1 = index1 >= warning1.length; - boolean end2 = index2 >= warning2.length; - if (end1 && end2) - return 0; - if (end1) - return -1; - if (end2) - return 1; - int posn1 = warning1[index1].getPreferredPosition(); - int posn2 = warning2[index2].getPreferredPosition(); - int diff = Integer.compare(posn1, posn2); - if (diff != 0) - return diff; - } - }; - warningList.sort(ordering); + // Limit output to one warning per constructor, field initializer, or initializer block + Set thingsWarnedAbout = new HashSet<>(); + filterWarnings(warning -> thingsWarnedAbout.add(warning.origin)); - // Now emit the warnings, but skipping over duplicates as we go through the list - DiagnosticPosition[] previous = null; - for (DiagnosticPosition[] warning : warningList) { - - // Skip duplicates - if (previous != null && extendsAsPrefix.test(previous, warning)) - continue; - previous = warning; - - // Emit warnings showing the entire stack trace - JCDiagnostic.Warning key = LintWarnings.PossibleThisEscape; - int remain = warning.length; - do { - DiagnosticPosition pos = warning[--remain]; - log.warning(pos, key); + // Emit warnings + for (Warning warning : warningList) { + LintWarning key = LintWarnings.PossibleThisEscape; + for (StackFrame frame : warning.stack) { + log.warning(frame.site.pos(), key); key = LintWarnings.PossibleThisEscapeLocation; - } while (remain > 0); + } } + + // Done warningList.clear(); } - // Analyze statements, but stop at (and record) the first warning generated - private void analyzeStatements(List stats) { - for (JCStatement stat : stats) { - scan(stat); - if (copyPendingWarning()) - break; + // Warning list editor (this is slightly more efficient than removeIf()) + private void filterWarnings(Predicate filter) { + int numRetained = 0; + for (Warning warning : warningList) { + if (filter.test(warning)) + warningList.set(numRetained++, warning); } + warningList.subList(numRetained, warningList.size()).clear(); } @Override @@ -542,10 +483,6 @@ public class ThisEscapeAnalyzer extends TreeScanner { private void visitVarDef(VarSymbol sym, JCExpression expr) { - // Skip if ignoring warnings for this field - if (suppressed.contains(sym)) - return; - // Scan initializer, if any scan(expr); if (isParamOrVar(sym)) @@ -579,19 +516,43 @@ public class ThisEscapeAnalyzer extends TreeScanner { } else refs.discardExprs(depth); - // If "super()": ignore - we don't try to track into superclasses - if (TreeInfo.name(invoke.meth) == names._super) + // If "super()": we don't invoke it (we don't track into superclasses) but we do execute any + // non-static field initializers and initialization blocks because this is when they happen. + if (TreeInfo.name(invoke.meth) == names._super) { + currentMethod.declaringClass.defs.stream() + .filter(def -> (TreeInfo.flags(def) & Flags.STATIC) == 0) + .forEach(def -> { + switch (def) { + case JCBlock block -> analyzeInitializer(invoke, block, receiverRefs, () -> visitBlock(block)); + case JCVariableDecl varDecl -> analyzeInitializer(invoke, varDecl, receiverRefs, () -> scan(varDecl)); + default -> { } + } + }); return; + } // "Invoke" the method invoke(invoke, sym, invoke.args, receiverRefs); } - private void invoke(JCTree site, Symbol sym, List args, RefSet receiverRefs) { + // Analyze a field initializer or initialization block after encountering a super() invocation + private void analyzeInitializer(JCMethodInvocation site, JCTree initializer, RefSet receiverRefs, Runnable action) { + RefSet refsPrev = refs; + refs = RefSet.newEmpty(); + int depthPrev = depth; + depth = 0; + callStack.add(new StackFrame(currentMethod, initializer, site)); + try { + refs.addAll(receiverRefs); + action.run(); + } finally { + callStack.remove(callStack.size() - 1); + depth = depthPrev; + refs = refsPrev; + } + } - // Skip if ignoring warnings for a constructor invoked via 'this()' - if (suppressed.contains(sym)) - return; + private void invoke(JCTree site, Symbol sym, List args, RefSet receiverRefs) { // Ignore final methods in java.lang.Object (getClass(), notify(), etc.) if (sym != null && @@ -627,7 +588,7 @@ public class ThisEscapeAnalyzer extends TreeScanner { } // Analyze method if possible, otherwise assume nothing - if (methodInfo != null && methodInfo.invokable()) + if (methodInfo != null && methodInfo.invokable) invokeInvokable(site, args, receiverRefs, methodInfo); else invokeUnknown(site, args, receiverRefs); @@ -644,12 +605,11 @@ public class ThisEscapeAnalyzer extends TreeScanner { } // Handle the invocation of a local analyzable method or constructor - private void invokeInvokable(JCTree site, List args, - RefSet receiverRefs, MethodInfo methodInfo) { - Assert.check(methodInfo.invokable()); + private void invokeInvokable(JCTree site, List args, RefSet receiverRefs, MethodInfo methodInfo) { + Assert.check(methodInfo.invokable); // Collect 'this' references found in method parameters - JCMethodDecl method = methodInfo.declaration(); + JCMethodDecl method = methodInfo.declaration; RefSet paramRefs = RefSet.newEmpty(); List params = method.params; while (args.nonEmpty() && params.nonEmpty()) { @@ -663,13 +623,13 @@ public class ThisEscapeAnalyzer extends TreeScanner { } // "Invoke" the method - JCClassDecl methodClassPrev = methodClass; - methodClass = methodInfo.declaringClass(); + MethodInfo currentMethodPrev = currentMethod; + currentMethod = methodInfo; RefSet refsPrev = refs; refs = RefSet.newEmpty(); int depthPrev = depth; depth = 0; - callStack.push(site); + callStack.add(new StackFrame(currentMethodPrev, null, site)); try { // Add initial references from method receiver @@ -706,10 +666,10 @@ public class ThisEscapeAnalyzer extends TreeScanner { .map(ref -> new ExprRef(depthPrev, ref)) .forEach(refsPrev::add); } finally { - callStack.pop(); + callStack.remove(callStack.size() - 1); depth = depthPrev; refs = refsPrev; - methodClass = methodClassPrev; + currentMethod = currentMethodPrev; } } @@ -755,7 +715,7 @@ public class ThisEscapeAnalyzer extends TreeScanner { RefSet receiverRefs = receiverRefsForConstructor(tree.encl, tsym); // "Invoke" the constructor - if (methodInfo != null && methodInfo.invokable()) + if (methodInfo != null && methodInfo.invokable) invokeInvokable(tree, tree.args, receiverRefs, methodInfo); else invokeUnknown(tree, tree.args, receiverRefs); @@ -787,9 +747,10 @@ public class ThisEscapeAnalyzer extends TreeScanner { // Determine if an unqualified "new Foo()" constructor gets 'this' as an implicit outer instance private boolean hasImplicitOuterInstance(TypeSymbol tsym) { - return tsym != methodClass.sym + ClassSymbol currentClassSym = currentMethod.declaringClass.sym; + return tsym != currentClassSym && tsym.hasOuterInstance() - && tsym.isEnclosedBy(methodClass.sym); + && tsym.isEnclosedBy(currentClassSym); } // @@ -829,13 +790,13 @@ public class ThisEscapeAnalyzer extends TreeScanner { MethodSymbol hasNext = null; MethodSymbol next = null; if (elemType == null) { - Symbol iteratorSym = rs.resolveQualifiedMethod(tree.expr.pos(), attrEnv, + Symbol iteratorSym = rs.resolveQualifiedMethod(tree.expr.pos(), topLevelEnv, tree.expr.type, names.iterator, List.nil(), List.nil()); if (iteratorSym instanceof MethodSymbol) { iterator = (MethodSymbol)iteratorSym; - Symbol hasNextSym = rs.resolveQualifiedMethod(tree.expr.pos(), attrEnv, + Symbol hasNextSym = rs.resolveQualifiedMethod(tree.expr.pos(), topLevelEnv, iterator.getReturnType(), names.hasNext, List.nil(), List.nil()); - Symbol nextSym = rs.resolveQualifiedMethod(tree.expr.pos(), attrEnv, + Symbol nextSym = rs.resolveQualifiedMethod(tree.expr.pos(), topLevelEnv, iterator.getReturnType(), names.next, List.nil(), List.nil()); if (hasNextSym instanceof MethodSymbol) hasNext = (MethodSymbol)hasNextSym; @@ -974,7 +935,7 @@ public class ThisEscapeAnalyzer extends TreeScanner { Stream methodRefs = refs.removeExprs(depth); // Explicit 'this' reference? The expression references whatever 'this' references - Type.ClassType currentClassType = (Type.ClassType)methodClass.sym.type; + Type.ClassType currentClassType = (Type.ClassType)currentMethod.declaringClass.sym.type; if (TreeInfo.isExplicitThisReference(types, currentClassType, tree)) { refs.find(ThisRef.class) .map(ref -> new ExprRef(depth, ref)) @@ -1059,7 +1020,7 @@ public class ThisEscapeAnalyzer extends TreeScanner { MethodSymbol sym = (MethodSymbol)tree.sym; // Check for implicit 'this' reference - ClassSymbol methodClassSym = methodClass.sym; + ClassSymbol methodClassSym = currentMethod.declaringClass.sym; if (methodClassSym.isSubClass(sym.owner, types)) { refs.find(ThisRef.class) .map(ref -> new ExprRef(depth, ref)) @@ -1243,53 +1204,49 @@ public class ThisEscapeAnalyzer extends TreeScanner { // Helper methods - private void visitTopLevel(Env env, JCClassDecl klass, Runnable action) { - Assert.check(attrEnv == null); + private void analyzeConstructor(MethodInfo constructor) { Assert.check(targetClass == null); - Assert.check(methodClass == null); + Assert.check(currentMethod == null); Assert.check(depth == -1); Assert.check(refs == null); - attrEnv = env; - targetClass = klass; - methodClass = klass; + targetClass = constructor.declaringClass; + currentMethod = constructor; try { // Add the initial 'this' reference refs = RefSet.newEmpty(); refs.add(new ThisRef(targetClass.sym, EnumSet.of(Indirection.DIRECT))); - // Perform action - this.visitScoped(false, action); + // Analyze constructor + visitScoped(false, () -> scan(constructor.declaration.body)); } finally { Assert.check(depth == -1); - attrEnv = null; - methodClass = null; + currentMethod = null; targetClass = null; refs = null; } } // Recurse through indirect code that might get executed later, e.g., a lambda. - // We stash any pending warning and the current RefSet, then recurse into the deferred - // code (still using the current RefSet) to see if it would leak. Then we restore the - // pending warning and the current RefSet. Finally, if the deferred code would have - // leaked, we create an indirect ExprRef because it must be holding a 'this' reference. - // If the deferred code would not leak, then obviously no leak is possible, period. + // We record the current number of (real) warnings, then recurse into the deferred + // code (still using the current RefSet) to see if that number increases, i.e., to + // see if it would leak. Then we discard any new warnings and the lambda's RefSet. + // Finally, if the deferred code would have leaked, we create an indirect ExprRef + // because the lambda must be holding a 'this' reference. If not, no leak is possible. private void visitDeferred(Runnable deferredCode) { - DiagnosticPosition[] pendingWarningPrev = pendingWarning; - pendingWarning = null; + int numWarningsPrev = warningList.size(); RefSet refsPrev = refs.clone(); boolean deferredCodeLeaks; try { deferredCode.run(); - deferredCodeLeaks = pendingWarning != null; + deferredCodeLeaks = warningList.size() > numWarningsPrev; // There can be ExprRef's if the deferred code returns something. // Don't let them escape unnoticed. deferredCodeLeaks |= refs.discardExprs(depth); } finally { refs = refsPrev; - pendingWarning = pendingWarningPrev; + warningList.subList(numWarningsPrev, warningList.size()).clear(); } if (deferredCodeLeaks) refs.add(new ExprRef(depth, syms.objectType.tsym, EnumSet.of(Indirection.INDIRECT))); @@ -1341,24 +1298,9 @@ public class ThisEscapeAnalyzer extends TreeScanner { // Note a possible 'this' reference leak at the specified location private void leakAt(JCTree tree) { - - // Generate at most one warning per statement - if (pendingWarning != null) - return; - - // Snapshot the current stack trace - callStack.push(tree.pos()); - pendingWarning = callStack.toArray(new DiagnosticPosition[0]); - callStack.pop(); - } - - // Copy pending warning, if any, to the warning list and reset - private boolean copyPendingWarning() { - if (pendingWarning == null) - return false; - warningList.add(pendingWarning); - pendingWarning = null; - return true; + callStack.add(new StackFrame(currentMethod, null, tree)); // include the point of leakage in the stack + warningList.add(new Warning(targetClass, new ArrayList<>(callStack))); + callStack.remove(callStack.size() - 1); } // Does the symbol correspond to a parameter or local variable (not a field)? @@ -1398,7 +1340,7 @@ public class ThisEscapeAnalyzer extends TreeScanner { private boolean checkInvariants(boolean analyzing, boolean allowExpr) { Assert.check(analyzing == isAnalyzing()); if (isAnalyzing()) { - Assert.check(methodClass != null); + Assert.check(currentMethod != null); Assert.check(targetClass != null); Assert.check(refs != null); Assert.check(depth >= 0); @@ -1409,7 +1351,6 @@ public class ThisEscapeAnalyzer extends TreeScanner { Assert.check(refs == null); Assert.check(depth == -1); Assert.check(callStack.isEmpty()); - Assert.check(pendingWarning == null); Assert.check(invocations.isEmpty()); } return true; @@ -1788,12 +1729,130 @@ public class ThisEscapeAnalyzer extends TreeScanner { } } +// StackFrame + + // Information about one frame on the call stack + private class StackFrame { + + final MethodInfo method; // the method containing the statement + final JCTree site; // the call site within the method + final JCTree initializer; // originating field or initialization block, else null + final boolean suppressible; // whether warning can be suppressed at this frame + + StackFrame(MethodInfo method, JCTree initializer, JCTree site) { + this.method = method; + this.initializer = initializer; + this.site = site; + this.suppressible = initializer != null || (method.constructor && method.declaringClass == targetClass); + } + + boolean isSuppressed() { + return suppressible && + suppressed.contains(initializer instanceof JCVariableDecl v ? v.sym : method.declaration.sym); + } + + int comparePos(StackFrame that) { + return Integer.compare(this.site.pos().getPreferredPosition(), that.site.pos().getPreferredPosition()); + } + + @Override + public String toString() { + return "StackFrame" + + "[" + method.declaration.sym + "@" + site.pos().getPreferredPosition() + + (initializer != null ? ",init@" + initializer.pos().getPreferredPosition() : "") + + "]"; + } + } + +// Warning + + // Information about one warning we have generated + private class Warning { + + final JCClassDecl declaringClass; // the class whose instance is leaked + final ArrayList stack; // the call stack where the leak happens + final JCTree origin; // the originating ctor, field, or init block + + Warning(JCClassDecl declaringClass, ArrayList stack) { + this.declaringClass = declaringClass; + this.stack = stack; + this.origin = stack.stream() + .map(frame -> frame.initializer) + .filter(Objects::nonNull) + .findFirst() + .orElseGet(() -> stack.get(0).method.declaration); // default to the initial constructor + } + + // Used to eliminate redundant warnings. Warning A is redundant with warning B if the call stack of A includes + // the call stack of B plus additional initial frame(s). For example, if constructor B = Foo(int x) generates a + // warning, then generating warning for some other constructor A when it invokes this(123) would be redundant. + boolean isRedundantWith(Warning that) { + int numExtra = this.stack.size() - that.stack.size(); + return numExtra >= 0 && + IntStream.range(0, that.stack.size()) + .allMatch(index -> this.stack.get(numExtra + index).comparePos(that.stack.get(index)) == 0); + }; + + // Order warnings by their stack frames, lexicographically in reverse calling order, which will cause + // all warnings that are isRedundantWith() some other warning to immediately follow that warning. + static int sortByStackFrames(Warning warning1, Warning warning2) { + int index1 = warning1.stack.size(); + int index2 = warning2.stack.size(); + while (true) { + boolean end1 = --index1 < 0; + boolean end2 = --index2 < 0; + if (end1 && end2) + return 0; + if (end1) + return -1; + if (end2) + return 1; + int diff = warning1.stack.get(index1).comparePos(warning2.stack.get(index2)); + if (diff != 0) + return diff; + } + }; + + // Determine whether this warning is suppressed. A single "this-escape" warning involves multiple source code + // positions, so we must determine suppression manually. We do this as follows: A warning is suppressed if + // "this-escape" is disabled at any position in the stack where that stack frame corresponds to a constructor + // or field initializer in the target class. That means, for example, @SuppressWarnings("this-escape") annotations + // on regular methods are ignored. Here we work our way back up the call stack from the point of the leak until + // we encounter a suppressible stack frame. + boolean isSuppressed() { + for (int index = stack.size() - 1; index >= 0; index--) { + if (stack.get(index).isSuppressed()) + return true; + } + return false; + } + + // If this is a field or initializer warning, trim the initial stack frame(s) up through the super() call + void trimInitializerFrames() { + for (int i = 0; i < stack.size(); i++) { + if (stack.get(i).initializer != null) { + stack.subList(0, i + 1).clear(); + break; + } + } + } + + @Override + public String toString() { + return "Warning" + + "[class=" + declaringClass.sym.flatname + + ",stack=[\n " + stack.stream().map(StackFrame::toString).collect(Collectors.joining("\n ")) + "]" + + "]"; + } + } + // MethodInfo // Information about a constructor or method in the compilation unit private record MethodInfo( JCClassDecl declaringClass, // the class declaring "declaration" JCMethodDecl declaration, // the method or constructor itself + boolean constructor, // the method is a constructor boolean analyzable, // it's a constructor that we should analyze boolean invokable) { // it may be safely "invoked" during analysis @@ -1801,6 +1860,7 @@ public class ThisEscapeAnalyzer extends TreeScanner { public String toString() { return "MethodInfo" + "[method=" + declaringClass.sym.flatname + "." + declaration.sym + + ",constructor=" + constructor + ",analyzable=" + analyzable + ",invokable=" + invokable + "]"; diff --git a/test/langtools/tools/javac/warnings/ThisEscape.java b/test/langtools/tools/javac/warnings/ThisEscape.java index 93ccb6e0830..070f371d161 100644 --- a/test/langtools/tools/javac/warnings/ThisEscape.java +++ b/test/langtools/tools/javac/warnings/ThisEscape.java @@ -1,6 +1,6 @@ /* * @test /nodynamiccopyright/ - * @bug 8015831 + * @bug 8015831 8355753 * @compile/ref=ThisEscape.out -Xlint:this-escape -XDrawDiagnostics ThisEscape.java * @summary Verify 'this' escape detection */ @@ -765,4 +765,22 @@ public class ThisEscape { return this.obj; } } + + // JDK-8355753 - @SuppressWarnings("this-escape") not respected for indirect leak via field + public static class SuppressedIndirectLeakViaField { + + private final int x = this.mightLeak(); // this leak should be suppressed + + public SuppressedIndirectLeakViaField() { + this(""); + } + + @SuppressWarnings("this-escape") + private SuppressedIndirectLeakViaField(String s) { + } + + public int mightLeak() { + return 0; + } + } }