From 486e0da2834e6203666c3ebf0ad2c2e85154d8ab Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Fri, 19 May 2023 14:58:17 +0100 Subject: [PATCH 01/23] Implemented env[] keyword and tests. --- expr_test.go | 76 +++++++++++++++++++++++++++++++++++++++++++ parser/lexer/state.go | 44 +++++++++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/expr_test.go b/expr_test.go index 6246c844b..1597da4c1 100644 --- a/expr_test.go +++ b/expr_test.go @@ -1811,6 +1811,82 @@ func TestEval_nil_in_maps(t *testing.T) { }) } +// Test the use of env keyword. Forms env[] and env[”] are valid. +// The enclosed identifier must be in the expression env. +func TestEnv_keyword(t *testing.T) { + env := map[string]interface{}{ + "space test": "ok", + "space_test": "not ok", // Seems to be some underscore substituting happening, check that. + "Section 1-2a": "ok", + "2015 Information Table": "ok", + "%*worst function name ever!!": func() string { + return "ok" + }(), + "env": func(string) string { + return "env func ok" + }, + "1": "o", + "2": "k", + "num": 10, + } + + // No error cases + var tests = []struct { + code string + want interface{} + }{ + {"env['space test']", "ok"}, + {"env[space test]", "ok"}, + {"env[Section 1-2a]", "ok"}, + {`env['2015 Information Table']`, "ok"}, + {"env[%*worst function name ever!!]", "ok"}, + {"env('Confusing!')", "env func ok"}, // not keyword, but some function named env + {"env[1] + env[2]", "ok"}, + {"1 + env[num] + env[num]", 21}, + } + + for _, tt := range tests { + t.Run(tt.code, func(t *testing.T) { + + program, err := expr.Compile(tt.code, expr.Env(env)) + require.NoError(t, err, "compile error") + + got, err := expr.Run(program, env) + require.NoError(t, err, "execution error") + + assert.Equal(t, tt.want, got, tt.code) + }) + } + + for _, tt := range tests { + t.Run(tt.code, func(t *testing.T) { + got, err := expr.Eval(tt.code, env) + require.NoError(t, err, "eval error: "+tt.code) + + assert.Equal(t, tt.want, got, "eval: "+tt.code) + }) + } + + // error cases + tests = []struct { + code string + want interface{} + }{ + {"env[this is a problem", "bad"}, + {"env['also a problem'", "bad"}, + } + + for _, tt := range tests { + t.Run(tt.code, func(t *testing.T) { + + _, err := expr.Eval(tt.code, expr.Env(env)) + require.Error(t, err, "compile error") + + }) + } + +} + type Bar interface { Bar() int } diff --git a/parser/lexer/state.go b/parser/lexer/state.go index 1212aa321..fec2ea9ca 100644 --- a/parser/lexer/state.go +++ b/parser/lexer/state.go @@ -6,6 +6,11 @@ import ( type stateFn func(*lexer) stateFn +// The lexer works as a state machine, creating and emitting tokens +// as it proceeds through the input file (string). State changes with the +// context of what characters have been encountered so far. This is the +// initial state; when a change is detected, it returns the function for the +// relevant next state to proceed. func root(l *lexer) stateFn { switch r := l.next(); { case r == eof: @@ -119,6 +124,8 @@ loop: return not case "in", "or", "and", "matches", "contains", "startsWith", "endsWith": l.emit(Operator) + case "env": + return literalIdentifier default: l.emit(Identifier) } @@ -128,6 +135,43 @@ loop: return root } +// Process what comes after the env keyword, which must be any +// set of characters or a string literal, enclosed in square brackets +func literalIdentifier(l *lexer) stateFn { + // Be very strict about syntax so as not to break someone's "env" identifier + if l.next() == '[' { + l.ignore() // Forget about the "env" keyword and its opening bracket + if r := l.next(); r == '\'' || r == '"' { + l.scanString(r) + str, err := unescape(l.word()) + if err != nil { + l.error("%v", err) + } + if l.next() == ']' { + l.backup() + l.emitValue(Identifier, str) + l.next() + } else { + return l.error("env keyword with no closing bracket") + } + } else { + for r := l.next(); r != ']' && r != eof; r = l.next() { + } + if r == eof { + return l.error("env keyword with no closing bracket") + } else { + l.backup() + l.emit(Identifier) + l.next() + } + } + } else { + l.backup() + l.emit(Identifier) + } + return root +} + func not(l *lexer) stateFn { l.emit(Operator) From 39225da3bceb43474d715dcc7b7bc5f645966f67 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Fri, 19 May 2023 23:16:10 +0100 Subject: [PATCH 02/23] env keyword must take string (no unquoted literals) --- expr_test.go | 21 +++++++++++---------- parser/lexer/state.go | 10 +--------- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/expr_test.go b/expr_test.go index 1597da4c1..3f8eb8bc8 100644 --- a/expr_test.go +++ b/expr_test.go @@ -1815,10 +1815,10 @@ func TestEval_nil_in_maps(t *testing.T) { // The enclosed identifier must be in the expression env. func TestEnv_keyword(t *testing.T) { env := map[string]interface{}{ - "space test": "ok", - "space_test": "not ok", // Seems to be some underscore substituting happening, check that. - "Section 1-2a": "ok", - "2015 Information Table": "ok", + "space test": "ok", + "space_test": "not ok", // Seems to be some underscore substituting happening, check that. + "Section 1-2a": "ok", + `c:\ndrive\2015 Information Table`: "ok", "%*worst function name ever!!": func() string { return "ok" }(), @@ -1836,13 +1836,12 @@ func TestEnv_keyword(t *testing.T) { want interface{} }{ {"env['space test']", "ok"}, - {"env[space test]", "ok"}, - {"env[Section 1-2a]", "ok"}, - {`env['2015 Information Table']`, "ok"}, - {"env[%*worst function name ever!!]", "ok"}, + {"env['Section 1-2a']", "ok"}, + {`env["c:\\ndrive\\2015 Information Table"]`, "ok"}, + {"env['%*worst function name ever!!']", "ok"}, {"env('Confusing!')", "env func ok"}, // not keyword, but some function named env - {"env[1] + env[2]", "ok"}, - {"1 + env[num] + env[num]", 21}, + {"env['1'] + env['2']", "ok"}, + {"1 + env['num'] + env['num']", 21}, } for _, tt := range tests { @@ -1874,6 +1873,8 @@ func TestEnv_keyword(t *testing.T) { }{ {"env[this is a problem", "bad"}, {"env['also a problem'", "bad"}, + {"env[space test]", "bad"}, + {"env[1] + env[2]", "bad"}, } for _, tt := range tests { diff --git a/parser/lexer/state.go b/parser/lexer/state.go index fec2ea9ca..76e1fc1e8 100644 --- a/parser/lexer/state.go +++ b/parser/lexer/state.go @@ -155,15 +155,7 @@ func literalIdentifier(l *lexer) stateFn { return l.error("env keyword with no closing bracket") } } else { - for r := l.next(); r != ']' && r != eof; r = l.next() { - } - if r == eof { - return l.error("env keyword with no closing bracket") - } else { - l.backup() - l.emit(Identifier) - l.next() - } + return l.error("env keyword must have string index") } } else { l.backup() From 2cb92ebfd5fe36b4c4aa26862fd5922aaff22cc6 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Mon, 22 May 2023 22:30:47 +0100 Subject: [PATCH 03/23] Fixed error failing to ignore closing bracket --- expr_test.go | 15 ++++++++++++--- parser/lexer/state.go | 3 +-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/expr_test.go b/expr_test.go index 3f8eb8bc8..8450c983d 100644 --- a/expr_test.go +++ b/expr_test.go @@ -1825,9 +1825,17 @@ func TestEnv_keyword(t *testing.T) { "env": func(string) string { return "env func ok" }, - "1": "o", - "2": "k", - "num": 10, + "1": "o", + "2": "k", + "num": 10, + "my list": []int{1, 2, 3, 4, 5}, + "MIN": func(a, b int) int { + if a < b { + return a + } else { + return b + } + }, } // No error cases @@ -1842,6 +1850,7 @@ func TestEnv_keyword(t *testing.T) { {"env('Confusing!')", "env func ok"}, // not keyword, but some function named env {"env['1'] + env['2']", "ok"}, {"1 + env['num'] + env['num']", 21}, + {"MIN(env['num'],0)", 0}, } for _, tt := range tests { diff --git a/parser/lexer/state.go b/parser/lexer/state.go index 76e1fc1e8..633b229e3 100644 --- a/parser/lexer/state.go +++ b/parser/lexer/state.go @@ -148,9 +148,8 @@ func literalIdentifier(l *lexer) stateFn { l.error("%v", err) } if l.next() == ']' { - l.backup() + l.ignore() l.emitValue(Identifier, str) - l.next() } else { return l.error("env keyword with no closing bracket") } From 14abd99f49d790ecc62c8dc2957930b662d73788 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Sat, 10 Jun 2023 17:39:32 +0100 Subject: [PATCH 04/23] Removed env keyword from lexer. --- parser/lexer/state.go | 35 ----------------------------------- 1 file changed, 35 deletions(-) diff --git a/parser/lexer/state.go b/parser/lexer/state.go index 633b229e3..1212aa321 100644 --- a/parser/lexer/state.go +++ b/parser/lexer/state.go @@ -6,11 +6,6 @@ import ( type stateFn func(*lexer) stateFn -// The lexer works as a state machine, creating and emitting tokens -// as it proceeds through the input file (string). State changes with the -// context of what characters have been encountered so far. This is the -// initial state; when a change is detected, it returns the function for the -// relevant next state to proceed. func root(l *lexer) stateFn { switch r := l.next(); { case r == eof: @@ -124,8 +119,6 @@ loop: return not case "in", "or", "and", "matches", "contains", "startsWith", "endsWith": l.emit(Operator) - case "env": - return literalIdentifier default: l.emit(Identifier) } @@ -135,34 +128,6 @@ loop: return root } -// Process what comes after the env keyword, which must be any -// set of characters or a string literal, enclosed in square brackets -func literalIdentifier(l *lexer) stateFn { - // Be very strict about syntax so as not to break someone's "env" identifier - if l.next() == '[' { - l.ignore() // Forget about the "env" keyword and its opening bracket - if r := l.next(); r == '\'' || r == '"' { - l.scanString(r) - str, err := unescape(l.word()) - if err != nil { - l.error("%v", err) - } - if l.next() == ']' { - l.ignore() - l.emitValue(Identifier, str) - } else { - return l.error("env keyword with no closing bracket") - } - } else { - return l.error("env keyword must have string index") - } - } else { - l.backup() - l.emit(Identifier) - } - return root -} - func not(l *lexer) stateFn { l.emit(Operator) From 268eb410c8c6fa94a65b356fbea95a4826d90dab Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Sat, 10 Jun 2023 19:51:17 +0100 Subject: [PATCH 05/23] Implemented env[] keyowrd for abitrary identfiers --- parser/parser.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/parser/parser.go b/parser/parser.go index fd26fe18b..4465d114c 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -560,11 +560,22 @@ func (p *parser) parsePostfixExpression(node Node) Node { p.expect(Bracket, "]") } else { - // Slice operator [:] was not found, - // it should be just an index node. - node = &MemberNode{ - Node: node, - Property: from, + // env keyword "promotes" a string index to an identifier + if idNode, ok := (node).(*IdentifierNode); ok && idNode.Value == "env" { + if stNode, ok := (from).(*StringNode); ok { + node = &IdentifierNode{ + Value: stNode.Value, + } + } else { + p.error("expected string index for env keyword") + } + } else { + // Slice operator [:] was not found, + // it should be just an index node. + node = &MemberNode{ + Node: node, + Property: from, + } } node.SetLocation(postfixToken.Location) p.expect(Bracket, "]") From 03616c7acc6202ed28d38fdcad3250281361cf10 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 27 Jun 2023 21:55:11 +0100 Subject: [PATCH 06/23] Allow for non-integer index with env keyword --- compiler/compiler.go | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/compiler/compiler.go b/compiler/compiler.go index 3cd32af0f..361ab6c4b 100644 --- a/compiler/compiler.go +++ b/compiler/compiler.go @@ -205,6 +205,9 @@ func (c *compiler) NilNode(_ *ast.NilNode) { } func (c *compiler) IdentifierNode(node *ast.IdentifierNode) { + if node.Value == "aenv" { + + } if c.mapEnv { c.emit(OpLoadFast, c.addConstant(node.Value)) } else if len(node.FieldIndex) > 0 { @@ -491,19 +494,25 @@ func (c *compiler) MemberNode(node *ast.MemberNode) { } } - c.compile(base) - if node.Optional { - ph := c.emit(OpJumpIfNil, placeholder) - c.chains[len(c.chains)-1] = append(c.chains[len(c.chains)-1], ph) - } - - if op == OpFetch { + // Implementation of direct env access + if ident, ok := base.(*ast.IdentifierNode); ok && ident.Value == "env" { c.compile(node.Property) - c.emit(OpFetch) + c.emit(OpFetchEnv) } else { - c.emitLocation(node.Location(), op, c.addConstant( - &runtime.Field{Index: index, Path: path}, - )) + c.compile(base) + if node.Optional { + ph := c.emit(OpJumpIfNil, placeholder) + c.chains[len(c.chains)-1] = append(c.chains[len(c.chains)-1], ph) + } + + if op == OpFetch { + c.compile(node.Property) + c.emit(OpFetch) + } else { + c.emitLocation(node.Location(), op, c.addConstant( + &runtime.Field{Index: index, Path: path}, + )) + } } deref: From 65d64c5c1ad5735bb6089a3e429ffa51e78ea167 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 27 Jun 2023 21:56:17 +0100 Subject: [PATCH 07/23] Allow for non-integer index with env keyword --- checker/checker.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/checker/checker.go b/checker/checker.go index 00025a33c..6a81e20d9 100644 --- a/checker/checker.go +++ b/checker/checker.go @@ -429,7 +429,11 @@ func (v *visitor) MemberNode(node *ast.MemberNode) (reflect.Type, info) { return t, info{} case reflect.Array, reflect.Slice: - if !isInteger(prop) && !isAny(prop) { + if isInteger(prop) || isAny(prop) { + // ok, we normally expect an integer or interface that could be one + } else if name, ok := node.Node.(*ast.IdentifierNode); ok && name.Value == "env" { + // also no problem - env is a keyword with string index + } else { return v.error(node.Property, "array elements can only be selected using an integer (got %v)", prop) } t, c := deref(base.Elem()) From a5c8d90213110be73d5908340118c930668f98db Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 27 Jun 2023 22:03:57 +0100 Subject: [PATCH 08/23] Removed env implementation (previous commit) --- parser/parser.go | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/parser/parser.go b/parser/parser.go index 4465d114c..fd26fe18b 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -560,22 +560,11 @@ func (p *parser) parsePostfixExpression(node Node) Node { p.expect(Bracket, "]") } else { - // env keyword "promotes" a string index to an identifier - if idNode, ok := (node).(*IdentifierNode); ok && idNode.Value == "env" { - if stNode, ok := (from).(*StringNode); ok { - node = &IdentifierNode{ - Value: stNode.Value, - } - } else { - p.error("expected string index for env keyword") - } - } else { - // Slice operator [:] was not found, - // it should be just an index node. - node = &MemberNode{ - Node: node, - Property: from, - } + // Slice operator [:] was not found, + // it should be just an index node. + node = &MemberNode{ + Node: node, + Property: from, } node.SetLocation(postfixToken.Location) p.expect(Bracket, "]") From 9e3c0441f2085c332f18268f81e9c90a1e426f07 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 27 Jun 2023 22:04:29 +0100 Subject: [PATCH 09/23] Added OpFetchEnv --- vm/opcodes.go | 1 + 1 file changed, 1 insertion(+) diff --git a/vm/opcodes.go b/vm/opcodes.go index b3117e73c..bb2bdd884 100644 --- a/vm/opcodes.go +++ b/vm/opcodes.go @@ -13,6 +13,7 @@ const ( OpLoadFunc OpFetch OpFetchField + OpFetchEnv OpMethod OpTrue OpFalse From bb22e0e292b0a97e7483123729182b1bacabe6d5 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 27 Jun 2023 22:05:51 +0100 Subject: [PATCH 10/23] Implemented OpFetchEnv --- vm/vm.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/vm/vm.go b/vm/vm.go index af4fc5bf7..808326929 100644 --- a/vm/vm.go +++ b/vm/vm.go @@ -123,6 +123,10 @@ func (vm *VM) Run(program *Program, env interface{}) (_ interface{}, err error) a := vm.pop() vm.push(runtime.FetchField(a, program.Constants[arg].(*runtime.Field))) + case OpFetchEnv: + b := vm.pop() + vm.push(runtime.Fetch(env, b)) + case OpMethod: a := vm.pop() vm.push(runtime.FetchMethod(a, program.Constants[arg].(*runtime.Method))) From 887f0b7a4fb658cfb6a971b5f8ea048729fdd7c2 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 27 Jun 2023 22:22:18 +0100 Subject: [PATCH 11/23] Improved error message for non-string env index --- vm/vm.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/vm/vm.go b/vm/vm.go index 808326929..6046bb7a6 100644 --- a/vm/vm.go +++ b/vm/vm.go @@ -125,6 +125,9 @@ func (vm *VM) Run(program *Program, env interface{}) (_ interface{}, err error) case OpFetchEnv: b := vm.pop() + if _, ok := b.(string); !ok { + panic("index for env[] keyord must be string") + } vm.push(runtime.Fetch(env, b)) case OpMethod: From f5ddd6a669aa09c75d89890bb3917bd85e196cad Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Sun, 9 Jul 2023 15:50:45 +0100 Subject: [PATCH 12/23] Tests env when index must be evaluated --- expr_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/expr_test.go b/expr_test.go index 8450c983d..78f75b0e3 100644 --- a/expr_test.go +++ b/expr_test.go @@ -1823,7 +1823,7 @@ func TestEnv_keyword(t *testing.T) { return "ok" }(), "env": func(string) string { - return "env func ok" + return "num" }, "1": "o", "2": "k", @@ -1836,6 +1836,8 @@ func TestEnv_keyword(t *testing.T) { return b } }, + "red": "n", + "irect": "um", } // No error cases @@ -1847,10 +1849,13 @@ func TestEnv_keyword(t *testing.T) { {"env['Section 1-2a']", "ok"}, {`env["c:\\ndrive\\2015 Information Table"]`, "ok"}, {"env['%*worst function name ever!!']", "ok"}, - {"env('Confusing!')", "env func ok"}, // not keyword, but some function named env + {"env('Confusing!')", "num"}, // not keyword, but some function named env {"env['1'] + env['2']", "ok"}, {"1 + env['num'] + env['num']", 21}, {"MIN(env['num'],0)", 0}, + {"env['nu' + 'm']", 10}, + {"env[red + irect]", 10}, + {"env[env('na')]", 10}, } for _, tt := range tests { From fc28fb2babf84d091aa7cb292e4d49417b478beb Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Sun, 9 Jul 2023 15:52:04 +0100 Subject: [PATCH 13/23] Deal with env[] keywod member node. --- checker/checker.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/checker/checker.go b/checker/checker.go index 6a81e20d9..b41364755 100644 --- a/checker/checker.go +++ b/checker/checker.go @@ -132,7 +132,7 @@ func (v *visitor) IdentifierNode(node *ast.IdentifierNode) (reflect.Type, info) if fn, ok := v.config.Functions[node.Value]; ok { // Return anyType instead of func type as we don't know the arguments yet. // The func type can be one of the fn.Types. The type will be resolved - // when the arguments are known in CallNode. + // when the arguments areknown in CallNode. return anyType, info{fn: fn} } if v.config.Types == nil { @@ -383,6 +383,9 @@ func (v *visitor) ChainNode(node *ast.ChainNode) (reflect.Type, info) { } func (v *visitor) MemberNode(node *ast.MemberNode) (reflect.Type, info) { + if an, ok := node.Node.(*ast.IdentifierNode); ok && an.Value == "env" { + return anyType, info{} + } base, _ := v.visit(node.Node) prop, _ := v.visit(node.Property) @@ -431,8 +434,6 @@ func (v *visitor) MemberNode(node *ast.MemberNode) (reflect.Type, info) { case reflect.Array, reflect.Slice: if isInteger(prop) || isAny(prop) { // ok, we normally expect an integer or interface that could be one - } else if name, ok := node.Node.(*ast.IdentifierNode); ok && name.Value == "env" { - // also no problem - env is a keyword with string index } else { return v.error(node.Property, "array elements can only be selected using an integer (got %v)", prop) } From 9b3857f1801e7003fb73dd7b55701ead436ca55a Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Sun, 9 Jul 2023 16:00:12 +0100 Subject: [PATCH 14/23] Allow for non-integer index with env keyword --- compiler/compiler.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/compiler/compiler.go b/compiler/compiler.go index 361ab6c4b..f2dfe15ff 100644 --- a/compiler/compiler.go +++ b/compiler/compiler.go @@ -205,9 +205,6 @@ func (c *compiler) NilNode(_ *ast.NilNode) { } func (c *compiler) IdentifierNode(node *ast.IdentifierNode) { - if node.Value == "aenv" { - - } if c.mapEnv { c.emit(OpLoadFast, c.addConstant(node.Value)) } else if len(node.FieldIndex) > 0 { From 9a2d59fbacfcb971e29e98eaa38eb5a45741f405 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Sun, 9 Jul 2023 19:26:47 +0100 Subject: [PATCH 15/23] Typo. --- checker/checker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checker/checker.go b/checker/checker.go index b41364755..cf0327641 100644 --- a/checker/checker.go +++ b/checker/checker.go @@ -132,7 +132,7 @@ func (v *visitor) IdentifierNode(node *ast.IdentifierNode) (reflect.Type, info) if fn, ok := v.config.Functions[node.Value]; ok { // Return anyType instead of func type as we don't know the arguments yet. // The func type can be one of the fn.Types. The type will be resolved - // when the arguments areknown in CallNode. + // when the arguments are known in CallNode. return anyType, info{fn: fn} } if v.config.Types == nil { From 3e8608f153aab2852b290afe11c0e99f24b40b97 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 11 Jul 2023 21:14:19 +0100 Subject: [PATCH 16/23] Check when env index is StringNode, handle type --- checker/checker.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/checker/checker.go b/checker/checker.go index cf0327641..a4e5e6d45 100644 --- a/checker/checker.go +++ b/checker/checker.go @@ -383,11 +383,18 @@ func (v *visitor) ChainNode(node *ast.ChainNode) (reflect.Type, info) { } func (v *visitor) MemberNode(node *ast.MemberNode) (reflect.Type, info) { + prop, _ := v.visit(node.Property) if an, ok := node.Node.(*ast.IdentifierNode); ok && an.Value == "env" { + // If the index is a constant string, can save some + // cycles later by finding the type of its referent + if name, ok := node.Property.(*ast.StringNode); ok { + if t, ok := v.config.Types[name.Value]; ok { + return t.Type, info{method: t.Method} + } // No error if no type found; it may be added to env between compile and run + } return anyType, info{} } base, _ := v.visit(node.Node) - prop, _ := v.visit(node.Property) if name, ok := node.Property.(*ast.StringNode); ok { if base == nil { @@ -432,9 +439,7 @@ func (v *visitor) MemberNode(node *ast.MemberNode) (reflect.Type, info) { return t, info{} case reflect.Array, reflect.Slice: - if isInteger(prop) || isAny(prop) { - // ok, we normally expect an integer or interface that could be one - } else { + if !isInteger(prop) && !isAny(prop) { return v.error(node.Property, "array elements can only be selected using an integer (got %v)", prop) } t, c := deref(base.Elem()) From e5303ca3e2865ffe0fb44a695ed7a0f73fc65a61 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 11 Jul 2023 21:16:42 +0100 Subject: [PATCH 17/23] Implement env keyword in IdentifierNode with OpLoadEnv --- compiler/compiler.go | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/compiler/compiler.go b/compiler/compiler.go index f2dfe15ff..0afe31731 100644 --- a/compiler/compiler.go +++ b/compiler/compiler.go @@ -205,6 +205,10 @@ func (c *compiler) NilNode(_ *ast.NilNode) { } func (c *compiler) IdentifierNode(node *ast.IdentifierNode) { + if node.Value == "env" { + c.emit(OpLoadEnv) + return + } if c.mapEnv { c.emit(OpLoadFast, c.addConstant(node.Value)) } else if len(node.FieldIndex) > 0 { @@ -491,26 +495,26 @@ func (c *compiler) MemberNode(node *ast.MemberNode) { } } - // Implementation of direct env access - if ident, ok := base.(*ast.IdentifierNode); ok && ident.Value == "env" { + // Implementation of direct env access - moved to identifier node + /* if ident, ok := base.(*ast.IdentifierNode); ok && ident.Value == "env" { c.compile(node.Property) c.emit(OpFetchEnv) - } else { - c.compile(base) - if node.Optional { - ph := c.emit(OpJumpIfNil, placeholder) - c.chains[len(c.chains)-1] = append(c.chains[len(c.chains)-1], ph) - } + } else { */ + c.compile(base) + if node.Optional { + ph := c.emit(OpJumpIfNil, placeholder) + c.chains[len(c.chains)-1] = append(c.chains[len(c.chains)-1], ph) + } - if op == OpFetch { - c.compile(node.Property) - c.emit(OpFetch) - } else { - c.emitLocation(node.Location(), op, c.addConstant( - &runtime.Field{Index: index, Path: path}, - )) - } + if op == OpFetch { + c.compile(node.Property) + c.emit(OpFetch) + } else { + c.emitLocation(node.Location(), op, c.addConstant( + &runtime.Field{Index: index, Path: path}, + )) } + /* } */ deref: if original.Deref { From f45da29bf20ceed5b46afba401cde4402ba49b66 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 11 Jul 2023 21:17:44 +0100 Subject: [PATCH 18/23] Removed unused code --- compiler/compiler.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/compiler/compiler.go b/compiler/compiler.go index 0afe31731..423dd6217 100644 --- a/compiler/compiler.go +++ b/compiler/compiler.go @@ -495,11 +495,6 @@ func (c *compiler) MemberNode(node *ast.MemberNode) { } } - // Implementation of direct env access - moved to identifier node - /* if ident, ok := base.(*ast.IdentifierNode); ok && ident.Value == "env" { - c.compile(node.Property) - c.emit(OpFetchEnv) - } else { */ c.compile(base) if node.Optional { ph := c.emit(OpJumpIfNil, placeholder) @@ -514,7 +509,6 @@ func (c *compiler) MemberNode(node *ast.MemberNode) { &runtime.Field{Index: index, Path: path}, )) } - /* } */ deref: if original.Deref { From e4130f357d836dc27434083dfd8ac5f6abd649a7 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 11 Jul 2023 21:20:53 +0100 Subject: [PATCH 19/23] Add OpLoadEnv, remove OpFetchEnv --- vm/opcodes.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vm/opcodes.go b/vm/opcodes.go index bb2bdd884..63b9f8a30 100644 --- a/vm/opcodes.go +++ b/vm/opcodes.go @@ -11,9 +11,9 @@ const ( OpLoadFast OpLoadMethod OpLoadFunc + OpLoadEnv OpFetch OpFetchField - OpFetchEnv OpMethod OpTrue OpFalse From 1cdb7469b6d9ba568159382b7a58a9886a3ecc03 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 11 Jul 2023 21:22:02 +0100 Subject: [PATCH 20/23] Add OpLoadEnv, remove OpFetchEnv --- vm/vm.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/vm/vm.go b/vm/vm.go index 6046bb7a6..3e5411b1f 100644 --- a/vm/vm.go +++ b/vm/vm.go @@ -123,13 +123,8 @@ func (vm *VM) Run(program *Program, env interface{}) (_ interface{}, err error) a := vm.pop() vm.push(runtime.FetchField(a, program.Constants[arg].(*runtime.Field))) - case OpFetchEnv: - b := vm.pop() - if _, ok := b.(string); !ok { - panic("index for env[] keyord must be string") - } - vm.push(runtime.Fetch(env, b)) - + case OpLoadEnv: + vm.push(env) case OpMethod: a := vm.pop() vm.push(runtime.FetchMethod(a, program.Constants[arg].(*runtime.Method))) From b1fe2613fd3b318430819d22a2edf6b681a43943 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 11 Jul 2023 21:22:41 +0100 Subject: [PATCH 21/23] Check for incorrect use of env keyword --- conf/types_table.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/conf/types_table.go b/conf/types_table.go index e917f5fa8..f4d401c9c 100644 --- a/conf/types_table.go +++ b/conf/types_table.go @@ -54,6 +54,9 @@ func CreateTypesTable(i interface{}) TypesTable { for _, key := range v.MapKeys() { value := v.MapIndex(key) if key.Kind() == reflect.String && value.IsValid() && value.CanInterface() { + if key.String() == "env" { // Could check for all keywords here + panic("attempt to misuse env keyword as env map key") + } types[key.String()] = Tag{Type: reflect.TypeOf(value.Interface())} } } @@ -94,10 +97,13 @@ func FieldsFromStruct(t reflect.Type) TypesTable { } } } - - types[FieldName(f)] = Tag{ - Type: f.Type, - FieldIndex: f.Index, + if fn := FieldName(f); fn == "env" { // Could check for all keywords here + panic("attempt to misuse env keyword as env struct field tag") + } else { + types[FieldName(f)] = Tag{ + Type: f.Type, + FieldIndex: f.Index, + } } } } From 84d3631769f3099010b4e1341dafd006b2dd4829 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 11 Jul 2023 21:23:41 +0100 Subject: [PATCH 22/23] Remove obsolete tests --- expr_test.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/expr_test.go b/expr_test.go index 78f75b0e3..7b325ff75 100644 --- a/expr_test.go +++ b/expr_test.go @@ -1822,9 +1822,6 @@ func TestEnv_keyword(t *testing.T) { "%*worst function name ever!!": func() string { return "ok" }(), - "env": func(string) string { - return "num" - }, "1": "o", "2": "k", "num": 10, @@ -1838,6 +1835,10 @@ func TestEnv_keyword(t *testing.T) { }, "red": "n", "irect": "um", + "String Map": map[string]string{ + "one": "two", + "three": "four", + }, } // No error cases @@ -1849,13 +1850,13 @@ func TestEnv_keyword(t *testing.T) { {"env['Section 1-2a']", "ok"}, {`env["c:\\ndrive\\2015 Information Table"]`, "ok"}, {"env['%*worst function name ever!!']", "ok"}, - {"env('Confusing!')", "num"}, // not keyword, but some function named env + {"env['String Map'].one", "two"}, {"env['1'] + env['2']", "ok"}, {"1 + env['num'] + env['num']", 21}, {"MIN(env['num'],0)", 0}, {"env['nu' + 'm']", 10}, {"env[red + irect]", 10}, - {"env[env('na')]", 10}, + {"env['String Map']?.five", ""}, } for _, tt := range tests { @@ -1885,15 +1886,11 @@ func TestEnv_keyword(t *testing.T) { code string want interface{} }{ - {"env[this is a problem", "bad"}, - {"env['also a problem'", "bad"}, - {"env[space test]", "bad"}, - {"env[1] + env[2]", "bad"}, + {"env()", "bad"}, } for _, tt := range tests { t.Run(tt.code, func(t *testing.T) { - _, err := expr.Eval(tt.code, expr.Env(env)) require.Error(t, err, "compile error") From fee4852e52a3b9422ec2e8e9eb4e2d66a7170184 Mon Sep 17 00:00:00 2001 From: matt mcConnell Date: Tue, 11 Jul 2023 21:54:16 +0100 Subject: [PATCH 23/23] Added tests for env. and env?. --- expr_test.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/expr_test.go b/expr_test.go index 7b325ff75..14cbf7415 100644 --- a/expr_test.go +++ b/expr_test.go @@ -1822,10 +1822,10 @@ func TestEnv_keyword(t *testing.T) { "%*worst function name ever!!": func() string { return "ok" }(), - "1": "o", - "2": "k", - "num": 10, - "my list": []int{1, 2, 3, 4, 5}, + "1": "o", + "2": "k", + "num": 10, + "mylist": []int{1, 2, 3, 4, 5}, "MIN": func(a, b int) int { if a < b { return a @@ -1839,6 +1839,10 @@ func TestEnv_keyword(t *testing.T) { "one": "two", "three": "four", }, + "OtherMap": map[string]string{ + "a": "b", + "c": "d", + }, } // No error cases @@ -1857,6 +1861,11 @@ func TestEnv_keyword(t *testing.T) { {"env['nu' + 'm']", 10}, {"env[red + irect]", 10}, {"env['String Map']?.five", ""}, + {"env.red", "n"}, + {"env?.blue", nil}, + {"env.mylist[1]", 2}, + {"env?.OtherMap?.a", "b"}, + {"env?.OtherMap?.d", ""}, } for _, tt := range tests {