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

Bleadperl v5.21.6-51-ge41e986 breaks BDB #14353

Closed
p5pRT opened this issue Dec 21, 2014 · 14 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Dec 21, 2014

Migrated from rt.perl.org#123475 (status was 'resolved')

Searchable as RT123475$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2014

From @cpansprout

Formerly the & prototype allowed ‘undef’ as an argument, contrary to the documentation. that changed in v5.21.6-51-ge41e986.

The question is​: Do we need to permit ‘undef’ and adjust the documentation? Or does BDB need to be fixed?

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2014

From @ikegami

On Sun, Dec 21, 2014 at 2​:33 PM, Father Chrysostomos <
perlbug-followup@​perl.org> wrote​:

# New Ticket Created by Father Chrysostomos
# Please include the string​: [perl #123475]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=123475 >

Formerly the & prototype allowed ‘undef’ as an argument, contrary to the
documentation. that changed in v5.21.6-51-ge41e986.

The question is​: Do we need to permit ‘undef’ and adjust the
documentation? Or does BDB need to be fixed?

It feels wrong to allow undef, but I don't have a good argument for or
against.

The only thing I have is that permitting undef would be inconsistent

perl -we"sub f(\@​) { } f(@​a); f(undef);"
Type of arg 1 to main​::f must be array (not undef operator) at -e line 1,
near "undef)"
Execution of -e aborted due to compilation errors.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 21, 2014

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 2, 2015

From @tonycoz

On Sun Dec 21 11​:33​:02 2014, sprout wrote​:

Formerly the & prototype allowed ‘undef’ as an argument, contrary to
the documentation. that changed in v5.21.6-51-ge41e986.

The question is​: Do we need to permit ‘undef’ and adjust the
documentation? Or does BDB need to be fixed?

I think it's a bug fix, and if BDB depends on that bug it needs to be fixed.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 24, 2015

From @andk

also affected


JNQUINTIN/Math-Permute-Array-0.043.tar.gz

(brought to my attention by Slaven Rezić)

--
andreas

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 25, 2015

From @cpansprout

On Tue Mar 24 00​:20​:05 2015, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

also affected
-------------
JNQUINTIN/Math-Permute-Array-0.043.tar.gz

(brought to my attention by Slaven Rezić)

Patch at <https://rt.cpan.org/Ticket/Display.html?id=103041>.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 27, 2015

From @rjbs

I can't easily compile BDB here. Is the fix as simple as changing the PROTOTYPE entry for set_sync_prepare in BDB.xs to ";&" ?

Could someone verify this?

--
rjbs

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 5, 2015

From @iabyn

On Mon, Apr 27, 2015 at 03​:15​:34PM -0700, Ricardo SIGNES via RT wrote​:

I can't easily compile BDB here. Is the fix as simple as changing the
PROTOTYPE entry for set_sync_prepare in BDB.xs to ";&" ?

Not really. It would also have to change the XS implementation to handle
a variable number of args, and still wouldn't handle the case of an
explicit 'undef' arg.

Looking at the usage in BDB, I think an undef value should still be
allowed as an argument to a '&' parameter.

BDB.pm does this as part of its initialisation​:

  set_sync_prepare (undef);

where set_sync_prepare() is used to register a callback. So in this case
it is explicitly initialising its state to "no callback registered". I
think this is a reasonable use of the & prototype.

Looking at the original '&' ticket,

  #123062​: & prototype is too permissive

I think it's correct that it should dis-allow array refs etc, but I think
undef should be re-allowed for 5.22.

--
Dave's first rule of Opera​:
If something needs saying, say it​: don't warble it.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 6, 2015

From @iabyn

On Tue, May 05, 2015 at 04​:33​:21PM +0100, Dave Mitchell wrote​:

On Mon, Apr 27, 2015 at 03​:15​:34PM -0700, Ricardo SIGNES via RT wrote​:

I can't easily compile BDB here. Is the fix as simple as changing the
PROTOTYPE entry for set_sync_prepare in BDB.xs to ";&" ?

Not really. It would also have to change the XS implementation to handle
a variable number of args, and still wouldn't handle the case of an
explicit 'undef' arg.

Looking at the usage in BDB, I think an undef value should still be
allowed as an argument to a '&' parameter.

BDB.pm does this as part of its initialisation​:

set\_sync\_prepare \(undef\);

where set_sync_prepare() is used to register a callback. So in this case
it is explicitly initialising its state to "no callback registered". I
think this is a reasonable use of the & prototype.

Looking at the original '&' ticket,

\#123062&#8203;: & prototype is too permissive

I think it's correct that it should dis-allow array refs etc, but I think
undef should be re-allowed for 5.22.

If no one objects, I'll merge the following commit once it's smoked​:

commit 343d03e
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Wed May 6 11​:56​:47 2015 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Wed May 6 12​:01​:06 2015 +0100

  allow undef as an arg to '&' prototype
 
  RT #123475
 
  Commit e41e986 (to fix [perl #123062]) restricted the types of
  args allowed for a function with a '&' prototype - previously it allowed
  array refs and the like. It also removed undef, so this was now a
  compile-time error​:
 
  sub foo (&) {...}
  foo(undef)
 
  However, some CPAN code used the idiom register_callback(undef) to
  explicitly disable a registered callback.
 
  So re-allow an explicit undef.

--
print+qq&$}$"$/$s$,$a$d$g$s$@​$.$q$,$​:$.$q$^$,$@​$a$$;$.$q$m&if+map{m,^\d{0\,},,${$​::{$'}}=chr($"+=$&||1)}q&10m22,42}6​:17a22.3@​3;^2dg3q/s"&=~m*\d\*.*g

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 6, 2015

From @rjbs

* Dave Mitchell <davem@​iabyn.com> [2015-05-05T11​:33​:21]

I think it's correct that it should dis-allow array refs etc, but I think
undef should be re-allowed for 5.22.

I wrote many, many long responses to this, each one different than the
previous, as I reached logical dead ends. The behavior and intent of
prototypes is, in my opinion, so incoherent as to make any reasoning from first
principles unlikely to bear fruit.

So, instead, I think we have a really simple choice. Here's what the docs say
about (&)​:

  An "&" requires an anonymous subroutine, which, if passed as the first
  argument, does not require the "sub" keyword or a subsequent comma.

The old behavior allows​:

  sub takes_sub (&) { ... }
  takes_sub(undef);

...which contradicts the documentation. We should change the behavior or we
should change the documentation.

Changing the documentation and leaving the code alone (relative to v5.20) will
let things like BDB continue to work, where a literal undef is passed to reset
a stored subroutine.

Changing the code and leaving the documentation alone (again, relative to
v5.20) will break anything that had relied on this behavior, but will expose
bugs where a literal undef had been passed to a subroutine with a (&)
prototype in effect. It will now issue a compile-time error. Presumably,
these situations are bugs iff later something attempts $arg->(), which would
currently be a runtime error. That doesn't mean that this wouldn't expose
existing bugs, but I think it does cut down on what we'd expect to see fixed.
Also, we should consider the prevention of future bugs, which won't ever need
to be detected at runtime.

Nonetheless, it is with heavy heart that I think I agree with the suggested
course of action​: go back to the old, worse behavior. If we were starting
from scratch, I would hold my ground, but I don't think the breakage is
justified here.

Boy, do I ever dislike prototypes!

If anybody wants to change my mind, do so quickly! ;) Or wait to try again in
5.23, I suppose...

--
rjbs

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 8, 2015

From @iabyn

On Wed, May 06, 2015 at 09​:26​:47AM -0400, Ricardo Signes wrote​:

I wrote many, many long responses to this, each one different than the
previous, as I reached logical dead ends. The behavior and intent of
prototypes is, in my opinion, so incoherent as to make any reasoning from first
principles unlikely to bear fruit.

So, instead, I think we have a really simple choice. Here's what the docs say
about (&)​:

An "&" requires an anonymous subroutine, which, if passed as the first
argument, does not require the "sub" keyword or a subsequent comma.

The old behavior allows​:

sub takes_sub (&) { ... }
takes_sub(undef);

...which contradicts the documentation. We should change the behavior or we
should change the documentation.

Changing the documentation and leaving the code alone (relative to v5.20) will
let things like BDB continue to work, where a literal undef is passed to reset
a stored subroutine.

Changing the code and leaving the documentation alone (again, relative to
v5.20) will break anything that had relied on this behavior, but will expose
bugs where a literal undef had been passed to a subroutine with a (&)
prototype in effect. It will now issue a compile-time error. Presumably,
these situations are bugs iff later something attempts $arg->(), which would
currently be a runtime error. That doesn't mean that this wouldn't expose
existing bugs, but I think it does cut down on what we'd expect to see fixed.
Also, we should consider the prevention of future bugs, which won't ever need
to be detected at runtime.

Nonetheless, it is with heavy heart that I think I agree with the suggested
course of action​: go back to the old, worse behavior. If we were starting
from scratch, I would hold my ground, but I don't think the breakage is
justified here.

Now merged with v5.21.11-77-g8283f70

--
The warp engines start playing up a bit, but seem to sort themselves out
after a while without any intervention from boy genius Wesley Crusher.
  -- Things That Never Happen in "Star Trek" #17

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 8, 2015

@iabyn - Status changed from 'open' to 'pending release'

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 2, 2015

From @khwilliamson

Thanks for submitting this ticket

The issue should be resolved with the release today of Perl v5.22, available at http​://www.perl.org/get.html
If you find that the problem persists, feel free to reopen this ticket

--
Karl Williamson for the Perl 5 porters team

@p5pRT p5pRT closed this Jun 2, 2015
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 2, 2015

@khwilliamson - Status changed from 'pending release' to 'resolved'

@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.