Issue 1268: Fix lazy slicing stopping at the first deleted #1299
Conversation
This commit fixes the issue in which using lazy Iterables as Positional indices will evaluate until the first deleted (undefined) value but not continue further. Within the default eagerize routine, it checks the number of reified elements upon encountering an undefined value to determine whether to continue. Addresses [Issue rakudo#1268](rakudo#1268)
|
Will need to write tests if accepted. |
|
Is there a way we can we do this without adding a public method? I also don't understand how this fixes anything for Seqs, since reified part will often just contain whatever was already reified: And so, IMO this type of fix just muddles up the conditions where a hole would be stopped at. "Stops at first gap or end of Iterable" is easy to understand. But if you also have to care about the kind of iterable you, as a user, have you now have 4 different cases to consider instead of just 2. IMO we should either fix this entirely or not fix it at all (and document behaviour as described in the Issue). Doing half-way measures doesn't improve usability or usefulness of the feature. |
This addresses the issue of unreified but pending List and Seq elements
causing the check of reified elems to return a correct but misleading
amount the would cause eagerization to halt.
An example of such an issue is below:
with [<a b c d e>] {
.[0]:delete;
with .Seq {
use nqp;
nqp::elems( nqp::getattr(.cache, List, '$!reified') ).say }
}
}
This would evaluate to 0 unless we were to explicitly reify before
taking the number of elems.
zoffixznet++ for pointing out this type of scenario.
|
I added a commit that addresses the issue of pending, unreified elements on the List or Seq as showcased in your example by explicitly reifying until lazy if the Reifier indicates that there is more to do. As for the public methods, I suppose we could encapsulate all of this logic within the eagerize block. I had just figured this was a bit cleaner. But if the number of reified elements is not something we want to be exposing to the user, I can get rid of them. |
No we shouldn't, especially not if calling the method causes, often complete, reification. BTW is the |
Good point. I'll remove the methods and incorporate the logic into the eagerizer in the next iteration. I'll also try removing the Seq handling as well. |
Remove the public NUM-REIFIED methods so as not to expose them (and their reification side-effects). Incorporated that logic into the default eagerize block in POSITIONS.
|
As discussed, I've removed the public methods and incorporated the logic into the default "eagerize" block for POSITIONS. It also passes the spectest. |
Update lazy positions comment to reflect current behavior.
|
I don't see anyone else commenting, but I'm still against this sort of piece-meal fixes. All it's doing is making the behaviour hard to predict. (the latest version of this PR assumes anything that isn't a Seq is a List, which I'm guessing will make this codepath explode for userland Positionals) |
|
I'm closing this PR as it's clear now that this was not the right path to take in order to fix this problem. There is still discussion at Issue 1268 about another approach. If that proves to be the right way forward, I will open another PR at a later date. |
This commit fixes the issue in which using lazy Iterables as Positional
indices will evaluate until the first deleted (undefined) value but not
continue further.
Within the default eagerize routine, it checks the number of reified
elements upon encountering an undefined value to determine whether to
continue.
Addresses Issue #1268