8230105: -XDfind=diamond crashes

lahodaj · lahodaj · commit 1d71dd8604c9 · 2019-08-30T12:24:16.000+02:00
Avoiding side-effects in Analyzer's speculative attribution.

Reviewed-by: mcimadamore, vromero
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Analyzer.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Analyzer.java
@@ -26,11 +26,12 @@
 package com.sun.tools.javac.comp;
 
 import java.util.ArrayDeque;
+import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Queue;
-import java.util.function.Predicate;
+import java.util.stream.Collectors;
 
 import com.sun.source.tree.LambdaExpressionTree;
 import com.sun.source.tree.NewClassTree;
@@ -42,6 +43,7 @@
 import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.comp.ArgumentAttr.LocalCacheContext;
+import com.sun.tools.javac.comp.DeferredAttr.AttributionMode;
 import com.sun.tools.javac.resources.CompilerProperties.Warnings;
 import com.sun.tools.javac.tree.JCTree;
 import com.sun.tools.javac.tree.JCTree.JCBlock;
@@ -446,7 +448,7 @@ void process(JCEnhancedForLoop oldTree, JCEnhancedForLoop newTree, boolean hasEr
      */
     Env<AttrContext> copyEnvIfNeeded(JCTree tree, Env<AttrContext> env) {
         if (!analyzerModes.isEmpty() &&
-                !env.info.isSpeculative &&
+                !env.info.attributionMode.isSpeculative &&
                 TreeInfo.isStatement(tree) &&
                 !tree.hasTag(LABELLED)) {
             Env<AttrContext> analyzeEnv =
@@ -562,10 +564,14 @@ void doAnalysis(RewritingContext rewriting) {
 
             //TODO: to further refine the analysis, try all rewriting combinations
             deferredAttr.attribSpeculative(treeToAnalyze, rewriting.env, attr.statInfo, new TreeRewriter(rewriting),
-                    t -> rewriting.diagHandler(), argumentAttr.withLocalCacheContext());
+                    t -> rewriting.diagHandler(), AttributionMode.ANALYZER, argumentAttr.withLocalCacheContext());
             rewriting.analyzer.process(rewriting.oldTree, rewriting.replacement, rewriting.erroneous);
         } catch (Throwable ex) {
-            Assert.error("Analyzer error when processing: " + rewriting.originalTree);
+            Assert.error("Analyzer error when processing: " +
+                         rewriting.originalTree + ":" + ex.toString() + "\n" +
+                         Arrays.stream(ex.getStackTrace())
+                               .map(se -> se.toString())
+                               .collect(Collectors.joining("\n")));
         } finally {
             log.useSource(prevSource.getFile());
             localCacheContext.leave();
@@ -628,6 +634,11 @@ public void visitBlock(JCBlock tree) {
             //do nothing (prevents seeing same stuff twice)
         }
 
+        @Override
+        public void visitLambda(JCLambda tree) {
+            //do nothing (prevents seeing same stuff in lambda expression twice)
+        }
+
         @Override
         public void visitSwitch(JCSwitch tree) {
             scan(tree.getExpression());
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ArgumentAttr.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ArgumentAttr.java
@@ -136,7 +136,7 @@ protected ArgumentAttr(Context context) {
      */
     void setResult(JCExpression tree, Type type) {
         result = type;
-        if (env.info.isSpeculative) {
+        if (env.info.attributionMode == DeferredAttr.AttributionMode.SPECULATIVE) {
             //if we are in a speculative branch we can save the type in the tree itself
             //as there's no risk of polluting the original tree.
             tree.type = result;
@@ -365,7 +365,7 @@ final public Type complete(DeferredType dt, ResultInfo resultInfo, DeferredAttrC
                 speculativeTypes.put(resultInfo, t);
                 return t;
             } else {
-                if (!env.info.isSpeculative) {
+                if (!env.info.attributionMode.isSpeculative) {
                     argumentTypeCache.remove(new UniquePos(dt.tree));
                 }
                 return deferredAttr.basicCompleter.complete(dt, resultInfo, deferredAttrContext);
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
@@ -735,11 +735,9 @@ Type attribType(JCTree tree, Env<AttrContext> env, Type pt) {
      */
     public Type attribStat(JCTree tree, Env<AttrContext> env) {
         Env<AttrContext> analyzeEnv = analyzer.copyEnvIfNeeded(tree, env);
-        try {
-            return attribTree(tree, env, statInfo);
-        } finally {
-            analyzer.analyzeIfNeeded(tree, analyzeEnv);
-        }
+        Type result = attribTree(tree, env, statInfo);
+        analyzer.analyzeIfNeeded(tree, analyzeEnv);
+        return result;
     }
 
     /** Attribute a list of expressions, returning a list of types.
@@ -933,7 +931,7 @@ Type attribIdentAsEnumType(Env<AttrContext> env, JCIdent id) {
 
     public void visitClassDef(JCClassDecl tree) {
         Optional<ArgumentAttr.LocalCacheContext> localCacheContext =
-                Optional.ofNullable(env.info.isSpeculative ?
+                Optional.ofNullable(env.info.attributionMode.isSpeculative ?
                         argumentAttr.withLocalCacheContext() : null);
         try {
             // Local and anonymous classes have not been entered yet, so we need to
@@ -3232,7 +3230,7 @@ public void visitReference(final JCMemberReference that) {
                 return;
             }
 
-            if (!env.info.isSpeculative && that.getMode() == JCMemberReference.ReferenceMode.NEW) {
+            if (!env.info.attributionMode.isSpeculative && that.getMode() == JCMemberReference.ReferenceMode.NEW) {
                 Type enclosingType = exprType.getEnclosingType();
                 if (enclosingType != null && enclosingType.hasTag(CLASS)) {
                     // Check for the existence of an apropriate outer instance
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/AttrContext.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/AttrContext.java
@@ -29,6 +29,7 @@
 import com.sun.tools.javac.util.*;
 import com.sun.tools.javac.code.*;
 import com.sun.tools.javac.code.Scope.WriteableScope;
+import com.sun.tools.javac.comp.DeferredAttr.AttributionMode;
 
 /** Contains information specific to the attribute and enter
  *  passes, to be used in place of the generic field in environments.
@@ -67,7 +68,7 @@ public class AttrContext {
 
     /** Is this a speculative attribution environment?
      */
-    boolean isSpeculative = false;
+    AttributionMode attributionMode = AttributionMode.FULL;
 
     /**
      *  Is this an attribution environment for an anonymous class instantiated using <> ?
@@ -133,7 +134,7 @@ AttrContext dup(WriteableScope scope) {
         info.defaultSuperCallSite = defaultSuperCallSite;
         info.isSerializable = isSerializable;
         info.isLambda = isLambda;
-        info.isSpeculative = isSpeculative;
+        info.attributionMode = attributionMode;
         info.isAnonymousDiamond = isAnonymousDiamond;
         info.isNewClass = isNewClass;
         info.preferredTreeForDiagnostics = preferredTreeForDiagnostics;
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/DeferredAttr.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/DeferredAttr.java
@@ -481,25 +481,28 @@ JCLambda attribSpeculativeLambda(JCLambda that, Env<AttrContext> env, ResultInfo
      */
     JCTree attribSpeculative(JCTree tree, Env<AttrContext> env, ResultInfo resultInfo) {
         return attribSpeculative(tree, env, resultInfo, treeCopier,
-                (newTree)->new DeferredAttrDiagHandler(log, newTree), null);
+                (newTree)->new DeferredAttrDiagHandler(log, newTree), AttributionMode.SPECULATIVE, null);
     }
 
     JCTree attribSpeculative(JCTree tree, Env<AttrContext> env, ResultInfo resultInfo, LocalCacheContext localCache) {
         return attribSpeculative(tree, env, resultInfo, treeCopier,
-                (newTree)->new DeferredAttrDiagHandler(log, newTree), localCache);
+                (newTree)->new DeferredAttrDiagHandler(log, newTree), AttributionMode.SPECULATIVE, localCache);
     }
 
     <Z> JCTree attribSpeculative(JCTree tree, Env<AttrContext> env, ResultInfo resultInfo, TreeCopier<Z> deferredCopier,
-                                 Function<JCTree, DeferredDiagnosticHandler> diagHandlerCreator,
+                                 Function<JCTree, DeferredDiagnosticHandler> diagHandlerCreator, AttributionMode attributionMode,
                                  LocalCacheContext localCache) {
         final JCTree newTree = deferredCopier.copy(tree);
         Env<AttrContext> speculativeEnv = env.dup(newTree, env.info.dup(env.info.scope.dupUnshared(env.info.scope.owner)));
-        speculativeEnv.info.isSpeculative = true;
+        speculativeEnv.info.attributionMode = attributionMode;
         Log.DeferredDiagnosticHandler deferredDiagnosticHandler = diagHandlerCreator.apply(newTree);
+        int nwarnings = log.nwarnings;
+        log.nwarnings = 0;
         try {
             attr.attribTree(newTree, speculativeEnv, resultInfo);
             return newTree;
         } finally {
+            log.nwarnings += nwarnings;
             enter.unenter(env.toplevel, newTree);
             log.popDiagnosticHandler(deferredDiagnosticHandler);
             if (localCache != null) {
@@ -1286,4 +1289,26 @@ public void visitReference(JCMemberReference tree) {
             }
         }
     }
+
+    /**
+     * Mode of attribution (used in AttrContext).
+     */
+    enum AttributionMode {
+        /**Normal, non-speculative, attribution.*/
+        FULL(false),
+        /**Speculative attribution on behalf of an Analyzer.*/
+        ANALYZER(true),
+        /**Speculative attribution.*/
+        SPECULATIVE(true);
+
+        AttributionMode(boolean isSpeculative) {
+            this.isSpeculative = isSpeculative;
+        }
+
+        boolean isSpeculative() {
+            return isSpeculative;
+        }
+
+        final boolean isSpeculative;
+    }
 }
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
@@ -2391,7 +2391,7 @@ Symbol findIdentInPackageInternal(Env<AttrContext> env, TypeSymbol pck,
         if (kind.contains(KindSelector.TYP)) {
             RecoveryLoadClass recoveryLoadClass =
                     allowModules && !kind.contains(KindSelector.PCK) &&
-                    !pck.exists() && !env.info.isSpeculative ?
+                    !pck.exists() && !env.info.attributionMode.isSpeculative ?
                         doRecoveryLoadClass : noRecovery;
             Symbol sym = loadClass(env, fullname, recoveryLoadClass);
             if (sym.exists()) {
diff --git a/test/langtools/tools/javac/analyzer/AnalyzerMandatoryWarnings.java b/test/langtools/tools/javac/analyzer/AnalyzerMandatoryWarnings.java
@@ -0,0 +1,16 @@
+/**
+ * @test
+ * @bug 8230105
+ * @summary Verify analyzers work reasonably in combination with mandatory warnings
+ * @compile/ref=AnalyzerMandatoryWarnings.out -Xlint:deprecation -XDrawDiagnostics -Xmaxwarns 1 -XDfind=lambda AnalyzerMandatoryWarnings.java
+ */
+public class AnalyzerMandatoryWarnings {
+    private void test() {
+        Runnable r = new Runnable() {
+            public void run() {
+                Depr r;
+            }
+        };
+    }
+}
+@Deprecated class Depr {}
diff --git a/test/langtools/tools/javac/analyzer/AnalyzerMandatoryWarnings.out b/test/langtools/tools/javac/analyzer/AnalyzerMandatoryWarnings.out
@@ -0,0 +1,2 @@
+AnalyzerMandatoryWarnings.java:11:17: compiler.warn.has.been.deprecated: Depr, compiler.misc.unnamed.package
+1 warning
diff --git a/test/langtools/tools/javac/analyzer/AnalyzerNotQuiteSpeculative.java b/test/langtools/tools/javac/analyzer/AnalyzerNotQuiteSpeculative.java
@@ -0,0 +1,25 @@
+/**
+ * @test /nodynamiccopyright/
+ * @bug 8230105
+ * @summary Ensuring speculative analysis on behalf of Analyzers works reasonably.
+ * @compile/ref=AnalyzerNotQuiteSpeculative.out -XDfind=diamond -XDrawDiagnostics AnalyzerNotQuiteSpeculative.java
+ */
+public class AnalyzerNotQuiteSpeculative {
+    private void test() {
+        Subclass1 c1 = null;
+        Subclass2 c2 = null;
+        Base b = null;
+
+        t(new C<Base>(c1).set(c2));
+        t(new C<Base>(b).set(c2));
+    }
+
+    public static class Base {}
+    public static class Subclass1 extends Base {}
+    public static class Subclass2 extends Base {}
+    public class C<T extends Base> {
+        public C(T t) {}
+        public C<T> set(T t) { return this; }
+    }
+    <T extends Base> void t(C<? extends Base> l) {}
+}
diff --git a/test/langtools/tools/javac/analyzer/AnalyzerNotQuiteSpeculative.out b/test/langtools/tools/javac/analyzer/AnalyzerNotQuiteSpeculative.out
@@ -0,0 +1,2 @@
+AnalyzerNotQuiteSpeculative.java:14:16: compiler.warn.diamond.redundant.args
+1 warning
diff --git a/test/langtools/tools/javac/analyzer/DoNoRunAnalyzersWhenException.java b/test/langtools/tools/javac/analyzer/DoNoRunAnalyzersWhenException.java
@@ -0,0 +1,107 @@
+/*
+ * Copyright (c) 2015, 2019, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8230105
+ * @summary Do not run Analyzers when Attr crashes with an exception.
+ * @modules jdk.compiler/com.sun.tools.javac.api
+ *          jdk.compiler/com.sun.tools.javac.comp
+ *          jdk.compiler/com.sun.tools.javac.tree
+ *          jdk.compiler/com.sun.tools.javac.util
+ */
+
+import java.io.IOException;
+import java.net.URI;
+import java.util.Arrays;
+import java.util.List;
+import javax.tools.JavaFileObject;
+import javax.tools.SimpleJavaFileObject;
+
+import com.sun.tools.javac.api.JavacTaskImpl;
+import com.sun.tools.javac.api.JavacTool;
+import com.sun.tools.javac.comp.Attr;
+import com.sun.tools.javac.tree.JCTree.JCVariableDecl;
+import com.sun.tools.javac.util.Context;
+import com.sun.tools.javac.util.Context.Factory;
+
+public class DoNoRunAnalyzersWhenException {
+
+    public static void main(String... args) throws Exception {
+        new DoNoRunAnalyzersWhenException().run();
+    }
+
+    void run() throws IOException {
+        JavacTool tool = JavacTool.create();
+        JavaSource source = new JavaSource("class Test {" +
+                                           "    { String STOP = \"\"; }" +
+                                           "}");
+        Context context = new Context();
+        CrashingAttr.preRegister(context);
+        List<JavaSource> inputs = Arrays.asList(source);
+        JavacTaskImpl task =
+                (JavacTaskImpl) tool.getTask(null, null, null, List.of("-XDfind=all"), null, inputs, context);
+        try {
+            task.analyze(null);
+            throw new AssertionError("Expected exception not seen.");
+        } catch (StopException ex) {
+            //ok
+        }
+    }
+
+    static class CrashingAttr extends Attr {
+
+        static void preRegister(Context context) {
+            context.put(attrKey, (Factory<Attr>) c -> new CrashingAttr(c));
+        }
+
+        CrashingAttr(Context context) {
+            super(context);
+        }
+
+        @Override public void visitVarDef(JCVariableDecl tree) {
+            if (tree.name.contentEquals("STOP"))
+                throw new StopException();
+            super.visitVarDef(tree);
+        }
+    }
+
+    static class StopException extends NullPointerException {}
+
+    class JavaSource extends SimpleJavaFileObject {
+
+        String source;
+
+        JavaSource(String source) {
+            super(URI.create("myfo:/Test.java"), JavaFileObject.Kind.SOURCE);
+            this.source = source;
+        }
+
+        @Override
+        public CharSequence getCharContent(boolean ignoreEncodingErrors) {
+            return source;
+        }
+
+    }
+
+}
diff --git a/test/langtools/tools/javac/analyzer/StuckLambdas.java b/test/langtools/tools/javac/analyzer/StuckLambdas.java
diff --git a/test/langtools/tools/javac/analyzer/StuckLambdas.out b/test/langtools/tools/javac/analyzer/StuckLambdas.out

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+AnalyzerMandatoryWarnings.java:11:17: compiler.warn.has.been.deprecated: Depr, compiler.misc.unnamed.package`
	`2`	`+1 warning`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+AnalyzerNotQuiteSpeculative.java:14:16: compiler.warn.diamond.redundant.args`
	`2`	`+1 warning`