Remove custom "execute" method from SClass user node in Painless #51776
Conversation
jdconrad
added
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra (:Core/Infra/Scripting) |
| @@ -125,7 +125,8 @@ public void writeStatementOffset(Location location) { | |||
| int offset = location.getOffset(); | |||
| // ensure we don't have duplicate stuff going in here. can catch bugs | |||
| // (e.g. nodes get assigned wrong offsets by antlr walker) | |||
| assert statements.get(offset) == false; | |||
| // TODO: introduce a way to ignore internal statements so this assert is not triggered | |||
There was a problem hiding this comment.
Consider cutting an issue for TODOs like this so they don't get lost, then ref the issue in the comment.
There was a problem hiding this comment.
Pls reference from TODO.
modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java
Show resolved
Hide resolved
modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java
Show resolved
Hide resolved
|
|
||
| /** | ||
| * Tests {@link Object#toString} implementations on all extensions of {@link ANode}. | ||
| */ | ||
| public class NodeToStringTests extends ESTestCase { | ||
|
|
||
| public void testEAssignment() { | ||
| /*public void testEAssignment() { |
There was a problem hiding this comment.
Can you do Test.skip("ignoring rapidly changing tests until quiescent, see issue #XXXX")
| private boolean methodEscape; | ||
|
|
||
| public SFunction(Location location, String rtnType, String name, | ||
| List<String> paramTypes, List<String> paramNames, | ||
| SBlock block, boolean synthetic) { | ||
| SBlock block, boolean isInternal, boolean isStatic, boolean synthetic, boolean doAutoReturn) { |
There was a problem hiding this comment.
doAutoReturn implies an action, perhaps autoReturnEnabled?
| private boolean isSynthetic; | ||
| private boolean doAutoReturn; |
There was a problem hiding this comment.
Even with the name change, a comment on the semantics of this var would be useful since it's a non-standard extension.
modules/lang-painless/src/main/java/org/elasticsearch/painless/ir/FunctionNode.java
Show resolved
Hide resolved
|
@stu-elastic Thank you for the review. I believe I've addressed all the PR comments at this point, so ready for the next round. |
| "for function [" + name + "] with [" + typeParameters.size() + "] parameters")); | ||
| } | ||
|
|
||
| // TODO: do not specialize for execute |
There was a problem hiding this comment.
no need to change, but I usually do something like
// TODO: do not specialize for execute, see #51841
There was a problem hiding this comment.
Got it. Will do for the next instance.
|
@stu-elastic Thanks again. |
One of the goals moving forward is to make the user tree nodes and ir tree nodes generic (as in not have custom code for specific methods, etc.). Eventually, the user tree should not need to process anything from the ScriptContextInfo class as any customization should be added as generic nodes by whatever is creating the user and ir trees, respectively. This PR moves all code required for the customized "execute" method from the SClass/ClassNode into the SFunction/FunctionNode as a first step towards the aforementioned goals. In upcoming PRs the customized "execute" method code will be completely removed from the SFunction/FunctionNode classes as well. The SClass/ClassNode now consist of only functions and fields instead of also having logic to process statements for a singular custom "execute" method.