Bug in `len()` implementation / Implementing the `length()` primitive
- Dominant language
- Rust
- Stars
- 145
- Forks
- 5
- PR merge metrics
- No merged PRs in 30d
Description
The [`len()`](https://github.com/dgkf/R/blob/2ef9780a2a7a155a22bd42cbad73d59a3b084324/src/object/vector/rep.rs#L71-L78) implementation of the `Rep` enum is not correct in all cases I believe:
* If there is no subset, it is correct
* If there is a single `Mask` subset, the length of the resulting vector is not the length of `last` subset but the sum of the recycled logical vector
* For single `Indices` subset, taking the minimum over the "actual vector" and the elements in the last subset does not work. Even if we assume that all indices are within bounds, it still fails in cases such as `(1:2)[c(1, 1, 1)]`.
* The names mechanism I don't yet understand, so I am not sure
* For a single range it should be correct
* If there are multiple subsets, things might go wrong as well
Context: I am just trying to see whether I can implement the `length()` primitive.
My current idea would be to add a `Option` field to the `Subset` variant of `Rep` [here](https://github.com/dgkf/R/blob/2ef9780a2a7a155a22bd42cbad73d59a3b084324/src/object/vector/rep.rs#L16).
Then, whenever a new subset is added to `Subsets` by calling `push()`, we check whether the length of the new vector is known. If it is, we set the field to `Option::Some(length)`, otherwise to `None`.
The `len()` method of the `Rep` then simply reads the field.
If we define `len()` like this for `Rep`, we can then define `len()` on `Vector` (https://github.com/dgkf/R/blob/2ef9780a2a7a155a22bd42cbad73d59a3b084324/src/object/vector/core.rs#L199) by calling `len()` on the `Rep`. If `Some(usize)` is returned, we know the length. Otherwise - if `None` is returned - we could modify the vector in-place, i.e. call materialize on the subset and replace the existing subset-representation of the vector with the materialized one (so we avoid materializing the vector more than once).
Alternatively we can also generalize this to use a size hint for lower and upper bounds as you suggested in this issue: https://github.com/dgkf/R/issues/98
What I don't like about this solution is that it is very tailored to the length. However, if the lazy vector representation is a core feature of the language, I expect that there will be more cases like this. With "like this" I mean that functions can in some cases be calculated on the lazy representation, whereas they otherwise might need to materialize the vector.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.