Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move AStatement mutable members into isolated Input/Output objects #53348

Merged
merged 27 commits into from
Mar 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
0b79ef6
remove isNull from AExpression
jdconrad Jan 12, 2020
7325642
remove explicit cast optimization
jdconrad Jan 13, 2020
6e41497
remove modification of semantic tree for casting
jdconrad Jan 13, 2020
247080b
remove ECast node
jdconrad Jan 13, 2020
63e904f
Merge branch 'master' into trees11
jdconrad Feb 26, 2020
0e12611
start of input/output in expressions
jdconrad Jan 13, 2020
f7ae0e7
partial change for input and output in expression nodes
jdconrad Jan 14, 2020
3a2b443
add input/output objects for expressions
jdconrad Jan 16, 2020
8dfb933
fix shift bug in EBinary
jdconrad Jan 16, 2020
6a811de
add input/output to statements
jdconrad Jan 17, 2020
07180d9
Merge branch 'master' into trees11
jdconrad Mar 2, 2020
02f7bb9
Merge branch 'trees11' into trees12
jdconrad Mar 2, 2020
ab9c2c7
Merge branch 'trees12' into trees13
jdconrad Mar 2, 2020
115c8e0
response to pr comment
jdconrad Mar 2, 2020
9a96289
Merge branch 'master' into trees11
jdconrad Mar 3, 2020
d23d394
Merge branch 'trees11' into trees12
jdconrad Mar 3, 2020
3d6018c
Merge branch 'trees12' into trees13
jdconrad Mar 3, 2020
39a8806
Merge branch 'master' into trees12
jdconrad Mar 3, 2020
5536429
Merge branch 'trees12' into trees13
jdconrad Mar 3, 2020
ceb8967
t Merge branch 'master' into trees12
jdconrad Mar 9, 2020
f65d4a7
Merge branch 'trees12' into trees13
jdconrad Mar 9, 2020
d906147
Merge branch 'master' into trees12
jdconrad Mar 9, 2020
08cf476
Merge branch 'trees12' into trees13
jdconrad Mar 9, 2020
5ef8186
Merge branch 'master' into trees13
rjernst Mar 9, 2020
a92f642
Merge branch 'master' into trees13
rjernst Mar 10, 2020
b79c54c
response to pr comments
rjernst Mar 13, 2020
e7cbaaa
Merge branch 'master' into trees13
rjernst Mar 13, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ public static class Output {
AExpression prefix;

// TODO: remove placeholders once analysis and write are combined into build
// TODO: https://github.com/elastic/elasticsearch/issues/53561
// This are used to support the transition from a mutable to immutable state.
// Currently, the IR tree is built during the user tree "write" phase, so
// these are stored on the node to set during the "semantic" phase and then
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,66 +30,77 @@
*/
public abstract class AStatement extends ANode {

/**
* Set to true when the final statement in an {@link SClass} is reached.
* Used to determine whether or not an auto-return is necessary.
*/
boolean lastSource = false;

/**
* Set to true when a loop begins. Used by {@link SBlock} to help determine
* when the final statement of a loop is reached.
*/
boolean beginLoop = false;

/**
* Set to true when inside a loop. Used by {@link SBreak} and {@link SContinue}
* to determine if a break/continue statement is legal.
*/
boolean inLoop = false;

/**
* Set to true when on the last statement of a loop. Used by {@link SContinue}
* to prevent extraneous continue statements.
*/
boolean lastLoop = false;

/**
* Set to true if a statement would cause the method to exit. Used to
* determine whether or not an auto-return is necessary.
*/
boolean methodEscape = false;

/**
* Set to true if a statement would cause a loop to exit. Used to
* prevent unreachable statements.
*/
boolean loopEscape = false;

/**
* Set to true if all current paths escape from the current {@link SBlock}.
* Used during the analysis phase to prevent unreachable statements and
* the writing phase to prevent extraneous bytecode gotos from being written.
*/
boolean allEscape = false;

/**
* Set to true if any continue statement occurs in a loop. Used to prevent
* unnecessary infinite loops.
*/
boolean anyContinue = false;
public static class Input {

/**
* Set to true when the final statement in an {@link SClass} is reached.
* Used to determine whether or not an auto-return is necessary.
*/
boolean lastSource = false;

/**
* Set to true when a loop begins. Used by {@link SBlock} to help determine
* when the final statement of a loop is reached.
*/
boolean beginLoop = false;

/**
* Set to true when inside a loop. Used by {@link SBreak} and {@link SContinue}
* to determine if a break/continue statement is legal.
*/
boolean inLoop = false;

/**
* Set to true when on the last statement of a loop. Used by {@link SContinue}
* to prevent extraneous continue statements.
*/
boolean lastLoop = false;
}

/**
* Set to true if any break statement occurs in a loop. Used to prevent
* extraneous loops.
*/
boolean anyBreak = false;
public static class Output {

/**
* Set to true if a statement would cause the method to exit. Used to
* determine whether or not an auto-return is necessary.
*/
boolean methodEscape = false;

/**
* Set to true if a statement would cause a loop to exit. Used to
* prevent unreachable statements.
*/
boolean loopEscape = false;

/**
* Set to true if all current paths escape from the current {@link SBlock}.
* Used during the analysis phase to prevent unreachable statements and
* the writing phase to prevent extraneous bytecode gotos from being written.
*/
boolean allEscape = false;

/**
* Set to true if any continue statement occurs in a loop. Used to prevent
* unnecessary infinite loops.
*/
boolean anyContinue = false;

/**
* Set to true if any break statement occurs in a loop. Used to prevent
* extraneous loops.
*/
boolean anyBreak = false;

/**
* Set to the approximate number of statements in a loop block to prevent
* infinite loops during runtime.
*/
int statementCount = 0;
}

/**
* Set to the approximate number of statements in a loop block to prevent
* infinite loops during runtime.
*/
int statementCount = 0;
// TODO: remove placeholders once analysis and write are combined into build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you ref issues in TODO's like this? eg TODO: ..., see #12345.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// TODO: https://github.com/elastic/elasticsearch/issues/53561
Input input;
Output output;

/**
* Standard constructor with location used for error tracking.
Expand All @@ -101,7 +112,9 @@ public abstract class AStatement extends ANode {
/**
* Checks for errors and collects data for the writing phase.
*/
abstract void analyze(ScriptRoot scriptRoot, Scope scope);
Output analyze(ScriptRoot scriptRoot, Scope scope, Input input) {
throw new UnsupportedOperationException();
}

/**
* Writes ASM based on the data collected during the analysis phase.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,17 +165,18 @@ Output analyze(ScriptRoot scriptRoot, Scope scope, Input input) {
if (block.statements.isEmpty()) {
throw createError(new IllegalArgumentException("cannot generate empty lambda"));
}
block.lastSource = true;
block.analyze(scriptRoot, lambdaScope);
captures = new ArrayList<>(lambdaScope.getCaptures());
AStatement.Input blockInput = new AStatement.Input();
blockInput.lastSource = true;
AStatement.Output blockOutput = block.analyze(scriptRoot, lambdaScope, blockInput);

if (block.methodEscape == false) {
if (blockOutput.methodEscape == false) {
throw createError(new IllegalArgumentException("not all paths return a value for lambda"));
}

maxLoopCounter = scriptRoot.getCompilerSettings().getMaxLoopCounter();

// prepend capture list to lambda's arguments
captures = new ArrayList<>(lambdaScope.getCaptures());
this.typeParameters = new ArrayList<>(captures.size() + typeParameters.size());
parameterNames = new ArrayList<>(captures.size() + paramNameStrs.size());
for (Variable var : captures) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ public SBlock(Location location, List<AStatement> statements) {
}

@Override
void analyze(ScriptRoot scriptRoot, Scope scope) {
Output analyze(ScriptRoot scriptRoot, Scope scope, Input input) {
this.input = input;
output = new Output();

if (statements == null || statements.isEmpty()) {
throw createError(new IllegalArgumentException("A block must contain at least one statement."));
}
Expand All @@ -54,22 +57,25 @@ void analyze(ScriptRoot scriptRoot, Scope scope) {
for (AStatement statement : statements) {
// Note that we do not need to check after the last statement because
// there is no statement that can be unreachable after the last.
if (allEscape) {
if (output.allEscape) {
throw createError(new IllegalArgumentException("Unreachable statement."));
}

statement.inLoop = inLoop;
statement.lastSource = lastSource && statement == last;
statement.lastLoop = (beginLoop || lastLoop) && statement == last;
statement.analyze(scriptRoot, scope);

methodEscape = statement.methodEscape;
loopEscape = statement.loopEscape;
allEscape = statement.allEscape;
anyContinue |= statement.anyContinue;
anyBreak |= statement.anyBreak;
statementCount += statement.statementCount;
Input statementInput = new Input();
statementInput.inLoop = input.inLoop;
statementInput.lastSource = input.lastSource && statement == last;
statementInput.lastLoop = (input.beginLoop || input.lastLoop) && statement == last;
Output statementOutput = statement.analyze(scriptRoot, scope, statementInput);

output.methodEscape = statementOutput.methodEscape;
output.loopEscape = statementOutput.loopEscape;
output.allEscape = statementOutput.allEscape;
output.anyContinue |= statementOutput.anyContinue;
output.anyBreak |= statementOutput.anyBreak;
output.statementCount += statementOutput.statementCount;
}

return output;
}

@Override
Expand All @@ -81,8 +87,8 @@ BlockNode write(ClassNode classNode) {
}

blockNode.setLocation(location);
blockNode.setAllEscape(allEscape);
blockNode.setStatementCount(statementCount);
blockNode.setAllEscape(output.allEscape);
blockNode.setStatementCount(output.statementCount);

return blockNode;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,20 @@ public SBreak(Location location) {
}

@Override
void analyze(ScriptRoot scriptRoot, Scope scope) {
if (!inLoop) {
Output analyze(ScriptRoot scriptRoot, Scope scope, Input input) {
this.input = input;
output = new Output();

if (input.inLoop == false) {
throw createError(new IllegalArgumentException("Break statement outside of a loop."));
}

loopEscape = true;
allEscape = true;
anyBreak = true;
statementCount = 1;
output.loopEscape = true;
output.allEscape = true;
output.anyBreak = true;
output.statementCount = 1;

return output;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,11 @@ public SCatch(Location location, DType baseException, SDeclaration declaration,
}

@Override
void analyze(ScriptRoot scriptRoot, Scope scope) {
declaration.analyze(scriptRoot, scope);
Output analyze(ScriptRoot scriptRoot, Scope scope, Input input) {
this.input = input;
output = new Output();

declaration.analyze(scriptRoot, scope, new Input());

Class<?> baseType = baseException.resolveType(scriptRoot.getPainlessLookup()).getType();
Class<?> type = scope.getVariable(location, declaration.name).getType();
Expand All @@ -59,18 +62,21 @@ void analyze(ScriptRoot scriptRoot, Scope scope) {
}

if (block != null) {
block.lastSource = lastSource;
block.inLoop = inLoop;
block.lastLoop = lastLoop;
block.analyze(scriptRoot, scope);

methodEscape = block.methodEscape;
loopEscape = block.loopEscape;
allEscape = block.allEscape;
anyContinue = block.anyContinue;
anyBreak = block.anyBreak;
statementCount = block.statementCount;
Input blockInput = new Input();
blockInput.lastSource = input.lastSource;
blockInput.inLoop = input.inLoop;
blockInput.lastLoop = input.lastLoop;
Output blockOutput = block.analyze(scriptRoot, scope, blockInput);

output.methodEscape = blockOutput.methodEscape;
output.loopEscape = blockOutput.loopEscape;
output.allEscape = blockOutput.allEscape;
output.anyContinue = blockOutput.anyContinue;
output.anyBreak = blockOutput.anyBreak;
output.statementCount = blockOutput.statementCount;
}

return output;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,23 @@ public SContinue(Location location) {
}

@Override
void analyze(ScriptRoot scriptRoot, Scope scope) {
if (!inLoop) {
Output analyze(ScriptRoot scriptRoot, Scope scope, Input input) {
this.input = input;
output = new Output();

if (input.inLoop == false) {
throw createError(new IllegalArgumentException("Continue statement outside of a loop."));
}

if (lastLoop) {
if (input.lastLoop) {
throw createError(new IllegalArgumentException("Extraneous continue statement."));
}

allEscape = true;
anyContinue = true;
statementCount = 1;
output.allEscape = true;
output.anyContinue = true;
output.statementCount = 1;

return output;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,17 @@ public SDeclBlock(Location location, List<SDeclaration> declarations) {
}

@Override
void analyze(ScriptRoot scriptRoot, Scope scope) {
Output analyze(ScriptRoot scriptRoot, Scope scope, Input input) {
this.input = input;
output = new Output();

for (SDeclaration declaration : declarations) {
declaration.analyze(scriptRoot, scope);
declaration.analyze(scriptRoot, scope, new Input());
}

statementCount = declarations.size();
output.statementCount = declarations.size();

return output;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,23 @@ public SDeclaration(Location location, DType type, String name, boolean requires
}

@Override
void analyze(ScriptRoot scriptRoot, Scope scope) {
Output analyze(ScriptRoot scriptRoot, Scope scope, Input input) {
this.input = input;
output = new Output();

DResolvedType resolvedType = type.resolveType(scriptRoot.getPainlessLookup());
type = resolvedType;

if (expression != null) {
Input expressionInput = new Input();
AExpression.Input expressionInput = new AExpression.Input();
expressionInput.expected = resolvedType.getType();
expression.analyze(scriptRoot, scope, expressionInput);
expression.cast();
}

scope.defineVariable(location, resolvedType.getType(), name, false);

return output;
}

@Override
Expand Down
Loading