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

overeager sprintf missing argument warnings #14092

Open
p5pRT opened this issue Sep 16, 2014 · 8 comments
Open

overeager sprintf missing argument warnings #14092

p5pRT opened this issue Sep 16, 2014 · 8 comments

Comments

@p5pRT
Copy link

p5pRT commented Sep 16, 2014

Migrated from rt.perl.org#122793 (status was 'open')

Searchable as RT122793$

@p5pRT
Copy link
Author

p5pRT commented Sep 16, 2014

From @demerphq

If we have an invalid format conversion we should not complain it is
missing an argument​:

$ perl -wE'say sprintf "%/"'
Missing argument in sprintf at -e line 1.
Invalid conversion in sprintf​: "%/" at -e line 1.
%/

I contend that the first warning should not be produced. If the format
conversion is unknown then it probably isn't intended to have an argument.

Similarly we should not warn multiple times about missing arguments in
sprintf. Once should be enough.

perl -wE'say sprintf "%s%s"'
Missing argument in sprintf at -e line 1.
Missing argument in sprintf at -e line 1.

My perl -v is as follows​:

This is perl 5, version 14, subversion 2 (v5.14.2) built for
x86_64-linux-gnu-thread-multi

Although this applies to bleadperl as well.

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2014

From @avar

On þri 16 sep 15​:20​:31 2014, demerphq wrote​:

If we have an invalid format conversion we should not complain it is
missing an argument​:

$ perl -wE'say sprintf "%/"'
Missing argument in sprintf at -e line 1.
Invalid conversion in sprintf​: "%/" at -e line 1.
%/

I contend that the first warning should not be produced. If the format
conversion is unknown then it probably isn't intended to have an argument.

Similarly we should not warn multiple times about missing arguments in
sprintf. Once should be enough.

perl -wE'say sprintf "%s%s"'
Missing argument in sprintf at -e line 1.
Missing argument in sprintf at -e line 1.

My perl -v is as follows​:

This is perl 5, version 14, subversion 2 (v5.14.2) built for
x86_64-linux-gnu-thread-multi

Although this applies to bleadperl as well.

FWIW since you mentioned this as possibly being related to my recent
v5.21.1-11-g4077a6b on #p5p it's not, that's adding the "redundant"
warning, as you note this has been here for a while. Although the
issue you note with the multiple warnings also exists for the
"redundant" warning.

These classes of warnings could definitely be improved, just some
general points to consider​:

* I think we don't want to have a warning system where we cause users
  to play warning whack-a-mole, if we detect issues we should display
  as many of them upfront as possible.

  I.e. if you make this mistake, wanting %s​:

  $ ./perl -we 'printf "Hello %S"'
  Missing argument in printf at -e line 1.
  Invalid conversion in printf​: "%S" at -e line 1.
  Hello %S

  I think it arguably makes more sense to warn about both the
  missing argument *and* the invalid converson, if you want a
  literal % you have to escape it, so maybe the user meant to use a
  real format.

* I think rather than collapsing this warning and only warning once
  about missing arguments it would be more useful to make the warning
  more specific, i.e. note the difference between these two​:

  $ ./perl -Ilib -wE 'printf "%S%/%\\"'
  Missing argument in printf at -e line 1.
  Invalid conversion in printf​: "%S" at -e line 1.
  Missing argument in printf at -e line 1.
  Invalid conversion in printf​: "%/" at -e line 1.
  Missing argument in printf at -e line 1.
  Invalid conversion in printf​: "%\" at -e line 1.
  %S%/%\

  IMO it would be more useful to say which invalid pseudo-format is
  "missing" its argument than collapsing it into one warning,
  possibly causing the user to play warning whack-a-mole.

Finally just as an implementation detail if we were to fold these
warnings getting this done requires some hacking in
Perl_sv_vcatpvfn_flags, now it just parses the format/arguments as a
stream and warns as it goes along, it would have to accumulate issues
and spew them out all at once before it returns.

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2014

From lasse.makholm@gmail.com

On 21 September 2014 19​:34, Ævar Arnfjörð Bjarmason via RT
<perlbug-followup@​perl.org> wrote​:

On þri 16 sep 15​:20​:31 2014, demerphq wrote​:

If we have an invalid format conversion we should not complain it is
missing an argument​:

$ perl -wE'say sprintf "%/"'
Missing argument in sprintf at -e line 1.
Invalid conversion in sprintf​: "%/" at -e line 1.
%/

I contend that the first warning should not be produced. If the format
conversion is unknown then it probably isn't intended to have an argument.

Similarly we should not warn multiple times about missing arguments in
sprintf. Once should be enough.

perl -wE'say sprintf "%s%s"'
Missing argument in sprintf at -e line 1.
Missing argument in sprintf at -e line 1.

My perl -v is as follows​:

This is perl 5, version 14, subversion 2 (v5.14.2) built for
x86_64-linux-gnu-thread-multi

Although this applies to bleadperl as well.

FWIW since you mentioned this as possibly being related to my recent
v5.21.1-11-g4077a6b on #p5p it's not, that's adding the "redundant"
warning, as you note this has been here for a while. Although the
issue you note with the multiple warnings also exists for the
"redundant" warning.

These classes of warnings could definitely be improved, just some
general points to consider​:

* I think we don't want to have a warning system where we cause users
to play warning whack-a-mole, if we detect issues we should display
as many of them upfront as possible.

I.e. if you make this mistake, wanting %s​:

    $ \./perl \-we 'printf "Hello %S"'
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%S" at \-e line 1\.
    Hello %S

I think it arguably makes more sense to warn about both the
missing argument \*and\* the invalid converson\, if you want a
literal % you have to escape it\, so maybe the user meant to use a
real format\.

* I think rather than collapsing this warning and only warning once
about missing arguments it would be more useful to make the warning
more specific, i.e. note the difference between these two​:

Repeating the missing argument warning doesn't make sense IMO. Consider​:

printf "%d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d\n ", @​stuff;

In that case, a message like​:

Missing 15 arguments in printf; expected 16, got 1

Is a lot more useful and results in fewer moles to be whacked than
"Missing argument in printf" repeated ad nauseum...

    $ \./perl \-Ilib \-wE 'printf "%S%/%\\\\"'
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%S" at \-e line 1\.
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%/" at \-e line 1\.
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%\\" at \-e line 1\.
    %S%/%\\

IMO it would be more useful to say which invalid pseudo\-format is
"missing" its argument than collapsing it into one warning\,
possibly causing the user to play warning whack\-a\-mole\.

That doesn't make sense IMO. Invalid conversions don't consume
arguments because they are invalid. Further, you can't know whether
they were even intended to be conversions or not. Since arguments are
consumed left-to-right, any missing arguments, by definition, are
gonna be at the end of the format string.

Perl (v5.14.2) itself seems indecisive as to whether an invalid
conversion consumes an argument or not​:

$ perl -w -e 'printf "%s%S%s\n", 1, 2'
Invalid conversion in printf​: "%S" at -e line 1.
1%S2
$ perl -w -e 'printf "%s%S%s\n", 1'
Missing argument in printf at -e line 1.
Invalid conversion in printf​: "%S" at -e line 1.
Missing argument in printf at -e line 1.
1%S
$

Why, by removing one arguement, do we jump from from 0 to 2 missing
arguments? I'd argue that this is a bug and that the second case
should only warn about one missing argument.

/L

Finally just as an implementation detail if we were to fold these
warnings getting this done requires some hacking in
Perl_sv_vcatpvfn_flags, now it just parses the format/arguments as a
stream and warns as it goes along, it would have to accumulate issues
and spew them out all at once before it returns.

---
via perlbug​: queue​: perl5 status​: new
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=122793

@p5pRT
Copy link
Author

p5pRT commented Sep 22, 2014

From @demerphq

On 21 September 2014 22​:22, Lasse Makholm <lasse.makholm@​gmail.com> wrote​:

On 21 September 2014 19​:34, Ævar Arnfjörð Bjarmason via RT
<perlbug-followup@​perl.org> wrote​:

On þri 16 sep 15​:20​:31 2014, demerphq wrote​:

If we have an invalid format conversion we should not complain it is
missing an argument​:

$ perl -wE'say sprintf "%/"'
Missing argument in sprintf at -e line 1.
Invalid conversion in sprintf​: "%/" at -e line 1.
%/

I contend that the first warning should not be produced. If the format
conversion is unknown then it probably isn't intended to have an
argument.

Similarly we should not warn multiple times about missing arguments in
sprintf. Once should be enough.

perl -wE'say sprintf "%s%s"'
Missing argument in sprintf at -e line 1.
Missing argument in sprintf at -e line 1.

My perl -v is as follows​:

This is perl 5, version 14, subversion 2 (v5.14.2) built for
x86_64-linux-gnu-thread-multi

Although this applies to bleadperl as well.

FWIW since you mentioned this as possibly being related to my recent
v5.21.1-11-g4077a6b on #p5p it's not, that's adding the "redundant"
warning, as you note this has been here for a while. Although the
issue you note with the multiple warnings also exists for the
"redundant" warning.

These classes of warnings could definitely be improved, just some
general points to consider​:

* I think we don't want to have a warning system where we cause users
to play warning whack-a-mole, if we detect issues we should display
as many of them upfront as possible.

I.e. if you make this mistake, wanting %s​:

    $ \./perl \-we 'printf "Hello %S"'
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%S" at \-e line 1\.
    Hello %S

I think it arguably makes more sense to warn about both the
missing argument \*and\* the invalid converson\, if you want a
literal % you have to escape it\, so maybe the user meant to use a
real format\.

* I think rather than collapsing this warning and only warning once
about missing arguments it would be more useful to make the warning
more specific, i.e. note the difference between these two​:

Repeating the missing argument warning doesn't make sense IMO. Consider​:

printf "%d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d\n ", @​stuff;

In that case, a message like​:

Missing 15 arguments in printf; expected 16, got 1

Is a lot more useful and results in fewer moles to be whacked than
"Missing argument in printf" repeated ad nauseum...

Yeah I agree.

    $ \./perl \-Ilib \-wE 'printf "%S%/%\\\\"'
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%S" at \-e line 1\.
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%/" at \-e line 1\.
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%\\" at \-e line 1\.
    %S%/%\\

IMO it would be more useful to say which invalid pseudo\-format is
"missing" its argument than collapsing it into one warning\,
possibly causing the user to play warning whack\-a\-mole\.

That doesn't make sense IMO. Invalid conversions don't consume
arguments because they are invalid. Further, you can't know whether
they were even intended to be conversions or not. Since arguments are
consumed left-to-right, any missing arguments, by definition, are
gonna be at the end of the format string.

Again I agree.

Perl (v5.14.2) itself seems indecisive as to whether an invalid
conversion consumes an argument or not​:

$ perl -w -e 'printf "%s%S%s\n", 1, 2'
Invalid conversion in printf​: "%S" at -e line 1.
1%S2
$ perl -w -e 'printf "%s%S%s\n", 1'
Missing argument in printf at -e line 1.
Invalid conversion in printf​: "%S" at -e line 1.
Missing argument in printf at -e line 1.
1%S
$

Why, by removing one arguement, do we jump from from 0 to 2 missing
arguments? I'd argue that this is a bug and that the second case
should only warn about one missing argument.

This is getting repetitive. I agree again.

Yves

@p5pRT
Copy link
Author

p5pRT commented Sep 22, 2014

From @avar

On Sun, Sep 21, 2014 at 10​:22 PM, Lasse Makholm <lasse.makholm@​gmail.com> wrote​:

On 21 September 2014 19​:34, Ævar Arnfjörð Bjarmason via RT
<perlbug-followup@​perl.org> wrote​:

On þri 16 sep 15​:20​:31 2014, demerphq wrote​:

If we have an invalid format conversion we should not complain it is
missing an argument​:

$ perl -wE'say sprintf "%/"'
Missing argument in sprintf at -e line 1.
Invalid conversion in sprintf​: "%/" at -e line 1.
%/

I contend that the first warning should not be produced. If the format
conversion is unknown then it probably isn't intended to have an argument.

Similarly we should not warn multiple times about missing arguments in
sprintf. Once should be enough.

perl -wE'say sprintf "%s%s"'
Missing argument in sprintf at -e line 1.
Missing argument in sprintf at -e line 1.

My perl -v is as follows​:

This is perl 5, version 14, subversion 2 (v5.14.2) built for
x86_64-linux-gnu-thread-multi

Although this applies to bleadperl as well.

FWIW since you mentioned this as possibly being related to my recent
v5.21.1-11-g4077a6b on #p5p it's not, that's adding the "redundant"
warning, as you note this has been here for a while. Although the
issue you note with the multiple warnings also exists for the
"redundant" warning.

These classes of warnings could definitely be improved, just some
general points to consider​:

* I think we don't want to have a warning system where we cause users
to play warning whack-a-mole, if we detect issues we should display
as many of them upfront as possible.

I.e. if you make this mistake, wanting %s​:

    $ \./perl \-we 'printf "Hello %S"'
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%S" at \-e line 1\.
    Hello %S

I think it arguably makes more sense to warn about both the
missing argument \*and\* the invalid converson\, if you want a
literal % you have to escape it\, so maybe the user meant to use a
real format\.

* I think rather than collapsing this warning and only warning once
about missing arguments it would be more useful to make the warning
more specific, i.e. note the difference between these two​:

Repeating the missing argument warning doesn't make sense IMO. Consider​:

printf "%d %d %d %d %d %d %d %d %d %d %d %d %d %d %d %d\n ", @​stuff;

In that case, a message like​:

Missing 15 arguments in printf; expected 16, got 1

Is a lot more useful and results in fewer moles to be whacked than
"Missing argument in printf" repeated ad nauseum...

Yes, that would of course be more useful, since you don't get into
playing warning whack-a-mole.

I'm saying that we shouldn't fix it by only printing the first
occurrence of the *current* warning we have instead of (in your
pathalogical case) printing it 15 times, of course if we emit a
warning saying 15 arguments are missing that's fine since there's no
loss of information, and you're conveying the same information with
more concisely.

    $ \./perl \-Ilib \-wE 'printf "%S%/%\\\\"'
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%S" at \-e line 1\.
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%/" at \-e line 1\.
    Missing argument in printf at \-e line 1\.
    Invalid conversion in printf&#8203;: "%\\" at \-e line 1\.
    %S%/%\\

IMO it would be more useful to say which invalid pseudo\-format is
"missing" its argument than collapsing it into one warning\,
possibly causing the user to play warning whack\-a\-mole\.

That doesn't make sense IMO. Invalid conversions don't consume
arguments because they are invalid. Further, you can't know whether
they were even intended to be conversions or not. Since arguments are
consumed left-to-right, any missing arguments, by definition, are
gonna be at the end of the format string.

Perl (v5.14.2) itself seems indecisive as to whether an invalid
conversion consumes an argument or not​:

$ perl -w -e 'printf "%s%S%s\n", 1, 2'
Invalid conversion in printf​: "%S" at -e line 1.
1%S2
$ perl -w -e 'printf "%s%S%s\n", 1'
Missing argument in printf at -e line 1.
Invalid conversion in printf​: "%S" at -e line 1.
Missing argument in printf at -e line 1.
1%S
$

Why, by removing one arguement, do we jump from from 0 to 2 missing
arguments? I'd argue that this is a bug and that the second case
should only warn about one missing argument.

/L

Finally just as an implementation detail if we were to fold these
warnings getting this done requires some hacking in
Perl_sv_vcatpvfn_flags, now it just parses the format/arguments as a
stream and warns as it goes along, it would have to accumulate issues
and spew them out all at once before it returns.

---
via perlbug​: queue​: perl5 status​: new
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=122793

@p5pRT
Copy link
Author

p5pRT commented Sep 22, 2014

From @jhi

On Sunday-201409-21, 13​:34, Ævar Arnfjörð Bjarmason via RT wrote​:

Finally just as an implementation detail if we were to fold these
warnings getting this done requires some hacking in
Perl_sv_vcatpvfn_flags, now it just parses the format/arguments as a
stream and warns as it goes along, it would have to accumulate issues
and spew them out all at once before it returns.

FWIW, while doing my recent floating point, ahem, renovations I've
come to dislike sv_catpvfn_flags quite a bit because of its streaming
approach.

@p5pRT
Copy link
Author

p5pRT commented Sep 22, 2014

From @demerphq

On 22 September 2014 17​:11, Jarkko Hietaniemi <jhi@​iki.fi> wrote​:

On Sunday-201409-21, 13​:34, Ævar Arnfjörð Bjarmason via RT wrote​:

Finally just as an implementation detail if we were to fold these
warnings getting this done requires some hacking in
Perl_sv_vcatpvfn_flags, now it just parses the format/arguments as a
stream and warns as it goes along, it would have to accumulate issues
and spew them out all at once before it returns.

FWIW, while doing my recent floating point, ahem, renovations I've
come to dislike sv_catpvfn_flags quite a bit because of its streaming
approach.

Funny, when I just now looked at fixing this ticket I started thinking the
same thing.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

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

No branches or pull requests

2 participants