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/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= diff --git a/hclsyntax/expression_ops.go b/hclsyntax/expression_ops.go index c1db0cec..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, @@ -82,6 +105,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 @@ -142,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{ @@ -158,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{ @@ -177,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 { 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}, + }, + }, + }, { ` `,