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

[operators][integrate]feat!: Integrate OperationExpr into GeneralForStatement #355

Merged
merged 49 commits into from
Oct 8, 2020
Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
aeb972b
Integrate OperationExpr into GeneralForStatement
summer-ji-eng Sep 29, 2020
5ed201c
make exprs optional
summer-ji-eng Sep 29, 2020
91741f3
format tests
summer-ji-eng Sep 29, 2020
17c9959
Add comments
summer-ji-eng Sep 29, 2020
7c5269f
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Sep 29, 2020
1cd43c6
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 1, 2020
ade35c7
remove null case for expressions
summer-ji-eng Oct 1, 2020
18cd6d9
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 2, 2020
c5f32ee
fix comments
summer-ji-eng Oct 2, 2020
25c8eca
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 2, 2020
1e75624
Update second version design
summer-ji-eng Oct 4, 2020
7698a95
Merge branch 'integrate_GeneralForStatement' of github.com:googleapis…
summer-ji-eng Oct 4, 2020
b52cf21
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 4, 2020
30b4ff4
fix: add final keyword checking in post increment operation expr
summer-ji-eng Oct 4, 2020
5a5ec4e
fix: add final keyword check on assignment expr
summer-ji-eng Oct 4, 2020
741edca
Integrate OperationExpr into GeneralForStatement
summer-ji-eng Sep 29, 2020
b681720
make exprs optional
summer-ji-eng Sep 29, 2020
5aeae69
format tests
summer-ji-eng Sep 29, 2020
c04297d
Add comments
summer-ji-eng Sep 29, 2020
9375d9b
remove null case for expressions
summer-ji-eng Oct 1, 2020
dbfb6ed
fix comments
summer-ji-eng Oct 2, 2020
d899782
Update second version design
summer-ji-eng Oct 4, 2020
8d647cd
rebase fix in assignment and increment expr
summer-ji-eng Oct 4, 2020
e5c54ef
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 6, 2020
aabd0a6
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 6, 2020
976c9c1
Merge branch 'integrate_GeneralForStatement' of github.com:googleapis…
summer-ji-eng Oct 6, 2020
c5b02b4
fix: add final keyword checking in post increment operation expr
summer-ji-eng Oct 4, 2020
9ac46dd
fix: add final keyword check on assignment expr
summer-ji-eng Oct 4, 2020
097ca65
Integrate OperationExpr into GeneralForStatement
summer-ji-eng Sep 29, 2020
c958696
make exprs optional
summer-ji-eng Sep 29, 2020
747b3b4
format tests
summer-ji-eng Sep 29, 2020
2d7bbf4
Add comments
summer-ji-eng Sep 29, 2020
08f4b2d
remove null case for expressions
summer-ji-eng Oct 1, 2020
83176f9
fix comments
summer-ji-eng Oct 2, 2020
2a6fd72
Update second version design
summer-ji-eng Oct 4, 2020
d7931b0
rebase fix in assignment and increment expr
summer-ji-eng Oct 4, 2020
3581aa3
Merge branch 'integrate_GeneralForStatement' of github.com:googleapis…
summer-ji-eng Oct 6, 2020
ac9904f
remove .circleci/config.yml
summer-ji-eng Oct 6, 2020
1124549
checkout master with files
summer-ji-eng Oct 6, 2020
ea276a0
git batching
summer-ji-eng Oct 6, 2020
93a1fc2
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 7, 2020
33bdbca
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 7, 2020
bd8e2f8
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 7, 2020
4e20a14
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 7, 2020
bc40948
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 7, 2020
1e7c838
delete comment code
summer-ji-eng Oct 7, 2020
06dd281
Initialization is expr
summer-ji-eng Oct 8, 2020
7816b01
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 8, 2020
6da3962
Merge branch 'master' into integrate_GeneralForStatement
summer-ji-eng Oct 8, 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 @@ -22,19 +22,11 @@

@AutoValue
public abstract class GeneralForStatement implements Statement {
public abstract Expr initializationExpr();
public abstract AssignmentExpr initializationExpr();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please confirm if this must be an AssignmentExpr (e.g. what about passing foobar() here)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it should not be limited to assignmentExpr, it should be expr which belongs to statmentExprList(https://docs.oracle.com/javase/specs/jls/se10/html/jls-14.html#jls).
My original though is to simplify the way to get variable. After consideration of back compatible, it should change back to expr. I will make change on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miraleung changed the interface use Expr for initialization. Before I merge the code, could you take one more look. Thanks.


// TODO(summerji): Integrate OperationExpr here. Start by uncommenting the following section to
// replace the localVariableExpr and maxSizeExpr getters after it.
/*
// Uses the same terminology as https://docs.oracle.com/javase/tutorial/java/nutsandbolts/for.html.
public abstract Expr terminationExpr();
public abstract Expr incrementExpr();
*/

public abstract VariableExpr localVariableExpr();

public abstract Expr maxSizeExpr();
public abstract Expr updateExpr();

public abstract ImmutableList<Statement> body();

Expand All @@ -43,60 +35,66 @@ public void accept(AstNodeVisitor visitor) {
visitor.visit(this);
}

// Convenience wrapper.
// incrementWith is convenience wrapper to generate index-base for-loop with lower and upper bound
// and post increment on variable, like code in ```for (int i = 0; i < getMax(); i++) {..}```
// TODO (unsupported): Add more convenience wrapper for the future generation needs.
public static GeneralForStatement incrementWith(
VariableExpr variableExpr, Expr maxSizeExpr, List<Statement> body) {
// TODO(summerji): Do some integration here, in JavaWriterVisitor, in ImportWriterVisitor, and
// add more tests.
VariableExpr localVariableExpr,
ValueExpr initialValueExpr,
Expr maxSizeExpr,
List<Statement> body) {
return builder()
.setLocalVariableExpr(variableExpr.toBuilder().setIsDecl(false).build())
.setMaxSizeExpr(maxSizeExpr)
.setInitializationExpr(
AssignmentExpr.builder()
.setVariableExpr(localVariableExpr)
.setValueExpr(initialValueExpr)
.build())
.setTerminationExpr(
RelationalOperationExpr.lessThanWithExprs(
localVariableExpr.toBuilder().setIsDecl(false).build(), maxSizeExpr))
.setUpdateExpr(
UnaryOperationExpr.postfixIncrementWithExpr(
localVariableExpr.toBuilder().setIsDecl(false).build()))
.setBody(body)
.build();
}

public static Builder builder() {
private static Builder builder() {
return new AutoValue_GeneralForStatement.Builder().setBody(Collections.emptyList());
}

@AutoValue.Builder
public abstract static class Builder {
public abstract Builder setInitializationExpr(Expr initializationExpr);

public abstract Builder setBody(List<Statement> body);

// Private.
abstract Builder setLocalVariableExpr(VariableExpr variableExpr);

abstract Builder setMaxSizeExpr(Expr maxSizeExpr);

abstract VariableExpr localVariableExpr();
abstract static class Builder {
// Private setter.
summer-ji-eng marked this conversation as resolved.
Show resolved Hide resolved
abstract Builder setInitializationExpr(AssignmentExpr initializationExpr);
// Private setter.
abstract Builder setTerminationExpr(Expr terminationExpr);
// Private setter.
abstract Builder setUpdateExpr(Expr incrementExpr);
// Private setter.
abstract Builder setBody(List<Statement> body);

abstract GeneralForStatement autoBuild();

// Type-checking will be done in the sub-expressions.
public GeneralForStatement build() {
VariableExpr varExpr = localVariableExpr();
Preconditions.checkState(
varExpr.scope().equals(ScopeNode.LOCAL),
String.format(
"Variable %s in a general for-loop cannot have a non-local scope",
varExpr.variable().identifier().name()));
Preconditions.checkState(
!varExpr.isStatic() && !varExpr.isFinal(),
String.format(
"Variable %s in a general for-loop cannot be static or final",
varExpr.variable().identifier().name()));
setInitializationExpr(
AssignmentExpr.builder()
.setVariableExpr(varExpr.toBuilder().setIsDecl(true).build())
.setValueExpr(
ValueExpr.withValue(
PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build()))
.build());
// TODO(summerji): Remove the following two lines.
// This temporary workaround will be removed soon, so it doesn't need a test.
setLocalVariableExpr(varExpr.toBuilder().setIsDecl(false).build());
GeneralForStatement build() {
GeneralForStatement generalForStatement = autoBuild();
VariableExpr localVarExpr = generalForStatement.initializationExpr().variableExpr();
// Declare a variable inside for-loop initialization expression.
if (localVarExpr.isDecl()) {
Preconditions.checkState(
localVarExpr.scope().equals(ScopeNode.LOCAL),
String.format(
"Variable %s declare in a general for-loop cannot have a non-local scope",
localVarExpr.variable().identifier().name()));
Preconditions.checkState(!localVarExpr.isStatic(), "Modifier 'static' not allow here.");
}
// TODO (unsupport): Add type-checking for initialization, termination, update exprs if public
// setters for users for future needs.
// Initialization and update expr should belong to StatementExpressionList.
// Termination expr must have type boolean or Boolean. And these three exprs are optional.
// More details at
// https://docs.oracle.com/javase/specs/jls/se10/html/jls-14.html#jls-StatementExpressionList
return autoBuild();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,8 @@ public void visit(ForStatement forStatement) {
@Override
public void visit(GeneralForStatement generalForStatement) {
generalForStatement.initializationExpr().accept(this);
generalForStatement.localVariableExpr().accept(this);
generalForStatement.maxSizeExpr().accept(this);
generalForStatement.terminationExpr().accept(this);
generalForStatement.updateExpr().accept(this);
statements(generalForStatement.body());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -540,17 +540,11 @@ public void visit(GeneralForStatement generalForStatement) {
semicolon();
space();

generalForStatement.localVariableExpr().accept(this);
space();
buffer.append(LEFT_ANGLE);
space();
generalForStatement.maxSizeExpr().accept(this);
generalForStatement.terminationExpr().accept(this);
semicolon();
space();

generalForStatement.localVariableExpr().accept(this);
// TODO(summerji): Remove the following temporary workaround.
buffer.append("++");
generalForStatement.updateExpr().accept(this);
rightParen();
space();
leftBrace();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -354,10 +354,16 @@ private static MethodDefinition createSplitResponseMethod(
// TODO(miraleung): Increment batchMessageIndexVarExpr.

VariableExpr forIndexVarExpr =
VariableExpr.withVariable(Variable.builder().setType(TypeNode.INT).setName("i").build());
VariableExpr.builder()
.setIsDecl(true)
.setVariable(Variable.builder().setType(TypeNode.INT).setName("i").build())
.build();
ValueExpr initValueExpr =
ValueExpr.withValue(PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build());
GeneralForStatement innerSubresponseForStatement =
GeneralForStatement.incrementWith(
forIndexVarExpr,
initValueExpr,
subresponseCountVarExpr,
innerSubresponseForExprs.stream()
.map(e -> ExprStatement.withExpr(e))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,57 +21,178 @@
import org.junit.Test;

public class GeneralForStatementTest {

/** ============================== incrementWith ====================================== */
@Test
// validGeneralForStatement_basicIsDecl tests declare a variable inside in initialization expr.
public void validGeneralForStatement_basicIsDecl() {
Variable variable = Variable.builder().setName("i").setType(TypeNode.INT).build();
VariableExpr variableExpr =
VariableExpr.builder().setVariable(variable).setIsDecl(true).build();
ValueExpr initValue =
ValueExpr.withValue(PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build());

MethodInvocationExpr maxSizeExpr =
MethodInvocationExpr.builder().setMethodName("maxSize").build();
MethodInvocationExpr.builder().setMethodName("maxSize").setReturnType(TypeNode.INT).build();

GeneralForStatement.incrementWith(
variableExpr, maxSizeExpr, Arrays.asList(createBodyStatement()));
variableExpr, initValue, maxSizeExpr, Arrays.asList(createBodyStatement()));
}

@Test
// validGeneralForStatement_basicIsNotDecl tests assigning a method-level local variable in
// initialization expr.
public void validGeneralForStatement_basicIsNotDecl() {
Variable variable = Variable.builder().setName("i").setType(TypeNode.INT).build();
VariableExpr variableExpr =
VariableExpr.builder().setVariable(variable).setIsDecl(false).build();
ValueExpr initValue =
ValueExpr.withValue(PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build());

MethodInvocationExpr maxSizeExpr =
MethodInvocationExpr.builder().setMethodName("maxSize").build();
MethodInvocationExpr.builder().setMethodName("maxSize").setReturnType(TypeNode.INT).build();

GeneralForStatement.incrementWith(
variableExpr, maxSizeExpr, Arrays.asList(createBodyStatement()));
variableExpr, initValue, maxSizeExpr, Arrays.asList(createBodyStatement()));
}

@Test
// validGeneralForStatement_emptyBody tests set empty body in update expr.
public void validGeneralForStatement_emptyBody() {
Variable variable = Variable.builder().setName("i").setType(TypeNode.INT).build();
VariableExpr variableExpr =
VariableExpr.builder().setVariable(variable).setIsDecl(false).build();
ValueExpr initValue =
ValueExpr.withValue(PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build());

MethodInvocationExpr maxSizeExpr =
MethodInvocationExpr.builder().setMethodName("maxSize").setReturnType(TypeNode.INT).build();

GeneralForStatement.incrementWith(
variableExpr, initValue, maxSizeExpr, Collections.emptyList());
}

@Test
// validForStatement_privateNotIsDeclVariable tests assigning an instance variable with private
// scope.
public void validForStatement_privateNotIsDeclVariable() {
Variable variable = Variable.builder().setName("i").setType(TypeNode.INT).build();
VariableExpr variableExpr =
VariableExpr.builder()
.setIsDecl(false)
.setVariable(variable)
.setScope(ScopeNode.PRIVATE)
.build();
ValueExpr initValue =
ValueExpr.withValue(PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build());
MethodInvocationExpr maxSizeExpr =
MethodInvocationExpr.builder().setMethodName("maxSize").build();
MethodInvocationExpr.builder().setMethodName("maxSize").setReturnType(TypeNode.INT).build();
GeneralForStatement.incrementWith(
variableExpr, initValue, maxSizeExpr, Collections.emptyList());
}

GeneralForStatement.incrementWith(variableExpr, maxSizeExpr, Collections.emptyList());
@Test
// validForStatement_instanceStaticVariable tests assigning an instance variable with public scope
// and static modifier.
public void validForStatement_instanceStaticVariable() {
Variable variable = Variable.builder().setName("i").setType(TypeNode.INT).build();
VariableExpr variableExpr =
VariableExpr.builder()
.setIsDecl(false)
.setVariable(variable)
.setScope(ScopeNode.PUBLIC)
.setIsStatic(true)
.build();
ValueExpr initValue =
ValueExpr.withValue(PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build());
MethodInvocationExpr maxSizeExpr =
MethodInvocationExpr.builder().setMethodName("maxSize").setReturnType(TypeNode.INT).build();
GeneralForStatement.incrementWith(
variableExpr, initValue, maxSizeExpr, Collections.emptyList());
}

@Test
public void invalidForStatement() {
Variable variable = Variable.builder().setName("str").setType(TypeNode.STRING).build();
// invalidForStatement_privateIsDeclVariable tests declaring a non-local variable inside of
// for-loop.
public void invalidForStatement_privateIsDeclVariable() {
Variable variable = Variable.builder().setName("i").setType(TypeNode.INT).build();
VariableExpr variableExpr =
VariableExpr.builder()
.setIsDecl(true)
.setVariable(variable)
.setScope(ScopeNode.PRIVATE)
.build();
ValueExpr initValue =
ValueExpr.withValue(PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build());
MethodInvocationExpr maxSizeExpr =
MethodInvocationExpr.builder().setMethodName("maxSize").setReturnType(TypeNode.INT).build();

assertThrows(
IllegalStateException.class,
() ->
GeneralForStatement.incrementWith(
variableExpr, initValue, maxSizeExpr, Collections.emptyList()));
}

@Test
// invalidForStatement_staticIsDeclVariable tests declare a static local variable in
// initialization expr.
public void invalidForStatement_staticIsDeclVariable() {
Variable variable = Variable.builder().setName("i").setType(TypeNode.INT).build();
VariableExpr variableExpr =
VariableExpr.builder().setVariable(variable).setIsDecl(true).setIsStatic(true).build();
ValueExpr initValue =
ValueExpr.withValue(PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build());
MethodInvocationExpr maxSizeExpr =
MethodInvocationExpr.builder().setMethodName("maxSize").setReturnType(TypeNode.INT).build();

assertThrows(
IllegalStateException.class,
() ->
GeneralForStatement.incrementWith(
variableExpr, initValue, maxSizeExpr, Collections.emptyList()));
}

@Test
// invalidForStatement_finalIsNotDeclVariable tests invalid case of assigning the initial value to
// a variable which is declared as a final instance variable.
public void invalidForStatement_finalIsNotDeclVariable() {
Variable variable = Variable.builder().setName("i").setType(TypeNode.INT).build();
VariableExpr variableExpr =
VariableExpr.builder()
.setVariable(variable)
.setIsDecl(false)
.setScope(ScopeNode.PUBLIC)
.setIsFinal(true)
.build();
ValueExpr initValue =
ValueExpr.withValue(PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build());
MethodInvocationExpr maxSizeExpr =
MethodInvocationExpr.builder().setMethodName("maxSize").setReturnType(TypeNode.INT).build();

assertThrows(
IllegalStateException.class,
() ->
GeneralForStatement.incrementWith(
variableExpr, initValue, maxSizeExpr, Collections.emptyList()));
}

@Test
// invalidForStatement_localFinalVariable tests declare a final variable in initialization expr,
// updateExpr should throw error.
public void invalidForStatement_localFinalVariable() {
Variable variable = Variable.builder().setName("i").setType(TypeNode.INT).build();
VariableExpr variableExpr =
VariableExpr.builder().setVariable(variable).setScope(ScopeNode.PRIVATE).build();
VariableExpr.builder().setVariable(variable).setIsDecl(true).setIsFinal(true).build();
ValueExpr initValue =
ValueExpr.withValue(PrimitiveValue.builder().setValue("0").setType(TypeNode.INT).build());
MethodInvocationExpr maxSizeExpr =
MethodInvocationExpr.builder().setMethodName("maxSize").build();
MethodInvocationExpr.builder().setMethodName("maxSize").setReturnType(TypeNode.INT).build();

assertThrows(
IllegalStateException.class,
() ->
GeneralForStatement.incrementWith(variableExpr, maxSizeExpr, Collections.emptyList()));
GeneralForStatement.incrementWith(
variableExpr, initValue, maxSizeExpr, Collections.emptyList()));
}

private static Statement createBodyStatement() {
Expand Down
Loading