Line numbers offset when signature has spaces in it

[schwern]

When the signature is not all on one line, the line numbers will be offset.

func hi(
    Str $who //= "World"
) {
    die $who;
}

Method::Signatures always acts like the signature is on one line.

[schwern]

I'm going to fix this and then would like to make a release if the alpha is going ok.

[schwern]

Checking back through the releases, this bug has always been there. I guess it's only now that signatures have gotten more complex that there's been a widespread need for multi-line signatures.

[schwern]

I'm weighing two solutions to the problem.

1. Faithfully reproduce the newlines in the generated signature
2. Add a #line before the body of the subroutine

I believe the first is necessary. Why? Consider the following associated bug.

use Method::Signatures;
use Carp;

func hello(
    $who = "World",
    $greeting = get_greeting()
) {
    die "$greeting, $who";
}

sub get_greeting {
    croak "Go away";
}

hello();
# Go away at /Users/schwern/tmp/test.plx line 9.
#   main::get_greeting() called at /Users/schwern/tmp/test.plx line 4
#   main::hello() called at /Users/schwern/tmp/test.plx line 12

In addition to throwing off all the line numbers after the signature, the call to get_greeting within the signature is incorrectly said to happen at line 4 (the beginning of the signature) when it should be line 6.

Reproducing the newlines will also dodge the perpetual problem of overflowing Devel::Declare's line buffer.

[schwern]

It turns out Devel::Declare::set_linestr() can't have newlines in it. Perl stops processing at the newline.

Not sure how to implement this on one line.

This also means defaults which span more than one line don't work.

[barefootcoder]

This is essentially a duplicate of RT/87544. Although I seem not to have gone back and added a comment there with my results (bad barefootcoder), my recollection was that I traced the issue back to DD using skipspace, which in turn uses the actual Perl code that skips whitespace. And, looking at that, it doesn't even notice if some of that whitespace is newlines. So the initial challenge seemed to be how to figure out we were skipping newlines in the first place, at least in the general case.

But it's been a while since I looked at it. I definitely never got as far as trying to stick newlines into the generated code. But if you figured out a way around the above and know how many newlines you need to stick in, can't you just call set_linestr more than once? Failing that, perhaps we could fallback on the #line trick. Just throwing out ideas here.

[schwern]

I believe set_linestr only sets the PL_linestr variable, it doesn't crank the parser. That's why code will almost always get_linestr, alter the result, and then set_linestr. I'm not sure at what point Devel::Declare invokes the Perl parser.

It's right in the DD documentation that newlines are no good.

       "set_linestr"

       This builtin sets the full text of the current line of the source
       document.  Beware that injecting a newline into the middle of the line
       is likely to fail in surprising ways.  Generally, Perl's parser can
       rely on the `current line' actually being only a single line.  Use
       other kinds of whitespace instead, in the code that you inject.

The #line trick was my plan, but that requires... wait for it... a newline! Unless we put all the generated code in an eval statement with literal "\n"; thinking about that makes me want to lie down.

Maybe Stack Overflow will figure it out.

[schwern]

Stack Overflow figured it out!

package Foo;

use strict;
use warnings;
use v5.12;

use parent "Devel::Declare::MethodInstaller::Simple";

sub import {
    my $class = shift;
    my $caller = caller;

    $class->install_methodhandler(
        into            => $caller,
        name            => 'method'
    );
}

sub __empty () { die }

sub parse_proto {
    my $self = shift;
    return q[print __LINE__."\n"; Foo::__empty(
) if 0; print __LINE__."\n";];
}

1;

The parser can be tricked into continuing past a newline if it's inside a function call argument list. if 0 means the function call is compiled away. Ta da!

[barefootcoder]

Okay, so remember how, above, I said:

... I traced the issue back to DD using skipspace, which in turn uses the actual Perl code that skips whitespace. And, looking at that, it doesn't even notice if some of that whitespace is newlines. So the initial challenge seemed to be how to figure out we were skipping newlines in the first place, at least in the general case.

I still believe this is the case. The solution that you (@schwern) and @tobyink have come up with is freakin' brilliant, don't get me wrong. But it's limited. You're putting back all the newlines that you know about. Which equates to "all the newlines in the signature itself" (i.e., between the parends). This doesn't help if the extranneous newlines are between the new keyword and the open parend, or (my personal favorite) between the close parend and the open curly brace. I've cooked up a gist which you can use the see the problem more clearly. Out of those 7 examples, this pull request is only going to fix one of them (the first one).

Which is still a huge improvement over what we had before. But not sufficient to call this issue closed, I don't think.

We finally know what to do once we figure out how many newlines there are--I never even got this far, personally. So I'm pretty stoked at this progress. But we still don't konw how to figure out how many newlines there are. I think (I'm pretty sure) this would require a patch to DD itself. Which is not a bad thing, certainly: every module based on DD (e.g. MooseX::Declare, TryCatch) has this exact problem, and they would all benefit from such a patch, in combination with this very creative solution. I've just never had time to dig into the DD guts and figure out how/where to do that. And then there's the question of whether a patch would even get accepted; ether is extremely fond of telling me that mst has declared DD to be a "big bag of crack" and everyone should stop using it now. And ether, as you may or may not know, is the current maintainer of DD. But maybe I could talk her into it, if one of us can scrap up the tuits to actually figure out how to do it.

My inclination is to add my proposed .t file with a skip_all in it for now (or I guess a TODO block would be better), so that we preserve the tests without creating failures, but leave this issue (and of course RT/87544) open for future tracking. But I'm open to suggestions here.

Meanwhile I'll go ahead and send this pull request (and the others recently merged) up to PAUSE as a new dev version.

[schwern]

I would recommend closing this issue and opening ones for each of the
examples that fail, in addition to the TODO block. That's much easier
to track the likely incremental progress we'll have on this.

I agree with Ether that DD is a big bag of crack, but it's the least
cracky of the very small set of alternatives. I will be happy to stop
using it when there's a working alternative (I think I saw one based on
the new API hooks introduced recently). Until someone comes up with
one, we don't have a lot of choice. Or she could hand off maintenance.