Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.94k stars 554 forks source link

Re: printf warns about too few arguments, but not too many #13540

Closed p5pRT closed 10 years ago

p5pRT commented 10 years ago

Migrated from rt.perl.org#121036 (status was 'rejected')

Searchable as RT121036$

p5pRT commented 10 years ago

From @avar

On Fri\, Jan 17\, 2014 at 10​:17 PM\, \avar@​cpan\.org wrote​:

For a long time perl has warned about too few arguments in printf​:

$ perl \-wle 'print $\]; printf "%s%s"\, qw\(a\)'
5\.019006
Missing argument in printf at \-e line 1\.
a

I've now just fixed a long-standing 6 year old bug in a codebase I maintain that comes down to not warning about unused printf arguments​:

$ perl \-wle 'print $\]; printf "%s%s"\, qw\(a b c\)'
5\.019006
ab

It would be very nice if perl were to have a warning for this. Although this might be intentional by alternating the printf format itself using a conditional\, it's much more likely that it's a symtom of something that's a logic error.

I've pushed the avar/printf-too-many-arguments branch to perl5.git which has a complete patch that implements this\, and as far as I can tell addresses all the concerns raised in this thread. Check it out at​:

http​://perl5.git.perl.org/perl.git/commitdiff/c299833c0ba6b2c1a04c7b98df90305d4d72b02d

I did some experimentation on splitting up the "all" warning category into "all" and "new" to address the concern Dave Mitchell raised. If that patch worked we could merge this warning in before 5.20.0\, since "use warnings" wouldn't turn it on by default\, you'd have to explicitly do "use warnings 'new'"\, or "use warnings 'redundant'" (currently the only thing in "new").

But I haven't gotten that working yet. Here's the WIP patch for that​:

http​://perl5.git.perl.org/perl.git/commitdiff/21781e853aa567a1e7c75462f3912a4ef1c4d7db

In the process of implementing this I also made a new "missing" category for the existing "Missing argument in %s" warning​:

http​://perl5.git.perl.org/perl.git/commitdiff/d4e7a8a601922309865854af2ebc7cd826fcd9ac

Before it was piggy-backing on the "uninitialized" category. I'm wondering whether I should merge that into blead.

It would "break" (for some value of) "no warnings qw(uninitalized)" on printf formats that have too few arguments\, i.e. someone that's explicitly silenced that warning in the past\, but would otherwise work just like the current behavior in blead. I think it should be merged before 5.20.0.

p5pRT commented 10 years ago

From @jkeenan

Avar indicated on mailing list that this ticket was opened by mistake. Closing.

p5pRT commented 10 years ago

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

p5pRT commented 10 years ago

@jkeenan - Status changed from 'open' to 'rejected'

p5pRT commented 10 years ago

From @iabyn

On Sun\, Jan 19\, 2014 at 12​:54​:15PM -0800\, Ævar Arnfjörð Bjarmason wrote​:

I did some experimentation on splitting up the "all" warning category into "all" and "new" to address the concern Dave Mitchell raised. If that patch worked we could merge this warning in before 5.20.0\, since "use warnings" wouldn't turn it on by default\, you'd have to explicitly do "use warnings 'new'"\, or "use warnings 'redundant'" (currently the only thing in "new").

I'm not sure how such mechanism would work in the long term. If someone writes some code that runs under 5.20 and does 'use warnings "new"'\, then will any new warnings we introduce in 5.21.x be enabled under '"new"? If so\, that user's code will then break under 5.22. So we'll have to add the 5.21.x warnings under "newer"\, and the 5.23.x warnings under "even_newer" etc.

So I don't think "new" is a good idea.

I'd suggest we either leave warnings as they always have been (so there's always a risk of existing code triggering new warnings on a newer perl)\, or we do some sort of system based on version number\, e.g. tied to\, or related to\, "use 5.20" or similar.

-- Little fly\, thy summer's play my thoughtless hand has terminated with extreme prejudice.   (with apologies to William Blake)

p5pRT commented 10 years ago

From @avar

On Wed\, Jan 22\, 2014 at 12​:39 PM\, Dave Mitchell \davem@​iabyn\.com wrote​:

On Sun\, Jan 19\, 2014 at 12​:54​:15PM -0800\, Ęvar Arnfjörš Bjarmason wrote​:

I did some experimentation on splitting up the "all" warning category into "all" and "new" to address the concern Dave Mitchell raised. If that patch worked we could merge this warning in before 5.20.0\, since "use warnings" wouldn't turn it on by default\, you'd have to explicitly do "use warnings 'new'"\, or "use warnings 'redundant'" (currently the only thing in "new").

I'm not sure how such mechanism would work in the long term. If someone writes some code that runs under 5.20 and does 'use warnings "new"'\, then will any new warnings we introduce in 5.21.x be enabled under '"new"? If so\, that user's code will then break under 5.22. So we'll have to add the 5.21.x warnings under "newer"\, and the 5.23.x warnings under "even_newer" etc.

So I don't think "new" is a good idea.

I'd suggest we either leave warnings as they always have been (so there's always a risk of existing code triggering new warnings on a newer perl)\, or we do some sort of system based on version number\, e.g. tied to\, or related to\, "use 5.20" or similar.

Agreed that it shouldn't be called "new"\, should be versioned etc.

I wasn't actually advocating for that warning category\, I agree it's stupid. I was observing that we have an inherent issue with introducing new warnings as long as any new warning will be turned on by default for existing "use warnings" statements.

This means that in practice we can only ever introduce warnings for things that are almost definitely issues\, not something like what your average C compiler has​:

  use warnings qw(all extra pedantic);