Hide most KEY_* symbols outside core

[khwilliamson]

This is a replacement for reverted 49fb862

It changes keywords.h to use #if's to limit the visibility of the symbols contained within it that don't need to be exported.

In researching this, I discovered that we had two different KEY_ symbols that evaluated to the same value, creating a potential clash. This was because these were non-standard uses for this mechanism. Since it is too easy for that to arise again, I enhanced the generating script to be able to handle non-standard ones, and folded those into it.

• This set of changes does not require a perldelta entry.

[mauke]

I don't like this. keywords.h is already an internal header. The way to opt into the KEY_* symbols being defined is to say #include "keywords.h".

Now, to get the same effect, you have to say

#define PERL_CORE
#include "keywords.h"

What's the point of requiring two preprocessor directives instead of one?

[khwilliamson]

What's the point of requiring two preprocessor directives instead of one?

The module shouldn't be including our internal header in the first place. Yes it can cheat, and that second directive makes it painfully obvious that that is what is going on. That's the point. To make it harder to inadvertently gain access to our internals.

@mauke previously wrote

The "extra steps" to gain access to the perl KEY_foo symbols are to add #include "keywords.h" to one's code. Almost no module does this. (The only CPAN modules I know of that do are warnings::unused (abandoned/unmaintained), Devel::Hook (only wants to see whether perl supports UNITCHECK, could be replaced by a version check), XS::Parse::Keyword, and XS::Parse::Sublike.) I don't think the status quo is all that bad.

There are at least two reasons to limit the visibility of symbols.

• To prevent the module's namespace from getting polluted by our internal symbols.
• To protect as much as possible against modules becoming reliant on our unpublished implementation, constricting our ability to change things out for a better implementation should that become desirable.

I hadn't appreciated that this header wasn't always exposed to the outside. So yes, the status quo isn't as bad as I had thought. But there has been agreement in the first pull request and no dissension at that time that these symbols are for internal use by the core. A few have leaked out, and this PR accommodates those, while limiting the spread. (I don't understand enough about XS to know if this adversely affects XS::Parse::Keyword. If it does I will submit a PR to correct that.)

[tonycoz]

I don't think it's needed - keywords.h is opt in, the names are only exposed if the user explicitly pulls them in.

[demerphq]

@tonycoz I dont think that addresses the concern that @khwilliamson expressed, we don't want people to use these files really, we could change them in any way at any time, and since we cant really stop them we want them to acknowledge that they are doing something dodgy when they do.

So I vote yes for this patch, although id make it something like:

#define YES_I_KNOW_CORE_PERL_INTERNAL_HEADERS_ARE_SUBJECT_TO_CHANGE_WITH_NO_NOTICE

and not

#define PERL_CORE

We keep saying we cant fix our internals because too much CPAN XS code depends on our current API, which means we should do everything we can to reduce our expose and increase our degrees of flexibility.

[tonycoz]

So I vote yes for this patch, although id make it something like:

#define YES_I_KNOW_CORE_PERL_INTERNAL_HEADERS_ARE_SUBJECT_TO_CHANGE_WITH_NO_NOTICE

and not

#define PERL_CORE

I don't think we need another level of "you're depending on the implementation - beware the alligators!" - we already warn about this in perlguts:

Inside the Perl core (PERL_CORE defined), you can get at the functions either with or without the Perl_ prefix, thanks to a bunch of defines that live in embed.h. Note that extension code should I set PERL_CORE; this exposes the full perl internals, and is likely to cause breakage of the XS in each new perl release.

Adding another name just means we didn't really mean it for PERL_CORE.

I just don't see that much reason to add the additional check.

[tonycoz]

Do we ever actually want to parse this as a keyword?

[khwilliamson]

Yes. The commit 93452d4 (this PR) removes that

[tonycoz]

Why parse it as a keyword? (and it does get parsed, but then discarded)

[khwilliamson]

reentr.c calls Perl_keyword(), and has a case for KEY_getspnam.

In blead, reentr.c knows that this keyword isn't returned, so has a special conditional to work around that. But perl.h also has a fake keyword KEY_sigvar. And the authors of both symbols created them with the same value, so that seeing that value is ambiguous. This may work now by coincidence, but it is brittle. The easiest way to solve this now and keep it from happening in the future is to slightly modify the existing infrastructure to allow fake keywords, and allow its existing implementation to choose unique values for each. This prevents future collisions and fixes the current one.

I presumed that code that switched on the return of Perl_keyword() had a default: case to cover the cases where something got returned that it didn't handle, but I see a couple of cases with no default. That seems brittle and potentially already buggy in blead.

[khwilliamson]

Adding another name just means we didn't really mean it for PERL_CORE.
I just don't see that much reason to add the additional check.

It would have been better to originally make PERL_CORE a longer symbol; but I agree that we can't really change it now.

I do feel fairly strongly about requiring PERL_CORE to gain access to these symbols. I can foresee someone deciding that this header needs to be pulled in by perl.h, and then these symbols suddenly and silently become visible, including any that conflict, such as KEY_END does. And the other two headers that have lots of symbols and aren't currently pulled in by perl.h already have guards, regcomp.h and feature.h. That makes keywords.h the exception, to an admittedly small sample size. But the precedent is there to limit the visibility of symbols that aren't by-default exposed to the average XS module.