develooper Front page | perl.perl5.porters | Postings from February 2018

Re: [perl #132902] Blead Breaks CPAN: Class::Std

Thread Previous | Thread Next
From:
demerphq
Date:
February 27, 2018 07:23
Subject:
Re: [perl #132902] Blead Breaks CPAN: Class::Std
Message ID:
CANgJU+V+=VNQ=Q_Uqwp=-KEiJHKZWGbi1Czo3LYiiTdjEVQoCg@mail.gmail.com
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/"

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About