Short-circuit behavior for the && and || operators

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
+}

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
+)

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=

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 {

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 {

hclsyntax/parser.go

@@ -563,20 +563,25 @@ 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
  var ok bool
  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