Documentation formatting, test coverage, check before mutating frozen…

… sets

doc/spec.md

@@ -3756,7 +3756,12 @@ x.remove(2)                             # error: element not found
 <a id='set·add'></a>
 ### set·add
 
-`S.add(x)` adds the value `x` to the set `S`. This has no effect if `x` is already in `S`.
+If `x` is not an element of set `S`, `S.add(x)` adds it to the set or fails if the set is frozen.
+If `x` already an element of the set, `add(x)` has no effect.
+
+`add` fails if the set does not contain `x` and is frozen.
+
+It returns None.
 
 ```python
 x = set([1, 2])
@@ -3767,27 +3772,30 @@ x.add(3)                             # None (x == set([1, 2, 3]))
 <a id='set·clear'></a>
 ### set·clear
 
-`S.clear()` removes all items from the set and returns None.
+`S.clear()` removes all items from the set and fails if the set is non-empty and frozen.
 
-`clear` fails if the set is frozen.
+It returns None.
 
 ```python
 x = set([1, 2, 3])
-x.discard(2)                             # None (x == set([1, 3]))
-x.discard(2)                             # None (x == set([1, 3]))
+x.clear(2)                               # None
+x                                        # set([])
 ```
 
 <a id='set·discard'></a>
 ### set·discard
 
-`S.discard(x)` removes `x` from the set and returns None.
+If `x` is an element of set `S`, `S.discard(x)` removes `x` from the set, or fails if the
+set is frozen. If `x` is not an element of the set, discard has no effect.
 
-`discard` fails if the set is frozen.
+It returns None.
 
 ```python
 x = set([1, 2, 3])
-x.discard(2)                             # None (x == set([1, 3]))
-x.discard(2)                             # None (x == set([1, 3]))
+x.discard(2)                             # None
+x                                        # set([1, 3])
+x.discard(2)                             # None
+x                                        # set([1, 3])
 ```
 
 <a id='set·pop'></a>
@@ -3813,7 +3821,8 @@ x.pop()                                 # error: empty set
 
 ```python
 x = set([1, 2, 3])
-x.remove(2)                             # None (x == set([1, 3]))
+x.remove(2)                             # None
+x                                       # set([1, 3])
 x.remove(2)                             # error: element not found
 ```
 

starlark/library.go

@@ -2179,6 +2179,11 @@ func set_add(_ *Thread, b *Builtin, args Tuple, kwargs []Tuple) (Value, error) {
 	if err := UnpackPositionalArgs(b.Name(), args, kwargs, 1, &elem); err != nil {
 		return nil, err
 	}
+	if found, err := b.Receiver().(*Set).Has(elem); err != nil {
+		return nil, nameErr(b, err)
+	} else if found {
+		return None, nil
+	}
 	err := b.Receiver().(*Set).Insert(elem)
 	if err != nil {
 		return nil, nameErr(b, err)
@@ -2191,6 +2196,9 @@ func set_clear(_ *Thread, b *Builtin, args Tuple, kwargs []Tuple) (Value, error)
 	if err := UnpackPositionalArgs(b.Name(), args, kwargs, 0); err != nil {
 		return nil, err
 	}
+	if b.Receiver().(*Set).Len() == 0 { // clear on an empty set is non-mutating
+		return None, nil
+	}
 	err := b.Receiver().(*Set).Clear()
 	if err != nil {
 		return nil, nameErr(b, err)
@@ -2204,6 +2212,11 @@ func set_discard(_ *Thread, b *Builtin, args Tuple, kwargs []Tuple) (Value, erro
 	if err := UnpackPositionalArgs(b.Name(), args, kwargs, 1, &k); err != nil {
 		return nil, err
 	}
+	if found, err := b.Receiver().(*Set).Has(k); err != nil {
+		return nil, nameErr(b, err)
+	} else if !found {
+		return None, nil
+	}
 	if _, err := b.Receiver().(*Set).Delete(k); err != nil {
 		return nil, nameErr(b, err) // dict is frozen or key is unhashable
 	}

starlark/testdata/set.star

@@ -15,7 +15,7 @@
 # - set += iterable, perhaps?
 # Test iterator invalidation.
 
-load("assert.star", "assert")
+load("assert.star", "assert", "freeze")
 
 # literals
 # Parser does not currently support {1, 2, 3}.
@@ -118,25 +118,50 @@ assert.eq(iter(), [1, 2, 3])
 assert.fails(lambda : x[0], "unhandled.*operation")
 
 # adding and removing
-x.add(4)
-assert.true(4 in x)
-
-# remove 
-x.remove(4)
-assert.true(4 not in x)
-assert.fails(lambda: x.remove(4), "remove: missing key")
+add_set = set([1,2,3])
+add_set.add(4)
+assert.true(4 in add_set)
+freeze(add_set) # no mutation of frozen set because key already present
+add_set.add(4)
+assert.fails(lambda: add_set.add(5), "add: cannot insert into frozen hash table")
+
+# remove
+remove_set = set([1,2,3])
+remove_set.remove(3)
+assert.true(3 not in remove_set)
+assert.fails(lambda: remove_set.remove(3), "remove: missing key")
+freeze(remove_set)
+assert.fails(lambda: remove_set.remove(3), "remove: cannot delete from frozen hash table")
 
 # discard
-x.add(4)
-x.discard(4)
-assert.true(4 not in x)
-assert.eq(x.discard(4), None)
+discard_set = set([1,2,3])
+discard_set.discard(3)
+assert.true(3 not in discard_set)
+assert.eq(discard_set.discard(3), None)
+freeze(discard_set)
+assert.eq(discard_set.discard(3), None)  # no mutation of frozen set because key doesn't exist
+assert.fails(lambda: discard_set.discard(1), "discard: cannot delete from frozen hash table")
+
 
 # pop
-z = set([1])
-assert.eq(z.pop(), 1)
-assert.fails(lambda: z.pop(), "pop: empty set")
+pop_set = set([1,2,3])
+assert.eq(pop_set.pop(), 1)
+assert.eq(pop_set.pop(), 2)
+assert.eq(pop_set.pop(), 3)
+assert.fails(lambda: pop_set.pop(), "pop: empty set")
+pop_set.add(1)
+pop_set.add(2)
+freeze(pop_set)
+assert.fails(lambda: pop_set.pop(), "pop: cannot delete from frozen hash table")
+
 
 # clear
-x.clear()
-assert.eq(len(x), 0)
+clear_set = set([1,2,3])
+clear_set.clear()
+assert.eq(len(clear_set), 0)
+freeze(clear_set) # no mutation of frozen set because its already empty
+assert.eq(clear_set.clear(), None) 
+
+other_clear_set = set([1,2,3])
+freeze(other_clear_set)
+assert.fails(lambda: other_clear_set.clear(), "clear: cannot clear frozen hash table")