Remove toString from Painless user tree nodes (elastic#54117)

* remove isNull from AExpression * remove explicit cast optimization * remove modification of semantic tree for casting * remove ECast node * start of input/output in expressions * partial change for input and output in expression nodes * add input/output objects for expressions * fix shift bug in EBinary * add input/output to statements * response to pr comment * updated expression nodes to remove member state * update statements to remove most mutable state * fix def optimization * remove SField * fix def optimization in assignment * move debug stream * add methods to generate class scope * move info around * remove toString on user nodes * fix imports * response to pr comments * response to PR comments * fix more todos * fix issue number * fix space Co-authored-by: Jack Conradson <[email protected]> Co-authored-by: Jack Conradson <[email protected]>
ywangd · Mar 27, 2020 · 1bb36d7 · 1bb36d7
1 parent 0c52a92
commit 1bb36d7
Show file tree

Hide file tree

Showing 72 changed files with 121 additions and 1,490 deletions.
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java
@@ -207,9 +207,10 @@ private static void addFactoryMethod(Map<String, Class<?>> additionalClasses, Cl
      * @return The ScriptRoot used to compile
      */
     ScriptRoot compile(Loader loader, String name, String source, CompilerSettings settings) {
+        String scriptName = Location.computeSourceName(name);
         ScriptClassInfo scriptClassInfo = new ScriptClassInfo(painlessLookup, scriptClass);
-        SClass root = Walker.buildPainlessTree(scriptClassInfo, name, source, settings, painlessLookup, null);
-        ScriptRoot scriptRoot = new ScriptRoot(painlessLookup, settings, scriptClassInfo, root);
+        SClass root = Walker.buildPainlessTree(scriptClassInfo, scriptName, source, settings, painlessLookup);
+        ScriptRoot scriptRoot = new ScriptRoot(painlessLookup, settings, scriptClassInfo, scriptName, source);
         ClassNode classNode = root.writeClass(scriptRoot);
         DefBootstrapInjectionPhase.phase(classNode);
         ScriptInjectionPhase.phase(scriptRoot, classNode);
@@ -236,10 +237,12 @@ ScriptRoot compile(Loader loader, String name, String source, CompilerSettings s
      * @return The bytes for compilation.
      */
     byte[] compile(String name, String source, CompilerSettings settings, Printer debugStream) {
+        String scriptName = Location.computeSourceName(name);
         ScriptClassInfo scriptClassInfo = new ScriptClassInfo(painlessLookup, scriptClass);
-        SClass root = Walker.buildPainlessTree(scriptClassInfo, name, source, settings, painlessLookup, debugStream);
-        ScriptRoot scriptRoot = new ScriptRoot(painlessLookup, settings, scriptClassInfo, root);
+        SClass root = Walker.buildPainlessTree(scriptClassInfo, scriptName, source, settings, painlessLookup);
+        ScriptRoot scriptRoot = new ScriptRoot(painlessLookup, settings, scriptClassInfo, scriptName, source);
         ClassNode classNode = root.writeClass(scriptRoot);
+        classNode.setDebugStream(debugStream);
         DefBootstrapInjectionPhase.phase(classNode);
         ScriptInjectionPhase.phase(scriptRoot, classNode);
 

diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java
@@ -159,7 +159,6 @@
 import org.elasticsearch.painless.node.SThrow;
 import org.elasticsearch.painless.node.STry;
 import org.elasticsearch.painless.node.SWhile;
-import org.objectweb.asm.util.Printer;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -170,26 +169,21 @@
 public final class Walker extends PainlessParserBaseVisitor<ANode> {
 
     public static SClass buildPainlessTree(ScriptClassInfo mainMethod, String sourceName,
-                                            String sourceText, CompilerSettings settings, PainlessLookup painlessLookup,
-                                            Printer debugStream) {
-        return new Walker(mainMethod, sourceName, sourceText, settings, painlessLookup, debugStream).source;
+                                           String sourceText, CompilerSettings settings, PainlessLookup painlessLookup) {
+        return new Walker(mainMethod, sourceName, sourceText, settings, painlessLookup).source;
     }
 
     private final ScriptClassInfo scriptClassInfo;
     private final SClass source;
     private final CompilerSettings settings;
-    private final Printer debugStream;
     private final String sourceName;
-    private final String sourceText;
     private final PainlessLookup painlessLookup;
 
     private Walker(ScriptClassInfo scriptClassInfo, String sourceName, String sourceText,
-                   CompilerSettings settings, PainlessLookup painlessLookup, Printer debugStream) {
+                   CompilerSettings settings, PainlessLookup painlessLookup) {
         this.scriptClassInfo = scriptClassInfo;
-        this.debugStream = debugStream;
         this.settings = settings;
-        this.sourceName = Location.computeSourceName(sourceName);
-        this.sourceText = sourceText;
+        this.sourceName = sourceName;
         this.painlessLookup = painlessLookup;
         this.source = (SClass)visit(buildAntlrTree(sourceText));
     }
@@ -279,7 +273,7 @@ public ANode visitSource(SourceContext ctx) {
                 location(ctx), statements), true, false, false, true);
         functions.add(execute);
 
-        return new SClass(scriptClassInfo, sourceName, sourceText, debugStream, location(ctx), functions);
+        return new SClass(location(ctx), functions);
     }
 
     @Override

diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ir/ClassNode.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ir/ClassNode.java
@@ -69,36 +69,9 @@ public BlockNode getClinitBlockNode() {
 
     /* ---- end tree structure, begin node data ---- */
 
-    private ScriptClassInfo scriptClassInfo;
-    private String name;
-    private String sourceText;
     private Printer debugStream;
     private ScriptRoot scriptRoot;
 
-    public void setScriptClassInfo(ScriptClassInfo scriptClassInfo) {
-        this.scriptClassInfo = scriptClassInfo;
-    }
-
-    public ScriptClassInfo getScriptClassInfo() {
-        return scriptClassInfo;
-    }
-
-    public void setName(String name) {
-        this.name = name;
-    }
-
-    public String getName() {
-        return name;
-    }
-
-    public void setSourceText(String sourceText) {
-        this.sourceText = sourceText;
-    }
-
-    public String getSourceText() {
-        return sourceText;
-    }
-
     public void setDebugStream(Printer debugStream) {
         this.debugStream = debugStream;
     }
@@ -125,7 +98,8 @@ public ClassNode() {
     }
 
     public byte[] write() {
-        BitSet statements = new BitSet(sourceText.length());
+        ScriptClassInfo scriptClassInfo = scriptRoot.getScriptClassInfo();
+        BitSet statements = new BitSet(scriptRoot.getScriptSource().length());
         scriptRoot.addStaticConstant("$STATEMENTS", statements);
 
         // Create the ClassWriter.
@@ -139,7 +113,7 @@ public byte[] write() {
         ClassWriter classWriter = new ClassWriter(scriptRoot.getCompilerSettings(), statements, debugStream,
                 scriptClassInfo.getBaseClass(), classFrames, classAccess, className, classInterfaces);
         ClassVisitor classVisitor = classWriter.getClassVisitor();
-        classVisitor.visitSource(Location.computeSourceName(name), null);
+        classVisitor.visitSource(Location.computeSourceName(scriptRoot.getScriptName()), null);
 
         org.objectweb.asm.commons.Method init;
 

diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ANode.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ANode.java
@@ -21,17 +21,8 @@
 
 import org.elasticsearch.painless.Location;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Iterator;
-import java.util.List;
 import java.util.Objects;
 
-import static java.lang.Math.max;
-import static java.util.Collections.emptyList;
-
 /**
  * The superclass for all nodes.
  */
@@ -40,7 +31,7 @@ public abstract class ANode {
     /**
      * The identifier of the script and character offset used for debugging and errors.
      */
-    final Location location;
+    protected final Location location;
 
     /**
      * Standard constructor with location used for error tracking.
@@ -55,81 +46,4 @@ public abstract class ANode {
     RuntimeException createError(RuntimeException exception) {
         return location.createError(exception);
     }
-
-    /**
-     * Subclasses should implement this with a method like {@link #singleLineToString(Object...)} or
-     * {@link #multilineToString(Collection, Collection)}.
-     */
-    public abstract String toString();
-
-    // Below are utilities for building the toString
-
-    /**
-     * Build {@link #toString()} for a node without inserting line breaks between the sub-nodes.
-     */
-    protected String singleLineToString(Object... subs) {
-        return singleLineToString(Arrays.asList(subs));
-    }
-
-    /**
-     * Build {@link #toString()} for a node without inserting line breaks between the sub-nodes.
-     */
-    protected String singleLineToString(Collection<? extends Object> subs) {
-        return joinWithName(getClass().getSimpleName(), subs, emptyList());
-    }
-
-    /**
-     * Build {@link #toString()} for a node that optionally ends in {@code (Args some arguments here)}. Usually function calls.
-     */
-    protected String singleLineToStringWithOptionalArgs(Collection<? extends ANode> arguments, Object... restOfSubs) {
-        List<Object> subs = new ArrayList<>();
-        Collections.addAll(subs, restOfSubs);
-        if (false == arguments.isEmpty()) {
-            subs.add(joinWithName("Args", arguments, emptyList()));
-        }
-        return singleLineToString(subs);
-    }
-
-    /**
-     * Build {@link #toString()} for a node that should have new lines after some of its sub-nodes.
-     */
-    protected String multilineToString(Collection<? extends Object> sameLine, Collection<? extends Object> ownLine) {
-        return joinWithName(getClass().getSimpleName(), sameLine, ownLine);
-    }
-
-    /**
-     * Zip two (potentially uneven) lists together into for {@link #toString()}.
-     */
-    protected List<String> pairwiseToString(Collection<? extends Object> lefts, Collection<? extends Object> rights) {
-        List<String> pairs = new ArrayList<>(max(lefts.size(), rights.size()));
-        Iterator<? extends Object> left = lefts.iterator();
-        Iterator<? extends Object> right = rights.iterator();
-        while (left.hasNext() || right.hasNext()) {
-            pairs.add(joinWithName("Pair",
-                    Arrays.asList(left.hasNext() ? left.next() : "<uneven>", right.hasNext() ? right.next() : "<uneven>"),
-                    emptyList()));
-        }
-        return pairs;
-    }
-
-    /**
-     * Build a {@link #toString()} for some expressions. Usually best to use {@link #singleLineToString(Object...)} or
-     * {@link #multilineToString(Collection, Collection)} instead because they include the name of the node by default.
-     */
-    protected String joinWithName(String name, Collection<? extends Object> sameLine,
-            Collection<? extends Object> ownLine) {
-        StringBuilder b = new StringBuilder();
-        b.append('(').append(name);
-        for (Object sub : sameLine) {
-            b.append(' ').append(sub);
-        }
-        if (ownLine.size() == 1 && sameLine.isEmpty()) {
-            b.append(' ').append(ownLine.iterator().next());
-        } else {
-            for (Object sub : ownLine) {
-                b.append("\n  ").append(Objects.toString(sub).replace("\n", "\n  "));
-            }
-        }
-        return b.append(')').toString();
-    }
 }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/DResolvedType.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/DResolvedType.java
@@ -73,9 +73,4 @@ public DResolvedType resolveType(PainlessLookup painlessLookup) {
     public Class<?> getType() {
         return type;
     }
-
-    @Override
-    public String toString() {
-        return "(DResolvedType [" + PainlessLookupUtility.typeToCanonicalTypeName(type) + "])";
-    }
 }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/DUnresolvedType.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/DUnresolvedType.java
@@ -52,10 +52,5 @@ public DResolvedType resolveType(PainlessLookup painlessLookup) {
 
         return new DResolvedType(location, type);
     }
-
-    @Override
-    public String toString() {
-        return "(DUnresolvedType [" + typeName + "])";
-    }
 }
 
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EAssignment.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EAssignment.java
@@ -36,8 +36,6 @@
 import org.elasticsearch.painless.lookup.def;
 import org.elasticsearch.painless.symbol.ScriptRoot;
 
-import java.util.ArrayList;
-import java.util.List;
 import java.util.Objects;
 
 /**
@@ -251,24 +249,4 @@ Output analyze(ClassNode classNode, ScriptRoot scriptRoot, Scope scope, Input in
 
         return output;
     }
-
-    @Override
-    public String toString() {
-        List<Object> subs = new ArrayList<>();
-        subs.add(lhs);
-        if (rhs != null) {
-            // Make sure "=" is in the symbol so this is easy to read at a glance
-            subs.add(operation == null ? "=" : operation.symbol + "=");
-            subs.add(rhs);
-            return singleLineToString(subs);
-        }
-        subs.add(operation.symbol);
-        if (pre) {
-            subs.add("pre");
-        }
-        if (post) {
-            subs.add("post");
-        }
-        return singleLineToString(subs);
-    }
 }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EBinary.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EBinary.java
@@ -151,9 +151,4 @@ Output analyze(ClassNode classNode, ScriptRoot scriptRoot, Scope scope, Input in
 
         return output;
     }
-
-    @Override
-    public String toString() {
-        return singleLineToString(left, operation.symbol, right);
-    }
 }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EBool.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EBool.java
@@ -74,9 +74,4 @@ Output analyze(ClassNode classNode, ScriptRoot scriptRoot, Scope scope, Input in
 
         return output;
     }
-
-    @Override
-    public String toString() {
-        return singleLineToString(left, operation.symbol, right);
-    }
 }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EBoolean.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EBoolean.java
@@ -57,9 +57,4 @@ Output analyze(ClassNode classNode, ScriptRoot scriptRoot, Scope scope, Input in
 
         return output;
     }
-
-    @Override
-    public String toString() {
-        return singleLineToString(constant);
-    }
 }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECallLocal.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ECallLocal.java
@@ -22,8 +22,8 @@
 import org.elasticsearch.painless.Location;
 import org.elasticsearch.painless.Scope;
 import org.elasticsearch.painless.ir.ClassNode;
-import org.elasticsearch.painless.ir.MemberCallNode;
 import org.elasticsearch.painless.ir.FieldNode;
+import org.elasticsearch.painless.ir.MemberCallNode;
 import org.elasticsearch.painless.lookup.PainlessClassBinding;
 import org.elasticsearch.painless.lookup.PainlessInstanceBinding;
 import org.elasticsearch.painless.lookup.PainlessMethod;
@@ -188,9 +188,4 @@ Output analyze(ClassNode classNode, ScriptRoot scriptRoot, Scope scope, Input in
 
         return output;
     }
-
-    @Override
-    public String toString() {
-        return singleLineToStringWithOptionalArgs(arguments, name);
-    }
 }
diff --git a/...es/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java b/...es/lang-painless/src/main/java/org/elasticsearch/painless/node/ECapturingFunctionRef.java
@@ -100,9 +100,4 @@ public String getPointer() {
     public List<Class<?>> getCaptures() {
         return Collections.singletonList(captured.getType());
     }
-
-    @Override
-    public String toString() {
-        return singleLineToString(variable, call);
-    }
 }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EComp.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EComp.java
@@ -107,9 +107,4 @@ Output analyze(ClassNode classNode, ScriptRoot scriptRoot, Scope scope, Input in
 
         return output;
     }
-
-    @Override
-    public String toString() {
-        return singleLineToString(left, operation.symbol, right);
-    }
 }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EConditional.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EConditional.java
@@ -99,9 +99,4 @@ Output analyze(ClassNode classNode, ScriptRoot scriptRoot, Scope scope, Input in
 
         return output;
     }
-
-    @Override
-    public String toString() {
-        return singleLineToString(condition, left, right);
-    }
 }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EConstant.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EConstant.java
@@ -77,13 +77,4 @@ Output analyze(ClassNode classNode, ScriptRoot scriptRoot, Scope scope, Input in
 
         return output;
     }
-
-    @Override
-    public String toString() {
-        String c = constant.toString();
-        if (constant instanceof String) {
-            c = "'" + c + "'";
-        }
-        return singleLineToString(constant.getClass().getSimpleName(), c);
-    }
 }
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EDecimal.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EDecimal.java
@@ -79,9 +79,4 @@ Output analyze(ClassNode classNode, ScriptRoot scriptRoot, Scope scope, Input in
 
         return output;
     }
-
-    @Override
-    public String toString() {
-        return singleLineToString(value);
-    }
 }