Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepostfix boundary does not get properly cloned; missing a test? #189

Closed
masak opened this issue Aug 30, 2016 · 6 comments
Closed

Prepostfix boundary does not get properly cloned; missing a test? #189

masak opened this issue Aug 30, 2016 · 6 comments

Comments

@masak
Copy link
Owner

masak commented Aug 30, 2016

Just discovered this in OpScope.pm:

has $!prepostfix-boundary = 0;

# meanwhile, later in the file:
method clone {
    my $opl = self.new(
        infixprec => @.infixprec.map(*.clone),
        prepostfixprec => @.prepostfixprec.map(*.clone),
        :$!prepostfix-boundary,    # <--- *this* line
    );
    # ...stuff ...
}

The intent is clear: we want to clone the $!prepostfix-boundary attribute with the rest of the opscope. Of course.

But private attributes aren't settable through the default new, as demonstrated here:

$ perl6 -e 'class C { has $!priv; method set($n) { $!priv = $n }; method divulge { $!priv }; method clone { self.new(:$!priv) } }; given C.new { .set(2); say .divulge; my $c2 = .clone; say $c2.divulge }'
2
(Any)

So

  • The indicated line does nothing, and
  • very likely this would fail a test we never wrote.

What test? Basically a recapitulation of the above demonstration, but in the context of the boundary between prefix and postfix ops. (This, by the way, is something I should write down somewhere. I see operators.md oddly has no mention of it. The commit that adds the attribute (and the bug), 6093795, doesn't mention the boundary at all.)

Ok, let's see.

sub prefix:<&>(x) {                         #  op order: &
    return x ~ " prefix:<&>";
}

sub postfix:<!>(x) is looser(prefix:<&>) {  #  op order: ! & (tightest)
    return x ~ " postfix:<!>";
}

{
    sub prefix:<$>(x) {                     #  op order: $ ! & (tightest)
        return x ~ " prefix:<$>";           # should be: ! & $ (tightest)
    }

    say($&"application order:"!);           #   outputs: application order: prefix:<&> postfix:<!> prefix:<$>
                                            # should be: application order: prefix:<&> prefix:<$> postfix:<!>
}

Prefixes never compete for precedence on their own, because syntactically they get a natural nesting and inside-out evaluation order. But they can compete via a postfix, since prefixes and postfixes share a precedence space.

The statement block above is exactly so that an opscope cloning will happen and we will lose our prepostfix-boundary (aka "the bug"). Had we kept that boundary, prefix:<$> would have been installed to the right of (=tighter than) prefix:<&>. But now it gets installed on its left, and the print statement demonstrates that.

Phew! No wonder we didn't have a test for this!

In fact, we could exploit our tightest built-in prefix op for the same effect, with a bit less code:

sub postfix:<!>(x) is looser(prefix:<^>) {  #  op order: ! ^ (tightest)
    return [];
}

sub prefix:<$>(x) {                         #  op order: $ ! ^ (tightest)
    return x.elems();                       # should be: ! ^ $ (tightest)
}

say($^5!);                                  #   outputs: 0
                                            # should be: []

...but this code is possibly coupled to the default built-ins, which might after all change in the future for good reasons. I think I like the longer test better.

Ok, that was a lot of talking. The task here is simple: add the test. See it fail. Make $!prepostfix-boundary public, so that it doesn't get clobbered during a clone. See the test pass. So it's a test and a one-line fix.

@vendethiel
Copy link
Collaborator

does this go in t/parsing? :P

@masak
Copy link
Owner Author

masak commented Aug 30, 2016

My first instinct would be to put it in t/features/custom-ops.t. But I could also see it going in a new t/features/prepostfix-boundary.t.

@vendethiel
Copy link
Collaborator

I'm currently putting it in the former, because it seems to make more sense. If you want it to be a separated file, I don't think it should be in t/features, maybe something more along or t/reg or so...

@vendethiel
Copy link
Collaborator

Ah, obviously it wasn't going to be that easy.

"Fixing" the bug by making the variable public gives:

ot ok 29 - prefix is looser by default

# Failed test 'prefix is looser by default'
# at ~/007/lib/_007/Test.pm (_007::Test) line 334
# expected: 'prefix is looser
# prefix is looser
# '
#      got: 'postfix is looser
# prefix is looser
# '

@masak
Copy link
Owner Author

masak commented Aug 30, 2016

Confirmed locally.

I added some debugging info. The numbers denote how $!prepostfix-boundary changes when installing the operator. The rest of the line is a list of the currently installed operators (after installing the latest one).

Here's a normal run, without the change. The first six lines are from the bulitins (and should probably not happen through that code path, see #185). The last four lines are the test.

0->1: (-)
1->2: (- !)
2->3: (- ! ^)
3->3: (- ! ^ [])
3->3: (- ! ^ [] ())
3->3: (- ! ^ [] () .)
0->1: (? - ! ^ [] () .)
1->1: (? - ! ^ [] () . !)
1->1: (? - ! ^ [] () . ! $)
1->2: (? % - ! ^ [] () . ! $)
ok 1 - prefix is looser by default
1..1

Here's a run with the change.

0->1: (-)
1->2: (- !)
2->3: (- ! ^)
3->3: (- ! ^ [])
3->3: (- ! ^ [] ())
3->3: (- ! ^ [] () .)
3->4: (- ! ^ ? [] () .)
4->4: (- ! ^ ? [] () . !)
4->4: (- ! ^ ? [] () . ! $)
4->5: (- ! ^ ? % [] () . ! $)
not ok 1 - prefix is looser by default

# Failed test 'prefix is looser by default'
# at /home/masak/ours/007/lib/_007/Test.pm (_007::Test) line 334
# expected: 'prefix is looser
# prefix is looser
# '
#      got: 'postfix is looser
# prefix is looser
# '
1..1
# Looks like you failed 1 test of 1

I haven't tried to analyze what goes wrong yet. Just posting these debug outputs first. I'll get back to you when I know more.

masak pushed a commit that referenced this issue Aug 30, 2016
This shouldn't be necessary, and by some alpha-renaming argument
is basically a no-op. Still, in #189
we hit some strangeness where the expression parser seems to confuse
prefix:<!> with postfix:<!>. Will investigate that further, of
course. But this fixes it, short term.
@masak
Copy link
Owner Author

masak commented Aug 30, 2016

I found the problem. Pushed a fix with 4423c0a. Try now.

The debug output gets everything right, and so the problem is not found there. But looking at it put be on the right track: what if something in 007 gets prefix:<!> (builtin) and postfix:<!> (custom) mixed up? The former is indeed looser than all our custom operators.

Anyway, now your fix should apply well. I'll open a new issue for the prefix:<!> vs postfix:<!> shenanigans.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants