Additional methods for Set type

[SamWheating]

Re: #492

Added set.add(x), set.pop(), set.remove(x), set.discard(x), set.clear(), as well as updates to the Spec and tests.

Demo:

>>> a = set([1,2,3])
>>> a.add(4)
>>> a
set([1, 2, 3, 4])
>>> a.remove(1)
>>> a
set([2, 3, 4])
>>> a.remove(1)
Traceback (most recent call last):
  <stdin>:1:9: in <expr>
Error in remove: remove: missing key
>>> a.discard(2)
>>> a
set([3, 4])
>>> a.discard(2)
>>> a.pop()
3
>>> a
set([4])
>>> a.pop()
4
>>> a.pop()
Traceback (most recent call last):
  <stdin>:1:6: in <expr>
Error in pop: pop: empty set
>>> a.add(1)
>>> a.add(2)
>>> a
set([1, 2])
>>> a.clear()
>>> a
set([])

[adonovan]

Hi Sam, many thanks for this contribution. I've made a number of comments but the necessary changes should all be fairly straightforward.

[adonovan]

All the mutating methods should ideally mention what happens when the set is frozen, especially given the subtlety that add may not always attempt to mutate the set (if the element is already present), so it does not always fail on a frozen dict. The spec should probably take a position on this, by stating that the lookup precedes the attempt at mutation (that's what dict.setdefault does, although the document is not particularly explicit).

[SamWheating]

Good point, all of the frozen checks defer to the underlying hashtable so I wasn't thinking about this case. I'll verify and document this behaviour.

Related - how do you feel about calling clear() on an empty, frozen dict or set? Should that succeed?

[SamWheating]

Landed on the following semantics around frozen sets:

• set.clear() will succeed on an empty frozen set
• set.discard(x) will succeed on a frozen set which does not contain x
• set.add(x) will succeed on a frozen set which already contains x
• pop() and remove() will always fail on frozen sets with cannot delete from frozen hash table.

Two edge cases to consider -

• Should pop() on an empty frozen set fail because its empty, or because its frozen?
• Should remove(x) on a frozen set not containing x fail because of missing key, or because its frozen?

[adonovan]

Should pop() on an empty frozen set fail because its empty, or because its frozen?
Should remove(x) on a frozen set not containing x fail because of missing key, or because its frozen?

Generally we don't specify which error you get when more than one applies. What's more important is that we specify whether successful shortcuts trump slow-path errors (as in the case of discarding an unhashable from an empty set: does it fail because unhashable or succeed because empty?). So what you have looks fine.

[adonovan]

If key is unhashable then it can't be a member of the set. Shouldn't it simply return in that case? I wonder what Python does.

[SamWheating]

Good point - looks like Python 3.7.13 will throw a typeError when trying to remove (or discard) an unhashable type from a set.

>>> a = set([1,2,3])
>>> a.remove(1)
>>> a.remove([4,5,6])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'list'

I think that this makes sense, especially in the case of remove where a successful return indicates that the item was found and removed. Thoughts?

[adonovan]

Add x = set() before each of these stanzas so that there is no residual effect across them.

[SamWheating]

Starlark doesn't permit this because of global variable reassignment, but I can just create a unique set for each block - any objections?

[SamWheating]

Thanks for the super thorough review - I think I have addressed all of your feedback now and I left a few discussions open where I need some additional confirmation.

[adonovan]

Sorry for the delay. More nitpicks, but we're nearly there. Thanks again.

[adonovan]

For discard, unhashable => not found, so I think this should not be an error. Can we add a test case?

[SamWheating]

Had some thoughts about this here:
#497 (comment)

If we're aiming for consistency with the python implementation we should be throwing an error when discarding with an unhashable key. Thoughts?

[adonovan]

That seems fine. Thanks for checking.

[SamWheating]

Thanks for the review, got everything addressed aside from a few open discussions.

While you're here - how would you feel about adding built-in set parsing and set comprehension? I'd be interested in working on the implementation but also think this might be too much divergence from the official spec.

[adonovan]

how would you feel about adding built-in set parsing and set comprehension? I'd be interested in working on the implementation but also think this might be too much divergence from the official spec.

I would rather not implement those since they change the dialect accepted by the parser. If you can persuade the Bazel folks to accept set as a standard feature, then these expressions would be appropriate, but I don't think they feel much need for it.

Thanks again for contributing this, and for your patience during the review.

[adonovan]

That seems fine. Thanks for checking.

[adonovan]

Should pop() on an empty frozen set fail because its empty, or because its frozen?
Should remove(x) on a frozen set not containing x fail because of missing key, or because its frozen?

Generally we don't specify which error you get when more than one applies. What's more important is that we specify whether successful shortcuts trump slow-path errors (as in the case of discarding an unhashable from an empty set: does it fail because unhashable or succeed because empty?). So what you have looks fine.