Adding del() to remove map entries (e.g del(x[y]) / del(x.s)) or variables; optimize small map Set (#302)

* Adding del() to remove map entries (e.g del(x[y])) or variables

* linter

* Implement the regular identifier delete

* Fix bug in shell where result map wasn't sorted, now caused failure thanks to optimization in small map get

* handle the . case for delete, share code

* slight improvement in comment/debug

* It's better when I don't forget to git add new files

* del() implies no caching/memoization. added test for that and some negative cases too

Author: ldemailly

Diff for: eval/eval.go

@@ -420,6 +420,64 @@ func (s *State) evalPrintLogError(node *ast.Builtin) object.Object {
 
 var ErrorKey = object.String{Value: "err"} // can't use error as that's a builtin.
 
+func (s *State) evalDelete(node ast.Node) object.Object {
+	s.env.TriggerNoCache()
+	switch node.Value().Type() {
+	case token.IDENT:
+		name := node.Value().Literal()
+		if name == "" {
+			return s.NewError("delete empty identifier")
+		}
+		return s.env.Delete(name)
+	case token.DOT:
+		idxE := node.(*ast.IndexExpression)
+		// index is the string value and not an identifier to resolve.
+		key := idxE.Index.Value()
+		if key.Type() != token.STRING && key.Type() != token.IDENT {
+			return s.Errorf("del expression with . not a string: %s", key.Literal())
+		}
+		index := object.String{Value: key.Literal()}
+		return s.deleteMapEntry(idxE, index)
+	case token.LBRACKET:
+		// Map/array [] index
+		idxE := node.(*ast.IndexExpression)
+		index := s.Eval(idxE.Index)
+		if index.Type() == object.ERROR {
+			return index
+		}
+		return s.deleteMapEntry(idxE, index)
+	default:
+		return s.NewError("delete not supported on " + node.Value().Type().String())
+	}
+}
+
+func (s *State) deleteMapEntry(idxE *ast.IndexExpression, index object.Object) object.Object {
+	if idxE.Left.Value().Type() != token.IDENT {
+		return s.NewError("delete index on non identifier: " + idxE.Left.Value().DebugString())
+	}
+	id := idxE.Left.Value().Literal()
+	obj, ok := s.env.Get(id)
+	if !ok {
+		// Nothing to delete, we're done
+		return object.FALSE
+	}
+	// TODO: handle arrays too? though delete arr[idx] == arr[0:idx]+arr[idx+1:] so... no point
+	if obj.Type() != object.MAP {
+		return s.NewError("delete index on non map: " + id + " " + obj.Type().String())
+	}
+	log.LogVf("remove map: %s from %s", index.Inspect(), id)
+	m := obj.(object.Map)
+	m, changed := m.Delete(index)
+	if !changed {
+		return object.FALSE
+	}
+	oerr := s.env.Set(id, m)
+	if oerr.Type() == object.ERROR {
+		return oerr
+	}
+	return object.TRUE
+}
+
 func (s *State) evalBuiltin(node *ast.Builtin) object.Object {
 	// all take 1 arg exactly except print and log which take 1+.
 	t := node.Type()
@@ -432,8 +490,13 @@ func (s *State) evalBuiltin(node *ast.Builtin) object.Object {
 	if oerr := argCheck(s, node.Literal(), minV, varArg, node.Parameters); oerr != nil {
 		return *oerr
 	}
-	if t == token.QUOTE {
+	// builtins that don't eval arguments (quote, del)
+	switch t {
+	case token.QUOTE:
 		return s.quote(node.Parameters[0])
+	case token.DEL:
+		return s.evalDelete(node.Parameters[0])
+	default:
 	}
 	var val object.Object
 	var rt object.Type

Diff for: extensions/shell.go

@@ -49,8 +49,9 @@ func createShellFunctions() {
 				cmd.Stdin = bytes.NewReader(obj)
 			}
 			err := cmd.Run()
-			res := object.MakeQuad(stdout, object.String{Value: sout.String()},
-				stderr, object.String{Value: serr.String()})
+			// keys must be sorted. stdErr before stdOut.
+			res := object.MakeQuad(stderr, object.String{Value: serr.String()},
+				stdout, object.String{Value: sout.String()})
 			if err != nil {
 				res = res.Set(eval.ErrorKey, object.String{Value: err.Error()})
 			} else {

Diff for: object/object.go

@@ -247,12 +247,17 @@ func CompareKeys(a, b keyValuePair) int {
 }
 
 func (m *BigMap) Get(key Object) (Object, bool) {
+	v, found, _ := m.get(key)
+	return v, found
+}
+
+func (m *BigMap) get(key Object) (Object, bool, int) {
 	kv := keyValuePair{Key: key}
 	i, ok := slices.BinarySearchFunc(m.kv, kv, CompareKeys) // log(n) search as we keep it sorted.
 	if !ok {
-		return NULL, false
+		return NULL, false, i
 	}
-	return m.kv[i].Value, true
+	return m.kv[i].Value, true, i
 }
 
 func (m *BigMap) Set(key, value Object) Map {
@@ -325,6 +330,16 @@ func (m *BigMap) Range(l, r int64) Object {
 	return res
 }
 
+func (m *BigMap) Delete(key Object) (Map, bool) {
+	_, found, idx := m.get(key)
+	if !found {
+		return m, false
+	}
+	copy(m.kv[idx:], m.kv[idx+1:])
+	m.kv = m.kv[:len(m.kv)-1]
+	return m, true
+}
+
 func NewMapSize(size int) Map {
 	if size <= MaxSmallMap {
 		return SmallMap{}
@@ -333,30 +348,46 @@ func NewMapSize(size int) Map {
 }
 
 func (m SmallMap) Get(key Object) (Object, bool) {
+	r, found, _ := m.get(key)
+	return r, found
+}
+
+// return the index where the key if not found would be inserted at,
+// replaces slices.BinarySearchFunc(m.smallKV[:m.len], kv, CompareKeys) for small maps.
+func (m SmallMap) get(key Object) (Object, bool, int) {
 	for i := range m.len {
-		if Cmp(m.smallKV[i].Key, key) == 0 {
-			return m.smallKV[i].Value, true
+		c := Cmp(m.smallKV[i].Key, key)
+		switch c {
+		case 1:
+			return NULL, false, i
+		case 0:
+			return m.smallKV[i].Value, true, i
 		}
 	}
-	return NULL, false
+	return NULL, false, m.len
 }
 
 func (m SmallMap) Set(key, value Object) Map {
-	kv := keyValuePair{Key: key, Value: value}
-	i, ok := slices.BinarySearchFunc(m.smallKV[:m.len], kv, CompareKeys)
+	_, ok, i := m.get(key) // slices.BinarySearchFunc(m.smallKV[:m.len], kv, CompareKeys)
 	if ok {
 		m.smallKV[i].Value = value
 		return m
 	}
-	r := slices.Insert(m.smallKV[0:m.len:MaxSmallMap], i, kv)
-	if m.len < MaxSmallMap {
-		copy(m.smallKV[0:MaxSmallMap], r)
-		m.len++
-		return m
+	m.len++
+	if m.len > MaxSmallMap {
+		// We need to switch to a big map.
+		res := &BigMap{kv: make([]keyValuePair, 0, m.len)}
+		res.kv = append(res.kv, m.smallKV[:i]...)
+		res.kv = append(res.kv, keyValuePair{Key: key, Value: value})
+		res.kv = append(res.kv, m.smallKV[i:m.len-1]...)
+		return res
 	}
-	// We need to switch to a real map.
-	res := &BigMap{kv: r}
-	return res
+	// create the space for the new key.
+	for j := m.len - 1; j > i; j-- {
+		m.smallKV[j] = m.smallKV[j-1]
+	}
+	m.smallKV[i] = keyValuePair{Key: key, Value: value}
+	return m
 }
 
 func (m SmallMap) Len() int {
@@ -385,6 +416,18 @@ func (m SmallMap) Range(l, r int64) Object {
 	return res
 }
 
+func (m SmallMap) Delete(key Object) (Map, bool) {
+	_, found, where := m.get(key)
+	if !found {
+		return m, false
+	}
+	for i := where; i < m.len-1; i++ {
+		m.smallKV[i] = m.smallKV[i+1]
+	}
+	m.len--
+	return m, true
+}
+
 func (m SmallMap) Append(right Map) Map {
 	if right.Len() <= MaxSmallMap { // Maybe same keys, try to keep it a SmallMap
 		res := SmallMap{len: m.len}
@@ -929,6 +972,7 @@ type Map interface {
 	Rest() Object
 	mapElements() []keyValuePair
 	Append(right Map) Map
+	Delete(key Object) (Map, bool)
 }
 
 func NewMap() Map {

Diff for: object/state.go

@@ -264,6 +264,23 @@ func (e *Environment) Get(name string) (Object, bool) {
 	return nil, false
 }
 
+// Delete removes the first entry found under that name from the environment.
+// TODO: check if references need special handling.
+func (e *Environment) Delete(name string) Object {
+	if e.depth == 0 {
+		e.numSet++
+	}
+	if _, ok := e.store[name]; ok {
+		delete(e.store, name)
+		log.Debugf("Delete(%s) found at %d %v", name, e.depth, e.cacheKey)
+		return TRUE
+	}
+	if e.outer != nil {
+		return e.outer.Delete(name)
+	}
+	return FALSE
+}
+
 // TriggerNoCache is used prevent this call stack from caching.
 // Meant to be used by extensions that for instance return random numbers or change state.
 func (e *Environment) TriggerNoCache() {

Diff for: parser/parser.go

@@ -94,6 +94,7 @@ func New(l *lexer.Lexer) *Parser {
 	p.registerPrefix(token.CATCH, p.parseBuiltin)
 	p.registerPrefix(token.QUOTE, p.parseBuiltin)
 	p.registerPrefix(token.UNQUOTE, p.parseBuiltin)
+	p.registerPrefix(token.DEL, p.parseBuiltin)
 
 	p.infixParseFns = make(map[token.Type]infixParseFn)
 	p.registerInfix(token.PLUS, p.parseInfixExpression)

Diff for: tests/delete.gr

@@ -0,0 +1,55 @@
+
+smallmap:={1:1,2:2,3:3,4:4}
+
+i:=3
+log("before del", smallmap)
+Assert("first delete should be true", del(smallmap[i-1]))
+log("after del", i-1, smallmap)
+// 2nd delete is false/noop
+Assert("second delete should be false", del(smallmap[i-1])==false)
+Assert("smallmap should be as expected", smallmap == {1:1,3:3,4:4})
+
+Assert("first del i should be true", del(i))
+Assert("second del i should be false", del(i)==false)
+IsErr("i shouldn't exist anymore", i, "identifier not found: i")
+
+largemap:=smallmap + {5:5,6:6,7:7,8:8,9:9,10:10}
+log("before del large", largemap)
+Assert("first delete should be true", del(largemap[10]))
+log("after del large", largemap)
+Assert("2nd delete should be false", del(largemap[10])==false)
+Assert("largemap should be as expected", largemap == {1:1,3:3,4:4,5:5,6:6,7:7,8:8,9:9})
+Assert("first delete should be true", del(largemap[7]))
+log("after del large", largemap)
+Assert("largemap should be as expected", largemap == {1:1,3:3,4:4,5:5,6:6,8:8,9:9})
+
+Assert("deleting whole map", del(largemap))
+IsErr("largemap shouldn't exist anymore", largemap, "identifier not found: largemap")
+
+// string . access syntax
+strmap := {"foo":"bar", "baz":"quux"}
+log("before del str", strmap)
+Assert("first delete should be true", del(strmap.foo))
+log("after del str", strmap)
+Assert("2nd delete should be false", del(strmap.foo)==false)
+Assert("strmap should be as expected", strmap == {"baz":"quux"})
+
+x="outer"
+func delTest(x) {
+	log("before del x", x)
+	del(x)
+	log("after del x", x)
+	x
+}
+
+Assert("delTest(str) should return outer", delTest("foo") == "outer")
+IsErr("can't (for now) delete registers", delTest(3), "delete not supported on REGISTER")
+IsErr("del needs to be of identifier/map", del(1+2), "delete not supported on PLUS")
+
+func delTest() {
+	del(x)
+}
+
+Assert("delTest() should be true first time", delTest())
+// Without no cache this wasn't working
+Assert("delTest() should be false second time", delTest() == false)

Diff for: token/token.go

@@ -145,6 +145,7 @@ const (
 	LOG
 	ERROR
 	CATCH
+	DEL
 
 	endIdentityTokens