tc39 / proposal-collection-methods

https://tc39.github.io/proposal-collection-methods/
Other
172 stars 8 forks source link

Result of `Set#find` #6

Closed zloirock closed 5 years ago

zloirock commented 6 years ago
new Set([undefined]).find(it => it == null); // => undefined
// but also
new Set().find(it => it == null); // => undefined

Maybe Set#find should return something else if it fails? For example, a well-known symbol?

ljharb commented 6 years ago

Defining a well-known symbol for this purpose seems broadly useful.

michaelficarra commented 6 years ago

Or we can define a built-in Option/Maybe type.

edit: The reason the well-known symbol is a bad idea (in exactly the same way as null) is that I can always put it in the set and then you don't know again whether it was found.

new Set([Symbol.whatever]).find(() => true);
ljharb commented 6 years ago

I would very much like to see an Option/Maybe/Result type in the language; that sounds like an excellent solution.

michaelficarra commented 6 years ago

@littledan was looking for feedback on some of the more useful data structures to add to the language. Maybe is at the top of my list.

Ginden commented 6 years ago

I don't feel good with deviating from behaviour already established for Array.prototype.find.

zloirock commented 6 years ago

Adding of Option / Maybe / Result, sure, interesting idea, but definitely out of scope this proposal. Also, it will make Set#find even more distant from Array#find.

ljharb commented 6 years ago

@Ginden arrays have findIndex that can return -1 to help you distinguish between “not found” and “found undefined”. How would this proposal allow a way to distinguish that?

bakkot commented 6 years ago

-0 is the only existing language value which would work here. (But let's not use it, please.)

Ginden commented 6 years ago

@ljharb Isn't it already covered by has method?

ljharb commented 6 years ago

@Ginden i suppose you could test an undefined result and determine if the set has that item; but that wouldn’t tell you whether the specific logic in the find callback ever returned true.

Imagine: const s = new Set([undefined]); const v = s.find(x => complexLogicThatSometimesReturnsTrueForUndefined(x)); s.has(v) === true doesn’t tell you if the find operation actually found anything.

Ginden commented 5 years ago

@ljharb I don't think it's possible to overcome this edge case. Therefore I see three solutions:

  1. Drop find
  2. Accept existence of undefined problem. Programmer can use side channel (eg. external variable) to deal with such extreme edge case
  3. Return some kind of Result - eg. [bool, result] array

Personally I favor 2.

ljharb commented 5 years ago

I think that option 2 alone is not tenable - arrays, perhaps unintentionally, solved this problem by having both .find and .findIndex (iow, a mechanism that takes a callback and lets you know whether it's in the array or not).

There's a few more options than those three:

  1. return a sentinel value of a well-known symbol, like Set.NotFound - this has the same downside of undefined tho, just less common, so i think it's a nonstarter
  2. add an additional method alongside .find like .some or .every - iow, something you can pass the same callback to and get a boolean from.

Option 5 is the one I'd favor.

michaelficarra commented 5 years ago

The only acceptable options are 1 and 3.

edit: Option 6: withdraw this proposal

zloirock commented 5 years ago

After rethinking, it looks ok for Set#find because mainly it's an equal of Array#find,

[undefined].find(it => it == null); // => undefined
[].find(it => it == null); // => undefined

, if someone need an equal of findIndex he could use double check with .has and it's not harder than Option/Maybe/Result.

But it's still a problem for Map.prototype.{ findKey, keyOf }.

ljharb commented 5 years ago

@michaelficarra why do you find option 5 unacceptable? (i agree that 2 and 4 are unacceptable)

Ginden commented 5 years ago
  1. add an additional method alongside .find like .some or .every - iow, something you can pass the same callback to and get a boolean from.

That sounds good for me - do you have any proposition for name?

michaelficarra commented 5 years ago

@ljharb Two lookups in the set. Even though it is constant time, I don't like adding a coefficient of 2 for what should be a simple and common operation.

ljharb commented 5 years ago

You’d only need to do that when you received undefined, which won’t be common but definitely would make the code less simple, fair.

@ginden .some seems simplest, and parallels arrays (altho I’m sure then that someone would want .every too)

Ginden commented 5 years ago

@michaelficarra It's not common operation, this is edge case that I have never seen or heard of in real world code.

@ljharb some and every are already included in proposal.

ljharb commented 5 years ago

In that case it seems like it already pairs with Array.

littledan commented 5 years ago

Hmm, JavaScript as a language uses undefined for analogous cases all over the place. What's the motivation for doing something different here?

Ginden commented 5 years ago

But it's still a problem for Map.prototype.{ findKey, keyOf }.

I'm thinking on removing Map.prototype.find in favor of findPair with signature this: Map -> [any, any].

ljharb commented 5 years ago

the terminology for that is “entry”, ftr - .findEntry perhaps. I do like that that’s less ambiguous, altho if you only wanted the value you’d end up with a lot of const [, value] = map.findEntry(…)

dead-claudia commented 5 years ago

Just thought I'd drop in to point out that [].find(() => true) returns undefined as well, so changing the proposed Set.prototype.find here would make it no longer align with arrays.