Blead Breaks CPAN: Class::Std

p5pRT commented 6 years ago

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

Searchable as RT132902$

p5pRT commented 6 years ago

From carlos@carlosguevara.com

It looks like blead broke Class::Std. The install now hangs with CPU utilization at 100% at this point: ##### # Testing Class::Std 0.013 t/00.load.t ............ ok t/access.t ............. ok Deep recursion on anonymous subroutine at /home/cpan/.cpan/build/Class-Std-0.013-0/blib/lib/Class/Std.pm line 572. Deep recursion on subroutine "Carp::croak" at /home/cpan/.cpan/build/Class-Std-0.013-0/blib/lib/Class/Std.pm line 290. Deep recursion on subroutine "Carp::shortmess" at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 196. Deep recursion on subroutine "Carp::shortmess_heavy" at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 193. Deep recursion on subroutine "Carp::ret_summary" at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 589. Deep recursion on subroutine "Carp::caller_info" at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 543. Deep recursion on subroutine "Carp::format_arg" at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 276. Deep recursion on anonymous subroutine at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 344. #####

Completed install after killing the hung process: http://www.cpantesters.org/cpan/report/c9ae98e0-18ca-11e8-9afc-de9a541938d7

p5pRT commented 6 years ago

From @demerphq

On 24 Feb 2018 09:46\, "Carlos Guevara" \perlbug\-followup@perl\.org wrote:

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

It looks like blead broke Class::Std. The install now hangs with CPU utilization at 100% at this point: ##### # Testing Class::Std 0.013 t/00.load.t ............ ok t/access.t ............. ok Deep recursion on anonymous subroutine at /home/cpan/.cpan/build/Class-Std-0.013-0/blib/lib/Class/Std.pm line 572. Deep recursion on subroutine "Carp::croak" at /home/cpan/.cpan/build/Class-Std-0.013-0/blib/lib/Class/Std.pm line 290. Deep recursion on subroutine "Carp::shortmess" at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 196. Deep recursion on subroutine "Carp::shortmess_heavy" at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 193. Deep recursion on subroutine "Carp::ret_summary" at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 589. Deep recursion on subroutine "Carp::caller_info" at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 543. Deep recursion on subroutine "Carp::format_arg" at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 276. Deep recursion on anonymous subroutine at /home/cpan/bin/perl-blead/lib/5.27.9/Carp.pm line 344. #####

Completed install after killing the hung process: http://www.cpantesters.org/cpan/report/c9ae98e0-18ca-11e8-9afc-de9a541938d7

Thanks\, this looks like fallout from a fix I pushed yesterday. I will investigate and fix.

Yves

p5pRT commented 6 years ago

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

p5pRT commented 6 years ago

From zefram@fysh.org

demerphq wrote:

Thanks\, this looks like fallout from a fix I pushed yesterday.

Confirmed: it bisects to commit c99363aa273278adcad39f32026629b700f9bbc3 "fix Perl #132828 - dont use overload to bypass overloads". The relevant part of Class::Std is overriding UNIVERSAL::can.

-zefram

p5pRT commented 6 years ago

From @demerphq

On 24 February 2018 at 03:44\, Zefram \zefram@fysh\.org wrote:

demerphq wrote:

Thanks\, this looks like fallout from a fix I pushed yesterday.

Confirmed: it bisects to commit c99363aa273278adcad39f32026629b700f9bbc3 "fix Perl #132828 - dont use overload to bypass overloads". The relevant part of Class::Std is overriding UNIVERSAL::can.

Which means its Class::Std's fault for not implementing can properly.

Nevertheless I will find a fix.

Thanks for verifying and the analysis Zefram.

Yves

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

p5pRT commented 6 years ago

From @demerphq

On 24 February 2018 at 03:46\, demerphq \demerphq@gmail\.com wrote:

On 24 February 2018 at 03:44\, Zefram \zefram@fysh\.org wrote:

demerphq wrote:

Thanks\, this looks like fallout from a fix I pushed yesterday.

Confirmed: it bisects to commit c99363aa273278adcad39f32026629b700f9bbc3 "fix Perl #132828 - dont use overload to bypass overloads". The relevant part of Class::Std is overriding UNIVERSAL::can.

Which means its Class::Std's fault for not implementing can properly.

Nevertheless I will find a fix.

Thanks for verifying and the analysis Zefram.

I have created a pull request against Class::Std to fix this.

https://github.com/chorny/Class-Std/pull/2

I also will push a patch to fix this in Carp.pm as well.

Yves

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

p5pRT commented 6 years ago

From @demerphq

On 24 February 2018 at 12:32\, demerphq \demerphq@gmail\.com wrote:

On 24 February 2018 at 03:46\, demerphq \demerphq@gmail\.com wrote:

On 24 February 2018 at 03:44\, Zefram \zefram@fysh\.org wrote:

demerphq wrote:

Thanks\, this looks like fallout from a fix I pushed yesterday.

Confirmed: it bisects to commit c99363aa273278adcad39f32026629b700f9bbc3 "fix Perl #132828 - dont use overload to bypass overloads". The relevant part of Class::Std is overriding UNIVERSAL::can.

Which means its Class::Std's fault for not implementing can properly.

Nevertheless I will find a fix.

Thanks for verifying and the analysis Zefram.

I have created a pull request against Class::Std to fix this.

https://github.com/chorny/Class-Std/pull/2

I also will push a patch to fix this in Carp.pm as well.

Fixed in 17157c41e6523264e0f5e7d4baa490a1a8f2322b

Yves

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

p5pRT commented 6 years ago

From @cpansprout

On Sat\, 24 Feb 2018 04:16:48 -0800\, demerphq wrote:

On 24 February 2018 at 12:32\, demerphq \demerphq@gmail\.com wrote:

On 24 February 2018 at 03:46\, demerphq \demerphq@gmail\.com wrote:

On 24 February 2018 at 03:44\, Zefram \zefram@fysh\.org wrote:

demerphq wrote:

Thanks\, this looks like fallout from a fix I pushed yesterday.

Confirmed: it bisects to commit c99363aa273278adcad39f32026629b700f9bbc3 "fix Perl #132828 - dont use overload to bypass overloads". The relevant part of Class::Std is overriding UNIVERSAL::can.

Which means its Class::Std's fault for not implementing can properly.

Nevertheless I will find a fix.

Thanks for verifying and the analysis Zefram.

I have created a pull request against Class::Std to fix this.

https://github.com/chorny/Class-Std/pull/2

I also will push a patch to fix this in Carp.pm as well.

Fixed in 17157c41e6523264e0f5e7d4baa490a1a8f2322b

I don’t like the current state of the code in Carp.pm\, but I can’t say I have a better suggestion off the top of my head. I have not thought it through thoroughly yet. Let me just outline my concerns\, before I forget them\, and later if I have time I might come up with a patch:

if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) {

This will only work on newer perls (5.16+ iirc)\, since older ones used ()\, not ((. And I think even current perl allows XS modules to register overloading via just ()\, without ((. I need to check.

‘can’ is really not the right thing to use. Overriding ‘can’ makes sense in the presence of AUTOLOAD\, but since overloading bypassing AUTOLOAD\, it should be bypassing ‘can‘ as well. This is why overload.pm implements its own ‘can’. Perhaps overload.pm should be loaded unconditionally when Carp is loaded (it is more lightweight than Carp\, after all). Then theoretically one could use overload::Overloaded\, but unfortunately old versions have the same ‘can’ bug that will cause the recursion. Maybe calling overload::mycan directly is the solution.

This is a real can of worms.

--

Father Chrysostomos

p5pRT commented 6 years ago

From @demerphq

On 25 Feb 2018 02:01\, "Father Chrysostomos via RT" \< perlbug-followup@perl.org> wrote:

On Sat\, 24 Feb 2018 04:16:48 -0800\, demerphq wrote:

On 24 February 2018 at 12:32\, demerphq \demerphq@gmail\.com wrote:

On 24 February 2018 at 03:46\, demerphq \demerphq@gmail\.com wrote:

On 24 February 2018 at 03:44\, Zefram \zefram@fysh\.org wrote:

demerphq wrote:

Thanks\, this looks like fallout from a fix I pushed yesterday.

Confirmed: it bisects to commit c99363aa273278adcad39f32026629b700f9bbc3 "fix Perl #132828 - dont use overload to bypass overloads". The relevant part of Class::Std is overriding UNIVERSAL::can.

Which means its Class::Std's fault for not implementing can properly.

Nevertheless I will find a fix.

Thanks for verifying and the analysis Zefram.

I have created a pull request against Class::Std to fix this.

https://github.com/chorny/Class-Std/pull/2

I also will push a patch to fix this in Carp.pm as well.

Fixed in 17157c41e6523264e0f5e7d4baa490a1a8f2322b

I don’t like the current state of the code in Carp.pm\, but I can’t say I have a better suggestion off the top of my head. I have not thought it through thoroughly yet. Let me just outline my concerns\, before I forget them\, and later if I have time I might come up with a patch:

if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) {

This will only work on newer perls (5.16+ iirc)\, since older ones used ()\, not ((. And I think even current perl allows XS modules to register overloading via just ()\, without ((. I need to check.

We can add a check for () as well.

‘can’ is really not the right thing to use.

Can you explain why? Since overloads are inherited it seems there is no choice.

Overriding ‘can’ makes sense in the presence of AUTOLOAD\, but since overloading bypassing AUTOLOAD\, it should be bypassing ‘can‘ as well. This is why overload.pm implements its own ‘can’. Perhaps overload.pm should be loaded unconditionally when Carp is loaded (it is more lightweight than Carp\, after all). Then theoretically one could use overload::Overloaded\, but unfortunately old versions have the same ‘can’ bug that will cause the recursion. Maybe calling overload::mycan directly is the solution.

If Carp unilaterally loads overload we don't need to use can at all. The only reason we use it is to avoid loading overload when we don't need to.

I already suggested that this policy was counter productive and the only reason I didn't change was that you expressed an opinion that we should be able to keep it.

If you no longer care about loading overload then the patch becomes trivial.

This is a real can of worms.

A real mycan of worms?

Yves

--

Father Chrysostomos

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

p5pRT commented 6 years ago

From @cpansprout

On Sat\, 24 Feb 2018 15:51:03 -0800\, demerphq wrote:

On 25 Feb 2018 02:01\, "Father Chrysostomos via RT" \< perlbug-followup@perl.org> wrote:

I don’t like the current state of the code in Carp.pm\, but I can’t say I have a better suggestion off the top of my head. I have not thought it through thoroughly yet. Let me just outline my concerns\, before I forget them\, and later if I have time I might come up with a patch:

if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) {

This will only work on newer perls (5.16+ iirc)\, since older ones used ()\, not ((. And I think even current perl allows XS modules to register overloading via just ()\, without ((. I need to check.

We can add a check for () as well.

‘can’ is really not the right thing to use.

Can you explain why? Since overloads are inherited it seems there is no choice.

The text that follows was meant to be the explanation. can and AUTOLOAD go together. Since overloading does not use AUTOLOAD\, using can to detect overloading can be problematic.

Overriding ‘can’ makes sense in the presence of AUTOLOAD\, but since overloading bypassing AUTOLOAD\, it should be bypassing ‘can‘ as well. This is why overload.pm implements its own ‘can’. Perhaps overload.pm should be loaded unconditionally when Carp is loaded (it is more lightweight than Carp\, after all). Then theoretically one could use overload::Overloaded\, but unfortunately old versions have the same ‘can’ bug that will cause the recursion.

Not just recursion. Look at the test I added in e6bb0a40852.

Maybe calling overload::mycan directly is the solution.

If Carp unilaterally loads overload we don't need to use can at all. The only reason we use it is to avoid loading overload when we don't need to.

I already suggested that this policy was counter productive and the only reason I didn't change was that you expressed an opinion that we should be able to keep it.

If you no longer care about loading overload then the patch becomes trivial.

Well\, since Carp is used *everywhere*\, I believe it should remain as lightweight as possible. Now that I’ve had a chance to look at the code in more detail\, I see that it only occurs for stack traces. I am loath to force another module to be loaded\, considering that many quick scripts never use stack traces. But loading overload.pm on demand in that case is itself problematic\, since Carp is often called when things are in an inconsistent state. You shouldn’t risk dying when trying to load something in order to report an error. (Yes\, this actually happened. That’s why Carp::Heavy was eliminated.)

That said\, I’m still on the fence about it. I know Zefram cares about this sort of thing\, too\, so I would ask him which is the best approach:

• Go ahead and load overload.pm unconditionally. It’s already much smaller than Carp.

• Copy & paste mycan into Carp. It’s just seven lines or so (but we need two versions depending on the perl version).

This is a real can of worms.

A real mycan of worms?

Yves

s/v//r is my answer. :-)

--

Father Chrysostomos

p5pRT commented 6 years ago

From @demerphq

On 25 February 2018 at 02:40\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sat\, 24 Feb 2018 15:51:03 -0800\, demerphq wrote:

On 25 Feb 2018 02:01\, "Father Chrysostomos via RT" \< perlbug-followup@perl.org> wrote:

I don’t like the current state of the code in Carp.pm\, but I can’t say I have a better suggestion off the top of my head. I have not thought it through thoroughly yet. Let me just outline my concerns\, before I forget them\, and later if I have time I might come up with a patch:

if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) {

This will only work on newer perls (5.16+ iirc)\, since older ones used ()\, not ((. And I think even current perl allows XS modules to register overloading via just ()\, without ((. I need to check.

We can add a check for () as well.

‘can’ is really not the right thing to use.

Can you explain why? Since overloads are inherited it seems there is no choice.

The text that follows was meant to be the explanation. can and AUTOLOAD go together. Since overloading does not use AUTOLOAD\, using can to detect overloading can be problematic.

Yeah I read that text\, but I am still not seeing the connection the way you do.

So\, it makes sense to override can if you use AUTOLOAD.

And overload's dont trigger AUTOLOAD. That is clear.

But\, it doesnt seem to obvious to me that that means that we shouldnt use can() for finding inheritable methods. Overload is *designed* so this works. That is why the methods are registered the way they are. (At least that is what the comments in overload indicate to me.)

It seems like you are saying something like: "Because the main reason to override can() is to work properly with AUTOLOAD\, people often implement their can overrides without accounting for overload subs\, and thus most of them are buggy\, thus we shouldn't use can()".

Which doesn't seem logical to me\, even if perhaps it is practical.

Why not simply say "We should document that can() overrides need to be robust to overload calls"? I mean\, i would consider any can() that doesn't handle overload subs buggy\, why don't you? It seems a strange basis to argue we shouldn't use the one thing we have available to use (Zefram mentions there is internal code that exactly what we want but it is not exposed).

To me that there are buggy can() implementations out there is a problem for those buggy can() implementations\, and maybe an indication we need better docs on this\, not an argument to avoid can() for this type of purpose.

Overriding ‘can’ makes sense in the presence of AUTOLOAD\, but since overloading bypassing AUTOLOAD\, it should be bypassing ‘can‘ as well. This is why overload.pm implements its own ‘can’. Perhaps overload.pm should be loaded unconditionally when Carp is loaded (it is more lightweight than Carp\, after all). Then theoretically one could use overload::Overloaded\, but unfortunately old versions have the same ‘can’ bug that will cause the recursion.

Not just recursion. Look at the test I added in e6bb0a40852.

Thanks\, i will look.

Maybe calling overload::mycan directly is the solution.

If Carp unilaterally loads overload we don't need to use can at all. The only reason we use it is to avoid loading overload when we don't need to.

I already suggested that this policy was counter productive and the only reason I didn't change was that you expressed an opinion that we should be able to keep it.

If you no longer care about loading overload then the patch becomes trivial.

Well\, since Carp is used *everywhere*\, I believe it should remain as lightweight as possible.

Overload is widely used as well. I also am not that sympathetic to the lightweight argument\, i don't see a single module used for this kind of purpose as a problem\, especially when you consider how much code we have in Carp to deal with these types of issues.

Now that I’ve had a chance to look at the code in more detail\, I see that it only occurs for stack traces. I am loath to force another module to be loaded\, considering that many quick scripts never use stack traces.

I bet you couldn't even see the extra module load in normal benchmarks of any kind of non-trivial script.

But loading overload.pm on demand in that case is itself problematic\, since Carp is often called when things are in an inconsistent state. You shouldn’t risk dying when trying to load something in order to report an error. (Yes\, this actually happened. That’s why Carp::Heavy was eliminated.)

I totally agree. No argument there at all. See 02c84d7f0f97e083f5d8ea9856488f3ede09364f for an example of me patching for this objective. ;-)

That said\, I’m still on the fence about it. I know Zefram cares about this sort of thing\, too\, so I would ask him which is the best approach:

• Go ahead and load overload.pm unconditionally. It’s already much smaller than Carp.

• Copy & paste mycan into Carp. It’s just seven lines or so (but we need two versions depending on the perl version).

Isnt mycan slow compared to the internals version? Maybe in modern perls we should expose the internals function Zefram mentioned:

(05:57:51) Zefram: annoyingly\, the exact thing you want is available as a Perl op\, method_named\, but you can't get at that op in isolation through Perl source code

That way BOTH carp and overload could use it in newer perls.

I don't really want to see Carp slowed down massively just because we haven't documented how people should implement UNIVERSAL::can overrides properly. That seems just wrong.

This is a real can of worms.

A real mycan of worms?

Yves

s/v//r is my answer. :-)

Heh.

BTW\, going forward there is another solution\, expose unoverloaded stringification via Internals:: or perhaps scalar::

It wont help older builds\, but in the long run it would avoid needing overload at all.

Yves

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

p5pRT commented 6 years ago

From @demerphq

On 25 February 2018 at 02:40\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sat\, 24 Feb 2018 15:51:03 -0800\, demerphq wrote:

On 25 Feb 2018 02:01\, "Father Chrysostomos via RT" \< perlbug-followup@perl.org> wrote:

I don’t like the current state of the code in Carp.pm\, but I can’t say I have a better suggestion off the top of my head. I have not thought it through thoroughly yet. Let me just outline my concerns\, before I forget them\, and later if I have time I might come up with a patch:

if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) {

This will only work on newer perls (5.16+ iirc)\, since older ones used ()\, not ((. And I think even current perl allows XS modules to register overloading via just ()\, without ((. I need to check.

Fixed in 687fa129abe7abf4cd6415c9ef46308287221049

Yves

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

p5pRT commented 6 years ago

From @cpansprout

On Sat\, 24 Feb 2018 18:42:22 -0800\, demerphq wrote:

On 25 February 2018 at 02:40\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sat\, 24 Feb 2018 15:51:03 -0800\, demerphq wrote: The text that follows was meant to be the explanation. can and AUTOLOAD go together. Since overloading does not use AUTOLOAD\, using can to detect overloading can be problematic.

Yeah I read that text\, but I am still not seeing the connection the way you do.

So\, it makes sense to override can if you use AUTOLOAD.

And overload's dont trigger AUTOLOAD. That is clear.

But\, it doesnt seem to obvious to me that that means that we shouldnt use can() for finding inheritable methods. Overload is *designed* so this works. That is why the methods are registered the way they are. (At least that is what the comments in overload indicate to me.)

It seems like you are saying something like: "Because the main reason to override can() is to work properly with AUTOLOAD\, people often implement their can overrides without accounting for overload subs\, and thus most of them are buggy\, thus we shouldn't use can()".

Which doesn't seem logical to me\, even if perhaps it is practical.

Why not simply say "We should document that can() overrides need to be robust to overload calls"? I mean\, i would consider any can() that doesn't handle overload subs buggy\, why don't you? It seems a strange basis to argue we shouldn't use the one thing we have available to use (Zefram mentions there is internal code that exactly what we want but it is not exposed).

To me that there are buggy can() implementations out there is a problem for those buggy can() implementations\, and maybe an indication we need better docs on this\, not an argument to avoid can() for this type of purpose.

The way I see it\, the fact that overloading uses weird-named subs is just an artefact of the implementation. Overloading can be inherited\, but it does not consist of method calls (though they may trigger methods)\, as shown by the fact that AUTOLOAD is not respected\, and by the fact that the fallback value (a scalar) is also inherited. In fact\, overloading originally did not share internal code with method calls at all; that was just the most convenient way to get inheritance to work.

I don’t think we should blur this distinction and call ‘can’\, because it puts the burden on too many programmers to ensure that their ‘can’ methods behave a way that ‘can’ was not designed for to begin with.

I do *not* consider what you call ‘buggy can() implementations’ to be buggy.

Not just recursion. Look at the test I added in e6bb0a40852.

Thanks\, i will look.

Based on your explanation above\, it has a ‘buggy can() implementation’.

Well\, since Carp is used *everywhere*\, I believe it should remain as lightweight as possible.

Overload is widely used as well. I also am not that sympathetic to the lightweight argument\, i don't see a single module used for this kind of purpose as a problem\, especially when you consider how much code we have in Carp to deal with these types of issues.

I’m not entirely opposed to loading overload.pm unconditionally. I just want it to be justified\, and not just convenient because it saves two lines of code. In this instance\, it is justifiable.

I totally agree. No argument there at all. See 02c84d7f0f97e083f5d8ea9856488f3ede09364f for an example of me patching for this objective. ;-)

That said\, I’m still on the fence about it. I know Zefram cares about this sort of thing\, too\, so I would ask him which is the best approach:

• Go ahead and load overload.pm unconditionally. It’s already much smaller than Carp.

• Copy & paste mycan into Carp. It’s just seven lines or so (but we need two versions depending on the perl version).

Isnt mycan slow compared to the internals version? Maybe in modern perls we should expose the internals function Zefram mentioned:

(05:57:51) Zefram: annoyingly\, the exact thing you want is available as a Perl op\, method_named\, but you can't get at that op in isolation through Perl source code

It already exists. It is called UNIVERSAL::can (which we can use\, since we control it; even though it was designed for methods\, it happens to work for this case). Unfortunately\, there are (buggy) CPAN modules that diddle with it. Fortunately\, Carp already has (partial) code to detect whether that has happened.

(Note: There is a comment in overload.pm about ‘real’ can() leaving stubs. I think we need to get to the bottom of that before trying UNIVERSAL::can. Leaving a "()" stub behind may break things\, since that’s where the fallback value is stored.)

--

Father Chrysostomos

p5pRT commented 6 years ago

From @cpansprout

On Sat\, 24 Feb 2018 22:24:01 -0800\, sprout wrote:

On Sat\, 24 Feb 2018 18:42:22 -0800\, demerphq wrote:

(05:57:51) Zefram: annoyingly\, the exact thing you want is available as a Perl op\, method_named\, but you can't get at that op in isolation through Perl source code

It already exists. It is called UNIVERSAL::can (which we can use\, since we control it; even though it was designed for methods\, it happens to work for this case). Unfortunately\, there are (buggy) CPAN modules that diddle with it. Fortunately\, Carp already has (partial) code to detect whether that has happened.

(Note: There is a comment in overload.pm about ‘real’ can() leaving stubs. I think we need to get to the bottom of that before trying UNIVERSAL::can. Leaving a "()" stub behind may break things\, since that’s where the fallback value is stored.)

Never mind that note. Since overload::Overloaded used to call ->can\, it obviously doesn’t matter for this case.

--

Father Chrysostomos

p5pRT commented 6 years ago

From carlos@carlosguevara.com

This is a bug report for perl from Carlos Guevara \carlos@carlosguevara\.com\, generated with the help of perlbug 1.41 running under perl 5.27.9.

It looks like blead broke Test::MockObject: http://www.cpantesters.org/cpan/report/cf360f98-19e0-11e8-809c-69696b55ae40

Flags: category=core severity=low

Site configuration information for perl 5.27.9:

Configured by root at Sun Feb 25 19:21:55 UTC 2018.

Summary of my perl5 (revision 5 version 27 subversion 9) configuration: Commit id: 33e5a354e61aad64afe0e1a4f53773e8547515bf Platform: osname=linux osvers=3.2.0-5-amd64 archname=x86_64-linux uname='linux cjg-wheezy 3.2.0-5-amd64 #1 smp debian 3.2.96-3 x86_64 gnulinux ' config_args='-des -Dprefix=~/bin/perl-blead -Dscriptdir=~/bin/perl-blead/bin -Dusedevel -Duse64bitall' hint=recommended useposix=true d_sigaction=define useithreads=undef usemultiplicity=undef use64bitint=define use64bitall=define uselongdouble=undef usemymalloc=n default_inc_excludes_dot=define bincompat5005=undef Compiler: cc='cc' ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2' optimize='-O2' cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion='' gccversion='4.7.2' gccosandvers='' intsize=4 longsize=8 ptrsize=8 doublesize=8 byteorder=12345678 doublekind=3 d_longlong=define longlongsize=8 d_longdbl=define longdblsize=16 longdblkind=3 ivtype='long' ivsize=8 nvtype='double' nvsize=8 Off_t='off_t' lseeksize=8 alignbytes=8 prototype=define Linker and Libraries: ld='cc' ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.7/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.13.so so=so useshrplib=false libperl=libperl.a gnulibc_version='2.13' Dynamic Linking: dlsrc=dl_dlopen.xs dlext=so d_dlsymun=undef ccdlflags='-Wl\,-E' cccdlflags='-fPIC' lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

@INC for perl 5.27.9: /home/cpan/bin/perl-blead/lib/site_perl/5.27.9/x86_64-linux /home/cpan/bin/perl-blead/lib/site_perl/5.27.9 /home/cpan/bin/perl-blead/lib/5.27.9/x86_64-linux /home/cpan/bin/perl-blead/lib/5.27.9

Environment for perl 5.27.9: HOME=/home/cpan LANG (unset) LANGUAGE (unset) LC_ALL=C LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/cpan/bin/perl-blead/bin:/home/cpan/bin:/home/cpan/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games PERL_BADLANG (unset) SHELL=/bin/bash

p5pRT commented 6 years ago

From @dur-randir

On Sun\, 25 Feb 2018 11:39:39 -0800\, carlos@carlosguevara.com wrote:

It looks like blead broke Test::MockObject: http://www.cpantesters.org/cpan/report/cf360f98-19e0-11e8-809c- 69696b55ae40

Bisect points to the same commit c99363aa273278adcad39f32026629b700f9bbc3 as in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132902

p5pRT commented 6 years ago

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

p5pRT commented 6 years ago

From zefram@fysh.org

Carlos Guevara wrote:

It looks like blead broke Test::MockObject:

It's down to Carp. Bisects to commit c99363aa273278adcad39f32026629b700f9bbc3 "fix Perl #132828 - dont use overload to bypass overloads".

-zefram

p5pRT commented 6 years ago

From @cpansprout

On Sat\, 24 Feb 2018 18:42:22 -0800\, demerphq wrote:

Isnt mycan slow compared to the internals version? Maybe in modern perls we should expose the internals function Zefram mentioned:

(05:57:51) Zefram: annoyingly\, the exact thing you want is available as a Perl op\, method_named\, but you can't get at that op in isolation through Perl source code

That way BOTH carp and overload could use it in newer perls.

I’ve just submitted a patch against the UNIVERSAL::can module\, making exactly that point: https://rt.cpan.org/Ticket/Display.html?id=124585

(As I mentioned in another message\, UNIVERSAL::can is that exposure of the internal function.)

--

Father Chrysostomos

p5pRT commented 6 years ago

From @demerphq

On 25 February 2018 at 21:28\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sat\, 24 Feb 2018 18:42:22 -0800\, demerphq wrote:

Isnt mycan slow compared to the internals version? Maybe in modern perls we should expose the internals function Zefram mentioned:

(05:57:51) Zefram: annoyingly\, the exact thing you want is available as a Perl op\, method_named\, but you can't get at that op in isolation through Perl source code

That way BOTH carp and overload could use it in newer perls.

I’ve just submitted a patch against the UNIVERSAL::can module\, making exactly that point: https://rt.cpan.org/Ticket/Display.html?id=124585

(As I mentioned in another message\, UNIVERSAL::can is that exposure of the internal function.)

I am really struggling to understand what you think is right.

So first off\, there are *two* UNIVERSAL::can()'s involved\, the *real* one\, in Perl. And the fake one\, in the CPAN module UNIVERSAL-can.

That module thinks that is "buggy" to call UNIVERSAL::can() as a function\, which I definitely do not agree with\, and is in fact what I think Carp should do for this check.

In one of the other posts in this thread\, you argue that can() implementations should not be aware of overload subs\, but now you think the right thing to do is patch a /non-core module/ to be aware of them.

You also have said you think that UNIVERSAL::can() should not be used for things like $obj->can("((") because overload subs do not respect AUTOLOAD. (I don't agree with that either.)

I do not see how it is right that a patch to a module not in core is required to fix behavior that is in core.

It seems to me that we need an exposure of UNIVERSAL::can() that does NOT live in the UNIVERSAL namespace and which cannot be overriden by a module\, and that in future Perls we should use that in overload and in Carp.

I am right now quite baffled.

Honestly at this point I think the right thing is to make Carp load overload unilaterally and make Carp use that\, and move on to more interesting things.

Yves

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

p5pRT commented 6 years ago

From @cpansprout

On Sun\, 25 Feb 2018 12:19:07 -0800\, zefram@fysh.org wrote:

Carlos Guevara wrote:

It looks like blead broke Test::MockObject:

It's down to Carp. Bisects to commit c99363aa273278adcad39f32026629b700f9bbc3 "fix Perl #132828 - dont use overload to bypass overloads".

Fixed in 4efd247.

--

Father Chrysostomos

p5pRT commented 6 years ago

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

p5pRT commented 6 years ago

From @cpansprout

On Sat\, 24 Feb 2018 22:24:01 -0800\, sprout wrote:

It already exists. It is called UNIVERSAL::can (which we can use\, since we control it; even though it was designed for methods\, it happens to work for this case). Unfortunately\, there are (buggy) CPAN modules that diddle with it. Fortunately\, Carp already has (partial) code to detect whether that has happened.

I’ve done this in commit 4efd247\, because it came up in another ticket. The use of ->can did cause CPAN breakage.

--

Father Chrysostomos

p5pRT commented 6 years ago

From @cpansprout

These two tickets should not have been merged. They are related\, but they are two different issues\, one caused by the fix for the other.

--

Father Chrysostomos

p5pRT commented 6 years ago

From [Unknown Contact. See original ticket]

These two tickets should not have been merged. They are related\, but they are two different issues\, one caused by the fix for the other.

--

Father Chrysostomos

p5pRT commented 6 years ago

From @demerphq

On 26 February 2018 at 03:22\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sat\, 24 Feb 2018 22:24:01 -0800\, sprout wrote:

It already exists. It is called UNIVERSAL::can (which we can use\, since we control it; even though it was designed for methods\, it happens to work for this case). Unfortunately\, there are (buggy) CPAN modules that diddle with it. Fortunately\, Carp already has (partial) code to detect whether that has happened.

I’ve done this in commit 4efd247\, because it came up in another ticket. The use of ->can did cause CPAN breakage.

Personally i find this very irritating that you just push patches related to this without properly engaging the feedback I provided.

On the same basis of unilateral imposition of ones own views without debate\, why shouldn't i just push a patch that makes Carp load overload.pm? It would be simpler\, faster\, and would avoid any need to call can() at all.

/me irritated that you aren't engaging properly.

cheers\, Yves

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

p5pRT commented 6 years ago

From @cpansprout

On Sun\, 25 Feb 2018 16:14:44 -0800\, demerphq wrote:

I am really struggling to understand what you think is right.

Well\, I can be enigmatic. :-)

So first off\, there are *two* UNIVERSAL::can()'s involved\, the *real* one\, in Perl. And the fake one\, in the CPAN module UNIVERSAL-can.

That module thinks that is "buggy" to call UNIVERSAL::can() as a function\, which I definitely do not agree with\, and is in fact what I think Carp should do for this check.

I agree with you here.

In one of the other posts in this thread\, you argue that can() implementations should not be aware of overload subs\, but now you think the right thing to do is patch a /non-core module/ to be aware of them.

Regular modules’ ‘can’ implementations should not have to care about them. Modules that meddle with UNIVERSAL::can need to bend over backwards to make sure they are not breaking things. With great power (overriding UNIVERSAL) comes great responsibility.

You also have said you think that UNIVERSAL::can() should not be used

I don’t think $some_object->can should be used. UNIVERSAL::can($object\, ...) should be fine.

I think this way\, not because I think that a method designed to look for methods is a good fit for overloading\, but because in this particular instance (core’s UNIVERSAL::can) it happens to do exactly the right thing. And\, in being a stable interface\, we can depend on it.

for things like $obj->can("((") because overload subs do not respect AUTOLOAD. (I don't agree with that either.)

I do not see how it is right that a patch to a module not in core is required to fix behavior that is in core.

If merely loading some module (and a not-so-unpopular module) will break the core\, and it didn’t in the previous Perl version\, then what should be someone else’s problem (a broken override) becomes *our* problem.

You may disagree with that\, but that’s fine. I’m willing to bend over backwards to get things working. You don’t have to.

It seems to me that we need an exposure of UNIVERSAL::can() that does NOT live in the UNIVERSAL namespace and which cannot be overriden by a module\, and that in future Perls we should use that in overload and in Carp.

We can’t stop buggy modules from overriding things. But we can patch the one buggy module that currently does it to ‘can’.

I am right now quite baffled.

Funny thing\, I used the number 0xbaff1ed_bee in one of the tests.

Honestly at this point I think the right thing is to make Carp load overload unilaterally and make Carp use that\, and move on to more interesting things.

I fixed this a different way before I read your message. I\, too\, intend to move on to more interesting things\, after I’ve checked one other thing that may be broken in Carp.

--

Father Chrysostomos

p5pRT commented 6 years ago

From @demerphq

On 26 February 2018 at 05:02\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sun\, 25 Feb 2018 16:14:44 -0800\, demerphq wrote:

I am really struggling to understand what you think is right.

Well\, I can be enigmatic. :-)

Indeed. :-) Perhaps a bit too close to the machine version though. ;-)

So first off\, there are *two* UNIVERSAL::can()'s involved\, the *real* one\, in Perl. And the fake one\, in the CPAN module UNIVERSAL-can.

That module thinks that is "buggy" to call UNIVERSAL::can() as a function\, which I definitely do not agree with\, and is in fact what I think Carp should do for this check.

I agree with you here.

\o/

In one of the other posts in this thread\, you argue that can() implementations should not be aware of overload subs\, but now you think the right thing to do is patch a /non-core module/ to be aware of them.

Regular modules’ ‘can’ implementations should not have to care about them. Modules that meddle with UNIVERSAL::can need to bend over backwards to make sure they are not breaking things. With great power (overriding UNIVERSAL) comes great responsibility.

I definitely agree with the latter two points. I am not entirely convinced about the first one\, but I can live your interpretation.

You also have said you think that UNIVERSAL::can() should not be used

I don’t think $some_object->can should be used. UNIVERSAL::can($object\, ...) should be fine.

I think this way\, not because I think that a method designed to look for methods is a good fit for overloading\, but because in this particular instance (core’s UNIVERSAL::can) it happens to do exactly the right thing. And\, in being a stable interface\, we can depend on it.

I agree with this.

for things like $obj->can("((") because overload subs do not respect AUTOLOAD. (I don't agree with that either.)

I do not see how it is right that a patch to a module not in core is required to fix behavior that is in core.

If merely loading some module (and a not-so-unpopular module) will break the core\, and it didn’t in the previous Perl version\, then what should be someone else’s problem (a broken override) becomes *our* problem.

You may disagree with that\, but that’s fine. I’m willing to bend over backwards to get things working. You don’t have to.

I'm willing to bend over backwards as well\, or I wouldn't have tried to fix this in the first place. :-)

[ And really\, that paragraph seems pretty close to suggesting I am acting in bad-faith here\, which I am most certainly not.]

The place I disagree is *not* about avoiding breakage\, but rather about the right way to do so. And even there\, I would not actually say I disagree with you\, I just am not convinced yet and feel the subject merits more debate than we have given.

In particular I feel that we are going through huge contortions and complexity simply to avoid loading overload.pm\, and imposing performance penalties on stack serialization to do so.

It feels like to me we have possibly already pushed past the point where our efforts to avoid loading overload.pm are more expensive than just loading overload.pm.

So for instance\, if I was able to show that adding a "use overload;" to the Carp had negligble or positive load time consequences would you concur that we should remove this complexity? If not\, what would convince you?

It seems to me that we need an exposure of UNIVERSAL::can() that does NOT live in the UNIVERSAL namespace and which cannot be overriden by a module\, and that in future Perls we should use that in overload and in Carp.

We can’t stop buggy modules from overriding things. But we can patch the one buggy module that currently does it to ‘can’.

I think anything that overrides an Internals:: (or equivalent) function gets to keep both pieces. And because said logic would not reside in UNIVERSAL\, it would not affect all the normal uses of can() that we both agree should work.

I am right now quite baffled.

Funny thing\, I used the number 0xbaff1ed_bee in one of the tests.

:-) [ Interestingly some time back I was told by an academic that not all language groups appreciate Irony the way English speakers do. ]

Honestly at this point I think the right thing is to make Carp load overload unilaterally and make Carp use that\, and move on to more interesting things.

I fixed this a different way before I read your message.

But that is my point\, this patch sequence has had too many patches and too little discussion. Maybe a bit more deliberation would improve the quality of our work.

I\, too\, intend to move on to more interesting things\, after I’ve checked one other thing that may be broken in Carp.

Please lets resolve this discussion before you move on.

I appreciate your work and opinion\, and I apologize if my attempts to fix Carp have lead you to do more work\, but at the same time\, I note that if we had just dropped the policy of eschewing "use overload;" like I suggested in the first place /none/ of these patches or bug reports would have happened.

So I feel like we really aught to address that point and come to a consensus before we move on.

Which is why I cc'ed Sawyer on my last post\, and why I am cc'ing him here.

Yves

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

p5pRT commented 6 years ago

From @cpansprout

On Sun\, 25 Feb 2018 20:28:02 -0800\, demerphq wrote:

On 26 February 2018 at 05:02\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sun\, 25 Feb 2018 16:14:44 -0800\, demerphq wrote: If merely loading some module (and a not-so-unpopular module) will break the core\, and it didn’t in the previous Perl version\, then what should be someone else’s problem (a broken override) becomes *our* problem.

You may disagree with that\, but that’s fine. I’m willing to bend over backwards to get things working. You don’t have to.

I'm willing to bend over backwards as well\, or I wouldn't have tried to fix this in the first place. :-)

[ And really\, that paragraph seems pretty close to suggesting I am acting in bad-faith here\, which I am most certainly not.]

Sorry if it came across that way. I know that in the past (for example\, when it came to hash randomization)\, you did not think it was p5p’s responsibility to patch all the broken modules\, whereas I thought we should patch as many as we could. In order words\, I’m trying to concede to your view\, pointing out that the burden falls upon those who think differently.

The place I disagree is *not* about avoiding breakage\, but rather about the right way to do so. And even there\, I would not actually say I disagree with you\, I just am not convinced yet and feel the subject merits more debate than we have given.

In particular I feel that we are going through huge contortions and complexity simply to avoid loading overload.pm\, and imposing performance penalties on stack serialization to do so.

It feels like to me we have possibly already pushed past the point where our efforts to avoid loading overload.pm are more expensive than just loading overload.pm.

So for instance\, if I was able to show that adding a "use overload;" to the Carp had negligble or positive load time consequences would you concur that we should remove this complexity? If not\, what would convince you?

If you look at my latest patch\, namely\, 4efd247d4c\, you will see that it actually does load overload.pm on startup\, iff UNIVERSAL::can is already loaded. Otherwise it just uses Perl’s UNIVERSAL::can\, which suffices.

It seems to me that we need an exposure of UNIVERSAL::can() that does NOT live in the UNIVERSAL namespace and which cannot be overriden by a module\, and that in future Perls we should use that in overload and in Carp.

We can’t stop buggy modules from overriding things. But we can patch the one buggy module that currently does it to ‘can’.

I think anything that overrides an Internals:: (or equivalent) function gets to keep both pieces.

I think the same is true of UNIVERSAL (but we already have existing modules to cope with).

And because said logic would not reside in UNIVERSAL\, it would not affect all the normal uses of can() that we both agree should work.

To use the new function would add more conditions to Carp. The logic I have already added:

+BEGIN { + *_mycan = _univ_mod_loaded('can') + ? do { require "overload.pm"; _fetch_sub overload => 'mycan' } + : \&UNIVERSAL::can +}

will still have to stay. So I see no need to proceed that way.

Honestly at this point I think the right thing is to make Carp load overload unilaterally and make Carp use that\, and move on to more interesting things.

I fixed this a different way before I read your message.

But that is my point\, this patch sequence has had too many patches and too little discussion. Maybe a bit more deliberation would improve the quality of our work.

I\, too\, intend to move on to more interesting things\, after I’ve checked one other thing that may be broken in Carp.

Please lets resolve this discussion before you move on.

I appreciate your work and opinion\, and I apologize if my attempts to fix Carp have lead you to do more work\,

Oh\, not at all. I actually really enjoy this kind of bug fixing\, the digital counterpart to tightrope walking. Thank you for the opportunity.

but at the same time\, I note that if we had just dropped the policy of eschewing "use overload;" like I suggested in the first place /none/ of these patches or bug reports would have happened.

Bug #132910\, which was erroneously merged into this ticket\, would have happened\, but we would not have noticed\, because it would only have happened with a new Carp on perl 5.14 or earlier.

So I feel like we really aught to address that point and come to a consensus before we move on.

I think you are proposing that we load overload.pm up front and just use overload::StrVal without checking for overloading. Am I right?

When it comes to recent versions of overload.pm (1.18+; perl 5.16)\, that will just work.

Version 1.01–1.17 (perl 5.8.1 to 5.14) load Scalar::Util at run time. *That* is a serious problem. (In fact\, Carp currently loading overload.pm at run time is problematic\, the ‘other thing that may be broken’ that I mentioned above.)

Carp is sometimes called from $SIG{__DIE__} handlers\, which may trigger due to a syntax error. After a syntax error\, BEGIN blocks won’t run. (This actually happened. See commit 018c7c82242.) I have written a test that fails in current blead because of this.

That means: - For perl 5.16 onwards\, we need to load overload.pm up front. - For perl 5.8.1 to 5.14\, we need to load Scalar::Util up front as well\, even though we might not use it.

Alternatively\, for perl 5.10.1 to 5.14\, we can copy the more recent overload::StrVal into Carp. It consists of: sub { no overloading; "$_[0]" }. You can’t get much faster than that. In which case we might as well use it also in current blead and avoid extra conditions. We don’t need to load overload.pm at all in perl 5.10.1+.

Did you know that the Romanian word for carp is crap (the fish\, not the verb)?

As for the stash vivification test\, I know why Zefram avoided vivifying utf8::. It broke things on ancient perl versions. That makes sense. But overload? I think the reason is that Carp is so ubiquitous that you don’t want it leaving droppings lying around that it might not even use\, every time anything loads it. But loading overloading.pm vivifies the overload stash. I think we have to live with it.

Now\, for perl 5.8.1 to 5.10.0\, we have (1) an overload::StrVal that loads Scalar::Util at run time\, and (2) an overload::Overloaded which for Carp is unusable\, since it calls ->can.

Either we go ahead and load Scalar::Util when loading Carp--which I don’t like\, but I can be persuaded--\, or we avoid overload::StrVal and do it the hard way\, writing a pure-Perl StrVal.

For Perl 5.8.0 (yes\, I think we should support 5.8.0 still)\, overload::StrVal has the worst implementation yet. It unconditionally blesses any ref passed to it\, which may well cause real problems. Use of overload::StrVal isn’t all that common\, compared to Carp. Carp will make it common and start blessing people’s references left and right when generating a stack trace. Avoid this old StrVal at all costs!

It looks as though the simplest approach to all this is: - For 5.10.1+\, use overloading.pm. - For 5.10.0-\, give Carp its own pure-Perl StrVal.

--

Father Chrysostomos

p5pRT commented 6 years ago

From @cpansprout

On Sun\, 25 Feb 2018 23:50:45 -0800\, sprout wrote:

Now\, for perl 5.8.1 to 5.10.0\, we have (1) an overload::StrVal that loads Scalar::Util at run time\, and (2) an overload::Overloaded which for Carp is unusable\, since it calls ->can.

Either we go ahead and load Scalar::Util when loading Carp--which I don’t like\, but I can be persuaded--\, or we avoid overload::StrVal and do it the hard way\, writing a pure-Perl StrVal.

For Perl 5.8.0 (yes\, I think we should support 5.8.0 still)\, overload::StrVal has the worst implementation yet. It unconditionally blesses any ref passed to it\, which may well cause real problems. Use of overload::StrVal isn’t all that common\, compared to Carp. Carp will make it common and start blessing people’s references left and right when generating a stack trace. Avoid this old StrVal at all costs!

To make things worse\, the old pure-Perl Scalar::Util::refaddr also unconditionally blesses references. It looks almost the same as the old StrVal\, so it was probably copied\, pasted and tweaked. That means if we were to use overload::StrVal from 5.8.1 to 5.10.0 (which uses refaddr) we would have to make sure the XS Scalar::Util was loaded. That’s getting too complicated.

It looks as though the simplest approach to all this is: - For 5.10.1+\, use overloading.pm. - For 5.10.0-\, give Carp its own pure-Perl StrVal.

All the more necessary\, considering what I just discovered about refaddr.

--

Father Chrysostomos

p5pRT commented 6 years ago

From zefram@fysh.org

Ticket [perl #132910] (BBC Test::MockObject) has in RT been merged into [perl #132902) (BBC Class::Std)\, which is marked resolved. That does not accurately reflect the status of the issues. Although the two BBCs arise from the same core commit\, they work by different mechanisms\, and have been fixed by different commits (#132902 by 17157c41 and #132910 by 4efd247d). The tickets should be separate.

-zefram

p5pRT commented 6 years ago

From @cpansprout

On Sun\, 25 Feb 2018 23:50:45 -0800\, sprout wrote:

It looks as though the simplest approach to all this is: - For 5.10.1+\, use overloading.pm. - For 5.10.0-\, give Carp its own pure-Perl StrVal.

Please review the patch on the sprout/carp-strval branch.

--

Father Chrysostomos

p5pRT commented 6 years ago

From @jkeenan

On 02/26/2018 03:03 AM\, Zefram wrote:

Ticket [perl #132910] (BBC Test::MockObject) has in RT been merged into [perl #132902) (BBC Class::Std)\, which is marked resolved. That does not accurately reflect the status of the issues. Although the two BBCs arise from the same core commit\, they work by different mechanisms\, and have been fixed by different commits (#132902 by 17157c41 and #132910 by 4efd247d). The tickets should be separate.

-zefram

I'm sorry for misunderstanding the problem. Is there any corrective action we can take?

Thank you very much. Jim Keenan

p5pRT commented 6 years ago

From @demerphq

On 26 February 2018 at 08:50\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sun\, 25 Feb 2018 20:28:02 -0800\, demerphq wrote:

On 26 February 2018 at 05:02\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sun\, 25 Feb 2018 16:14:44 -0800\, demerphq wrote: If merely loading some module (and a not-so-unpopular module) will break the core\, and it didn’t in the previous Perl version\, then what should be someone else’s problem (a broken override) becomes *our* problem.

You may disagree with that\, but that’s fine. I’m willing to bend over backwards to get things working. You don’t have to.

I'm willing to bend over backwards as well\, or I wouldn't have tried to fix this in the first place. :-)

[ And really\, that paragraph seems pretty close to suggesting I am acting in bad-faith here\, which I am most certainly not.]

Sorry if it came across that way. I know that in the past (for example\, when it came to hash randomization)\, you did not think it was p5p’s responsibility to patch all the broken modules\, whereas I thought we should patch as many as we could. In order words\, I’m trying to concede to your view\, pointing out that the burden falls upon those who think differently.

For the record\, I don't feel that that accurately portrays my views. I recall helping patch at least a half dozen modules or so\, although beyond CGI and Perl itself I don't recall which.

There were a lot of people claiming back then that their code was not buggy\, and that the hash change "broke their code". I was very much opposed to this view\, as we had documented that key order was NOT defined. Despite that many people assumed that despite the order being maintained they were entitled to expect that the behavior was repeatable. But that isn't what undefined means.

Many of the modules involved were "baking in" specific DD dumps for testing purposes\, and I it would not surprise me if I pushed back on people claiming it was my/our responsibility to fix their tests.

The big issue here and then was that if people write buggy code that a Perl change tickles\, then it is not solely our responsibility to fix. We are part of a community\, and the users of Perl need to play their role too.

The place I disagree is *not* about avoiding breakage\, but rather about the right way to do so. And even there\, I would not actually say I disagree with you\, I just am not convinced yet and feel the subject merits more debate than we have given.

In particular I feel that we are going through huge contortions and complexity simply to avoid loading overload.pm\, and imposing performance penalties on stack serialization to do so.

It feels like to me we have possibly already pushed past the point where our efforts to avoid loading overload.pm are more expensive than just loading overload.pm.

So for instance\, if I was able to show that adding a "use overload;" to the Carp had negligble or positive load time consequences would you concur that we should remove this complexity? If not\, what would convince you?

If you look at my latest patch\, namely\, 4efd247d4c\, you will see that it actually does load overload.pm on startup\, iff UNIVERSAL::can is already loaded. Otherwise it just uses Perl’s UNIVERSAL::can\, which suffices.

Ok.

It seems to me that we need an exposure of UNIVERSAL::can() that does NOT live in the UNIVERSAL namespace and which cannot be overriden by a module\, and that in future Perls we should use that in overload and in Carp.

We can’t stop buggy modules from overriding things. But we can patch the one buggy module that currently does it to ‘can’.

I think anything that overrides an Internals:: (or equivalent) function gets to keep both pieces.

I think the same is true of UNIVERSAL (but we already have existing modules to cope with).

Personally due to the nature of UNIVERSAL being the base class for all classes\, I think this is less clear.

And because said logic would not reside in UNIVERSAL\, it would not affect all the normal uses of can() that we both agree should work.

To use the new function would add more conditions to Carp. The logic I have already added:

+BEGIN { + *_mycan = _univ_mod_loaded('can') + ? do { require "overload.pm"; _fetch_sub overload => 'mycan' } + : \&UNIVERSAL::can +}

will still have to stay. So I see no need to proceed that way.

Why? If we simply replace that with;

use overload ();

then we do not need to call UNIVERSAL::can() at all.

Honestly at this point I think the right thing is to make Carp load overload unilaterally and make Carp use that\, and move on to more interesting things.

I fixed this a different way before I read your message.

But that is my point\, this patch sequence has had too many patches and too little discussion. Maybe a bit more deliberation would improve the quality of our work.

I\, too\, intend to move on to more interesting things\, after I’ve checked one other thing that may be broken in Carp.

Please lets resolve this discussion before you move on.

I appreciate your work and opinion\, and I apologize if my attempts to fix Carp have lead you to do more work\,

Oh\, not at all. I actually really enjoy this kind of bug fixing\, the digital counterpart to tightrope walking. Thank you for the opportunity.

Heh\, ok. Ill take that at face value. :-)

but at the same time\, I note that if we had just dropped the policy of eschewing "use overload;" like I suggested in the first place /none/ of these patches or bug reports would have happened.

Bug #132910\, which was erroneously merged into this ticket\, would have happened\, but we would not have noticed\, because it would only have happened with a new Carp on perl 5.14 or earlier.

I dont follow. If we simply did

use overload ();

then we could replace this logic:

# overload uses the presence of a special # "method" named "((" or "()" to signal # it is in effect. This test seeks to see if it has been set up. if (_mycan($pack\, "((") || _mycan($pack\, "()")) { # Argument is blessed into a class with overloading\, and # so might have an overloaded stringification. We don't # want to risk getting the overloaded stringification\, # so we need to use overload::StrVal() below. But it's # possible that the overload module hasn't been loaded: # overload methods can be installed without it. So load # the module here. The bareword form of require is here # eschewed to avoid this compile-time effect of vivifying # the target module's stash. require "overload.pm"; } my $sub = _fetch_sub(overload => 'StrVal'); return $sub ? &$sub($arg) : "$arg";

with a simple:

return overload::StrVal($arg);

So I feel like we really aught to address that point and come to a consensus before we move on.

I think you are proposing that we load overload.pm up front and just use overload::StrVal without checking for overloading. Am I right?

Yep.

When it comes to recent versions of overload.pm (1.18+; perl 5.16)\, that will just work.

Version 1.01–1.17 (perl 5.8.1 to 5.14) load Scalar::Util at run time. *That* is a serious problem. (In fact\, Carp currently loading overload.pm at run time is problematic\, the ‘other thing that may be broken’ that I mentioned above.)

Ok\, I see.

Carp is sometimes called from $SIG{__DIE__} handlers\, which may trigger due to a syntax error. After a syntax error\, BEGIN blocks won’t run. (This actually happened. See commit 018c7c82242.) I have written a test that fails in current blead because of this.

That means: - For perl 5.16 onwards\, we need to load overload.pm up front. - For perl 5.8.1 to 5.14\, we need to load Scalar::Util up front as well\, even though we might not use it.

I dont see any of this as an issue.

Alternatively\, for perl 5.10.1 to 5.14\, we can copy the more recent overload::StrVal into Carp. It consists of: sub { no overloading; "$_[0]" }. You can’t get much faster than that. In which case we might as well use it also in current blead and avoid extra conditions. We don’t need to load overload.pm at all in perl 5.10.1+.

Doesnt this also suffer the problem you mentioned of loading code at run time?

Did you know that the Romanian word for carp is crap (the fish\, not the verb)?

No. How appropriate.

As for the stash vivification test\, I know why Zefram avoided vivifying utf8::. It broke things on ancient perl versions. That makes sense. But overload? I think the reason is that Carp is so ubiquitous that you don’t want it leaving droppings lying around that it might not even use\, every time anything loads it. But loading overloading.pm vivifies the overload stash. I think we have to live with it.

Now\, for perl 5.8.1 to 5.10.0\, we have (1) an overload::StrVal that loads Scalar::Util at run time\, and (2) an overload::Overloaded which for Carp is unusable\, since it calls ->can.

Either we go ahead and load Scalar::Util when loading Carp--which I don’t like\, but I can be persuaded--\, or we avoid overload::StrVal and do it the hard way\, writing a pure-Perl StrVal.

For Perl 5.8.0 (yes\, I think we should support 5.8.0 still)\, overload::StrVal has the worst implementation yet. It unconditionally blesses any ref passed to it\, which may well cause real problems. Use of overload::StrVal isn’t all that common\, compared to Carp. Carp will make it common and start blessing people’s references left and right when generating a stack trace. Avoid this old StrVal at all costs!

Would some of this be solved by moving overload.pm to dist and allowing it to be released on cpan like Carp is?

It looks as though the simplest approach to all this is: - For 5.10.1+\, use overloading.pm. - For 5.10.0-\, give Carp its own pure-Perl StrVal.

Ok\, well\, if the patches you have pushed work then I am fine to drop this. I will just say I find some of the reasons for this stuff to be excessive pandering to backwards compat. But i guess there isnt much we can do about that.

Yves

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

p5pRT commented 6 years ago

From @demerphq

On 26 February 2018 at 10:33\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sun\, 25 Feb 2018 23:50:45 -0800\, sprout wrote:

It looks as though the simplest approach to all this is: - For 5.10.1+\, use overloading.pm. - For 5.10.0-\, give Carp its own pure-Perl StrVal.

Please review the patch on the sprout/carp-strval branch.

It is definitely cleaner than what we have now.

My only complaint is I hate this:

qw 'UNIVERSAL isa'

:-)

Anyway\, looks much saner to me than what we do now.

cheers\, Yves

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

p5pRT commented 6 years ago

From @cpansprout

On Mon\, 26 Feb 2018 23:23:40 -0800\, demerphq wrote:

On 26 February 2018 at 08:50\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sun\, 25 Feb 2018 20:28:02 -0800\, demerphq wrote:

On 26 February 2018 at 05:02\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sun\, 25 Feb 2018 16:14:44 -0800\, demerphq wrote: Bug #132910\, which was erroneously merged into this ticket\, would have happened\, but we would not have noticed\, because it would only have happened with a new Carp on perl 5.14 or earlier.

I dont follow. If we simply did

use overload ();

then we could replace this logic:

# overload uses the presence of a special # "method" named "((" or "()" to signal # it is in effect. This test seeks to see if it has been set up. if (_mycan($pack\, "((") || _mycan($pack\, "()")) { # Argument is blessed into a class with overloading\, and # so might have an overloaded stringification. We don't # want to risk getting the overloaded stringification\, # so we need to use overload::StrVal() below. But it's # possible that the overload module hasn't been loaded: # overload methods can be installed without it. So load # the module here. The bareword form of require is here # eschewed to avoid this compile-time effect of vivifying # the target module's stash. require "overload.pm"; } my $sub = _fetch_sub(overload => 'StrVal'); return $sub ? &$sub($arg) : "$arg";

with a simple:

return overload::StrVal($arg);

You’re right.

Alternatively\, for perl 5.10.1 to 5.14\, we can copy the more recent overload::StrVal into Carp. It consists of: sub { no overloading; "$_[0]" }. You can’t get much faster than that. In which case we might as well use it also in current blead and avoid extra conditions. We don’t need to load overload.pm at all in perl 5.10.1+.

Doesnt this also suffer the problem you mentioned of loading code at run time?

Not if we load overloading.pm up front\, which my patch does. I think it’s unavoidable.

For Perl 5.8.0 (yes\, I think we should support 5.8.0 still)\, overload::StrVal has the worst implementation yet. It unconditionally blesses any ref passed to it\, which may well cause real problems. Use of overload::StrVal isn’t all that common\, compared to Carp. Carp will make it common and start blessing people’s references left and right when generating a stack trace. Avoid this old StrVal at all costs!

Would some of this be solved by moving overload.pm to dist and allowing it to be released on cpan like Carp is?

Please\, no. That would be a nightmare. We would end up playing this same compatibility game with overload.pm.

--

Father Chrysostomos

p5pRT commented 6 years ago

From @cpansprout

On Mon\, 26 Feb 2018 23:51:01 -0800\, demerphq wrote:

On 26 February 2018 at 10:33\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sun\, 25 Feb 2018 23:50:45 -0800\, sprout wrote:

It looks as though the simplest approach to all this is: - For 5.10.1+\, use overloading.pm. - For 5.10.0-\, give Carp its own pure-Perl StrVal.

Please review the patch on the sprout/carp-strval branch.

It is definitely cleaner than what we have now.

My only complaint is I hate this:

qw 'UNIVERSAL isa'

:-)

Would you prefer qw FUNIVERSAL isaF? It has FUN in it. :-)

Anyway\, looks much saner to me than what we do now.

Thank you. I’ve pushed it as 5c8d1071 (with qw/.../).

--

Father Chrysostomos

p5pRT commented 6 years ago

From @bulk88

On Tue\, 27 Feb 2018 09:16:44 -0800\, sprout wrote:

On Mon\, 26 Feb 2018 23:51:01 -0800\, demerphq wrote:

On 26 February 2018 at 10:33\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Sun\, 25 Feb 2018 23:50:45 -0800\, sprout wrote:

It looks as though the simplest approach to all this is: - For 5.10.1+\, use overloading.pm. - For 5.10.0-\, give Carp its own pure-Perl StrVal.

Please review the patch on the sprout/carp-strval branch.

It is definitely cleaner than what we have now.

My only complaint is I hate this:

qw 'UNIVERSAL isa'

:-)

Would you prefer qw FUNIVERSAL isaF? It has FUN in it. :-)

Anyway\, looks much saner to me than what we do now.

Thank you. I’ve pushed it as 5c8d1071 (with qw/.../).

Commit 5c8d fails badly on Windows for me.

C:\p525\src\win32>cd ..\t & perl harness -v ../dist/Carp/t/stack_after_err.t & cd ..\win32 ../dist/Carp/t/stack_after_err.t .. 1..4 # Failed test 'Carp does not try to load modules on demand for overloaded args ' not ok 1 - Carp does not try to load modules on demand for overloaded args# at t/stack_after_err.t line 22.

# '' # doesn't match '(?^s:Looks lark.*o=ARRAY.* CODE)' not ok 2 - StrVal fallback in the presence of UNIVERSAL::isa # Failed test 'StrVal fallback in the presence of UNIVERSAL::isa' # at t/stack_after_err.t line 69. # '' # doesn't match '(?^s:Looks lark.*o=ARRAY.* CODE)' not ok 3 - StrVal fallback in the presence of UNIVERSAL::can# Failed test 'Str Val fallback in the presence of UNIVERSAL::can'

# at t/stack_after_err.t line 69. # '' # doesn't match '(?^s:Looks lark.*o=ARRAY.* CODE)' not ok 4 - StrVal fallback in the presence of UNIVERSAL::can/isa# Failed test 'StrVal fallback in the presence of UNIVERSAL::can/isa'

# at t/stack_after_err.t line 69. # '' # doesn't match '(?^s:Looks lark.*o=ARRAY.* CODE)' # Looks like you failed 4 tests of 4. Dubious\, test returned 4 (wstat 1024\, 0x400) Failed 4/4 subtests

Test Summary Report

../dist/Carp/t/stack_after_err.t (Wstat: 1024 Tests: 4 Failed: 4) Failed tests: 1-4 Non-zero exit status: 4 Files=1\, Tests=4\, 1 wallclock secs ( 0.00 usr + 0.00 sys = 0.00 CPU) Result: FAIL

C:\p525\src\win32>

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 6 years ago

From @bulk88

On Tue\, 27 Feb 2018 20:04:14 -0800\, bulk88 wrote:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process

Command line: C:\p525\src\t\perl.exe -e use Carp; sub foom { Carp::confess("Looks lark we got a error: $_[0]") } BEGIN { *{"o::()"} = sub {}; *{'o::(""'} = sub {"hay"}; $o::OVERLOAD{dummy}++; # perls before 5.18 need this *{"CODE::()"} = sub {}; $SIG{__DIE__} = sub { foom (@_\, bless([]\, o)\, sub {}) } } $a +

but Carp.t which has identical code (or almost?) does put "s in the command line foe -e's arg.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 6 years ago

From @bulk88

On Tue\, 27 Feb 2018 20:21:24 -0800\, bulk88 wrote:

On Tue\, 27 Feb 2018 20:04:14 -0800\, bulk88 wrote:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process

If Win32's open3 sees a " char anywhere in the command line\, it processes/splits/dont have time to research more\, the command line differently.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 6 years ago

From @cpansprout

On Tue\, 27 Feb 2018 20:21:24 -0800\, bulk88 wrote:

On Tue\, 27 Feb 2018 20:04:14 -0800\, bulk88 wrote:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process ------------------------ Command line: C:\p525\src\t\perl.exe -e use Carp; sub foom { Carp::confess("Looks lark we got a error: $_[0]") } BEGIN { *{"o::()"} = sub {}; *{'o::(""'} = sub {"hay"}; $o::OVERLOAD{dummy}++; # perls before 5.18 need this *{"CODE::()"} = sub {}; $SIG{__DIE__} = sub { foom (@_\, bless([]\, o)\, sub {}) } } $a +

------------------------------------

but Carp.t which has identical code (or almost?) does put "s in the command line foe -e's arg.

I suspect IPC::Open3 (or something it calls) is adding the quotation marks if the argument has none.

Would it be feasible to feed the program to stdin and drop -e?

--

Father Chrysostomos

p5pRT commented 6 years ago

From @demerphq

On 28 February 2018 at 06:41\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Tue\, 27 Feb 2018 20:21:24 -0800\, bulk88 wrote:

On Tue\, 27 Feb 2018 20:04:14 -0800\, bulk88 wrote:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process ------------------------ Command line: C:\p525\src\t\perl.exe -e use Carp; sub foom { Carp::confess("Looks lark we got a error: $_[0]") } BEGIN { *{"o::()"} = sub {}; *{'o::(""'} = sub {"hay"}; $o::OVERLOAD{dummy}++; # perls before 5.18 need this *{"CODE::()"} = sub {}; $SIG{__DIE__} = sub { foom (@_\, bless([]\, o)\, sub {}) } } $a +

------------------------------------

but Carp.t which has identical code (or almost?) does put "s in the command line foe -e's arg.

I suspect IPC::Open3 (or something it calls) is adding the quotation marks if the argument has none.

Would it be feasible to feed the program to stdin and drop -e?

I just pushed:

commit 01d4cc0fe2b9e198c9146d4c84e781b5d2d3117f Author: Yves Orton \demerphq@gmail\.com Date: Wed Feb 28 16:02:17 2018 +0100

rework Carp/t/stack_after_err.t to not use perl -e

cheers\, Yves

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

p5pRT commented 6 years ago

From @cpansprout

On Wed\, 28 Feb 2018 07:03:52 -0800\, demerphq wrote:

On 28 February 2018 at 06:41\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

Would it be feasible to feed the program to stdin and drop -e?

I just pushed:

commit 01d4cc0fe2b9e198c9146d4c84e781b5d2d3117f Author: Yves Orton \demerphq@gmail\.com Date: Wed Feb 28 16:02:17 2018 +0100

rework Carp/t/stack_after_err.t to not use perl -e

cheers\, Yves

Thank you.

--

Father Chrysostomos

p5pRT commented 6 years ago

From @demerphq

On 28 February 2018 at 16:03\, demerphq \demerphq@gmail\.com wrote:

On 28 February 2018 at 06:41\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Tue\, 27 Feb 2018 20:21:24 -0800\, bulk88 wrote:

On Tue\, 27 Feb 2018 20:04:14 -0800\, bulk88 wrote:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process ------------------------ Command line: C:\p525\src\t\perl.exe -e use Carp; sub foom { Carp::confess("Looks lark we got a error: $_[0]") } BEGIN { *{"o::()"} = sub {}; *{'o::(""'} = sub {"hay"}; $o::OVERLOAD{dummy}++; # perls before 5.18 need this *{"CODE::()"} = sub {}; $SIG{__DIE__} = sub { foom (@_\, bless([]\, o)\, sub {}) } } $a +

------------------------------------

but Carp.t which has identical code (or almost?) does put "s in the command line foe -e's arg.

I suspect IPC::Open3 (or something it calls) is adding the quotation marks if the argument has none.

Would it be feasible to feed the program to stdin and drop -e?

I just pushed:

commit 01d4cc0fe2b9e198c9146d4c84e781b5d2d3117f Author: Yves Orton \demerphq@gmail\.com Date: Wed Feb 28 16:02:17 2018 +0100
rework Carp/t/stack\_after\_err\.t to not use perl \-e

Bulk88 can you confirm this patch fixed the win32 build issue?

Yves

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

p5pRT commented 6 years ago

From @bulk88

On Thu\, 01 Mar 2018 00:22:19 -0800\, demerphq wrote:

On 28 February 2018 at 16:03\, demerphq \demerphq@gmail\.com wrote:

On 28 February 2018 at 06:41\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Tue\, 27 Feb 2018 20:21:24 -0800\, bulk88 wrote:

On Tue\, 27 Feb 2018 20:04:14 -0800\, bulk88 wrote:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process ------------------------ Command line: C:\p525\src\t\perl.exe -e use Carp; sub foom { Carp::confess("Looks lark we got a error: $_[0]") } BEGIN { *{"o::()"} = sub {}; *{'o::(""'} = sub {"hay"}; $o::OVERLOAD{dummy}++; # perls before 5.18 need this *{"CODE::()"} = sub {}; $SIG{__DIE__} = sub { foom (@_\, bless([]\, o)\, sub {}) } } $a +

------------------------------------

but Carp.t which has identical code (or almost?) does put "s in the command line foe -e's arg.

I suspect IPC::Open3 (or something it calls) is adding the quotation marks if the argument has none.

Would it be feasible to feed the program to stdin and drop -e?

I just pushed:

commit 01d4cc0fe2b9e198c9146d4c84e781b5d2d3117f Author: Yves Orton \demerphq@gmail\.com Date: Wed Feb 28 16:02:17 2018 +0100

rework Carp/t/stack_after_err.t to not use perl -e

Bulk88 can you confirm this patch fixed the win32 build issue?

Yves

works for me

C:\perl521\src\win32>cd ..\t & perl harness -v ../dist/Carp/t/stack_after_err. t & cd ..\win32 ../dist/Carp/t/stack_after_err.t .. 1..4 ok 1 - Carp does not try to load modules on demand for overloaded args ok 2 - StrVal fallback in the presence of UNIVERSAL::isa ok 3 - StrVal fallback in the presence of UNIVERSAL::can ok 4 - StrVal fallback in the presence of UNIVERSAL::can/isa ok All tests successful. Files=1\, Tests=4\, 0 wallclock secs ( 0.00 usr + 0.00 sys = 0.00 CPU) Result: PASS

C:\perl521\src\win32>

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 6 years ago

From @demerphq

On 1 March 2018 at 09:47\, bulk88 via RT \perlbug\-followup@perl\.org wrote:

On Thu\, 01 Mar 2018 00:22:19 -0800\, demerphq wrote:

On 28 February 2018 at 16:03\, demerphq \demerphq@gmail\.com wrote:

On 28 February 2018 at 06:41\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Tue\, 27 Feb 2018 20:21:24 -0800\, bulk88 wrote:

On Tue\, 27 Feb 2018 20:04:14 -0800\, bulk88 wrote:

Commit 5c8d fails badly on Windows for me.

There are no quotes in the cmd line to the process ------------------------ Command line: C:\p525\src\t\perl.exe -e use Carp; sub foom { Carp::confess("Looks lark we got a error: $_[0]") } BEGIN { *{"o::()"} = sub {}; *{'o::(""'} = sub {"hay"}; $o::OVERLOAD{dummy}++; # perls before 5.18 need this *{"CODE::()"} = sub {}; $SIG{__DIE__} = sub { foom (@_\, bless([]\, o)\, sub {}) } } $a +

------------------------------------

but Carp.t which has identical code (or almost?) does put "s in the command line foe -e's arg.

I suspect IPC::Open3 (or something it calls) is adding the quotation marks if the argument has none.

Would it be feasible to feed the program to stdin and drop -e?

I just pushed:

commit 01d4cc0fe2b9e198c9146d4c84e781b5d2d3117f Author: Yves Orton \demerphq@gmail\.com Date: Wed Feb 28 16:02:17 2018 +0100

rework Carp/t/stack_after_err.t to not use perl -e

Bulk88 can you confirm this patch fixed the win32 build issue?

Yves

works for me ------------------------------------------- C:\perl521\src\win32>cd ..\t & perl harness -v ../dist/Carp/t/stack_after_err. t & cd ..\win32 ../dist/Carp/t/stack_after_err.t .. 1..4 ok 1 - Carp does not try to load modules on demand for overloaded args ok 2 - StrVal fallback in the presence of UNIVERSAL::isa ok 3 - StrVal fallback in the presence of UNIVERSAL::can ok 4 - StrVal fallback in the presence of UNIVERSAL::can/isa ok All tests successful. Files=1\, Tests=4\, 0 wallclock secs ( 0.00 usr + 0.00 sys = 0.00 CPU) Result: PASS

Great. Thanks for the report!

cheers\, Yves

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

Perl / perl5