From 8e1262fc25138d3c5f90d95e7f163a9116ce2e5b Mon Sep 17 00:00:00 2001 From: jaideeppyne Date: Fri, 21 Aug 2026 14:40:08 +0530 Subject: [PATCH 1/2] fix(conf): deref pointer-to-map environment instead of panicking EnvWithCache selected the map branch using the dereferenced value's kind, but then read the map length and keys from the original (pointer) value. Passing a *map as the env therefore panicked with: reflect: call of reflect.Value.Len on ptr to non-array Value Iterate the dereferenced value in the map branch so a pointer-to-map env is treated exactly like the map it points to (same strict mode and element types), matching the existing behaviour for pointer-to-struct environments. Fixes #825 Co-Authored-By: Claude Opus 4.8 Signed-off-by: jaideeppyne --- conf/env.go | 12 +++++---- test/issues/825/issue_test.go | 48 +++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 5 deletions(-) create mode 100644 test/issues/825/issue_test.go diff --git a/conf/env.go b/conf/env.go index 0acd44570..da4ac0829 100644 --- a/conf/env.go +++ b/conf/env.go @@ -32,22 +32,24 @@ func EnvWithCache(c *Cache, env any) Nature { v := reflect.ValueOf(env) t := v.Type() - switch deref.Value(v).Kind() { + d := deref.Value(v) + + switch d.Kind() { case reflect.Struct: n := c.FromType(t) n.Strict = true return n case reflect.Map: - n := c.FromType(v.Type()) + n := c.FromType(d.Type()) if n.TypeData == nil { n.TypeData = new(TypeData) } n.Strict = true - n.Fields = make(map[string]Nature, v.Len()) + n.Fields = make(map[string]Nature, d.Len()) - for _, key := range v.MapKeys() { - elem := v.MapIndex(key) + for _, key := range d.MapKeys() { + elem := d.MapIndex(key) if !elem.IsValid() || !elem.CanInterface() { panic(fmt.Sprintf("invalid map value: %s", key)) } diff --git a/test/issues/825/issue_test.go b/test/issues/825/issue_test.go new file mode 100644 index 000000000..3cb871760 --- /dev/null +++ b/test/issues/825/issue_test.go @@ -0,0 +1,48 @@ +package issue_test + +import ( + "testing" + + "github.com/expr-lang/expr" + "github.com/expr-lang/expr/internal/testify/assert" + "github.com/expr-lang/expr/internal/testify/require" +) + +// TestIssue825 verifies that passing a pointer to a map as the environment is +// dereferenced instead of panicking. +// +// conf.EnvWithCache selected the map branch using the dereferenced value's +// kind, but then read the map keys/length from the original (pointer) value, +// panicking with: +// +// reflect: call of reflect.Value.Len on ptr to non-array Value +func TestIssue825(t *testing.T) { + m := map[string]any{"foo": 42} + + program, err := expr.Compile("foo + 1", expr.Env(&m)) + require.NoError(t, err) + + out, err := expr.Run(program, m) + require.NoError(t, err) + assert.Equal(t, 43, out) +} + +// TestIssue825_Strict verifies that a pointer-to-map env keeps the strict-mode +// and element-type information of the dereferenced map, i.e. it behaves exactly +// like compiling with the map value itself. +func TestIssue825_Strict(t *testing.T) { + m := map[string]int{"a": 1} + + // Unknown names are rejected (strict), just like a plain map env. + _, err := expr.Compile("unknown + 1", expr.Env(&m)) + require.Error(t, err) + require.Contains(t, err.Error(), "unknown name unknown") + + // The element type (int) is inferred from the dereferenced map. + program, err := expr.Compile("a + 1", expr.Env(&m)) + require.NoError(t, err) + + out, err := expr.Run(program, m) + require.NoError(t, err) + assert.Equal(t, 2, out) +} From 0e542063afe0dce35e2d7a5e315c08de49ab0ed7 Mon Sep 17 00:00:00 2001 From: jaideeppyne Date: Tue, 25 Aug 2026 05:11:22 +0530 Subject: [PATCH 2/2] fix(conf): reject pointer-to-map environment with a clear error Per #825 (wontfix for deref-support): rather than dereferencing a *map env, reject it with a descriptive message instead of the opaque "reflect: call of reflect.Value.Len on ptr to non-array Value" panic. Passing a map by value is unaffected. --- conf/env.go | 19 ++++++++++------ test/issues/825/issue_test.go | 41 +++++++++++++++-------------------- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/conf/env.go b/conf/env.go index da4ac0829..9994090df 100644 --- a/conf/env.go +++ b/conf/env.go @@ -32,24 +32,29 @@ func EnvWithCache(c *Cache, env any) Nature { v := reflect.ValueOf(env) t := v.Type() - d := deref.Value(v) - - switch d.Kind() { + switch deref.Value(v).Kind() { case reflect.Struct: n := c.FromType(t) n.Strict = true return n case reflect.Map: - n := c.FromType(d.Type()) + // A pointer to a map is not supported as an environment (see #825). + // Reject it with a clear message instead of dereferencing it or + // panicking deep inside reflect. + if v.Kind() == reflect.Ptr { + panic(fmt.Sprintf("environment must be a map, not a pointer to a map: %s", t)) + } + + n := c.FromType(v.Type()) if n.TypeData == nil { n.TypeData = new(TypeData) } n.Strict = true - n.Fields = make(map[string]Nature, d.Len()) + n.Fields = make(map[string]Nature, v.Len()) - for _, key := range d.MapKeys() { - elem := d.MapIndex(key) + for _, key := range v.MapKeys() { + elem := v.MapIndex(key) if !elem.IsValid() || !elem.CanInterface() { panic(fmt.Sprintf("invalid map value: %s", key)) } diff --git a/test/issues/825/issue_test.go b/test/issues/825/issue_test.go index 3cb871760..2528ba167 100644 --- a/test/issues/825/issue_test.go +++ b/test/issues/825/issue_test.go @@ -9,40 +9,35 @@ import ( ) // TestIssue825 verifies that passing a pointer to a map as the environment is -// dereferenced instead of panicking. +// rejected with a clear message instead of panicking deep inside reflect. // -// conf.EnvWithCache selected the map branch using the dereferenced value's -// kind, but then read the map keys/length from the original (pointer) value, -// panicking with: +// Supporting *map by dereferencing it was declined by the maintainer (#825 is +// labeled wontfix); the agreed direction was to reject *map with an error +// message. Previously conf.EnvWithCache selected the map branch on the +// dereferenced kind but then read the map keys/length from the original +// (pointer) value, panicking with the opaque: // // reflect: call of reflect.Value.Len on ptr to non-array Value func TestIssue825(t *testing.T) { m := map[string]any{"foo": 42} - program, err := expr.Compile("foo + 1", expr.Env(&m)) - require.NoError(t, err) - - out, err := expr.Run(program, m) - require.NoError(t, err) - assert.Equal(t, 43, out) + assert.PanicsWithValue(t, + "environment must be a map, not a pointer to a map: *map[string]interface {}", + func() { + _, _ = expr.Compile("foo > 0", expr.Env(&m)) + }, + ) } -// TestIssue825_Strict verifies that a pointer-to-map env keeps the strict-mode -// and element-type information of the dereferenced map, i.e. it behaves exactly -// like compiling with the map value itself. -func TestIssue825_Strict(t *testing.T) { - m := map[string]int{"a": 1} - - // Unknown names are rejected (strict), just like a plain map env. - _, err := expr.Compile("unknown + 1", expr.Env(&m)) - require.Error(t, err) - require.Contains(t, err.Error(), "unknown name unknown") +// TestIssue825_MapStillWorks guards the common case: a map passed by value is +// unaffected and continues to work exactly as before. +func TestIssue825_MapStillWorks(t *testing.T) { + m := map[string]any{"foo": 42} - // The element type (int) is inferred from the dereferenced map. - program, err := expr.Compile("a + 1", expr.Env(&m)) + program, err := expr.Compile("foo + 1", expr.Env(m)) require.NoError(t, err) out, err := expr.Run(program, m) require.NoError(t, err) - assert.Equal(t, 2, out) + assert.Equal(t, 43, out) }