Chainable ops silently fail to be chained when negated #1304

zoffixznet · 2017-12-11T20:05:37Z

Don't know if there's some logic behind this behaviour that I'm missing, but it looks like the ops that can normally be chained with different ops don't chain right when negating them (i.e. the Bool return of one seems to be used as input for the second op in the chain).

IMO it's LTA that it silently gives wrong result. Perhaps there's a way to make it work or make it complain?

$ perl6 -e 'say 1 < 3 > 2 '
True
$ perl6 -e 'say 1 !< 3 !> 2 '
True
$ perl6 -v
This is Rakudo version 2017.11-8-g7939014 built on MoarVM version 2017.11
implementing Perl 6.c.

The text was updated successfully, but these errors were encountered:

RT#125575: https://rt.perl.org/Ticket/Display.html?id=125575 Includes fudges for mixed combinations, pending resolution of R#1304: rakudo/rakudo#1304

zoffixznet · 2017-12-11T20:08:08Z

If this issue is rejected, please rip out the fudged tests in Raku/roast@5cbef9d

AlexDaniel · 2017-12-11T20:08:22Z

Sounds like RT#121987 is a dup. Maybe we should close RT#121987 in favor of this ticket.

zoffixznet · 2017-12-11T20:09:13Z

Maybe we should close RT#121987 in favor of this ticket.

👍 I like GitHub's tracker more.

AlexDaniel · 2017-12-11T20:11:37Z

So, more tests here (from RT#121987).

jstuder-gh · 2018-01-07T22:00:09Z

I don't know if this is the right solution for this problem, but I have one possible fix that addresses it and reapplies chaining when using the negation metaoperator.

NQP Commit:
jstuder-gh/nqp@84eef7c

Rakudo Commit:
jstuder-gh@0790938

NQP is modified because this modifies the 'chain' op (MoarVM only for now) so that it can use the first child node as the callee (as the negation metaop produces a subtree to determine the operator's behavior; seen below).

- QAST::Op(call &METAOP_NEGATE)  :is_pure<?>
  - QAST::Var(lexical &infix:«<») <wanted>

Of course something similar would need to be done for the JVM and JS backends (I have not looked into what this would entail). I suppose this could complicate upkeep of the different backends and I don't know how this would affect optimization efforts.

If you think that this is a good solution, I can clean up these commits a bit (reword, add more context) and submit as PRs in their respective repos.

zoffixznet · 2018-01-07T23:27:54Z

Don't know much about this part of the codebase, but I'm not seeing where negation is being done. In the NQP commit, the code has logic to fish out the original chained op, but does it negate the result afterwards?

Also, looks like the optimizer gets to METAOP_NEGATE and the nameless call first and rewrites it to a prefix:<!> call, with the nameless call removed.

Does your version give right results for stuff like say 1 !< 3 !> 2? I think the optimizer's business is done before the chain op handling in NQP, so if it removes nameless stuff , perhaps the logic you added never gets triggered.

BTW, you can see what QAST gets generated with --target command line arg; it shows QAST before (--target=ast) and after static optimizer (--target=optimize)

$ perl6 --target=optimize -e 'say 1 !< 3 !> 2'

[...]

- QAST::Op(callstatic &say) <sunk> :statement_id<?> say 1 !< 3 !> 2
                    - QAST::Op(call &prefix:<!>) 
                      - QAST::Op(call &infix:«>») 
                        - QAST::Op(call &prefix:<!>) 
                          - QAST::Op(call &infix:«<») 
                            - QAST::Want <wanted> 1
                              - QAST::WVal(Int) 
                              - Ii
                              - QAST::IVal(1) 
                            - QAST::Want <wanted> 3
                              - QAST::WVal(Int) 
                              - Ii
                              - QAST::IVal(3) 
                        - QAST::Want <wanted> 2
                          - QAST::WVal(Int) 
                          - Ii
                          - QAST::IVal(2)

And you can get the same output by using .dump method on QAST nodes. note($qast.dump)

jstuder-gh · 2018-01-08T01:20:12Z

Sorry, I didn't provide enough context on the modifications I was making.

The changes in Rakudo make it so that if the 'pasttype' on the base operator is 'chain' (as they are for '<' and '>' in the grammar) and the metaop is '!', then the nameless 'call' that would normally wrap the expression nodes is set to 'chain'.

But I needed to modify the 'chain' op too (in NQP). Before the name of the op served as the operator sub and the children the operands. This modification makes it so that, if there is no name provided to the chain, child 0 serves as the operator and children 1 and 2 the operands.

The result of the expression on this branch correctly results in False.

$ perl6 -e 'say 1 !< 3 !> 2'
> False

The target 'ast' and 'optimize' trees are included below:

perl6 --target=ast -e 'say 1 !< 3 !> 2'

- QAST::Op(call &say) <sunk> :statement_id<?> say 1 !< 3 !> 2
  - QAST::Op(chain) <wanted> !>
    - QAST::Op(call &METAOP_NEGATE)  :is_pure<?>
      - QAST::Var(lexical &infix:«>») <wanted>
    - QAST::Op(chain) <wanted> !<
      - QAST::Op(call &METAOP_NEGATE)  :is_pure<?>
        - QAST::Var(lexical &infix:«<») <wanted>
      - QAST::Want <wanted> 1
        - QAST::WVal(Int) 
        - Ii
        - QAST::IVal(1) 
      - QAST::Want <wanted> 3
        - QAST::WVal(Int) 
        - Ii
        - QAST::IVal(3) 
    - QAST::Want <wanted> 2
      - QAST::WVal(Int) 
      - Ii
      - QAST::IVal(2) 


perl6 --target=optimize -e 'say 1 !< 3 !> 2

- QAST::Op(callstatic &say) <sunk> :statement_id<?> say 1 !< 3 !> 2
  - QAST::Op(chain) <wanted> !>
    - QAST::Op(callstatic &METAOP_NEGATE)  :is_pure<?>
      - QAST::Var(lexical &infix:«>») <wanted>
    - QAST::Op(chain) <wanted> !<
      - QAST::Op(callstatic &METAOP_NEGATE)  :is_pure<?>
        - QAST::Var(lexical &infix:«<») <wanted>
      - QAST::Want <wanted> 1
        - QAST::WVal(Int) 
        - Ii
        - QAST::IVal(1)
      - QAST::Want <wanted> 3
        - QAST::WVal(Int)
        - Ii
        - QAST::IVal(3)
    - QAST::Want <wanted> 2
      - QAST::WVal(Int)
      - Ii
      - QAST::IVal(2)

As you can see, they are pretty similar besides a few 'callstatic' ops being substituted for 'call'. So this implementation is functional and gets the proper result, but may be missing out on some of the benefits of optimization.

zoffixznet · 2018-01-08T01:32:53Z

Ah, now I see. Changing it to chain indeed makes it miss the call optimization that expects a call op.

Right results are more important than fast results, so IMO I think it's OK to PR these changes (ensure they pass all tests in make spectest in rakudo) and add a # TODO comment that these need to be made static-optimized.

If it helps, QAST nodes can be annotated and the annotations can later be used in other places to do things:

<Zoffix___> m: use QAST:from<NQP>; my $qast := QAST::Op.new(:op<call>, :name<&die>); $qast.annotate('cat', 'meow'); say $qast.ann: 'cat'
<camelia> rakudo-moar b4df753df: OUTPUT: «meow␤»

<Zoffix___> m: use QAST:from<NQP>; my $qast := QAST::Op.new(:op<call>, :name<&die>); $qast.annotate('cat', 'meow'); say $qast.has_ann: 'cat'
<camelia> rakudo-moar b4df753df: OUTPUT: «1␤»

jstuder-gh · 2018-01-08T02:05:05Z

Do you want me to take a stab at the 'chain' mapping for the other backends before submitting a PR, or should I just submit what I have currently (and those can come later)?

zoffixznet · 2018-01-08T02:42:28Z

You can submit what you currently have.

Modify the 'chain' op to allow the option to use the first child as the callee. Before the name of the op served as the operator sub and the children the operands. This modification makes it so that, if there is no name provided to the chain, child 0 serves as the operator and children 1 and 2 the operands. This modification is being made to coincide with a Rakudo development allowing negated chained ops to continue to work as chained. See <rakudo/rakudo#1304>.

If the behavior on the base operator is to allow 'chaining' (as it is for '<' and '>' in the grammar) and the metaop is '!', then preserve chaining on the op wrapping the metaop and baseop. Previously, applying the negation metaoperator toward operators that allowed chaining (eg. '<', or '>') wouldn't preserve the chaining behavior. This commit is being made in conjunction with development in NQP altering the chain op. See <rakudo#1304>.

zoffixznet · 2018-01-08T03:23:54Z

PRs merged; tests unfudged in Raku/roast@bfe44db948

Modify the 'chain' op to allow the option to use the first child as the callee. Before the name of the op served as the operator sub and the children the operands. This modification makes it so that, if there is no name provided to the chain, child 0 serves as the operator and children 1 and 2 the operands. This modification is being made to coincide with a Rakudo development allowing negated chained ops to continue to work as chained. See <rakudo/rakudo#1304>.

This statement was previously restricted to MoarVM only as the JVM's chain op was not yet set up to handle a child as the callee. Now, as of commit b88a982b5421d1e4bb68bee42102ce68e164efc0 (in the NQP repo), the chain op's JVM implementation works the same way that MoarVM's does, making this negated chaining implementation functional on the JVM backend as well. See <rakudo#1304>.

See <rakudo/rakudo#1304>.

Modify the 'chain' op to allow the option to use the first child as the callee. Before the name of the op served as the operator sub and the children the operands. This modification makes it so that, if there is no name provided to the chain, child 0 serves as the operator and children 1 and 2 the operands. This modification is being made to coincide with a Rakudo development allowing negated chained ops to continue to work as chained. See <rakudo/rakudo#1304>.

This statement was previously restricted to MoarVM only as the JVM's chain op was not yet set up to handle a child as the callee. Now, as of commit 84278100acbf589a37f5baa85ffe27d68de52d13 (in the NQP repo), the chain op's JVM implementation works the same way that MoarVM's does, making this negated chaining implementation functional on the JVM backend as well. See <rakudo#1304>.

zoffixznet added a commit to Raku/roast that referenced this issue Dec 11, 2017

Test chaining of !before/!after

5cbef9d

RT#125575: https://rt.perl.org/Ticket/Display.html?id=125575 Includes fudges for mixed combinations, pending resolution of R#1304: rakudo/rakudo#1304

AlexDaniel added the LTA Less Than Awesome; typically an error message that could be better label Dec 11, 2017

AlexDaniel changed the title ~~[LTA] Chainable ops silently fail to be chained when negated~~ Chainable ops silently fail to be chained when negated Dec 11, 2017

This was referenced Jan 8, 2018

Rakudo Issue 1304: Preserve operator chaining when operators negated Raku/nqp#387

Merged

Issue 1304: Preserve operator chaining when operators negated #1375

Merged

zoffixznet closed this as completed Jan 8, 2018

jstuder-gh mentioned this issue Jan 17, 2018

Rakudo Issue 1304: [js] Allow 'chain' op to use child as callee Raku/nqp#390

Merged

jstuder-gh added a commit to jstuder-gh/roast that referenced this issue Jan 27, 2018

Unfudge now passing negated chain tests on JVM

d4deb15

See <rakudo/rakudo#1304>.

This was referenced Jan 27, 2018

Rakudo Issue 1304: [jvm] Allow 'chain' op to use child as callee Raku/nqp#402

Merged

Issue 1304 Continued: Re-enable negated chaining on the JVM #1448

Merged

Rakudo Issue 1304: Unfudge now passing negated chain tests on JVM Raku/roast#381

Merged

p6rt mentioned this issue Jan 5, 2020

Negated generic comparisons do not chain correctly Raku/old-issue-tracker#3398

Closed

p6rt mentioned this issue Jan 5, 2020

negation of before/after cannot be chained Raku/old-issue-tracker#4386

Closed

Xliff mentioned this issue Feb 27, 2024

Experiencing Segfault in NativeCall code for Raku v2024.01-67-g4c86ef7722 (moar v2024.01-6-gedbd26473) #5534

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chainable ops silently fail to be chained when negated #1304

Chainable ops silently fail to be chained when negated #1304

zoffixznet commented Dec 11, 2017

zoffixznet commented Dec 11, 2017

AlexDaniel commented Dec 11, 2017 •

edited

zoffixznet commented Dec 11, 2017

AlexDaniel commented Dec 11, 2017 •

edited

jstuder-gh commented Jan 7, 2018

zoffixznet commented Jan 7, 2018

jstuder-gh commented Jan 8, 2018

zoffixznet commented Jan 8, 2018

jstuder-gh commented Jan 8, 2018

zoffixznet commented Jan 8, 2018

zoffixznet commented Jan 8, 2018

Chainable ops silently fail to be chained when negated #1304

Chainable ops silently fail to be chained when negated #1304

Comments

zoffixznet commented Dec 11, 2017

zoffixznet commented Dec 11, 2017

AlexDaniel commented Dec 11, 2017 • edited

zoffixznet commented Dec 11, 2017

AlexDaniel commented Dec 11, 2017 • edited

jstuder-gh commented Jan 7, 2018

zoffixznet commented Jan 7, 2018

jstuder-gh commented Jan 8, 2018

zoffixznet commented Jan 8, 2018

jstuder-gh commented Jan 8, 2018

zoffixznet commented Jan 8, 2018

zoffixznet commented Jan 8, 2018

AlexDaniel commented Dec 11, 2017 •

edited

AlexDaniel commented Dec 11, 2017 •

edited