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][1/2]feat: Add RelationalOperationExpr to enable equality operators #289

Merged
merged 11 commits into from
Sep 17, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ public interface AstNodeVisitor {

public void visit(UnaryOperationExpr unaryOperationExpr);

public void visit(RelationalOperationExpr relationalOperationExpr);

public void visit(LogicalOperationExpr logicalOperationExpr);

/** =============================== COMMENT =============================== */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.google.api.generator.engine.ast;

import com.google.auto.value.AutoValue;
import com.google.common.base.Preconditions;

@AutoValue
public abstract class RelationalOperationExpr implements OperationExpr {
public abstract Expr lhsExpr();

public abstract Expr rhsExpr();

public abstract OperatorKind operatorKind();

@Override
public TypeNode type() {
return TypeNode.BOOLEAN;
}

@Override
public void accept(AstNodeVisitor visitor) {
visitor.visit(this);
}

// Convenience wrapper.
public static RelationalOperationExpr equalToWithExprs(Expr lhsExpr, Expr rhsExpr) {
miraleung marked this conversation as resolved.
Show resolved Hide resolved
return builder()
.setLhsExpr(lhsExpr)
.setRhsExpr(rhsExpr)
.setOperatorKind(OperatorKind.RELATIONAL_EQUAL_TO)
.build();
}

// Convenience wrapper.
public static RelationalOperationExpr notEqualToWithExprs(Expr lhsExpr, Expr rhsExpr) {
return builder()
.setLhsExpr(lhsExpr)
.setRhsExpr(rhsExpr)
.setOperatorKind(OperatorKind.RELATIONAL_NOT_EQUAL_TO)
.build();
}

// TODO(summerji): Add convenience wrapper lessThanWithExprs
// public static RelationalOperationExpr lessThanWithExprs(Expr lhsExpr, Expr rhsExpr) {
// return builder()
// .setLhsExpr(lhsExpr)
// .setRhsExpr(rhsExpr)
// .setOperatorKind(OperatorKind.RELATIONAL_LESS_THAN)
// .build();
// }

private static Builder builder() {
return new AutoValue_RelationalOperationExpr.Builder();
}

@AutoValue.Builder
abstract static class Builder {

// Private setter.
abstract Builder setLhsExpr(Expr expr);

// Private setter.
abstract Builder setRhsExpr(Expr expr);

// Private setter.
abstract Builder setOperatorKind(OperatorKind operator);

abstract RelationalOperationExpr autoBuild();

private RelationalOperationExpr build() {
RelationalOperationExpr relationalOperationExpr = autoBuild();
TypeNode lhsExprType = relationalOperationExpr.lhsExpr().type();
TypeNode rhsExprType = relationalOperationExpr.rhsExpr().type();
OperatorKind operator = relationalOperationExpr.operatorKind();
final String errorMsg =
String.format(
"Relational operator %s can not be applied to %s, %s.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Could we please rephrase this so the reader knows what types were expected (similar to the other classes)?

Copy link
Contributor Author

@summer-ji-eng summer-ji-eng Sep 15, 2020

Choose a reason for hiding this comment

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

Screen Shot 2020-09-15 at 1 18 54 PM

I am following the Java compiler error message style for operator type-checking. Unlike logical operator, the type-checking is comparatively simple than relational, we could ask for users given boolean type expression. In theory, we could rephrase the error message base on input type condition, like if lhs is numeric type, rhs can only be applied on any numeric type or any numeric boxed type. But it has a lot of condition need to support. Consider of ROI, I wonder if we should follow the Java compiler error message to be consistent with common Java convention.

However, if you prefer to specify the type-checking with detailed error message, we could open a separated ticket and refactor all other type-checking error messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

SG, thanks for the background.

operator, lhsExprType.toString(), rhsExprType.toString());

if (operator.equals(OperatorKind.RELATIONAL_EQUAL_TO)
|| operator.equals(OperatorKind.RELATIONAL_NOT_EQUAL_TO)) {
Preconditions.checkState(isValidEqualityType(lhsExprType, rhsExprType), errorMsg);
summer-ji-eng marked this conversation as resolved.
Show resolved Hide resolved
}

return relationalOperationExpr;
}

// isValidEqualityType checks expressions' type for equality operator (==) and non-equality
// operator (!=).
private boolean isValidEqualityType(TypeNode lhsType, TypeNode rhsType) {
// If the expression's types are matched, return true
if (lhsType.equals(rhsType)) return true;
summer-ji-eng marked this conversation as resolved.
Show resolved Hide resolved

// If the expressions' type are array, the types should be array and matched, or either is
summer-ji-eng marked this conversation as resolved.
Show resolved Hide resolved
// null type;
if (lhsType.isArray() || rhsType.isArray()) {
return lhsType.equals(TypeNode.NULL) || rhsType.equals(TypeNode.NULL);
}

// If lhs expression type is numeric type (char, byte, short, int, long, double), the rhs
// expression type should be any numeric type or any numeric boxed type
if (TypeNode.isNumericType(lhsType) && TypeNode.isNumericType(rhsType)) return true;
summer-ji-eng marked this conversation as resolved.
Show resolved Hide resolved

// If lhs expression type is new Object or null, the rhs type should be a reference type or
// null or boxed type;
if (lhsType.equals(TypeNode.OBJECT) || lhsType.equals(TypeNode.NULL)) {
return TypeNode.isReferenceType(rhsType)
|| rhsType.equals(TypeNode.OBJECT)
|| rhsType.equals(TypeNode.NULL)
|| TypeNode.isBoxedType(rhsType);
summer-ji-eng marked this conversation as resolved.
Show resolved Hide resolved
}

// If lhs expression type is Boxed type or a referenced type, rhs should be null or object,
// other cases have been covered in previous conditions.
if (TypeNode.isBoxedType(lhsType) || TypeNode.isReferenceType(lhsType)) {
return rhsType.equals(TypeNode.NULL) || rhsType.equals(TypeNode.OBJECT);
}

return false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,10 @@ public static boolean isNumericType(TypeNode type) {
|| type.equals(TypeNode.CHAR);
}

public static boolean isBoxedType(TypeNode type) {
return isReferenceType(type) && BOXED_TYPE_MAP.containsValue(type);
}

public boolean isPrimitiveType() {
return isPrimitiveType(typeKind());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.google.api.generator.engine.ast.NewObjectExpr;
import com.google.api.generator.engine.ast.Reference;
import com.google.api.generator.engine.ast.ReferenceConstructorExpr;
import com.google.api.generator.engine.ast.RelationalOperationExpr;
import com.google.api.generator.engine.ast.ReturnExpr;
import com.google.api.generator.engine.ast.ScopeNode;
import com.google.api.generator.engine.ast.Statement;
Expand Down Expand Up @@ -227,6 +228,12 @@ public void visit(UnaryOperationExpr unaryOperationExpr) {
unaryOperationExpr.expr().accept(this);
}

@Override
public void visit(RelationalOperationExpr relationalOperationExpr) {
relationalOperationExpr.lhsExpr().accept(this);
relationalOperationExpr.rhsExpr().accept(this);
}

@Override
public void visit(LogicalOperationExpr logicalOperationExpr) {
logicalOperationExpr.lhsExpr().accept(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.google.api.generator.engine.ast.NewObjectExpr;
import com.google.api.generator.engine.ast.OperatorKind;
import com.google.api.generator.engine.ast.ReferenceConstructorExpr;
import com.google.api.generator.engine.ast.RelationalOperationExpr;
import com.google.api.generator.engine.ast.ReturnExpr;
import com.google.api.generator.engine.ast.ScopeNode;
import com.google.api.generator.engine.ast.Statement;
Expand Down Expand Up @@ -410,6 +411,15 @@ public void visit(UnaryOperationExpr unaryOperationExpr) {
}
}

@Override
public void visit(RelationalOperationExpr relationalOperationExpr) {
relationalOperationExpr.lhsExpr().accept(this);
space();
operator(relationalOperationExpr.operatorKind());
space();
relationalOperationExpr.rhsExpr().accept(this);
}

@Override
public void visit(LogicalOperationExpr logicalOperationExpr) {
logicalOperationExpr.lhsExpr().accept(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ TESTS = [
"WhileStatementTest",
"ArithmeticOperationExprTest",
"UnaryOperationExprTest",
"RelationalOperationExprTest",
"LogicalOperationExprTest",
]

Expand Down
Loading