Metaop semantics with QuantHashes

[lizmat]

Basically this situation, expanded to all QuantHashes:

my %d is SetHash = ^10;
dd %d;  # SetHash.new(6,2,9,0,8,5,4,3,1,7)
%d := %d (-) %d.grep: *.key %% 2;
dd %d;  # SetHash.new(9,5,3,1,7)

Note that to get this result, you need to bind the result of (-) to %d. If you however *assign the result, you wind up with a SetHash that contains a SetHash

my %d is SetHash = ^10;
dd %d;  # SetHash.new(6,2,9,0,8,5,4,3,1,7)
%d = %d (-) %d.grep: *.key %% 2;
dd %d;  # SetHash.new(SetHash.new(9,5,7,1,3))

Which implies, that if you want to use a metaop for this:

my %d is SetHash = ^10;
dd %d;  # SetHash.new(6,2,9,0,8,5,4,3,1,7)
%d (-)= %d.grep: *.key %% 2;
dd %d;  # SetHash.new(SetHash.new(9,5,7,1,3))

you wind up with a result that people find to be unexpected. But is entirely up to spec, as you should be able to put QuantHashes inside of QuantHashes. And in fact, I've used that approach for a recent Perl Weekly Challenge.

On the other hand, if we just had used hashes:

my %d = ^10 Z=> True xx *;
dd %d;  # Hash %d = {"0" => Bool::True, "1" => Bool::True, "2" => Bool::True, "3" => Bool::True, "4" => Bool::True, "5" => Bool::True, "6" => Bool::True, "7" => Bool::True, "8" => Bool::True, "9" => Bool::True}
%d (-)= %d.grep: *.key %% 2;
dd %d;  # Hash %d = {"1" => Bool::True, "3" => Bool::True, "5" => Bool::True, "7" => Bool::True, "9" => Bool::True}

it does work as some people expect. So I guess there's something to be said for that semantic as well, since we're supposed to assume that QuantHashes are just object Hashes with a limitation on what they can have as a value.

Making metaops special case QuantHashes, feels like a bad idea to me.

Since I fear that there is code out there using QuantHashes with the current semantics, if we should decide to give QuantHashes the same semantics as object Hashes in this situation, I think we will need to make it version dependent.

[lizmat]

FWIW, there is a simple patch to create the Hash semantics for QuantHashes:

diff --git a/src/core/QuantHash.pm6 b/src/core/QuantHash.pm6
index 7ab7e1139..464d1e6ab 100644
--- a/src/core/QuantHash.pm6
+++ b/src/core/QuantHash.pm6
@@ -1,4 +1,4 @@
-my role QuantHash does Associative {
+my role QuantHash does Associative does Iterable {

However, this results in the following spectest failures:

Test Summary Report
-------------------
t/spec/S02-types/bag.t                                          (Wstat: 1024 Tests: 237 Failed: 4)
  Failed tests:  71, 73, 75, 77
  Non-zero exit status: 4
t/spec/S02-types/baghash.rakudo.moar                            (Wstat: 768 Tests: 308 Failed: 3)
  Failed tests:  83, 85, 87
  Non-zero exit status: 3
t/spec/S02-types/mix.t                                          (Wstat: 1024 Tests: 227 Failed: 4)
  Failed tests:  73, 75, 77, 79
  Non-zero exit status: 4
t/spec/S02-types/mixhash.rakudo.moar                            (Wstat: 768 Tests: 274 Failed: 3)
  Failed tests:  84, 86, 88
  Non-zero exit status: 3
t/spec/S02-types/set.rakudo.moar                                (Wstat: 1280 Tests: 229 Failed: 5)
  Failed tests:  70, 72, 74, 76, 219
  Non-zero exit status: 5
t/spec/S02-types/sethash.t                                      (Wstat: 1024 Tests: 262 Failed: 4)
  Failed tests:  89, 91, 93, 95
  Non-zero exit status: 4
t/spec/S03-operators/set_difference.t                           (Wstat: 1024 Tests: 63 Failed: 4)
  Failed tests:  9-10, 12-13
  Non-zero exit status: 4
  Parse errors: Bad plan.  You planned 340 tests but ran 63.
t/spec/S03-operators/set_intersection.t                         (Wstat: 65280 Tests: 29 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 570 tests but ran 29.
t/spec/S03-operators/set_multiply.t                             (Wstat: 65280 Tests: 29 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 604 tests but ran 29.
t/spec/S03-operators/set_subset.t                               (Wstat: 65280 Tests: 98 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 1704 tests but ran 98.
t/spec/S03-operators/set_union.t                                (Wstat: 65280 Tests: 47 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 462 tests but ran 47.
t/spec/S03-operators/set_addition.t                             (Wstat: 15 Tests: 2 Failed: 0)
  Non-zero wait status: 15
  Parse errors: Bad plan.  You planned 604 tests but ran 2.

[vrurg]

First of all, sorry for the commit you had to revert. It usually really hard to get somebody to review a PR. Therefore I mostly base my decisions on spectest, docs, and related tickets. From this point of view I found nothing what would tell me not to merge it.

With regard to the subject, I would say that the specs are not disabling storing of a QuantHash in another QuantHash do not contradict to DWIM logic by which assignment must copy over:

> my @a; my @b = 1,2,3; @a = @b; say @a.perl
[1, 2, 3]
> my @a; my @b = 1,2,3; @a = (@b,); say @a.perl
[[1, 2, 3],]

I see no reason why say a SetHash should behave differently? Because the current:

my %d is SetHash; my %d2 is SetHash = <a b c>; dd %d2; %d = %d2; dd %d
SetHash.new("b","a","c")
SetHash.new(SetHash.new("b","a","c"))

is totally contr-intuitive and illogical.

As to Iterable – was there any reason to implement QuantHashes this way in first place? It seems quite logical. Anyway, I tried adding the role in order to see what happens to the spectest and it didn't even compile for me.

A little bit of related side note. While looking into rakudo/rakudo#1203 I noticed that the way (-)= works is basically described as a sequence of copy-over operations using iterators. That seems like a huge speed- and memory-wise performance hit to me. I think <op>= operators must be explicitly defined and operate directly on their lhs. This would avoid another possible confusion for a user. Consider a class inherited from a QuantHash class. Objects of this class has some kind of internal state which would be just lost if <op>= is used on them!

[lizmat]

First of all, sorry for the commit you had to revert. It usually really hard to get somebody to review a PR. Therefore I mostly base my decisions on spectest, docs, and related tickets. From this point of view I found nothing what would tell me not to merge it.

Indeed, if I gave the impression that you could have known, I'm sorry: I didn't want to convey that.

... is totally contr-intuitive and illogical.

The difference comes from Array being Iterable, and QuantHashes not.

adding the role in order to see what happens to the spectest and it didn't even compile for me

How did you do that? With the diff I posted??

That seems like a huge speed- and memory-wise performance hit to me

Metaops are currently implemented to work, correctly. I agree there are still some significant gains to be had here. In the past, however, there has been quite a reluctance to add any specific meta-ops to the grammar / core. FWIW, I'm not entirely sure why anymore.

[vrurg]

BTW, I think the roast test reversal must not have been made. Fudged, perhaps, until the fix is in place.

Indeed, if I gave the impression that you could have known, I'm sorry: I didn't want to convey that.

No, no! I'm just telling you where the mistake came from. Nothing else. :)

The difference comes from Array being Iterable, and QuantHashes not.

I would disagree up to certain extent. For that matter Maps are not iterables too. But they're. As long as there is a collection of something there could be a way to iterate through it. Why not? I don't insist, but it does look reasonable to me.

How did you do that? With the diff I posted??

Oh, easily! Buy forgetting to stash push debug prints and having a nqp::say with multiple params... An airport is not the best workplace, you know...

I'm finishing with rakudo/rakudo#3012 and gonna give it a try again. Wonder if that's something fixable or not.

Metaops are currently implemented to work, correctly. I agree there are still some significant gains to be had here. In the past, however, there has been quite a reluctance to add any specific meta-ops to the grammar / core. FWIW, I'm not entirely sure why anymore.

I know that = is considered special and non-overridable. But a quick test with (-)= reveals that it's possible to have it defined. And as long as it gives us performance boost I don't see a reason not to have those ops in place. Sets are already slow enough for me to refuse to use them last year when I needed fast code. Plain Hash was faster enough to use it instead.

BTW, I think the roast test reversal must not have been made. Fudged, perhaps, until the fix is in place. But otherwise I consider it reasonable.

[lizmat]

dd Map ~~ Iterable   # Bool::True

so Maps are Iterable. Iterable is just a flag that means "iterate over me when this seems appropriate". Such as with a for loop.

BTW, I think the roast test reversal must not have been made. Fudged, perhaps, until the fix is in place. But otherwise I consider it reasonable.

Well, I reverted to make sure we could get a release fast. But also because they were at the wrong place. We have separate test files for handling stuff for the various aspects of QuantHashes and set operators.
S02-types/sethash.t would probably have been a better place.

And again... we have 6 similar situations, this was only fixing one of them. So if you want to create fudged tests for these, they would have to involve all of these situations.

[vrurg]

so Maps are Iterable. Iterable is just a flag that means "iterate over me when this seems appropriate". Such as with a for loop.

And you know what? I looked into why bag.t fails (got no time for others because Theo caught me sitting at airports Starbucks and we spend good time chatting to each other) and it fails because it does the right thing when Iterable is there. Look:

    my $b = bag set <foo bar foo bar baz foo>;
    isa-ok $b, Bag, '&Bag.new given a Set produces a Bag';
    is +$b, 1, "... with one element";

The line which fails is 'one element'. But it is three now and this is good! Would I need a bag of sets I'd do bag (set <...>, ) or even bag [set <...>] should probably result in same outcome!

The serious problem is about S03-operators/set_addition.t which stucks. But I guess it is solvable.

Now I wonder if applying Iterable is a thing to be considered for 6.e then how would it be done without copying over the whole QuantHash.pm6 into core.e/?

Well, I reverted to make sure we could get a release fast. But also because they were at the wrong place. We have separate test files for handling stuff for the various aspects of QuantHashes and set operators.
S02-types/sethash.t would probably have been a better place.

And again... we have 6 similar situations, this was only fixing one of them. So if you want to create fudged tests for these, they would have to involve all of these situations.

Do you have all cases collected somewhere? I took the path of closing old tickets when I can. This seems like a part of this path.

[lizmat]

We have 3 types of QuantHashes in 2 tastes: Set, SetHash, Bag, BagHash, Mix, MixHash.

If only the tests are failing when making QuantHash Iterable, then making a 6.e version could be as simple as creating a new, local QuantHash role with Iterable mixed in?

[vrurg]

Ok, I misunderstood what you meant by 'cases'. Now it's clear.

I investigated it deeper while being on the plane. Here is what I came to. The tests failing because of the wrong number of elements are the easiest part, obviously. Another side of the change is that in many cases multi-dispatch is now choosing (Iterable:D, ...) variants of operators instead of (Any, Any) default. This causes set_addition.t test to freeze on a lazy sequence (expected). Other tests are failing mostly on type mismatches.

Third kind of problem is flattening of a QuantHash type object. So far, easily fixed with list method returning self which is makes total sense because we're an iterable now.

Fixes for these cases are simple but require careful analyzing. I hope that spectests cover enough cases including edge ones to make the transition as clear as possible.

Hopefully, I have enough info now to start a branch for this case.

[vrurg]

So far, I cannot create 6.e-specific version of QuantHash. Simply putting it into CORE.e.setting doesn't work. And it shouldn't because all QuantHash classes are created in CORE.setting. Moreover, they're pre-composed.

As a test I moved SetHash and Setty into CORE.e.setting. Still, no changes, they're just getting ignored.

For now I'm unsure what happens to CORE.<letter>.setting. Need either to get into this subject and reverse-engineer it, or need somebody's help. In either case, what I currently see here is that the whole bunch of QuantHash would have to be duplicated in CORE.e.setting anyway because we can't just replace a role when there're classes built on top of it.

[vrurg]

@lizmat You might be interested looking at #71.

[vrurg]

Closing because, as Jonathan reminded in #104, quant hashes are items by design.