Skip to content
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

Reference vs. value equality surprises, array function naming #127

Open
jmagaram opened this issue Mar 29, 2023 · 3 comments
Open

Reference vs. value equality surprises, array function naming #127

jmagaram opened this issue Mar 29, 2023 · 3 comments

Comments

@jmagaram
Copy link
Contributor

jmagaram commented Mar 29, 2023

An important distinction is reference equality versus value equality. The code below will lead to surprising bugs and behavior if you become accustomed to the way ReScript uses value equality almost everywhere like pattern matching and the == operator.

let xs = []
xs->Array.push({"a": 1})
xs->Array.includes({"a": 1}) // false

type color = Red | Green | Black | Rgb(int, int, int)
let s = Core__Set.make()
s->Core__Set.add(Red)
s->Core__Set.has(Red) // true
s->Core__Set.add(Rgb(2,2,2))
s->Core__Set.has(Rgb(2,2,2)) // false

The behavior is brittle. Suppose you are working with an abstract type. Does it support reference equality or value equality? You shouldn't have to know. If the developer changes the implementation - like adding an @unboxed - the behavior changes. If the compiler changes how it represents things, the behavior changes.

Maybe we should have a naming standard for functions that use reference equality. It is a safety measure like InPlace. I don't know what to call it. Here are some ideas: includesUnsafe, includesExactly, includesStrict. This affects Array.includes, Array.indexOf, Array.lastIndexOf, variations (like WithIndex and AsOption), Set.has, and some others like functions on TypedArray. It should be noticeable enough that the developer reads the help to see what is going on. If they are using primitives or their own data, maybe it is not a problem.

Another issue is whether to provide value-equality versions like Array.includes that does an Array.find under-the-hood. I don't think this is necessary. The docs for the unsafe functions can point users to the safe alternatives.

Another issue is a bit unrelated, but because we need good names for all the Array functions I'm putting it here. A bunch of the array functions take an optional callback with an index. As mentioned elsewhere, WithIndex is verbose leading to things like findLastIndexWithIndex and everyWithIndex. Some libraries use the shorthand i like mapi. I'd be ok with that, or using the suffix With or By to indicate some extra data is being supplied to get the job done. Some array functions convert a numeric index to an option, and so Opt is added to the end leading to findLastIndexWithIndexOpt. This sounds very weird to me. I also believe the numeric version should require more characters. I prefer indexNum (returns -1) vs. index (returns None). Maybe we don't need option variants for versions we want to discourage. Some functions take a starting index; I find OfFrom unintuitive. Lastly, when uncurried happens, will we be able to have optional arguments at the end? If yes, we might not need as many functions for the optional parameters.

If there is any agreement on any of this, someone should put all the Array functions in a spreadsheet and try to come up with some good names that all fit together. Then sort them as they will appear in the Intellisense menu and see if it looks right. Here is an attempt at some of the function names...

JsName Result ReferenceEquality JSVariations Variations
Includes Bool TRUE startIndex
IndexOf Index TRUE startIndex
LastIndexOf Index TRUE startIndex
FindIndex Index FALSE indexInCallback indexToOption
FindLastIndex Index FALSE indexInCallback indexToOption
Find Item FALSE indexInCallback
FindLast Item FALSE indexInCallback

This is what it looks like in the Intellisense menu.

find
findIndex
findIndexNum
findIndexNumWith
findIndexWith
findLast
findLastIndex
findLastIndexNum
findLastIndexNumWith
findLastIndexWith
findLastWith
findWith
includesExactly
includesExactlyStartAt
indexOfExactly
indexOfExactlyStartAt
lastIndexOfExactly
lastIndexOfExactlyStartAt
@cristianoc
Copy link
Contributor

The general philosophy in the language is that the provided built-in deep equality is only provided as a convenience, with imperfections.
Even, there were suggestions to almost "deprecate" it by giving warnings to users when it is used.
In general, I would expect that it is not used in any libraries at all.
Rather, if a library function needs to perform some comparison, it should take a user-provided comparison as argument.

@cristianoc
Copy link
Contributor

That said, lightweight bindings to familiar functions form the JS ecosystem will likely inherit all the peculiar attributes of those functions.

@cristianoc
Copy link
Contributor

So the convention I would expect is: it's always shallow equality unless an explicit comparison function is provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants