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

& prototype is too permissive #14186

Closed
p5pRT opened this issue Oct 26, 2014 · 9 comments
Closed

& prototype is too permissive #14186

p5pRT opened this issue Oct 26, 2014 · 9 comments

Comments

@p5pRT
Copy link

p5pRT commented Oct 26, 2014

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

Searchable as RT123062$

@p5pRT
Copy link
Author

p5pRT commented Oct 26, 2014

From @cpansprout

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

In actuality, it also allows ‘undef’, \@​array, \%hash, \&sub, and \($list, @​of, %refs) in addition to sub{...}.

Recently I accidentally extended it to \$scalar as well, so I want to undo that accidental change and make it more restrictive.

The way the code is written suggests to me that undef was intentional but \@​array and \%hash (and maybe even \&sub) were accidentaly. But \&sub seems useful, and that may be in use already on CPAN. \@​array and \%hash were clearly accidental, and lists of refs screw up the number of arguments that the sub thinks its getting.

So should I allow sub{}, undef, and \&sub?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2014

From @rjbs

* Father Chrysostomos <perlbug-followup@​perl.org> [2014-10-26T15​:40​:50]

The way the code is written suggests to me that undef was intentional but
\@​array and \%hash (and maybe even \&sub) were accidentaly. But \&sub seems
useful, and that may be in use already on CPAN. \@​array and \%hash were
clearly accidental, and lists of refs screw up the number of arguments that
the sub thinks its getting.

So should I allow sub{}, undef, and \&sub?

I think \&sub should stay. It Just Makes Sense™.

Putting aside that the code looks intentional, why do you suppose undef would
be allowed? That is​: why would that intention have existed? I'd have expected
it would be much more consitent for it to act like \@​.

There may be more harm in forbidding undef, or making a warning, but I'm
curious as to why this would've been done on purpose. I'd look into blame, but
my timer is now telling me to start dinner.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2014

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

@p5pRT
Copy link
Author

p5pRT commented Nov 24, 2014

From @cpansprout

On Sun Nov 23 14​:06​:08 2014, perl.p5p@​rjbs.manxome.org wrote​:

* Father Chrysostomos <perlbug-followup@​perl.org> [2014-10-
26T15​:40​:50]

The way the code is written suggests to me that undef was intentional
but
\@​array and \%hash (and maybe even \&sub) were accidentaly. But
\&sub seems
useful, and that may be in use already on CPAN. \@​array and \%hash
were
clearly accidental, and lists of refs screw up the number of
arguments that
the sub thinks its getting.

So should I allow sub{}, undef, and \&sub?

I think \&sub should stay. It Just Makes Sense™.

Putting aside that the code looks intentional, why do you suppose
undef would
be allowed? That is​: why would that intention have existed?

I don’t know. Maybe it was just a mistake. But the code clearly checks for refgen or undef, and croaks on anything else. Refgen allows sub{} and \&sub, but also other things as I detailed above, so that check is clearly not sufficient. But undef? Who knows!

I'd have
expected
it would be much more consitent for it to act like \@​.

Same here.

There may be more harm in forbidding undef, or making a warning,

Maybe it’s not too late (in the dev cycle) to change it now and see whether anything breaks. It clearly contradicts the documentation.

but
I'm
curious as to why this would've been done on purpose. I'd look into
blame, but
my timer is now telling me to start dinner.

You have to dig through a few layers, but explicit ‘allow refgen or undef’ goes back to​:

commit 4633a7c
Author​: Larry Wall <lwall@​scalpel.netlabs.com>
Date​: Tue Nov 21 10​:01​:00 1995 +1200

  5.002 beta 1

If you ‘git show 4633a7c​:Changes’ and search for ‘added rudimentary prototypes’ you will see the description, but it says even less than perlsub already says on the topic.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Nov 24, 2014

From @cpansprout

On Sun Nov 23 16​:01​:38 2014, sprout wrote​:

On Sun Nov 23 14​:06​:08 2014, perl.p5p@​rjbs.manxome.org wrote​:

* Father Chrysostomos <perlbug-followup@​perl.org> [2014-10-
26T15​:40​:50]

The way the code is written suggests to me that undef was
intentional
but
\@​array and \%hash (and maybe even \&sub) were accidentaly. But
\&sub seems
useful, and that may be in use already on CPAN. \@​array and \%hash
were
clearly accidental, and lists of refs screw up the number of
arguments that
the sub thinks its getting.

So should I allow sub{}, undef, and \&sub?

I think \&sub should stay. It Just Makes Sense™.

Putting aside that the code looks intentional, why do you suppose
undef would
be allowed? That is​: why would that intention have existed?

I don’t know. Maybe it was just a mistake. But the code clearly
checks for refgen or undef, and croaks on anything else. Refgen
allows sub{} and \&sub, but also other things as I detailed above, so
that check is clearly not sufficient. But undef? Who knows!

I'd have
expected
it would be much more consitent for it to act like \@​.

Same here.

There may be more harm in forbidding undef, or making a warning,

Maybe it’s not too late (in the dev cycle) to change it now and see
whether anything breaks. It clearly contradicts the documentation.

I’ve changed it in e41e986. I’m hoping that the amount of breakage will be zilch, because of that contradiction.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Nov 24, 2014

@cpansprout - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed Nov 24, 2014
@p5pRT
Copy link
Author

p5pRT commented Nov 24, 2014

From @rjbs

* Father Chrysostomos via RT <perlbug-followup@​perl.org> [2014-11-23T23​:42​:23]

I’ve changed it in e41e986. I’m hoping that the amount of breakage will
be zilch, because of that contradiction.

Cool, I hope so, too.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Dec 21, 2014

From @cpansprout

On Mon Nov 24 09​:45​:52 2014, perl.p5p@​rjbs.manxome.org wrote​:

* Father Chrysostomos via RT <perlbug-followup@​perl.org> [2014-11-
23T23​:42​:23]

I’ve changed it in e41e986. I’m hoping that the amount of
breakage will
be zilch, because of that contradiction.

Cool, I hope so, too.

We have one instance of breakage here​: https://rt.cpan.org/Ticket/Display.html?id=101064

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Dec 21, 2014

From @cpansprout

On Sun Dec 21 07​:20​:48 2014, sprout wrote​:

On Mon Nov 24 09​:45​:52 2014, perl.p5p@​rjbs.manxome.org wrote​:

* Father Chrysostomos via RT <perlbug-followup@​perl.org> [2014-11-
23T23​:42​:23]

I’ve changed it in e41e986. I’m hoping that the amount of
breakage will
be zilch, because of that contradiction.

Cool, I hope so, too.

We have one instance of breakage here​:
https://rt.cpan.org/Ticket/Display.html?id=101064

I have created ticket #123475 for that.

--

Father Chrysostomos

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

1 participant