`iterUniqueBy` treats iterator return value as part of sequence

[lightmare]

Description

iterUniqueBy calls the "uniqueness" predicate function on the iterator's return value. This is wrong, the return value is not part of the sequence, and cannot be skipped. A yield* ... expression doesn't distinguish between missing value and undefined.

source:
https://github.com/stdlib-js/iter-unique-by/blob/3c5a1bd4b7343469b5f03a9778c4e5b4db435454/lib/main.js#L139-L147

Related Issues

Haven't found any related to iter-unique

Questions

No.

Demo

No response

Reproduction

import iterUniqueBy from "@stdlib/iter-unique-by"

function notSameLength(a, b) {
  return a.length !== b.length
}

const filtered = iterUniqueBy(["baba", "yaga"].values(), notSameLength)

console.log(Array.from(filtered))

Expected Results

// output
[ 'baba' ]

Actual Results

file:///tmp/bug/repro.js:4
  return a.length !== b.length
                        ^

TypeError: Cannot read properties of undefined (reading 'length')
    at notSameLength (file:///tmp/bug/repro.js:4:25)
    at isUnique (/tmp/bug/node_modules/.pnpm/@stdlib+iter-unique-by@0.0.6/node_modules/@stdlib/iter-unique-by/lib/main.js:59:19)
    at Object.next (/tmp/bug/node_modules/.pnpm/@stdlib+iter-unique-by@0.0.6/node_modules/@stdlib/iter-unique-by/lib/main.js:142:38)
    at Function.from (<anonymous>)
    at file:///tmp/bug/repro.js:9:19

Version

@stdlib/iter-unique-by 0.0.6

Environments

Node.js

Browser Version

No response

Node.js / npm Version

Node.js v17.2.0 / pnpm 6.23.6

Platform

No response

Checklist

• Read and understood the Code of Conduct.
• Searched for existing issues and pull requests.

[github-actions]

🎉 Welcome! 🎉

And thank you for opening your first issue! We will get back to you shortly. 🏃 💨

[kgryte]

@lightmare Thanks for filing an issue. I am able to reproduce the described behavior; however, I am not convinced that this is a bug in the implementation of iterUniqueBy.

Namely, we cannot readily distinguish between the case where a value property is present, but not set, and when it is present, but set to undefined.

In your example above, however, you have more knowledge of the input data. In which case, you can amend your predicate function to

function notSameLength( a, b ) {
    return ( b !== void 0 ) && ( a.length !== b.length );
}

In this case, you know that undefined is not a legitimate value based on the input data; an assumption we cannot make.

If I make the above change and then run the iterator pipeline, I get the expected results:

import iterUniqueBy from "@stdlib/iter-unique-by"

function notSameLength( a, b ) {
    return ( b !== void 0 ) && ( a.length !== b.length )
}

const filtered = iterUniqueBy(["baba", "yaga"].values(), notSameLength)

console.log(Array.from(filtered))
// => [ 'baba' ]

[kgryte]

Aside: for posterity, just to confirm that yield * is not doing something unexpected here, this behavior is expected according to the ECMAScript specification.

Namely, value is to be set to the result of iteratorValue, even when done is true. This is not true for the iterator protocol more generally, where value can be elided.

As stated above, we cannot know a priori whether an upstream iterator explicitly sets undefined or whether the execution context sets value even when no value exists, so we cannot know whether the iterator should skip the final value if undefined.

As this is a potential footgun, it may be worthwhile adding a note to the documentation, as knowing whether one needs to include an explicit guard against undefined requires implementation knowledge, both of the utility and the execution context.

[lightmare]

Namely, we cannot readily distinguish between the case where a value property is present, but not set, and when it is present, but set to undefined.

Problem is that the code in iterUniqueBy does distinguish between these two.

This might be an issue of invalid assumptions on my side, as I'm not familiar with this library, and came to it with some preconceived notions of what iterators are. So it caught me by surprise that you treat the iterator's return value as an element of the sequence.

After checking some other functions, I see that this is not specific to iterUniqueBy, it's by design throughout the library. iterLength behaves the same.
To me this doesn't make any sense:

iterLength([1,2,3].values()) // returns 4

I wanted to test this with generator functions, but that didn't work because isIteratorLike requires it.next.length === 0.
And when I subverted that check, iterLength of course counted the (implied) return value into the sequence:

function* g() {
  yield* [1, 2, 3]
}
iterLength(g()) // throws
iterLength({ s: g(), next() { return this.s.next() }}) // returns 4

This design is not entirely compatible with built-in iterators and generators, so it would deserve some prose in the docs explaining the differences from ES spec: what exactly iterator means in stdlib, that the return value is considered an element of the sequence, and that generators are not directly usable.

[kgryte]

@lightmare I am going to course correct here, and admit that we misinterpreted an ambiguity in the spec returning iterator return values. We'll be refactoring our iterator implementations accordingly over the coming week. These changes should be included in the next stdlib release.

Thanks for filing this issue. Will circle back with updates.

[kgryte]

@lightmare The reported issue has now been resolved upstream via 2ae9a80. These changes should be in the next release of the individual package.

To confirm that the changes match the expected behavior in the OP, here is the following output from the stdlib REPL:

In [1]: function notSameLength( a, b ) { return a.length !== b.length; };

In [2]: var arr = [ 'baba', 'yaga' ];

In [3]: Array.from( iterUniqueBy( arr.values(), notSameLength ) )
Out[3]: [ 'baba' ]

[kgryte]

@lightmare The changes have been pushed out to the individual package: https://github.com/stdlib-js/iter-unique-by/blob/e9b882e4d6241cfe585ee1cea889c106a52cd779/lib/main.js.

To use immediately, you can install the package directly from GitHub. Otherwise, as mentioned in the previous comment, the behavior should be fixed in the next published release.

Thanks for reporting this issue and your patience!

[lightmare]

Thanks. I've checked a bunch of functions (iterLength, iterConcat, iterMap, iterUniqueBy) and they no longer include iterator return value in the sequence. Closing.

[kgryte]

@lightmare Thanks for checking!