From f3681c61382c25e44ce9c897b10dc344d1659ce8 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 12 May 2017 13:43:33 -0700 Subject: [PATCH 1/2] Add missing test cases for conditionals using unknown values Currently these fail, due to a regression that wasn't caught due to us not having these tests to begin with. They will be fixed in a subsequent commit. --- check_types_test.go | 64 ++++++++++++++++++++++++++ eval_test.go | 108 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 169 insertions(+), 3 deletions(-) diff --git a/check_types_test.go b/check_types_test.go index 5856913..d56f701 100644 --- a/check_types_test.go +++ b/check_types_test.go @@ -237,6 +237,70 @@ func TestTypeCheck(t *testing.T) { true, }, + { + // conditional with unknown value is permitted + `foo ${true ? known : unknown}`, + &ast.BasicScope{ + VarMap: map[string]ast.Variable{ + "known": ast.Variable{ + Type: ast.TypeString, + Value: "bar", + }, + "unknown": ast.Variable{ + Type: ast.TypeUnknown, + Value: UnknownValue, + }, + }, + }, + false, + }, + + { + // conditional with unknown value the other way permitted too + `foo ${true ? unknown : known}`, + &ast.BasicScope{ + VarMap: map[string]ast.Variable{ + "known": ast.Variable{ + Type: ast.TypeString, + Value: "bar", + }, + "unknown": ast.Variable{ + Type: ast.TypeUnknown, + Value: UnknownValue, + }, + }, + }, + false, + }, + + { + // conditional with two unknowns is allowed + `foo ${true ? unknown : unknown}`, + &ast.BasicScope{ + VarMap: map[string]ast.Variable{ + "unknown": ast.Variable{ + Type: ast.TypeUnknown, + Value: UnknownValue, + }, + }, + }, + false, + }, + + { + // conditional with unknown condition is allowed + `foo ${unknown ? 1 : 2}`, + &ast.BasicScope{ + VarMap: map[string]ast.Variable{ + "unknown": ast.Variable{ + Type: ast.TypeUnknown, + Value: UnknownValue, + }, + }, + }, + false, + }, + { // currently lists are not allowed at all `foo ${true ? arr1 : arr2}`, diff --git a/eval_test.go b/eval_test.go index 8643b54..a8aaf30 100644 --- a/eval_test.go +++ b/eval_test.go @@ -1197,6 +1197,108 @@ func TestEvalInternal(t *testing.T) { ast.TypeUnknown, }, + { + // false expression can be unknown, and is returned + `foo ${false ? "12" : unknown}`, + &ast.BasicScope{ + VarMap: map[string]ast.Variable{ + "unknown": ast.Variable{ + Value: UnknownValue, + Type: ast.TypeUnknown, + }, + }, + }, + false, + UnknownValue, + ast.TypeUnknown, + }, + + { + // false expression can be unknown, and result is unknown even + // if it's not selected. + // (Ideally this would not be true, but we're accepting this + // for now since this assumption is built in to the core evaluator) + `foo ${true ? "12" : unknown}`, + &ast.BasicScope{ + VarMap: map[string]ast.Variable{ + "unknown": ast.Variable{ + Value: UnknownValue, + Type: ast.TypeUnknown, + }, + }, + }, + false, + UnknownValue, + ast.TypeUnknown, + }, + + { + // true expression can be unknown, and is returned + `foo ${false ? unknown : "bar"}`, + &ast.BasicScope{ + VarMap: map[string]ast.Variable{ + "unknown": ast.Variable{ + Value: UnknownValue, + Type: ast.TypeUnknown, + }, + }, + }, + false, + UnknownValue, + ast.TypeUnknown, + }, + + { + // true expression can be unknown, and result is unknown even + // if it's not selected. + // (Ideally this would not be true, but we're accepting this + // for now since this assumption is built in to the core evaluator) + `foo ${false ? unknown : "bar"}`, + &ast.BasicScope{ + VarMap: map[string]ast.Variable{ + "unknown": ast.Variable{ + Value: UnknownValue, + Type: ast.TypeUnknown, + }, + }, + }, + false, + UnknownValue, + ast.TypeUnknown, + }, + + { + // both values can be unknown + `foo ${false ? unknown : unknown}`, + &ast.BasicScope{ + VarMap: map[string]ast.Variable{ + "unknown": ast.Variable{ + Value: UnknownValue, + Type: ast.TypeUnknown, + }, + }, + }, + false, + UnknownValue, + ast.TypeUnknown, + }, + + { + // condition can be unknown, and result is unknown + `foo ${unknown ? "baz" : "bar"}`, + &ast.BasicScope{ + VarMap: map[string]ast.Variable{ + "unknown": ast.Variable{ + Value: UnknownValue, + Type: ast.TypeUnknown, + }, + }, + }, + false, + UnknownValue, + ast.TypeUnknown, + }, + { "foo ${-bar}", &ast.BasicScope{ @@ -1911,13 +2013,13 @@ func TestEvalInternal(t *testing.T) { out, outType, err := internalEval(node, &EvalConfig{GlobalScope: tc.Scope}) if err != nil != tc.Error { - t.Fatalf("Error: %s\n\nInput: %s", err, tc.Input) + t.Fatalf("Error: %s\nInput: %s", err, tc.Input) } if tc.ResultType != ast.TypeInvalid && outType != tc.ResultType { - t.Fatalf("Bad: %s\n\nInput: %s", outType, tc.Input) + t.Fatalf("Wrong result type\nInput: %s\nGot: %#s\nWant: %s", tc.Input, outType, tc.ResultType) } if !reflect.DeepEqual(out, tc.Result) { - t.Fatalf("\n Got: %#v\n Want: %#v\n\nInput: %s\n", out, tc.Result, tc.Input) + t.Fatalf("Wrong result value\nInput: %s\nGot: %#s\nWant: %s", tc.Input, out, tc.Result) } }) } From 284057211ecb372edd01d23cd4bec19d56a9e5da Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 12 May 2017 13:49:17 -0700 Subject: [PATCH 2/2] Allow one or both conditional operands to be unknown In #52 we relaxed the rule that any unknown value would immediately cause an early exit, which in turn requires the type checker for each node to specifically deal with unknown values. However, we missed dealing with the check that both types match in a conditional, causing a conditional to fail if either side of it is unknown. This causes errors in Terraform, as described in hashicorp/terraform#14399. --- check_types.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/check_types.go b/check_types.go index 7a191e8..f16da39 100644 --- a/check_types.go +++ b/check_types.go @@ -414,7 +414,7 @@ func (tc *typeCheckConditional) TypeCheck(v *TypeCheck) (ast.Node, error) { } // The types of the true and false expression must match - if trueType != falseType { + if trueType != falseType && trueType != ast.TypeUnknown && falseType != ast.TypeUnknown { // Since passing around stringified versions of other types is // common, we pragmatically allow the false expression to dictate @@ -460,7 +460,13 @@ func (tc *typeCheckConditional) TypeCheck(v *TypeCheck) (ast.Node, error) { } // Result type (guaranteed to also match falseType due to the above) - v.StackPush(trueType) + if trueType == ast.TypeUnknown { + // falseType may also be unknown, but that's okay because two + // unknowns means our result is unknown anyway. + v.StackPush(falseType) + } else { + v.StackPush(trueType) + } return tc.n, nil }