From 0c2b8c01cadd64df2b07f52e3ab249100d48c7ec Mon Sep 17 00:00:00 2001 From: Claire Gaestel <213631+nyobe@users.noreply.github.com> Date: Tue, 18 Feb 2025 12:40:36 -0800 Subject: [PATCH 1/5] syntax error for fn invocation with extra keys --- ast/expr.go | 8 +- .../parse/invalid-invocation/env.yaml | 6 ++ .../parse/invalid-invocation/expected.json | 78 +++++++++++++++++++ 3 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 ast/testdata/parse/invalid-invocation/env.yaml create mode 100644 ast/testdata/parse/invalid-invocation/expected.json diff --git a/ast/expr.go b/ast/expr.go index 4dbe23c1..a1a9f61f 100644 --- a/ast/expr.go +++ b/ast/expr.go @@ -625,14 +625,20 @@ func FromBase64(value Expr) *FromBase64Expr { } func tryParseFunction(node *syntax.ObjectNode) (Expr, syntax.Diagnostics, bool) { + var diags syntax.Diagnostics if node.Len() != 1 { + for i := 0; i < node.Len(); i++ { + if strings.HasPrefix(node.Index(i).Key.Value(), "fn::") { + diags = append(diags, syntax.NodeError(node, "fn:: expression must be the only key within object")) + return nil, diags, false + } + } return nil, nil, false } kvp := node.Index(0) var parse func(node *syntax.ObjectNode, name *StringExpr, args Expr) (Expr, syntax.Diagnostics) - var diags syntax.Diagnostics switch kvp.Key.Value() { case "fn::fromJSON": parse = parseFromJSON diff --git a/ast/testdata/parse/invalid-invocation/env.yaml b/ast/testdata/parse/invalid-invocation/env.yaml new file mode 100644 index 00000000..a57d41da --- /dev/null +++ b/ast/testdata/parse/invalid-invocation/env.yaml @@ -0,0 +1,6 @@ +values: + multiple-keys: + fn::rotate::provider: + inputs: {} + state: + ohno: true diff --git a/ast/testdata/parse/invalid-invocation/expected.json b/ast/testdata/parse/invalid-invocation/expected.json new file mode 100644 index 00000000..2603b550 --- /dev/null +++ b/ast/testdata/parse/invalid-invocation/expected.json @@ -0,0 +1,78 @@ +{ + "decl": { + "Description": null, + "Imports": null, + "Values": { + "Entries": [ + { + "Key": { + "Value": "multiple-keys" + }, + "Value": { + "Entries": [ + { + "Key": { + "Value": "fn::rotate::provider" + }, + "Value": { + "Entries": [ + { + "Key": { + "Value": "inputs" + }, + "Value": { + "Entries": [] + } + } + ] + } + }, + { + "Key": { + "Value": "state" + }, + "Value": { + "Entries": [ + { + "Key": { + "Value": "ohno" + }, + "Value": { + "Value": true + } + } + ] + } + } + ] + } + } + ] + } + }, + "diags": [ + { + "Severity": 1, + "Summary": "fn:: expression must be the only key within object", + "Detail": "", + "Subject": { + "Filename": "invalid-invocation", + "Start": { + "Line": 3, + "Column": 5, + "Byte": 29 + }, + "End": { + "Line": 6, + "Column": 17, + "Byte": 95 + } + }, + "Context": null, + "Expression": null, + "EvalContext": null, + "Extra": null, + "Path": "values[\"multiple-keys\"]" + } + ] +} From e99287a368ff01281af968f45f62b785f376fc76 Mon Sep 17 00:00:00 2001 From: Claire Gaestel <213631+nyobe@users.noreply.github.com> Date: Tue, 18 Feb 2025 13:08:21 -0800 Subject: [PATCH 2/5] syntax error for fn invocation directly under values --- ast/environment.go | 3 ++ .../parse/invalid-invocation2/env.yaml | 2 + .../parse/invalid-invocation2/expected.json | 43 +++++++++++++++++++ 3 files changed, 48 insertions(+) create mode 100644 ast/testdata/parse/invalid-invocation2/env.yaml create mode 100644 ast/testdata/parse/invalid-invocation2/expected.json diff --git a/ast/environment.go b/ast/environment.go index 03e7b4d1..89cfe785 100644 --- a/ast/environment.go +++ b/ast/environment.go @@ -128,6 +128,9 @@ func (d *MapDecl[T]) parse(name string, node syntax.Node) syntax.Diagnostics { var v T vname := name + "." + kvp.Key.Value() + if strings.HasPrefix(kvp.Key.Value(), "fn::") { + diags.Extend(syntax.NodeError(kvp.Key, "fn expression not allowed at the top level")) + } vdiags := parseNode(vname, &v, kvp.Value) diags.Extend(vdiags...) diff --git a/ast/testdata/parse/invalid-invocation2/env.yaml b/ast/testdata/parse/invalid-invocation2/env.yaml new file mode 100644 index 00000000..dcaeaf33 --- /dev/null +++ b/ast/testdata/parse/invalid-invocation2/env.yaml @@ -0,0 +1,2 @@ +values: + fn::secret: "hello" diff --git a/ast/testdata/parse/invalid-invocation2/expected.json b/ast/testdata/parse/invalid-invocation2/expected.json new file mode 100644 index 00000000..ddc3b2e9 --- /dev/null +++ b/ast/testdata/parse/invalid-invocation2/expected.json @@ -0,0 +1,43 @@ +{ + "decl": { + "Description": null, + "Imports": null, + "Values": { + "Entries": [ + { + "Key": { + "Value": "fn::secret" + }, + "Value": { + "Value": "hello" + } + } + ] + } + }, + "diags": [ + { + "Severity": 1, + "Summary": "fn expression not allowed at the top level", + "Detail": "", + "Subject": { + "Filename": "invalid-invocation2", + "Start": { + "Line": 2, + "Column": 3, + "Byte": 10 + }, + "End": { + "Line": 2, + "Column": 13, + "Byte": 20 + } + }, + "Context": null, + "Expression": null, + "EvalContext": null, + "Extra": null, + "Path": "values[\"fn::secret\"]" + } + ] +} From 04eee2a190e774d035b3734c0e9c7e21f1187be9 Mon Sep 17 00:00:00 2001 From: Claire <213631+nyobe@users.noreply.github.com> Date: Wed, 19 Feb 2025 10:41:04 -0800 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: Pat Gavlin --- ast/environment.go | 2 +- ast/expr.go | 7 ++++--- ast/testdata/parse/invalid-invocation/expected.json | 2 +- ast/testdata/parse/invalid-invocation2/expected.json | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/ast/environment.go b/ast/environment.go index 89cfe785..9ee598ca 100644 --- a/ast/environment.go +++ b/ast/environment.go @@ -129,7 +129,7 @@ func (d *MapDecl[T]) parse(name string, node syntax.Node) syntax.Diagnostics { var v T vname := name + "." + kvp.Key.Value() if strings.HasPrefix(kvp.Key.Value(), "fn::") { - diags.Extend(syntax.NodeError(kvp.Key, "fn expression not allowed at the top level")) + diags.Extend(syntax.NodeError(kvp.Key, fmt.Sprintf("builtin function call %q not allowed at the top level", kvp.Key.Value()))) } vdiags := parseNode(vname, &v, kvp.Value) diags.Extend(vdiags...) diff --git a/ast/expr.go b/ast/expr.go index a1a9f61f..91b516c2 100644 --- a/ast/expr.go +++ b/ast/expr.go @@ -628,11 +628,12 @@ func tryParseFunction(node *syntax.ObjectNode) (Expr, syntax.Diagnostics, bool) var diags syntax.Diagnostics if node.Len() != 1 { for i := 0; i < node.Len(); i++ { - if strings.HasPrefix(node.Index(i).Key.Value(), "fn::") { - diags = append(diags, syntax.NodeError(node, "fn:: expression must be the only key within object")) - return nil, diags, false + if k := node.Index(i).Key.Value(); strings.HasPrefix(k, "fn::") { + diags = append(diags, syntax.NodeError(node, fmt.Sprintf("illegal call to builtin function: %q must be the only key within its containing object", k))) + } } + return nil, diags, false return nil, nil, false } diff --git a/ast/testdata/parse/invalid-invocation/expected.json b/ast/testdata/parse/invalid-invocation/expected.json index 2603b550..a46a3f07 100644 --- a/ast/testdata/parse/invalid-invocation/expected.json +++ b/ast/testdata/parse/invalid-invocation/expected.json @@ -53,7 +53,7 @@ "diags": [ { "Severity": 1, - "Summary": "fn:: expression must be the only key within object", + "Summary": "illegal call to builtin function: \"fn::rotate::provider\" must be the only key within its containing object", "Detail": "", "Subject": { "Filename": "invalid-invocation", diff --git a/ast/testdata/parse/invalid-invocation2/expected.json b/ast/testdata/parse/invalid-invocation2/expected.json index ddc3b2e9..fe24a84c 100644 --- a/ast/testdata/parse/invalid-invocation2/expected.json +++ b/ast/testdata/parse/invalid-invocation2/expected.json @@ -18,7 +18,7 @@ "diags": [ { "Severity": 1, - "Summary": "fn expression not allowed at the top level", + "Summary": "builtin function call \"fn::secret\" not allowed at the top level", "Detail": "", "Subject": { "Filename": "invalid-invocation2", From aa3828c4f66480ec81776df257a5986bf889a2e7 Mon Sep 17 00:00:00 2001 From: Claire Gaestel <213631+nyobe@users.noreply.github.com> Date: Wed, 19 Feb 2025 11:20:06 -0800 Subject: [PATCH 4/5] changelog --- CHANGELOG_PENDING.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index bf5a76e2..3c65af26 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -17,3 +17,6 @@ ### Bug Fixes ### Breaking changes + +- It is now a syntax error to call a builtin function incorrectly. + [449](https://github.com/pulumi/esc/pull/449) \ No newline at end of file From 94b20cfe18d52cee9c535d3249b0c0b0e457aadf Mon Sep 17 00:00:00 2001 From: Claire Gaestel <213631+nyobe@users.noreply.github.com> Date: Wed, 19 Feb 2025 13:42:09 -0800 Subject: [PATCH 5/5] lint --- ast/expr.go | 1 - 1 file changed, 1 deletion(-) diff --git a/ast/expr.go b/ast/expr.go index 91b516c2..7eacf4c2 100644 --- a/ast/expr.go +++ b/ast/expr.go @@ -634,7 +634,6 @@ func tryParseFunction(node *syntax.ObjectNode) (Expr, syntax.Diagnostics, bool) } } return nil, diags, false - return nil, nil, false } kvp := node.Index(0)