From 3e4d79f5eeacd8ea5a18af28ece514795f3bbded Mon Sep 17 00:00:00 2001
From: Paul Buetow <paul@buetow.org>
Date: Mon, 23 Mar 2026 21:51:31 +0200
Subject: internal/rpn: fix error handling, variable name validation, and
 locking issues

- Use error wrapping with ErrVariableNotFound for consistent error checking
- Add isValidVariableName() for comprehensive variable name validation
- Fix race condition in ClearVariables() by clearing instead of replacing map
- Eliminate double-locking in FormatVariables() with internal helper function
---
 internal/rpn/operations.go      |  4 ++--
 internal/rpn/operations_test.go |  5 +++--
 internal/rpn/variables.go       | 44 ++++++++++++++++++++++++++++++++++++-----
 3 files changed, 44 insertions(+), 9 deletions(-)

(limited to 'internal')

diff --git a/internal/rpn/operations.go b/internal/rpn/operations.go
index 1ebd78b..cab77db 100644
--- a/internal/rpn/operations.go
+++ b/internal/rpn/operations.go
@@ -419,7 +419,7 @@ func (o *Operations) UseVariable(stack *Stack, name string) error {
 
 	val, exists := o.vars.GetVariable(name)
 	if !exists {
-		return fmt.Errorf("undefined variable: %s", name)
+		return fmt.Errorf("%w: %s", ErrVariableNotFound, name)
 	}
 
 	stack.Push(val)
@@ -435,7 +435,7 @@ func (o *Operations) DeleteVariable(name string) error {
 
 	deleted := o.vars.DeleteVariable(name)
 	if !deleted {
-		return fmt.Errorf("undefined variable: %s", name)
+		return fmt.Errorf("%w: %s", ErrVariableNotFound, name)
 	}
 	return nil
 }
diff --git a/internal/rpn/operations_test.go b/internal/rpn/operations_test.go
index 7aa6a32..f548317 100644
--- a/internal/rpn/operations_test.go
+++ b/internal/rpn/operations_test.go
@@ -1,6 +1,7 @@
 package rpn
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 	"testing"
@@ -456,8 +457,8 @@ func TestOperationsUseVariableUndefined(t *testing.T) {
 	if err == nil {
 		t.Error("UseVariable for undefined variable should return error")
 	}
-	if !strings.Contains(err.Error(), "undefined variable") {
-		t.Errorf("UseVariable error = %v, should contain 'undefined variable'", err)
+	if !errors.Is(err, ErrVariableNotFound) {
+		t.Errorf("UseVariable error = %v, should be ErrVariableNotFound", err)
 	}
 }
 
diff --git a/internal/rpn/variables.go b/internal/rpn/variables.go
index 69f5b18..0f39a92 100644
--- a/internal/rpn/variables.go
+++ b/internal/rpn/variables.go
@@ -100,10 +100,24 @@ func NewVariables() VariableStore {
 	}
 }
 
+// isValidVariableName checks if a variable name is valid.
+// Variable names must be non-empty and contain only alphanumeric characters and underscores.
+func isValidVariableName(name string) bool {
+	if name == "" {
+		return false
+	}
+	for _, r := range name {
+		if !('a' <= r && r <= 'z' || 'A' <= r && r <= 'Z' || '0' <= r && r <= '9' || r == '_') {
+			return false
+		}
+	}
+	return true
+}
+
 // SetVariable assigns a value to a variable name.
 // Usage: `name value =` stores value in variable.
 func (v *Variables) SetVariable(name string, value float64) error {
-	if name == "" {
+	if !isValidVariableName(name) {
 		return ErrInvalidVariableName
 	}
 
@@ -161,12 +175,24 @@ func (v *Variables) ClearVariables() {
 	v.mu.Lock()
 	defer v.mu.Unlock()
 
-	v.variables = make(map[string]float64)
+	for k := range v.variables {
+		delete(v.variables, k)
+	}
 }
 
-// FormatVariables formats all variables for display.
-func (v *Variables) FormatVariables() string {
-	infos := v.ListVariables()
+// formatVariablesUnsafe returns a list of variable info without acquiring a lock.
+// The caller must hold a read lock.
+func (v *Variables) formatVariablesUnsafe() string {
+	var infos []VariableInfo
+	for name, value := range v.variables {
+		infos = append(infos, VariableInfo{Name: name, Value: value})
+	}
+
+	// Sort by name for consistent output
+	sort.Slice(infos, func(i, j int) bool {
+		return infos[i].Name < infos[j].Name
+	})
+
 	if len(infos) == 0 {
 		return "No variables defined"
 	}
@@ -181,6 +207,14 @@ func (v *Variables) FormatVariables() string {
 	return sb.String()
 }
 
+// FormatVariables formats all variables for display.
+func (v *Variables) FormatVariables() string {
+	v.mu.RLock()
+	defer v.mu.RUnlock()
+
+	return v.formatVariablesUnsafe()
+}
+
 // Count returns the number of defined variables.
 func (v *Variables) Count() int {
 	v.mu.RLock()
-- 
cgit v1.2.3