Skip Menu |
Report information
Id: 121374
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: jv <JVromans [at] Squirrel.nl>
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type:
Perl Version: 5.19.10
Fixed In: 5.22.0



Subject: Wrong line number in signature errors
Download (untitled) / with headers
text/plain 492b
Signature errors like "Too few arguments for subroutine" are shown with the line number of the subroutine definition. They should show the line number of the actual subroutine call. The attached program shows: The signatures feature is experimental at /home/jv/tmp/t.pl line 7. This is correct, for that's the line the subroutine using the signature is defined. But then: Testing call at line 13 Too few arguments for subroutine at /home/jv/tmp/t.pl line 7. This should be line 13.
Subject: t.pl
Download t.pl
text/x-perl 233b
#!/usr/bin/perl use strict; use warnings; use feature qw(signatures); sub foo( $arg ) { print "ARG = $arg\n" } print "Testing call at line ", 1+__LINE__, "\n"; foo("bar"); print "Testing call at line ", 1+__LINE__, "\n"; foo();
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 951b
On Tue Mar 04 01:17:31 2014, jv wrote: Show quoted text
> Signature errors like "Too few arguments for subroutine" are shown > with the line number of the subroutine definition. They should show > the line number of the actual subroutine call. > > The attached program shows: > > The signatures feature is experimental at /home/jv/tmp/t.pl line 7. > > This is correct, for that's the line the subroutine using the > signature is defined. But then: > > Testing call at line 13 > Too few arguments for subroutine at /home/jv/tmp/t.pl line 7. > > This should be line 13.
Confirmed as present in blead. ##### $ ./perl -v | head -2 | tail -1 This is perl 5, version 21, subversion 5 (v5.21.5 (v5.21.4-63-g0c7df90)) built for x86_64-linux ##### This RT was not received any comment since it was filed 6 months ago. Do people agree that we have a problem? Can someone take a crack at providing a solution? Thank you very much. -- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.6k
On Tue Sep 23 18:42:40 2014, jkeenan wrote: Show quoted text
> On Tue Mar 04 01:17:31 2014, jv wrote:
> > Signature errors like "Too few arguments for subroutine" are shown > > with the line number of the subroutine definition. They should show > > the line number of the actual subroutine call. > > > > The attached program shows: > > > > The signatures feature is experimental at /home/jv/tmp/t.pl line 7. > > > > This is correct, for that's the line the subroutine using the > > signature is defined. But then: > > > > Testing call at line 13 > > Too few arguments for subroutine at /home/jv/tmp/t.pl line 7. > > > > This should be line 13.
> > Confirmed as present in blead. > ##### > $ ./perl -v | head -2 | tail -1 > This is perl 5, version 21, subversion 5 (v5.21.5 (v5.21.4-63- > g0c7df90)) built for x86_64-linux > ##### > > This RT was not received any comment since it was filed 6 months ago. > > Do people agree that we have a problem?
Yes. Show quoted text
> Can someone take a crack at providing a solution?
That’s not so easy. Look how it deparses: $ perl5.21.4 -Mfeature=signatures -MO=Deparse -e 'sub ($){}' The signatures feature is experimental at -e line 1. use feature 'signatures'; sub { die 'Too many arguments for subroutine' unless @_ <= 1; die 'Too few arguments for subroutine' unless @_ >= 1; (); } ; -e syntax OK It’s doing a literal die() there, so the line number is off. Getting the right line number means munging the op tree to use caller or doing something more sneaky like omitting nextstate ops (harder to get right, but should run faster). The code in question is at the bottom of toke.c. My brain is too full to do it right now. -- Father Chrysostomos
From: ilmari [...] ilmari.org (Dagfinn Ilmari Mannsåker)
Date: Wed, 24 Sep 2014 12:07:21 +0100
Subject: Re: [perl #121374] Wrong line number in signature errors
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.3k
"Father Chrysostomos via RT" <perlbug-followup@perl.org> writes: Show quoted text
> On Tue Sep 23 18:42:40 2014, jkeenan wrote:
>> On Tue Mar 04 01:17:31 2014, jv wrote:
>> > Signature errors like "Too few arguments for subroutine" are shown >> > with the line number of the subroutine definition. They should show >> > the line number of the actual subroutine call. >> > >> > The attached program shows: >> > >> > The signatures feature is experimental at /home/jv/tmp/t.pl line 7. >> > >> > This is correct, for that's the line the subroutine using the >> > signature is defined. But then: >> > >> > Testing call at line 13 >> > Too few arguments for subroutine at /home/jv/tmp/t.pl line 7. >> > >> > This should be line 13.
>> >> Confirmed as present in blead. >> ##### >> $ ./perl -v | head -2 | tail -1 >> This is perl 5, version 21, subversion 5 (v5.21.5 (v5.21.4-63- >> g0c7df90)) built for x86_64-linux >> ##### >> >> This RT was not received any comment since it was filed 6 months ago. >> >> Do people agree that we have a problem?
> > Yes.
Indeed, this is one of the things that need to be fixed before it can lose its experimental status. I just noticed that although L<perlsub/Signatures> describes the feature as experimental, it's not listed in L<perlexperiment>. -- "A disappointingly low fraction of the human race is, at any given time, on fire." - Stig Sandbeck Mathisen
Subject: Re: [perl #121374] Wrong line number in signature errors
Date: Wed, 24 Sep 2014 11:02:18 -0400
From: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 373b
* Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> [2014-09-24T07:07:21] Show quoted text
> Indeed, this is one of the things that need to be fixed before it can > lose its experimental status. I just noticed that although > L<perlsub/Signatures> describes the feature as experimental, it's not > listed in L<perlexperiment>.
Good grief! Thanks for noting that. I'll sort it out. -- rjbs
Download signature.asc
application/pgp-signature 473b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.4k
On Tue Sep 23 19:16:23 2014, sprout wrote: Show quoted text
> > Getting the right line number means munging the op tree to use caller > or doing something more sneaky like omitting nextstate ops (harder to > get right, but should run faster). > > The code in question is at the bottom of toke.c. My brain is too full > to do it right now.
Here's a patch to do it the easy way. Instead of die "Too many arguments in subroutine" if @_ > 1; it generates die sprintf "Too many arguments in subroutine at %s line %d.\n", (caller)[1, 2] if @_ > 1; What's missing is tests for it. I could - create a new test file - add 2 or 3 tests to t/op/signatures.t - convert the existing tests in t/op/signatures.t from string eval to block eval and do an exact comparison on $@ Explanation: t/op/signatures.t has a ton of tests that look like sub t006 ($a) { $a || "z" } is prototype(\&t006), undef; is eval("t006()"), undef; like $@, qr/\AToo few arguments for subroutine at/; is eval("t006(0)"), "z"; is eval("t006(456)"), 456; is eval("t006(456, 789)"), undef; like $@, qr/\AToo many arguments for subroutine at/; ... As far as I can tell there's no reason this should use a string eval. With block eval I'd get predictable filenames (i.e. not "(eval 123)") in the error messages, so I could write 'is $@, "..."' instead of 'like'. ... Or I could change all of those to qr/\AToo many arguments for subroutine at \(eval \d+) line 1\.$/. That would keep the string eval but make sure the reported location refers to the call. Suggestions?
Subject: 0001-fix-line-numbers-in-arity-errors-121374.patch
From d84eb559f481c226245cf388b3894a3f2a7f1dda Mon Sep 17 00:00:00 2001 From: Lukas Mai <l.mai@web.de> Date: Mon, 27 Oct 2014 22:32:34 +0100 Subject: [PATCH] fix line numbers in arity errors [#121374] --- toke.c | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/toke.c b/toke.c index f4e8258..f71cfcd 100644 --- a/toke.c +++ b/toke.c @@ -11507,10 +11507,16 @@ Perl_parse_subsignature(pTHX) scalar(newUNOP(OP_RV2AV, 0, newGVOP(OP_GV, 0, PL_defgv))), newSVOP(OP_CONST, 0, newSViv(1))), - newLISTOP(OP_DIE, 0, newOP(OP_PUSHMARK, 0), - newSVOP(OP_CONST, 0, - newSVpvs("Odd name/value argument " - "for subroutine")))); + op_convert_list(OP_DIE, 0, + op_convert_list(OP_SPRINTF, 0, + op_append_list(OP_LIST, + newSVOP(OP_CONST, 0, + newSVpvs("Odd name/value argument for subroutine at %s line %d.\n")), + newSLICEOP(0, + op_append_list(OP_LIST, + newSVOP(OP_CONST, 0, newSViv(1)), + newSVOP(OP_CONST, 0, newSViv(2))), + newOP(OP_CALLER, 0)))))); if (pos != min_arity) chkop = newLOGOP(OP_AND, 0, newBINOP(OP_GT, 0, @@ -11573,9 +11579,16 @@ Perl_parse_subsignature(pTHX) scalar(newUNOP(OP_RV2AV, 0, newGVOP(OP_GV, 0, PL_defgv))), newSVOP(OP_CONST, 0, newSViv(min_arity))), - newLISTOP(OP_DIE, 0, newOP(OP_PUSHMARK, 0), - newSVOP(OP_CONST, 0, - newSVpvs("Too few arguments for subroutine"))))), + op_convert_list(OP_DIE, 0, + op_convert_list(OP_SPRINTF, 0, + op_append_list(OP_LIST, + newSVOP(OP_CONST, 0, + newSVpvs("Too few arguments for subroutine at %s line %d.\n")), + newSLICEOP(0, + op_append_list(OP_LIST, + newSVOP(OP_CONST, 0, newSViv(1)), + newSVOP(OP_CONST, 0, newSViv(2))), + newOP(OP_CALLER, 0))))))), initops); } if (max_arity != -1) { @@ -11586,9 +11599,16 @@ Perl_parse_subsignature(pTHX) scalar(newUNOP(OP_RV2AV, 0, newGVOP(OP_GV, 0, PL_defgv))), newSVOP(OP_CONST, 0, newSViv(max_arity))), - newLISTOP(OP_DIE, 0, newOP(OP_PUSHMARK, 0), - newSVOP(OP_CONST, 0, - newSVpvs("Too many arguments for subroutine"))))), + op_convert_list(OP_DIE, 0, + op_convert_list(OP_SPRINTF, 0, + op_append_list(OP_LIST, + newSVOP(OP_CONST, 0, + newSVpvs("Too many arguments for subroutine at %s line %d.\n")), + newSLICEOP(0, + op_append_list(OP_LIST, + newSVOP(OP_CONST, 0, newSViv(1)), + newSVOP(OP_CONST, 0, newSViv(2))), + newOP(OP_CALLER, 0))))))), initops); } return initops; -- 2.1.2
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.7k
On Mon Oct 27 15:05:43 2014, mauke- wrote: Show quoted text
> On Tue Sep 23 19:16:23 2014, sprout wrote:
> > > > Getting the right line number means munging the op tree to use caller > > or doing something more sneaky like omitting nextstate ops (harder to > > get right, but should run faster). > > > > The code in question is at the bottom of toke.c. My brain is too > > full > > to do it right now.
> > Here's a patch to do it the easy way. Instead of > > die "Too many arguments in subroutine" if @_ > 1; > > it generates > > die sprintf "Too many arguments in subroutine at %s line %d.\n", > (caller)[1, 2] if @_ > 1;
Thank you for the patch. Show quoted text
> What's missing is tests for it. I could > - create a new test file > - add 2 or 3 tests to t/op/signatures.t > - convert the existing tests in t/op/signatures.t from string eval > to block eval and do an exact comparison on $@ > > Explanation: > t/op/signatures.t has a ton of tests that look like > > sub t006 ($a) { $a || "z" } > is prototype(\&t006), undef; > is eval("t006()"), undef; > like $@, qr/\AToo few arguments for subroutine at/; > is eval("t006(0)"), "z"; > is eval("t006(456)"), 456; > is eval("t006(456, 789)"), undef; > like $@, qr/\AToo many arguments for subroutine at/; > ... > > As far as I can tell there's no reason this should use a string eval. > With block eval I'd get predictable filenames (i.e. not "(eval 123)") > in the error messages, so I could write 'is $@, "..."' instead of > 'like'. > > ... Or I could change all of those to qr/\AToo many arguments for > subroutine at \(eval \d+) line 1\.$/. That would keep the string eval > but make sure the reported location refers to the call. > > Suggestions?
Another approach is to add more tests to t/lib/croak/toke. Do it whichever way makes it easiest for you. -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 456b
On Mon Oct 27 15:52:09 2014, sprout wrote: Show quoted text
> On Mon Oct 27 15:05:43 2014, mauke- wrote:
> > ... Or I could change all of those to qr/\AToo many arguments for > > subroutine at \(eval \d+) line 1\.$/. That would keep the string eval > > but make sure the reported location refers to the call. > > > > Suggestions?
> > Another approach is to add more tests to t/lib/croak/toke. > > Do it whichever way makes it easiest for you.
OK! Test patch attached.
Subject: 0001-test-line-numbers-in-arity-errors.patch

Message body is not shown because it is too large.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 697b
On Tue Oct 28 13:31:20 2014, mauke- wrote: Show quoted text
> On Mon Oct 27 15:52:09 2014, sprout wrote:
> > On Mon Oct 27 15:05:43 2014, mauke- wrote:
> > > ... Or I could change all of those to qr/\AToo many arguments for > > > subroutine at \(eval \d+) line 1\.$/. That would keep the string eval > > > but make sure the reported location refers to the call. > > > > > > Suggestions?
> > > > Another approach is to add more tests to t/lib/croak/toke. > > > > Do it whichever way makes it easiest for you.
> > OK! Test patch attached. >
Thank you. Both applied: $ git log --oneline -2 aff539a test line numbers in arity errors a2b5b20 fix line numbers in arity errors [#121374] -- Father Chrysostomos
Subject: Your ticket against Perl 5 has been resolved
Download (untitled) / with headers
text/plain 222b
Thanks for submitting this ticket The issue should be resolved with the release today of Perl v5.22. If you find that the problem persists, feel free to reopen this ticket -- Karl Williamson for the Perl 5 porters team


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org