From b440b1729993fc7660b7a92961e405db5ccb3977 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 14 Jun 2022 09:55:26 -0700 Subject: [PATCH 1/3] go.mod: Specify that we use the Go 1.17 language in this module The main motivation here is that we were apparently using bit shifting with signed shift amounts even though modern Go toolchains seem to raise that as an error if the go.mod file selects a language version less than Go 1.13. Go 1.17 is therefore newer than we strictly need to solve that particular problem, but gets us caught up with a relatively release of the language. This alone doesn't break compatibility for anyone using older versions of Go with HCL, since the Go toolchain will still attempt to compile modules targeting later versions and will only mention the newer language version if compilation would already have failed for some other reason. --- go.mod | 13 ++++++++++--- go.sum | 2 -- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index b8d84e9c..cf503a04 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/hashicorp/hcl/v2 -go 1.12 +go 1.17 require ( github.com/agext/levenshtein v1.2.1 @@ -12,11 +12,18 @@ require ( github.com/kr/pretty v0.1.0 github.com/kylelemons/godebug v0.0.0-20170820004349-d65d576e9348 github.com/mitchellh/go-wordwrap v0.0.0-20150314170334-ad45545899c7 - github.com/pmezard/go-difflib v1.0.0 // indirect github.com/sergi/go-diff v1.0.0 github.com/spf13/pflag v1.0.2 - github.com/stretchr/testify v1.2.2 // indirect github.com/zclconf/go-cty v1.8.0 github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b golang.org/x/crypto v0.0.0-20220517005047-85d78b3ac167 ) + +require ( + github.com/kr/text v0.1.0 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/stretchr/testify v1.2.2 // indirect + golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1 // indirect + golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1 // indirect + golang.org/x/text v0.3.6 // indirect +) diff --git a/go.sum b/go.sum index 525c8df7..2d2ddb33 100644 --- a/go.sum +++ b/go.sum @@ -41,8 +41,6 @@ github.com/zclconf/go-cty v1.8.0/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUA github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b h1:FosyBZYxY34Wul7O/MSKey3txpPYyCqVO5ZyceuQJEI= github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b/go.mod h1:ZRKQfBXbGkpdV6QMzT3rU1kSTAnfu1dO8dPKjYprgj8= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= -golang.org/x/crypto v0.0.0-20220331220935-ae2d96664a29 h1:tkVvjkPTB7pnW3jnid7kNyAMPVWllTNOf/qKDze4p9o= -golang.org/x/crypto v0.0.0-20220331220935-ae2d96664a29/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/crypto v0.0.0-20220517005047-85d78b3ac167 h1:O8uGbHCqlTp2P6QJSLmCojM4mN6UemYv8K+dCnmHmu0= golang.org/x/crypto v0.0.0-20220517005047-85d78b3ac167/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/net v0.0.0-20180811021610-c39426892332/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= From 9ba67570a2d3654fd3cb6dc61292557b244d72ab Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 14 Jun 2022 10:59:05 -0700 Subject: [PATCH 2/3] hclsyntax: && and || operators are right-associative Previously we just treated all binary operators as being left-associative, which for many of them didn't make a lot of difference anyway because they were associative operators. The logical AND and OR operators are also effectively associative the way we have them implemented right now, because HCL always evaluates both sides of a binary operator anyway and so there's no way to depend on the associativity for either of these operations. However, we'd like to implement a similar short-circuit behavior as we see in many other similar languages, at which point the evaluation order of the operations would become important in order to constrain which expressions get fully evaluated and which are "short-circuited". Treating these operators as right-associative instead of left-associative will make the subsequent implementation more intuitive, since the LHS of the outermost operation will decide whether to evaluate the RHS, rather than recursing into the LHS completely first and then unwinding if the innermost term causes the short-circuit. Since we expect HCL expression evaluation to always be side-effect-free it doesn't technically matter what associativity we use -- the result would be the same either way -- but this approach will make the control flow during evaluation match intuition about how a short-circuit behaves. This commit does not yet actually implement the short-circuit behavior, which will hopefully follow in a subsequent commit. --- hclsyntax/expression_ops.go | 4 + hclsyntax/parser.go | 31 ++++-- hclsyntax/parser_test.go | 192 ++++++++++++++++++++++++++++++++++++ 3 files changed, 220 insertions(+), 7 deletions(-) diff --git a/hclsyntax/expression_ops.go b/hclsyntax/expression_ops.go index c1db0cec..a351096d 100644 --- a/hclsyntax/expression_ops.go +++ b/hclsyntax/expression_ops.go @@ -82,6 +82,10 @@ var ( ) var binaryOps []map[TokenType]*Operation +var rightAssociativeBinaryOps = map[TokenType]struct{}{ + TokenOr: {}, + TokenAnd: {}, +} func init() { // This operation table maps from the operator's token type diff --git a/hclsyntax/parser.go b/hclsyntax/parser.go index ec83d3dc..5be492ba 100644 --- a/hclsyntax/parser.go +++ b/hclsyntax/parser.go @@ -563,13 +563,17 @@ func (p *parser) parseBinaryOps(ops []map[TokenType]*Operation) (Expression, hcl return lhs, diags } - // We'll keep eating up operators until we run out, so that operators - // with the same precedence will combine in a left-associative manner: - // a+b+c => (a+b)+c, not a+(b+c) + // Most of our operators are left-associative: + // a+b+c => (a+b)+c, not a+(b+c) + // For those, we recursively hunt for operators only of lower precedence, + // so that any subsequent operators of the same precedence will be eaten + // up by this loop and gather on the left side. // - // Should we later want to have right-associative operators, a way - // to achieve that would be to call back up to ParseExpression here - // instead of iteratively parsing only the remaining operators. + // A few operators are instead right-associative: + // a && b && c => a && (b && c), not (a && b) && c. + // For those, we recursively hunt for operators of the same or lower + // precedence, so that the recursive call handling the right hand side will + // eat up all of the operators of the same precedence instead. for { next := p.Peek() var newOp *Operation @@ -577,6 +581,7 @@ func (p *parser) parseBinaryOps(ops []map[TokenType]*Operation) (Expression, hcl if newOp, ok = thisLevel[next.Type]; !ok { break } + _, rightAssoc := rightAssociativeBinaryOps[next.Type] // Are we extending an expression started on the previous iteration? if operation != nil { @@ -592,7 +597,19 @@ func (p *parser) parseBinaryOps(ops []map[TokenType]*Operation) (Expression, hcl operation = newOp p.Read() // eat operator token var rhsDiags hcl.Diagnostics - rhs, rhsDiags = p.parseBinaryOps(remaining) + if rightAssoc { + // For right-associative, we use the same ops we were + // given so that the right-hand side can eat up all + // of the operations of the same precedence. + rhs, rhsDiags = p.parseBinaryOps(ops) + } else { + // For left-associative, we use only operators of + // lower precedence so that this will terminate when + // encountering an operator of the same precedence as + // this loop is handling, allowing this loop to eat + // up those operations instead. + rhs, rhsDiags = p.parseBinaryOps(remaining) + } diags = append(diags, rhsDiags...) if p.recovery && rhsDiags.HasErrors() { return lhs, diags diff --git a/hclsyntax/parser_test.go b/hclsyntax/parser_test.go index e347d399..20fad1da 100644 --- a/hclsyntax/parser_test.go +++ b/hclsyntax/parser_test.go @@ -2195,6 +2195,198 @@ block "valid" {} }, }, }, + { + // This test is for the associativity of the && operator during + // parsing. Specifically, our evaluator relies on the following + // being parsed as if it were `b && (c && d)` rather than + // `(b && c) && d` in order to correctly implement the short-circuit + // behavior. + `a = b && c && d`, + 0, + &Body{ + Attributes: Attributes{ + "a": { + Name: "a", + Expr: &BinaryOpExpr{ + LHS: &ScopeTraversalExpr{ + Traversal: hcl.Traversal{ + hcl.TraverseRoot{ + Name: "b", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 5, Byte: 4}, + End: hcl.Pos{Line: 1, Column: 6, Byte: 5}, + }, + }, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 5, Byte: 4}, + End: hcl.Pos{Line: 1, Column: 6, Byte: 5}, + }, + }, + Op: OpLogicalAnd, + RHS: &BinaryOpExpr{ + LHS: &ScopeTraversalExpr{ + Traversal: hcl.Traversal{ + hcl.TraverseRoot{ + Name: "c", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 10, Byte: 9}, + End: hcl.Pos{Line: 1, Column: 11, Byte: 10}, + }, + }, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 10, Byte: 9}, + End: hcl.Pos{Line: 1, Column: 11, Byte: 10}, + }, + }, + Op: OpLogicalAnd, + RHS: &ScopeTraversalExpr{ + Traversal: hcl.Traversal{ + hcl.TraverseRoot{ + Name: "d", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 15, Byte: 14}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 15, Byte: 14}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 10, Byte: 9}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 5, Byte: 4}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + NameRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 1, Byte: 0}, + End: hcl.Pos{Line: 1, Column: 2, Byte: 1}, + }, + EqualsRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 3, Byte: 2}, + End: hcl.Pos{Line: 1, Column: 4, Byte: 3}, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 1, Byte: 0}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + }, + Blocks: Blocks{}, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 1, Byte: 0}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + EndRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + }, + { + // This test is for the associativity of the || operator during + // parsing. Specifically, our evaluator relies on the following + // being parsed as if it were `b || (c || d)` rather than + // `(b || c) || d` in order to correctly implement the short-circuit + // behavior. + `a = b || c || d`, + 0, + &Body{ + Attributes: Attributes{ + "a": { + Name: "a", + Expr: &BinaryOpExpr{ + LHS: &ScopeTraversalExpr{ + Traversal: hcl.Traversal{ + hcl.TraverseRoot{ + Name: "b", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 5, Byte: 4}, + End: hcl.Pos{Line: 1, Column: 6, Byte: 5}, + }, + }, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 5, Byte: 4}, + End: hcl.Pos{Line: 1, Column: 6, Byte: 5}, + }, + }, + Op: OpLogicalOr, + RHS: &BinaryOpExpr{ + LHS: &ScopeTraversalExpr{ + Traversal: hcl.Traversal{ + hcl.TraverseRoot{ + Name: "c", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 10, Byte: 9}, + End: hcl.Pos{Line: 1, Column: 11, Byte: 10}, + }, + }, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 10, Byte: 9}, + End: hcl.Pos{Line: 1, Column: 11, Byte: 10}, + }, + }, + Op: OpLogicalOr, + RHS: &ScopeTraversalExpr{ + Traversal: hcl.Traversal{ + hcl.TraverseRoot{ + Name: "d", + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 15, Byte: 14}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 15, Byte: 14}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 10, Byte: 9}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 5, Byte: 4}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + NameRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 1, Byte: 0}, + End: hcl.Pos{Line: 1, Column: 2, Byte: 1}, + }, + EqualsRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 3, Byte: 2}, + End: hcl.Pos{Line: 1, Column: 4, Byte: 3}, + }, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 1, Byte: 0}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + }, + Blocks: Blocks{}, + SrcRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 1, Byte: 0}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + EndRange: hcl.Range{ + Start: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + End: hcl.Pos{Line: 1, Column: 16, Byte: 15}, + }, + }, + }, { ` `, From 4790ab6f27e364cc070b1d003a9d9cc3aa1aa95d Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 14 Jun 2022 12:16:56 -0700 Subject: [PATCH 3/3] hclsyntax: Short-circuit behavior for the && and || operators Previously we just always evaluated both operands of any binary operator and then executed the operator. For the logical operators that was inconsistent with their treatment in several other languages with a similar grammar to ours, leading to users being surprised that HCL didn't short circuit the same way as their favorite other languages did. Our initial exclusion of short-circuiting here was, as with various other parts of HCL's design, motivated by the goals of producing consistent results even when unknown values are present and of proactively returning as many errors as possible in order to give better context when debugging. However, in acknowledgement of the fact that logical operator short-circuiting has considerable ergonomic benefits when writing out compound conditional expressions where later terms rely on the guarantees of earlier terms, this commit implements a compromise design where we can get the short-circuit benefits for dynamic-value-related errors without losing our ability to proactively detect type-related errors even when short-circuiting. Specifically, if a particular operator participates in short-circuiting then it gets an opportunity to decide for a particular known LHS value whether to short-circuit. If it decides to do so then HCL will evaluate the RHS in a type-checking-only mode, where we ignore any specific values in the variable scope but will still raise errors if the RHS expression tries to do anything to them that is inconsistent with their type. If the LHS of a short-circuit-capable operator turns out to be an unknown value of a suitable type, we'll pessimistically treat it as a short-circuit and defer full evaluation of the RHS until we have more information, so as to avoid raising errors that would be guarded away once the LHS becomes known. The effect of this compromise is that it's possible to use the short-circuit operators in common value-related guard situations, like checking whether a value is null or is a number that would be valid to index a particular list: foo != null && foo.something idx < 3 && foo[idx] On the other hand though, it is _not_ possible to use the behavior to guard against problems that are related to types rather than to values, which allows us to still proactively detect various kinds of errors that are likely to be mistakes: foo != null && fo.something # Typoed "foo" as "fo" foo != null && foo.smething # Typoed "something" as "smething" num < 3 && num.something # Numbers never have attributes Those coming from dynamic languages will probably still find these errors surprising, but HCL's other features follow a similar sort of hybrid model of statically checking types where possible and that tradeoff seems to have worked well to make it possible to express more complicated situations while still providing some of the help typically expected from a static type system for "obviously wrong" situations. It should typically be possible to adjust an expression to make it friendly to short-circuit guard style by being more precise about the types being used. --- eval_context.go | 34 +++++++++++ hclsyntax/expression_ops.go | 62 ++++++++++++++++++- hclsyntax/expression_test.go | 111 +++++++++++++++++++++++++++++++++++ 3 files changed, 204 insertions(+), 3 deletions(-) diff --git a/eval_context.go b/eval_context.go index 915910ad..87f9bb58 100644 --- a/eval_context.go +++ b/eval_context.go @@ -23,3 +23,37 @@ func (ctx *EvalContext) NewChild() *EvalContext { func (ctx *EvalContext) Parent() *EvalContext { return ctx.parent } + +// NewChildAllVariablesUnknown is an extension of NewChild which, in addition +// to creating a child context, also pre-populates its Variables table +// with variable definitions masking every variable define in the reciever +// and its ancestors with an unknown value of the same type as the original. +// +// The child does not initially have any of its own functions defined, and so +// it can still inherit any defined functions from the reciever. +// +// Because is function effectively takes a snapshot of the variables as they +// are defined at the time of the call, it is incorrect to subsequently +// modify the variables in any of the ancestor contexts in a way that would +// change which variables are defined or what value types they each have. +// +// This is a specialized helper function intended to support type-checking +// use-cases, where the goal is only to check whether values are being used +// in a way that makes sense for their types while not reacting to their +// actual values. +func (ctx *EvalContext) NewChildAllVariablesUnknown() *EvalContext { + ret := ctx.NewChild() + ret.Variables = make(map[string]cty.Value) + + currentAncestor := ctx + for currentAncestor != nil { + for name, val := range currentAncestor.Variables { + if _, ok := ret.Variables[name]; !ok { + ret.Variables[name] = cty.UnknownVal(val.Type()) + } + } + currentAncestor = currentAncestor.parent + } + + return ret +} diff --git a/hclsyntax/expression_ops.go b/hclsyntax/expression_ops.go index a351096d..f6674949 100644 --- a/hclsyntax/expression_ops.go +++ b/hclsyntax/expression_ops.go @@ -13,16 +13,39 @@ import ( type Operation struct { Impl function.Function Type cty.Type + + // ShortCircuit is an optional callback for binary operations which, if + // set, will be called with the result of evaluating the LHS expression. + // + // ShortCircuit may return cty.NilVal to allow evaluation to proceed + // as normal, or it may return a non-nil value to force the operation + // to return that value and perform only type checking on the RHS + // expression, as opposed to full evaluation. + ShortCircuit func(lhs cty.Value) cty.Value } var ( OpLogicalOr = &Operation{ Impl: stdlib.OrFunc, Type: cty.Bool, + + ShortCircuit: func(lhs cty.Value) cty.Value { + if lhs.RawEquals(cty.True) { + return cty.True + } + return cty.NilVal + }, } OpLogicalAnd = &Operation{ Impl: stdlib.AndFunc, Type: cty.Bool, + + ShortCircuit: func(lhs cty.Value) cty.Value { + if lhs.RawEquals(cty.False) { + return cty.False + } + return cty.NilVal + }, } OpLogicalNot = &Operation{ Impl: stdlib.NotFunc, @@ -146,10 +169,7 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) var diags hcl.Diagnostics givenLHSVal, lhsDiags := e.LHS.Value(ctx) - givenRHSVal, rhsDiags := e.RHS.Value(ctx) diags = append(diags, lhsDiags...) - diags = append(diags, rhsDiags...) - lhsVal, err := convert.Convert(givenLHSVal, lhsParam.Type) if err != nil { diags = append(diags, &hcl.Diagnostic{ @@ -162,6 +182,35 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) EvalContext: ctx, }) } + + // If this is a short-circuiting operator and the LHS produces a + // short-circuiting result then we'll evaluate the RHS only for type + // checking purposes, ignoring any specific values, as a compromise + // between the convenience of a total short-circuit behavior and the + // benefit of not masking type errors on the RHS that we could still + // give earlier feedback about. + var forceResult cty.Value + rhsCtx := ctx + if e.Op.ShortCircuit != nil { + if !givenLHSVal.IsKnown() { + // If this is a short-circuit operator and our LHS value is + // unknown then we can't predict whether we would short-circuit + // yet, and so we must proceed under the assumption that we _will_ + // short-circuit to avoid raising any errors on the RHS that would + // eventually be hidden by the short-circuit behavior once LHS + // becomes known. + forceResult = cty.UnknownVal(e.Op.Type) + rhsCtx = ctx.NewChildAllVariablesUnknown() + } else if forceResult = e.Op.ShortCircuit(givenLHSVal); forceResult != cty.NilVal { + // This ensures that we'll only be type-checking against any + // variables used on the RHS, while not raising any errors about + // their values. + rhsCtx = ctx.NewChildAllVariablesUnknown() + } + } + + givenRHSVal, rhsDiags := e.RHS.Value(rhsCtx) + diags = append(diags, rhsDiags...) rhsVal, err := convert.Convert(givenRHSVal, rhsParam.Type) if err != nil { diags = append(diags, &hcl.Diagnostic{ @@ -181,6 +230,13 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) return cty.UnknownVal(e.Op.Type), diags } + // If we short-circuited above and still passed the type-check of RHS then + // we'll halt here and return the short-circuit result rather than actually + // executing the opertion. + if forceResult != cty.NilVal { + return forceResult, diags + } + args := []cty.Value{lhsVal, rhsVal} result, err := impl.Call(args) if err != nil { diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index 77d959fb..0c2840a9 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -1740,6 +1740,64 @@ EOT cty.False, 0, }, + { + // Logical AND operator short-circuit behavior + `nullobj != null && nullobj.is_thingy`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "nullobj": cty.NullVal(cty.Object(map[string]cty.Type{ + "is_thingy": cty.Bool, + })), + }, + }, + cty.False, + 0, // nullobj != null prevents evaluating nullobj.is_thingy + }, + { + // Logical AND short-circuit handling of unknown values + // If the first operand is an unknown bool then we can't know if + // we will short-circuit or not, and so we must assume we will + // and wait until the value becomes known before fully evaluating RHS. + `unknown < 4 && list[zero]`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Number), + "zero": cty.Zero, + "list": cty.ListValEmpty(cty.Bool), + }, + }, + cty.UnknownVal(cty.Bool), + 0, + }, + { + // Logical OR operator short-circuit behavior + `nullobj == null || nullobj.is_thingy`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "nullobj": cty.NullVal(cty.Object(map[string]cty.Type{ + "is_thingy": cty.Bool, + })), + }, + }, + cty.True, + 0, // nullobj == null prevents evaluating nullobj.is_thingy + }, + { + // Logical OR short-circuit handling of unknown values + // If the first operand is an unknown bool then we can't know if + // we will short-circuit or not, and so we must assume we will + // and wait until the value becomes known before fully evaluating RHS. + `unknown > 4 || list[zero]`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Number), + "zero": cty.Zero, + "list": cty.ListValEmpty(cty.Bool), + }, + }, + cty.UnknownVal(cty.Bool), + 0, + }, { `true ? var : null`, &hcl.EvalContext{ @@ -1983,6 +2041,59 @@ func TestExpressionErrorMessages(t *testing.T) { // describe coherently. "The true and false result expressions must have consistent types. At least one deeply-nested attribute or element is not compatible across both the 'true' and the 'false' value.", }, + + // Error messages describing situations where the logical operator + // short-circuit behavior still found a type error on the RHS that + // we therefore still report, because the LHS only guards against + // value-related problems in the RHS. + { + // It's not valid to access an attribute on a non-object-typed + // value even if we've proven it isn't null. + "notobj != null && notobj.foo", + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "notobj": cty.True, + }, + }, + "Unsupported attribute", + "Can't access attributes on a primitive-typed value (bool).", + }, + { + // It's not valid to access an attribute on a non-object-typed + // value even if we've proven it isn't null. + "notobj == null || notobj.foo", + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "notobj": cty.True, + }, + }, + "Unsupported attribute", + "Can't access attributes on a primitive-typed value (bool).", + }, + { + // It's not valid to access an index on an unindexable type + // even if we've proven it isn't null. + "notlist != null && notlist[0]", + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "notlist": cty.True, + }, + }, + "Invalid index", + "This value does not have any indices.", + }, + { + // Short-circuit can't avoid an error accessing a variable that + // doesn't exist at all, so we can still report typos. + "value != null && valeu", + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "value": cty.True, + }, + }, + "Unknown variable", + `There is no variable named "valeu". Did you mean "value"?`, + }, } for _, test := range tests {