Spec reviewer signoff

[gsathya]

Part of the stage 2 → 3 requirements is signoff from the official spec reviewers. Please review!

• @michaelficarra
• @bakkot
• @rwaldron
• @tabatkins

[ljharb]

It seems that some of the new methods lack the If S does not have a [[SetData]] internal slot, throw a TypeError exception. check that Set.prototype.add has. Could that be added?

[ljharb]

Could intersection/union perhaps make use of https://tc39.github.io/ecma262/#sec-add-entries-from-iterable ?

[gsathya]

Thanks for the quick review!

It seems that some of the new methods lack the If S does not have a [[SetData]] internal slot, throw a TypeError exception. check that Set.prototype.add has. Could that be added?

I left those out because some of these call out to the underlying Set.prototype.add, Set.prototype.has & Set.prototype.delete methods which do the type check. This also makes these methods work seamlessly for those who monkey patch the add/has/delete method, like how the SetConstructor works. Do you think it's necessary to the type check here as well?

isSubsetOf and isDisjointWith directly use the [[SetData]] internal slot of the Set object, which requires the type check. I could remove the type check from isSupersetOf, if you think that's better?

Could intersection/union perhaps make use of https://tc39.github.io/ecma262/#sec-add-entries-from-iterable ?

Interesting idea. I think it'd be better to create a similar AddEntryFromIterable that works for Sets, rather than Maps. Then we could refactor out code from the SetConstructor and share it with union/intersection, What do you think?

The length notes are only needed when the length doesn't match the implicit algorithm in https://tc39.github.io/ecma262/#sec-ecmascript-standard-built-in-objects - (btw, there's an isDisjoinWith misspelling in a number of places), so none of these need an explicit length definition.

Fixed in #33.

[ljharb]

Even if you monkeypatch those methods, you'll still be subclassing Set, so you'll still have the internal slot - are these methods that would be useful to .call on a non-subclass of Set? If not, I'd expect the slot checking there as well. Same applies to the others imo.

Sure! It'd be really nice to have shared steps and avoid duplication.

[ljharb]

(I just put up tc39/ecma262#1373 ; if that lands, then it'll be slightly less boilerplate for you to have this check on all the Set methods :-D )

[bakkot]

I'll try to do this soon. Do you have a timescale for this? E.g. are you hoping to present at the January meeting?

[gsathya]

I'll try to do this soon. Do you have a timescale for this? E.g. are you hoping to present at the January meeting?

Thanks! Yes, I'm hoping to present this in January and ask for Stage 3.

[rwaldron]

Just finished my review. Nitpick: I would use the same variable naming conventions as the existing spec, for example: newSet => S (or change the spec to the much nicer newSet). Everything else that stood out has already been mentioned here or in #35, so I will consider my review complete.

[gsathya]

Even if you monkeypatch those methods, you'll still be subclassing Set, so you'll still have the internal slot - are these methods that would be useful to .call on a non-subclass of Set?

After all the discussion in #41, I think we've decided keep this generic.

Sure! It'd be really nice to have shared steps and avoid duplication.

Done, here #47.

Just finished my review. Nitpick: I would use the same variable naming conventions as the existing spec, for example: newSet => S (or change the spec to the much nicer newSet). Everything else that stood out has already been mentioned here or in #35, so I will consider my review complete.

Thanks @rwaldron! Sounds good, I'll update the ES262 spec when I integrate this as part of Stage 4.

[gsathya]

@bakkot, @tabatkins, @michaelficarra -- I think the spec is in a really good shape now (with @bakkot's concerns being addressed). Please do review and let me know if there are any issues. I'd like to present this for Stage 3 at the January meeting.

[bakkot]

LGTM. I continue to disagree on #43, but I've made my case. If you can get consensus from the rest of the committee on the current eager behavior, the spec text for it seems fine.

I also want to note explicitly that this doesn't do the fall-back-to-iterator thing for the receiver of isSupersetOf and isDisjointWith discussed in #41 (comment). I don't necessarily mind this; I agree that supporting calling these methods with a non-Set receiver is very much an edge case. (Also, I realized that .calling .union on for example an Array will do entirely the wrong thing, because of SpeciesConstructor, so it seems less important to make .calling other methods on non-Sets work.) Just wanted to make sure it was intentional and noted.

[gsathya]

LGTM. I continue to disagree on #43, but I've made my case. If you can get consensus from the rest of the committee on the current eager behavior, the spec text for it seems fine.

Thanks. I definitely plan to bring this up to the committee during my presentation.

I also want to note explicitly that this doesn't do the fall-back-to-iterator thing for the receiver of isSupersetOf and isDisjointWith discussed in #41 (comment). I don't necessarily mind this; I agree that supporting calling these methods with a non-Set receiver is very much an edge case.

ACK

[michaelficarra]

@gsathya Can you summarise the design changes (and their implications) that resulted from all of the recent discussions? I'm unsure whether we've reached enough stability in the design to ask for stage 3 yet.

[gsathya]

Can you summarise the design changes (and their implications) that resulted from all of the recent discussions?

Sure, I'll update this issue once I make this summary for my TC39 presentation.

I'm unsure whether we've reached enough stability in the design to ask for stage 3 yet.

Yeah, fair enough. I doubt we can come to consensus on #50 by next week

[bakkot]

This got stage 3! Cleaning up old issues.