New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Result of `Set#find` #6

Open
zloirock opened this Issue Jan 30, 2018 · 24 comments

Comments

Projects
None yet
7 participants
@zloirock
Copy link
Contributor

zloirock commented Jan 30, 2018

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

This comment has been minimized.

Copy link
Member

ljharb commented Jan 30, 2018

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

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Jan 31, 2018

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

This comment has been minimized.

Copy link
Member

ljharb commented Jan 31, 2018

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

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Jan 31, 2018

@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

This comment has been minimized.

Copy link
Collaborator

Ginden commented Jan 31, 2018

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

@zloirock

This comment has been minimized.

Copy link
Contributor Author

zloirock commented Jan 31, 2018

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

This comment has been minimized.

Copy link
Member

ljharb commented Jan 31, 2018

@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

This comment has been minimized.

Copy link

bakkot commented Jan 31, 2018

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

@Ginden

This comment has been minimized.

Copy link
Collaborator

Ginden commented Feb 1, 2018

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

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Feb 1, 2018

@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

This comment has been minimized.

Copy link
Collaborator

Ginden commented Jan 9, 2019

@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

This comment has been minimized.

Copy link
Member

ljharb commented Jan 9, 2019

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:
4. 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
5. 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

This comment has been minimized.

Copy link
Member

michaelficarra commented Jan 10, 2019

The only acceptable options are 1 and 3.

edit: Option 6: withdraw this proposal

@zloirock

This comment has been minimized.

Copy link
Contributor Author

zloirock commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Member

ljharb commented Jan 10, 2019

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

@Ginden

This comment has been minimized.

Copy link
Collaborator

Ginden commented Jan 10, 2019

  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

This comment has been minimized.

Copy link
Member

michaelficarra commented Jan 10, 2019

@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

This comment has been minimized.

Copy link
Member

ljharb commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Collaborator

Ginden commented Jan 10, 2019

@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

This comment has been minimized.

Copy link
Member

ljharb commented Jan 10, 2019

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

@littledan

This comment has been minimized.

Copy link
Member

littledan commented Jan 13, 2019

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

@Ginden

This comment has been minimized.

Copy link
Collaborator

Ginden commented Feb 8, 2019

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

This comment has been minimized.

Copy link
Member

ljharb commented Feb 8, 2019

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(…)

@isiahmeadows

This comment has been minimized.

Copy link

isiahmeadows commented Feb 9, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment