Add valid_identifier_{pvn,sv} API functions

[leonerd]

I've wanted to perform this kind of check from core code or XS modules for quite some time, but oddly we didn't appear to have an API for it until now.

There's likely places already in internal code that could be neatened by calling these functions. We can identify and fix those up later.

• This set of changes requires a perldelta entry, and it is included.

[leonerd]

I put these functions in toke.c just because they feel related to the Perl tokenizer - i.e. it's all about the rules that perl applies to perl source code. But there's no firm reason for them to live there, if elsewhere would be better.

[khwilliamson]

It also occurs to me that maybe identifiers should be restricted to every character being in the same script. This word pауpаl is not what you likely think. All but one letter in it is Cyrillic. This could be used in an attack of pull requests of malicious source code. We already have a function that checks that a string is all from the same script. It could be called just before the return true, or it could easily be changed to have an extra parameter which would be true if the string also needed to be a legal identifier

And it occurs to me that the loop could be simplified further if the underlying _safe code checked that the string had positive length. Right now I think we would get a malformed warning without that check in the loop.

[khwilliamson]

Shouldn't these link to the new functions?

[khwilliamson]

Couldn't this loop be more simply written as

while (s < end) {
    s += UTF8SKIP((U8 *) s);
    if (s >= end || ! isIDCONT_utf8_safe((U8 *) s, end)) {
        return false;
    }
}

And the second loop below similarly simplified?

[leonerd]

Not quite; it still needs the if(s == end) break condition, but it seems fine if I add that.

Updated and force-pushed.

[leonerd]

It also occurs to me that maybe identifiers should be restricted to every character being in the same script. This word pауpаl is not what you likely think. All but one letter in it is Cyrillic. This could be used in an attack of pull requests of malicious source code. We already have a function that checks that a string is all from the same script. It could be called just before the return true, or it could easily be changed to have an extra parameter which would be true if the string also needed to be a legal identifier

That could be a useful behaviour to have somewhere, perhaps with an optional flag. But my intention with this function was simply to match what the parser does, for purposes of using it to address this comment: #22765 (comment)

[tonycoz]

This is a bit tricky, in the context of the parser it's fine - we only allow non-ASCII code points in identifiers, but if this is a general use function it has the Unicode bug:

$ ./perl -Ilib -Mutf8 -MXS::APItest -E 'my $x = "café"; say valid_identifier($x) || 0; utf8::downgrade($x); say valid_identifier($x) || 0'
1
0

This could be "fixed" (I hope) by using isIDFIRST_L1()/isIDCONT_L1(), but that would allow identifiers the parser wouldn't otherwise accept when used during parsing.

[leonerd]

Yeah I tried to make it clear from the docs that this is all about what the parser would accept.

[leonerd]

Addressed the latest round of comments, and pushed again. Will need a squash before I merge it (I kept the fixes as separate commits to view them easier).

[Leont]

It sounds useful, except it's not clear to me when it would be useful. Do you have some examples?

[tonycoz]

It sounds useful, except it's not clear to me when it would be useful. Do you have some examples?

I don't know who or what you're responding to here.

nm

[leonerd]

It sounds useful, except it's not clear to me when it would be useful. Do you have some examples?

I'm about to add a call to it in the class field :reader attribute handler, to check that the requested method name is actually valid. Then the new :writer can do the same. Inspired by this PR comment - #22765 (comment)

[khwilliamson]

One set of my comments got lost. Having worked with these kinds of functions a lot, I believe the better API is to pass the end pointer instead of the length. That pointer typically must be calculated anyway, as this function now uses the length solely for that purpose, so just skip the intermediate value. I can give further reasons if this is not enough to convince you

[leonerd]

@khwilliamson Oh I see. Mm. I could add a _pve variant and put the real logic in there, and then have the _pvn and _sv versions just invoke that by calculations. I suspect having a _pvn in the API anyway would still be useful. It's still the expected pattern that the majority of existing functions all follow, so it would seem odd to not have that.

[leonerd]

@khwilliamson ^-- that latest commit then adds such a function.

[khwilliamson]

I like the _pve idea, which _sv should call directly. Other than that lgtm

[khwilliamson]

Use SvEND and call the _pve form saves work

[leonerd]

I tried that just now but it broke the stringify test. Maybe the two separate calls to SvPV and SvEND cause it to stringify two separate buffers? Anyway, just using pv + len here works fine.

[leonerd]

force-pushed to squash before merge