-
Notifications
You must be signed in to change notification settings - Fork 33
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
Comparison and Boolean Operations #38
Conversation
The binary operators follow the relative precedence rules of C, since many other languages have derived their rules from C and so they are expected to be the most intuitive to the broadest group of people. Just as unary -n has been parsed as 0-n, unary !n parses as false==n, continuing to avoid the need to introduce special processing for unary operations, though at the likely expense of some somewhat-confusing error messages during eval for certain cases. This doesn't yet parse the ? : ternary operator, even though we now have tokens for it. That will follow in a later change, since it requires some more complex parsing and evaluation changes.
These are pretty straightforward since they deal only in booleans both for their operands and their results. The type checking for Arithmetic nodes is now split up by class of operator, so that the numeric operations can have different type checking rules than the logical operators. A future change will introduce a third category: comparison operators.
Equality and inequality apply to all of the primitive types, while the <, >, <= and >= operators apply only to the numeric types. This also implicitly implements the logical NOT operator (!) because the parser represents it as an equality comparison with false. Just as with the other arithmetic operations, these are transformed into calls to builtin functions, with one function per type that supports comparison operations.
This mimics the (.. ? .. : ..) from C, which has since been mimicked in several other languages and is thus likely to be familiar to many users. It is a ternary operator, but its parsing -- like C, again -- treats it as a funny sort of binary operator where the operand is the middle expression and the wrapping punctuation. A new AST node type was introduced, rather than continuing to stretch the meaning of the Arithmetic node type, because the type checking rules and evaluation steps will be quite different than the arithmetic operators, and the "type transparency" of the two output expressions will make it it inconvenient to implement by mapping to a built-in function as we do for the arithmetic operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MARTIN!
I've been wanting to do this for months, using effectively this exact syntax. The implementation looks awesome, the tests look great, looks Terraform 0.8 is going to get a surprise.
func (n *Conditional) Accept(v Visitor) Node { | ||
n.CondExpr = n.CondExpr.Accept(v) | ||
n.TrueExpr = n.TrueExpr.Accept(v) | ||
n.FalseExpr = n.FalseExpr.Accept(v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this for the future (not this PR), but I think we can do something clever here somehow to make it so we don't have to fully evaluate both sides (lazy evaluate).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yes, that's true. I touched on that briefly and then moved on considering it to just be a marginal performance thing, but now that you mention it I suppose some specific things like Terraform's file(...)
function could benefit from being short-circuited both here and in the &&
and ||
operators as a future improvement, so people can do stuff like:
something = "${var.data_file != "" ? file(var.data_file) : "default_something"}"
// anything goes | ||
default: | ||
switch compareType { | ||
case ast.TypeFloat, ast.TypeInt: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think strings could make sense here, just copying behavior of Go spec's string comparison.
Great feature! |
This set of commits introduces new operators to support comparison and boolean operations. The new operators are as follows:
==
and!=
<
,>
,<=
and>=
&&
,||
and unary!
... ? ... : ...
This is a logical (:grimacing:) extension of work started in #26.
I went with "C-style" operator punctuation because I think this has been the syntax most prolifically emulated by other programming languages and so users are likely to be familiar with it from at least one of C's descendants. Additionally, HCL (in which HIL is most commonly embedded) is using "C-inspired" brace-delimited blocks. (perhaps it's more accurate to attribute this syntax to B, but hey...)
My primary motivation for working on this is to make a further step towards the use-cases discussed in hashicorp/terraform#1604.
A pattern has emerged for "faking" conditional resources in Terraform using variables mapping to counts, as follows:
Including this changeset in Terraform would immediately "pave the cowpath" here and introduce a more succinct way to express that idea:
In the discussion on hashicorp/terraform#1604 there is also some consensus around a more "intentional" mechanism for conditionally including resources, which Terraform could support via some additional features building on the boolean operations added in these commits:
Due to the same type system limitations that befell full implementation of #34, for this initial changeset the
... ? ... : ...
operator is constrained to not work on array or map types. In principle it could support direct usage of variables of these types in a similar way to how indexing is supported, but since this seems to be a rather marginal case I decided to omit this complexity right now until either the type system gets improved to represent collection element types or until we find a specific motivating use-case.