Add a Starlark set data type

[tetromino]

Modeled on the Python 3 set type and the existing implementations in Go, Rust, and the proposed one for Java, with the following differences:

• set literals and set comprehensions not supported (unlike python3)
  • Rationale: readability; {} can be confusing (empty set or empty dict?)
• copy() method not supported (unlike python3)
  • Rationale: we do not have it on lists or dictionaries; if we add it, we ought to add it to all containers for symmetry.
• comparison operators not supported (unlike starlark-go and python3)
  • Rationale: readability; python3-style comparison operators on sets provide only a partial order, resulting in unexpected behavior if one, for example, attempts to sort a list of sets.
• update() method supported (unlike starlark-go)
  • Rationale: follow python's example, useful for in-place mutation when rhs is not a set
• isdisjoint(), intersection_update(), difference_update(), symmetric_difference_update() method supported (unlike starlark-go and starlark-rust)
  • Rationale: follow python's example for a tiny efficiency gain - e.g. if we didn't have s.intersection_update(rhs), and rhs was a non-set sequence, we'd need to instead do s &= set(rhs), which would mean allocating an unnecessary temporary set for rhs's elements.
• multiple-argument form of union(), intersection(), difference() and corresponding _update methods supported (unlike starlark-go and starlark-rust)
  • Rationale: follow python's example for a tiny efficiency gain for the non-_update methods - allows avoiding temporary intermediate sets when unioning (or intersecting, etc.) a collection of sequences/sets into a set. (For the _update methods, the multi-argument form is syntactic sugar and doesn't provide an efficiency gain, but seems useful for api symmetry.)
• |, &, -, and ^ operators (and their augmented forms) require both sides to be sets if lhs is a set (unlike starlark-go)
  • Rationale: preserve syntactic compatibility with python3; starlark's | operator already requires both sides to be dicts if lhs is a dict.

Fixes #264

@adonovan @stepancheg FYI

[comius]

Can you add a rationale for the differences you're listing?

[stepancheg]

multiple-argument form of union(), intersection(), difference() and corresponding _update methods supported (unlike starlark-go and starlark-rust).

Do we need multiple-arguments? They are used like once in a year, and maybe not worth extra complexity.

But this is not a big deal.

[tetromino]

Can you add a rationale for the differences you're listing?

@comius - rationales added.

Do we need multiple-arguments? They are used like once in a year, and maybe not worth extra complexity.

@stepancheg - for union, intersection, and difference, the multi-argument form provides a tiny efficiency gain by allowing you to avoid temporaries. In other words, s.union(t, u, v) allocates one new set object, while s | t | u | v or s.union(t).union(u).union(v) would allocate 3 set objects, 2 of which are unnecessary temporaries.

[comius]

rationales added.

Thanks! Very nice.

[adonovan]

Nice. Most of my comments are based on comparison with the Starlark-Go spec.

[brandjon]

Pre-existing -- and don't change it in this PR -- but I think we changed this behavior so that unhashable types remain unhashable even after freezing. That's currently the behavior in the Java interpreter for Dict, for example. Something we can look at later.

[tetromino]

Acknowledged.

[brandjon]

All the justifications of differences from prior implementations sound good to me.

set literals and set comprehensions not supported (unlike python3)

• Rationale: readability; {} can be confusing (empty set or empty dict?)

I feel like "{} is a dict, not a set" is just one of those things you learn when you're dabbling in a Python dialect. But I suppose you could also go the other way, and say that even a non-empty set literal makes it harder to distinguish between sets and dicts on first sight. In either case, even if we wanted a literal syntax, it would be a separate extension.

[tetromino]

@adonovan @brandjon - ptal at current state of the text?

[brandjon]

LGTM!

[tetromino]

@brandjon - following the behavior python3 clearly aspires towards (but doesn't quite attain), I've added a requirement for the argument to set methods to not contain unhashable values.

[brandjon]

Did you drop the "not an iterable sequence" wording because it's implied/obvious from context?

[tetromino]

Yes - the first line already states that x must be an iterable sequence.

[brandjon]

Nit: Do you mean "in particular, even if x is unhashable"? Without the "even" I'm not sure what "in particular" is emphasizing.

[tetromino]

I put the "in particular" to indicate that "x is unhashable" is a potentially non-obvious subcase of "the set doesn't contain x"

[tetromino]

Reworded to be less confusing.

[brandjon]

Discussed offline but FTR: Apparently the behavior in Python, that my_set.discard(set([...])) succeeds, is intentional and is there to support the case where my_set has a frozenset element matching the operand [1]. So it's a convenience feature that makes the API more inconsistent. In that case, we're willingly choosing to break Python's precedent in the name of self-consistency.

[1] Source: reference doc quoted in first post of this thread.