Remove un-used doc fields phase in Painless (#75851)

Originally, a doc fields phase was created to collect information about what fields are accessed using constant values. This was going to be used for detecting cyclical field access in runtime fields, but another approach was taken instead. This change deletes the un-used phase.
elastic · Jul 30, 2021 · 8b66263 · 8b66263
1 parent bfe138d
commit 8b66263
Show file tree

Hide file tree

Showing 6 changed files with 0 additions and 255 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
@@ -17,7 +17,6 @@
 import org.elasticsearch.painless.phase.DefaultIRTreeToASMBytesPhase;
 import org.elasticsearch.painless.phase.DefaultStaticConstantExtractionPhase;
 import org.elasticsearch.painless.phase.DefaultStringConcatenationOptimizationPhase;
-import org.elasticsearch.painless.phase.DocFieldsPhase;
 import org.elasticsearch.painless.phase.IRTreeVisitor;
 import org.elasticsearch.painless.phase.PainlessSemanticAnalysisPhase;
 import org.elasticsearch.painless.phase.PainlessSemanticHeaderPhase;
@@ -214,8 +213,6 @@ ScriptScope compile(Loader loader, String name, String source, CompilerSettings
         ScriptScope scriptScope = new ScriptScope(painlessLookup, settings, scriptClassInfo, scriptName, source, root.getIdentifier() + 1);
         new PainlessSemanticHeaderPhase().visitClass(root, scriptScope);
         new PainlessSemanticAnalysisPhase().visitClass(root, scriptScope);
-        // TODO: Make this phase optional #60156
-        new DocFieldsPhase().visitClass(root, scriptScope);
         new PainlessUserTreeToIRTreePhase().visitClass(root, scriptScope);
         ClassNode classNode = (ClassNode)scriptScope.getDecoration(root, IRNodeDecoration.class).getIRNode();
         new DefaultStringConcatenationOptimizationPhase().visitClass(classNode, null);
@@ -251,7 +248,6 @@ byte[] compile(String name, String source, CompilerSettings settings, Printer de
         ScriptScope scriptScope = new ScriptScope(painlessLookup, settings, scriptClassInfo, scriptName, source, root.getIdentifier() + 1);
         new PainlessSemanticHeaderPhase().visitClass(root, scriptScope);
         new PainlessSemanticAnalysisPhase().visitClass(root, scriptScope);
-        new DocFieldsPhase().visitClass(root, scriptScope);
         new PainlessUserTreeToIRTreePhase().visitClass(root, scriptScope);
         ClassNode classNode = (ClassNode)scriptScope.getDecoration(root, IRNodeDecoration.class).getIRNode();
         new DefaultStringConcatenationOptimizationPhase().visitClass(classNode, null);
@@ -281,7 +277,6 @@ byte[] compile(String name, String source, CompilerSettings settings, Printer de
             semanticPhaseVisitor.visitClass(root, scriptScope);
         }
 
-        new DocFieldsPhase().visitClass(root, scriptScope);
         new PainlessUserTreeToIRTreePhase().visitClass(root, scriptScope);
         if (irPhaseVisitor != null) {
             irPhaseVisitor.visitClass(root, scriptScope);

diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java
@@ -296,8 +296,6 @@ private <T> T generateFactory(
                 reflect = method;
             } else if ("newFactory".equals(method.getName())) {
                 reflect = method;
-            } else if ("docFields".equals(method.getName())) {
-                docFieldsReflect = method;
             }
         }
 
@@ -330,32 +328,6 @@ private <T> T generateFactory(
         deterAdapter.returnValue();
         deterAdapter.endMethod();
 
-        if (docFieldsReflect != null) {
-            if (false == docFieldsReflect.getReturnType().equals(List.class)) {
-                throw new IllegalArgumentException("doc_fields must return a List");
-            }
-            if (docFieldsReflect.getParameterCount() != 0) {
-                throw new IllegalArgumentException("doc_fields may not take parameters");
-            }
-            org.objectweb.asm.commons.Method docFields = new org.objectweb.asm.commons.Method(docFieldsReflect.getName(),
-                MethodType.methodType(List.class).toMethodDescriptorString());
-            GeneratorAdapter docAdapter = new GeneratorAdapter(Opcodes.ASM5, docFields,
-                writer.visitMethod(Opcodes.ACC_PUBLIC, docFieldsReflect.getName(), docFields.getDescriptor(), null, null));
-            docAdapter.visitCode();
-            docAdapter.newInstance(WriterConstants.ARRAY_LIST_TYPE);
-            docAdapter.dup();
-            docAdapter.push(scriptScope.docFields().size());
-            docAdapter.invokeConstructor(WriterConstants.ARRAY_LIST_TYPE, WriterConstants.ARRAY_LIST_CTOR_WITH_SIZE);
-            for (int i = 0; i < scriptScope.docFields().size(); i++) {
-                docAdapter.dup();
-                docAdapter.push(scriptScope.docFields().get(i));
-                docAdapter.invokeInterface(WriterConstants.LIST_TYPE, WriterConstants.LIST_ADD);
-                docAdapter.pop(); // Don't want the result of calling add
-            }
-            docAdapter.returnValue();
-            docAdapter.endMethod();
-        }
-
         writer.visitEnd();
         Class<?> factory = loader.defineFactory(className.replace('/', '.'), writer.toByteArray());
 

diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/phase/DocFieldsPhase.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/phase/DocFieldsPhase.java
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/symbol/ScriptScope.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/symbol/ScriptScope.java
@@ -13,10 +13,8 @@
 import org.elasticsearch.painless.lookup.PainlessLookup;
 import org.elasticsearch.painless.node.ANode;
 
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
@@ -36,7 +34,6 @@ public class ScriptScope extends Decorator {
     protected int syntheticCounter = 0;
 
     protected boolean deterministic = true;
-    protected List<String> docFields = new ArrayList<>();
     protected Set<String> usedVariables = Collections.emptySet();
     protected Map<String, Object> staticConstants = new HashMap<>();
 
@@ -97,17 +94,6 @@ public boolean isDeterministic() {
         return deterministic;
     }
 
-    /**
-     * Document fields read or written using constant strings
-     */
-    public List<String> docFields() {
-        return Collections.unmodifiableList(docFields);
-    }
-
-    public void addDocField(String field) {
-        docFields.add(field);
-    }
-
     public void setUsedVariables(Set<String> usedVariables) {
         this.usedVariables = usedVariables;
     }

diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/DocFieldsPhaseTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/DocFieldsPhaseTests.java
diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/FactoryTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/FactoryTests.java
@@ -14,14 +14,10 @@
 import org.elasticsearch.script.ScriptFactory;
 import org.elasticsearch.script.TemplateScript;
 
-import java.util.Arrays;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-import static org.hamcrest.Matchers.equalTo;
-
 public class FactoryTests extends ScriptTestCase {
 
     @Override
@@ -35,7 +31,6 @@ protected Map<ScriptContext<?>, List<Whitelist>> scriptContexts() {
         contexts.put(VoidReturnTestScript.CONTEXT, Whitelist.BASE_WHITELISTS);
         contexts.put(FactoryTestConverterScript.CONTEXT, Whitelist.BASE_WHITELISTS);
         contexts.put(FactoryTestConverterScriptBadDef.CONTEXT, Whitelist.BASE_WHITELISTS);
-        contexts.put(DocFieldsTestScript.CONTEXT, Whitelist.BASE_WHITELISTS);
 
         return contexts;
     }
@@ -179,8 +174,6 @@ public void testFactory() {
         FactoryTestScript script = factory.newInstance(Collections.singletonMap("test", 2));
         assertEquals(4, script.execute(2));
         assertEquals(5, script.execute(3));
-        // The factory interface doesn't define `docFields` so we don't generate it.
-        expectThrows(NoSuchMethodException.class, () -> factory.getClass().getMethod("docFields"));
         script = factory.newInstance(Collections.singletonMap("test", 3));
         assertEquals(5, script.execute(2));
         assertEquals(2, script.execute(-1));
@@ -487,35 +480,4 @@ public void testConverterFactoryBadDef() {
         assertNotNull(ise);
         assertEquals("convertFromDef must take a single Object as an argument, not [int]", ise.getMessage());
     }
-
-    public abstract static class DocFieldsTestScript {
-        public static final ScriptContext<DocFieldsTestScript.Factory> CONTEXT = new ScriptContext<>(
-            "test",
-            DocFieldsTestScript.Factory.class
-        );
-
-        public interface Factory {
-            DocFieldsTestScript newInstance();
-
-            List<String> docFields();
-        }
-
-        public static final String[] PARAMETERS = new String[] {};
-
-        public abstract String execute();
-
-        public final Map<String, String> getDoc() {
-            Map<String, String> doc = new HashMap<>();
-            doc.put("cat", "meow");
-            doc.put("dog", "woof");
-            return doc;
-        }
-    }
-
-    public void testDocFields() {
-        DocFieldsTestScript.Factory f =
-                scriptEngine.compile("test", "doc['cat'] + doc['dog']", DocFieldsTestScript.CONTEXT, Collections.emptyMap());
-        assertThat(f.docFields(), equalTo(Arrays.asList("cat", "dog")));
-        assertThat(f.newInstance().execute(), equalTo("meowwoof"));
-    }
 }