Add a new %{^HOOK} hash, similar to %SIG, and add support for "require__before" and "require__after" hooks.

[demerphq]

This PR adds a new %{^HOOK} hash, similar to %SIG, but for hooking keywords. The hash is magic in a similar way as %SIG, in that it validates its contents on store and actually maps to internal variables which store the values instead. The first two members of the hash are require__before and require__after. Members are expected to be of the form $keyword . "__" . $phase, where $keyword is the keyword that executes the hook, and $phase is when the hook is executed (Currently either "before" or "after"). The initial user visible implementation includes support for a hook which is executed before require, and a hook which is executed after require. The require__before hook may return a callback that is also executed after the require, but can be constructed in such a way that it shares state with the before hook that executed. The before hooks execute before nearly any other require related logic, including checking if the required item was already required, and the after hooks executed after the require completed, regardless as to whether the require died or was successful.

With the required__before hook we could fix Devel::TraceUse to not alter the stack when it is in use. It could also be used to detect circular dependencies, log all used modules, or track the memory and time taken to require a given module (with or without its dependencies).

In order to implement this there are some nice goodies from an internal point of view. mortal_destructor_sv() and mortal_svfunc_x() (and their macro wrappers SAVEMORTALDESTRUCTOR_SV() and SAVEMORTALSVFUNC_X()) can be used to execute a callback at the end of the current statement (either Perl or C callback respective). Also there is the SvREFCNT_dec_set_NULL() macro which can be used to decrement an SV* pointer and set it to NULL in a single statement, and a related function SvREFCNT_dec_ret_NULL() which decrements an SV's refcount and returns NULL, which is used by the SvREFCNT_dec_set_NULL() macro but can also be helpful in cases where a function wants to do this to an argument and then return NULL. There are also various other minor improvements to regen/mg_vtable.pl (trimming trailing whitespace on generated documentation and renaming of some confusingly named internal variables) and improvements to the documentation of the related functions in pod/perlguts.pod.

Everything has documentation, including the new utility functions and macros.

@haarg @ap @book

[leonerd]

I've always thought that the abuse of %SIG which __DIE__ and __WARN__ make is incredibly ugly; I would much have preferred those two to have been written ${^DIEHOOK} and ${^WARNHOOK} so that every element of %SIG really does relate to a real POSIX signal.

It's rather too late to change those now, but can we avoid adding any more? I.e. can this please be spelled ${^REQUIREHOOK} instead?

[demerphq]

I.e. can this please be spelled ${^REQUIREHOOK} instead?

FWIW, my original implementation worked like that. But it was more work and it felt unperlish to me so i switched to %SIG. While I agree with your sentiment, and likely I would agree that if $SIG{__WARN__} was ${^WARN} or something it might be better, but that boat has sailed and I think that having /some/ hooks in magic caret vars and /some/ in %SIG is worse than having them all in %SIG, the element of surprise is reduced at the very least. There is also a small advantage with %SIG that you can enumerate all the hooks that are in effect by walking %SIG, which has some level of utility and affords a certain sense of future portability. For instance if its in %SIG then a generic "disable all hooks" is just local %SIG=(); (ignoring @INC hooks), with hooks all over the place that becomes rather more difficult and impossible to future proof against.

There is another advantage at the implementations level that adding support for validating a new special key in %SIG is nearly trivial, but adding a new var with new magic to front an internal state is rather more complex and invasive. I found it interesting how much simpler the code was when it was based on %SIG.

So frankly I'd prefer that we don't use a caret var for this. If PSC decides we must then so be it, it can be done, but I don't think its as good an idea as it sounds at first. And I mean that literally, my unpushed original code for this used ${^REQUIRE_HOOK} much as you suggested, but after playing with it I came to the conclusion that it just felt weird to have $SIG{__WARN__} on one hand and ${^REQUIRE_HOOK} on the other.

[demerphq]

@leonerd one option might be to introduce a %{^HOOK} variable. We could mirror warn and die between the two hashes. Then use that in new hooks.

[leonerd]

Ahyes; that sounds like a better forward plan; a new place to put hooks would be nice.

In that case, since we'd want to move WARN and DIE out of the current place anyway, it probably makes little difference whether we add REQUIRE in to SIG for now anyway. Might as well continue on this PR as it is for now and move all of them later on.

[ap]

I've always thought that the abuse of %SIG which __DIE__ and __WARN__ make is incredibly ugly

I agree it would be cleaner to only have %SIG concerned with POSIX signals, and have those other hooks live elsewhere. But they do make a sort of sense to me as well, considering they too are a program interruption somewhat akin to a signal.

A __REQUIRE__ hook in %SIG is a very weird place for it though. The only reason to put it there is that __DIE__ and __WARN__ happen to be there already, which also happens to make the implementation easier. Without that path dependence, it would seem a bizarre design decision.

One option might be to introduce a %{^HOOK} variable. We could mirror warn and die between the two hashes. Then use that in new hooks.

This, however, I have a severe allergic reaction to. Please don’t. Code which uses the new interface would not work on older perls, not because older perls lack capabilities to support the feature being used, but simply because the way to access the capability changed name. To put it another way: because fashion. Worse, dealing with these hooks is relevant inside some rather plumbing-level modules, where forcing churn is just going to impose more forced perl version obsolescence on the ecosystem.

The same reasoning is going to apply in the future, of course, so adding a %{^HOOK} variable now – but for require only, with no mirroring of the %SIG hooks – seems like a good idea, if that seems like something there will be more use for later. (Btw, also, can we have lowercase keys in that case? (Or rather, simply use the name of the function being hooked, verbatim.) That %SIG keys are all uppercase makes sense, but IMO %{^HOOK} should not be beholden to that precedent.)

[demerphq]

@ap: to you it is a category error to put REQUIRE with DIE and WARN in %SIG. But i dont agree with you really, I see a "signal" generated when we are about to compile more code to be compatible with the notion of a generic signal, as much as DIE or WARN do anyway. Remember that require is triggered invisibly by use, and internally by using of various functionality or variables. So when someone "uses" code, having a "signal" triggered at the start of that process isn't that weird to me. Certainly to me it is not so obviously wrong that it seems like a "bizarre design decision". To me it seems a design decision well within the general concept of a signal triggered when perl does something invisibly under the hood. Especially as the require hook really is a true /signal/, unlike DIE or WARN, in that it cannot affect the execution of the require other than to throw an exception, both DIE and WARN allow you to modify the behavior of the die/warn functions. So they arent just signal handlers but also mutators.

The only reason you seem to be distinguishing DIE and WARN from REQUIRE is that DIE and WARN are part of an exception handling process. To me the thing that the %SIG keys have in common is that they can all be triggered automatically by perl or the system itself when things happen. Perl requires modules behind the scenes, just as it throws exceptions, and thus it seems to me that they have more in common than they differ, and in fact, that REQUIRE, not being a mutator, actually has more in common with other signal handlers than it does with DIE or WARN and that it is really those two which are exceptional. [1]

With regard to %{^HOOK} or whatever we call it (if we go ahead with it), i was thinking we would just "tie" the DIE and WARN keys in %SIG to their equivalents in %{^HOOK}, not necessarily removing the keys from %SIG until there has been a very very long deprecation cycle. The only thing that might break back compat is someone using only %{^HOOK} instead of %SIG, but that is the same as someone using defer{}. It would also be possible to create a shim to make %{^HOOK} work properly on older perls if we wanted.

But i take your point that introducing a %{^HOOK} variable may just end up making things worse not better, that was part of my reason for saying "the only thing worse than putting this in %SIG while WARN and DIE are there is to have a different hook not in SIG." The precedent has been established, no matter how explicable we find that precedent, and we should just stick with it.

[1] the return behavior of the REQUIRE hook is a bit exceptional. but the actual location of the hook to me not so much.

[demerphq]

@ap: Consider this case. opening a scalar file buffer auto-requires PerlIO::scalar.

$ ./perl -Ilib -le'$SIG{__REQUIRE__}= sub { print "__REQUIRE__: @_" }; open my $fh,"<", \"" or die;'
__REQUIRE__: PerlIO.pm
__REQUIRE__: PerlIO/scalar.pm
__REQUIRE__: XSLoader.pm
__REQUIRE__: strict.pm

To me this feels very much like what perl level %SIGnals do. Tell us when something internal has happened that we might want to know about.

[demerphq]

So are there any objections to this moving forward?

[demerphq]

This feels warnocked. I would like to merge this for the next release.

[haarg]

@ap: to you it is a category error to put __REQUIRE__ with __DIE__ and __WARN__ in %SIG. But i dont agree with you really, I see a "signal" generated when we are about to compile more code to be compatible with the notion of a generic signal, as much as __DIE__ or __WARN__ do anyway. Remember that require is triggered invisibly by use, and internally by using of various functionality or variables. So when someone "uses" code, having a "signal" triggered at the start of that process isn't that weird to me. Certainly to me it is not so obviously wrong that it seems like a "bizarre design decision". To me it seems a design decision well within the general concept of a signal triggered when perl does something invisibly under the hood. Especially as the require hook really is a true /signal/, unlike DIE or WARN, in that it cannot affect the execution of the require other than to throw an exception, both DIE and WARN allow you to modify the behavior of the die/warn functions. So they arent just signal handlers but also mutators.

But all of the other %SIG entries do change the execution, just by being installed. For POSIX signals, installing a %SIG entry prevents the system default signal handler from being executed.

The only reason you seem to be distinguishing DIE and WARN from REQUIRE is that DIE and WARN are part of an exception handling process. To me the thing that the %SIG keys have in common is that they can all be triggered automatically by perl or the system itself when things happen. Perl requires modules behind the scenes, just as it throws exceptions, and thus it seems to me that they have more in common than they differ, and in fact, that REQUIRE, not being a mutator, actually has more in common with other signal handlers than it does with DIE or WARN and that it is really those two which are exceptional. [1]

With regard to %{^HOOK} or whatever we call it (if we go ahead with it), i was thinking we would just "tie" the __DIE__ and __WARN__ keys in %SIG to their equivalents in %{^HOOK}, not necessarily removing the keys from %SIG until there has been a very very long deprecation cycle. The only thing that might break back compat is someone using only %{^HOOK} instead of %SIG, but that is the same as someone using defer{}. It would also be possible to create a shim to make %{^HOOK} work properly on older perls if we wanted.

But i take your point that introducing a %{^HOOK} variable may just end up making things worse not better, that was part of my reason for saying "the only thing worse than putting this in %SIG while WARN and DIE are there is to have a different hook not in SIG." The precedent has been established, no matter how explicable we find that precedent, and we should just stick with it.

[1] the return behavior of the __REQUIRE__ hook is a bit exceptional. but the actual location of the hook to me not so much.

I don't think that require being used "behind the scenes" is really justification for it being handled differently from hooking into any other CORE:: function. And I think the fact that __REQUIRE__ functions entirely differently from all the other %SIG entries means it doesn't belong there. The magic return value is different from any other %SIG entry, and I don't think the desire to wrap require justifies the special case.

The original problem leading to this PR is that wrapping CORE::GLOBAL::require is problematic. This also applies to a number of other CORE functions, such as open and system. Maintaining context and correct parsing is hard or impossible. So I'd like to see a more general solution for attaching before/after actions to core subs. A separate variable from %SIG seems more appropriate. Since the hooks that this PR wants to implement work differently from the existing __DIE__ and __WARN__ handlers work, I don't think they need to be moved or aliased into the new feature.

[haarg]

I'll add that while I'd like to see a more general solution for this type of hook, I think it's fine if there's an initial implementation that only works for require. But I'd like the shape of it to allow future extension, and I don't think %SIG is the right place for that.

[demerphq]

Hi @haarg. I have outlined why i dont think a separate variable is a good idea. So while I respect your opinion, if that is the strongest point you have here, then I think we will have to just agree to disagree. IMO The only thing worse than having __WARN__, __DIE__ and __REQUIRE__ be in %SIG is having __WARN__ and __DIE__ be in a different data structure from __REQUIRE__ itself. Having this live in %SIG means that %SIG=(); clears the hooks and restores "normal behavior". If we introduce a new variable for this, then coders need to know about both. I think that is clearly worse than whatever conceptual friction you have with putting this stuff alongside the other hooks in %SIG.

I actually do agree that it would have been better if Larry had put the DIE and WARN hooks somewhere else, and he had I would have put the REQUIRE hook in the same place, but that boat has sailed. Lets not make the problem worse.

Also, I do agree that I would not be pushing REQUIRE at all if it was sane to override require and other core ops properly. But it isn't and it seems likely it wont be for some time, if ever. So I think we should go ahead with REQUIRE as is.

[Leont]

I don't quite like it in %SIG either (and hate the existing exceptions anyway).

[demerphq]

@Leont yeah, I understand. I dont like a number of design decisions in Perl. :-) But once they are baked in we have to consider this from the point of view of "least worst" options, not as though we have a clear field. And I think that the least worst option here is just swallow our dislike of this. The other option opens up its own can of portability worms. Specifcally that code that currently exists and does:

local %SIG;

wll continue to work as it used to: disabling all signal handlers, OS level and Perl level.

If we introduce a new var that is no longer true, and it becomes yet another thing people need to deal with.

So the question here isnt "is this the ideal way to do this", it is "what is the least sucky way to do this". And IMO the objections about it being in %SIG are a matter of taste, which is important, but it lacks technical foundation.

[Grinnz]

I'm not sure what you mean. @haarg 's objections were entirely technical (and seem reasonable to me).

[wchristian]

Gotta agree that rather than messing more with %SIG, which should be posix signals, the optimal solution here would be a completely separate mechanism for hooking perl core function, which could then also include die and warn and permit future hooking on those without the issues %SIG replacements present, maybe allowing phasing that out in some far future.

[Leont]

This will treat f and p as refcounted, which is probably a bad idea.

Just using a different vtable is probably the easiest solution, this should be the simpler case anyway.

[demerphq]

I reworked the code a little bit to use SVFUNC_t which requires a SV * argument instead of DESTRUCTORFUNC_t which supports a void * argument. Also the code you quoted is/was just broken, the function needed to be wrapped in a SVt_IV and the f argument needed to be converted into an IV with PTR2IV(). But once I did that it seems work as expected and passes the tests I added to XS-APITest.

I am not really sure what we win by creating a new vtable to handle this and it would cost us one of the magic chars. It seems to me with the adjustments I just made it is fine as it is.

Can you review once more please?

[haarg]

I generally like this. I'm not entirely certain about the interface - having two types of after hooks feels like it might indicate there's a better option. But that isn't a rejection.

But is this something that should be experimental?

[demerphq]

But is this something that should be experimental?

I don't see how that would be advantageous. This doesn't get in the way and its not a core part of the language nor does it affect code that already exists. (Like most use of experimental features.)

having two types of after hooks feels like it might indicate there's a better option

There aren't two types of after hook. The is a post-before hook and an after hook. They aren't the same thing. I explained why we have both in previous post, sharing state with /just/ an after hook is error prone and unreliable. On the other hand, @ap requested we provide a separate after hook for people who don't want to use a before hook and just want to do something after each require. I just gave him what he requested.

[iabyn]

On Fri, Mar 17, 2023 at 06:37:57AM -0700, Yves Orton wrote: @demerphq commented on this pull request. > +#define SAVEMORTALSVFUNC_X(f,sv) \ + mortal_svfunc_x((SVFUNC_t)(f), sv) + +#define SAVEMORTALDESTRUCTOR_SV(coderef,args) \ + mortal_destructor_sv(coderef,args) + The match SAVEMORTALIZESV(), which where the naming comes from.

Yeah, but SAVEMORTALIZESV() still uses the save stack. It's action is to mortalise the SV in leave_scope() on scope exit So I don't think SAVE should be in the names.

…

-- The Enterprise successfully ferries an alien VIP from one place to another without serious incident. -- Things That Never Happen in "Star Trek" #7

[haarg]

I don't see how that would be advantageous. This doesn't get in the way and its not a core part of the language nor does it affect code that already exists. (Like most use of experimental features.)

It isn't syntax, but it's still a part of the language. There are plenty of other experimental things that have had warnings but no feature flag, because they didn't interfere with any existing syntax. postderef, const_attr, and builtin for example. smartmatch as well (not switch).

having two types of after hooks feels like it might indicate there's a better option

There aren't two types of after hook. The is a post-before hook and an after hook. They aren't the same thing.

Those are both "after" hooks, and I never said they were the same thing. I understand the need for carrying state between the hooks. Having both types feels a bit awkward, but I'm not sure there's a better option.

[Leont]

Yeah, but SAVEMORTALIZESV() still uses the save stack. It's action is to
mortalise the SV in leave_scope() on scope exit

So I don't think SAVE should be in the names.

I think that is a strong argument for renaming them before the next major release.

[demerphq]

I think that is a strong argument for renaming them before the next major release.

Done already. I noted in reply to @leonerd's original request, which davem was replying to, but because he replied via email it was not logged in the right place.

(This is just for the record, you and I discussed this already in IRC.)