Implement PPC0027: any() and all() tests #22773

leonerd · 2024-11-22T16:39:20Z

As per https://github.com/Perl/PPCs/blob/main/ppcs/ppc0027-any-and-all.md

scottchiefbaker · 2024-11-22T17:24:43Z

Since List::Utils is already a core module why are any and all getting promoted into core? What makes something core worthy?

Grinnz · 2024-11-22T17:48:16Z

So are all the Scalar::Util functions that have been added to builtin. There are a number of benefits, primarily that the op can be written more efficiently (in this case, the List::Util versions suffer from the performance difference such that grep is usually more efficient even though it doesn't short-circuit); and that when it is deemed acceptable for a feature/builtin bundle, it will be automatically available under "use VERSION" and -E.

leonerd · 2024-11-22T23:16:27Z

Undrafted, ready(ish) for review. Still some outstanding questions that could do with concrete answers.

tonycoz · 2024-11-25T02:56:50Z

The PPC doesn't say why any EXPR, LIST is disallowed, but one of the obvious advantages would be not having to disambiguate between a block and a hashref literal:

$ ./perl -Ilib -Mfeature=any_all -e 'print "ok\n" if any { d => 1, /a/ } qw(b c a)'
any is experimental at -e line 1.
any EXPR, LIST is not allowed at -e line 1.

You do allude to this, I'll have a fiddle.

tonycoz · 2024-11-25T04:44:25Z

This prevents the ambiguity for me:

diff --git a/toke.c b/toke.c
index 40ab911e66..6d1931e079 100644
--- a/toke.c
+++ b/toke.c
@@ -8001,7 +8001,7 @@ yyl_word_or_keyword(pTHX_ char *s, STRLEN len, I32 key, I32 orig_keyword, struct
     case KEY_all:
         Perl_ck_warner_d(aTHX_
             packWARN(WARN_EXPERIMENTAL__ANY_ALL), "all is experimental");
-        LOP(OP_ALLSTART, XREF);
+        LOP(OP_ALLSTART, XBLOCK);
 
     case KEY_and:
         if (!PL_lex_allbrackets && PL_lex_fakeeof >= LEX_FAKEEOF_LOWLOGIC)
@@ -8011,7 +8011,7 @@ yyl_word_or_keyword(pTHX_ char *s, STRLEN len, I32 key, I32 orig_keyword, struct
     case KEY_any:
         Perl_ck_warner_d(aTHX_
             packWARN(WARN_EXPERIMENTAL__ANY_ALL), "any is experimental");
-        LOP(OP_ANYSTART, XREF);
+        LOP(OP_ANYSTART, XBLOCK);
 
     case KEY_atan2:
         LOP(OP_ATAN2,XTERM);

It even worked for the any({block} LIST) syntax, which you don't test.

tonycoz · 2024-11-25T22:23:59Z

I do think it could use some minimal tests for any(BLOCK, LIST) form, especially since Deparse can generate that:

$ ./perl -Ilib -MO=Deparse,-p -e 'use feature "any_all"; if (any { /a/ } some_list() ) { print "ok\n" }'
any is experimental at -e line 1.
use feature 'any_all';
if (any({/a/;} some_list())) {
    print("ok\n");
}
-e syntax OK

This experiment needs an entry in pod/perlexperiment.pod.

tonycoz · 2024-11-25T22:45:58Z

op.c

+            /* any { BLOCK } would create an OP_NULL[OP_SCOPE[...]] or
+             * OP_NULL[OP_LEAVE[...]] here. If we don't see this structure
+             * then it must have been any EXPR, ... which we forbid
+             * TODO: See if we can forbid this somehow in perly.y itself


bison supports conditions on the grammar rules, but you have to opt into them with %glr-parser. They're more intended for disambiguation than controlling rules in this way.

The most obvious way I could see was to make any/all return a new token instead of LSTOP and ended up with a proof of concept:

diff --git a/perly.y b/perly.y index 6a6510823f..2df4a28557 100644 --- a/perly.y +++ b/perly.y @@ -85,7 +85,7 @@ %token <opval> PLUGEXPR PLUGSTMT %token <opval> LABEL %token <ival> LOOPEX DOTDOT YADAYADA -%token <ival> FUNC0 FUNC1 FUNC UNIOP LSTOP +%token <ival> FUNC0 FUNC1 FUNC UNIOP BLKLSTOP LSTOP %token <ival> POWOP MULOP ADDOP %token <ival> DOLSHARP HASHBRACK NOAMP %token <ival> COLONATTR FORMLBRACK FORMRBRACK @@ -126,7 +126,7 @@ %left <ival> OROP <pval> PLUGIN_LOGICAL_OR_LOW_OP %left <ival> ANDOP <pval> PLUGIN_LOGICAL_AND_LOW_OP %right <ival> NOTOP -%nonassoc LSTOP LSTOPSUB +%nonassoc BLKLSTOP LSTOP LSTOPSUB %left PERLY_COMMA %right <ival> ASSIGNOP <pval> PLUGIN_ASSIGN_OP %right <ival> PERLY_QUESTION_MARK PERLY_COLON @@ -1084,6 +1084,10 @@ listop : LSTOP indirob listexpr /* map {...} @args or print $fh @args */ { $$ = op_convert_list($LSTOP, OPf_STACKED, op_prepend_elem(OP_LIST, newGVREF($LSTOP,$indirob), $listexpr) ); } + | BLKLSTOP block listexpr /* all/any { ... } @args */ + { $$ = op_convert_list($BLKLSTOP, OPf_STACKED, + op_prepend_elem(OP_LIST, newUNOP(OP_NULL, 0, op_scope($block)), $listexpr) ); + } | FUNC PERLY_PAREN_OPEN indirob expr PERLY_PAREN_CLOSE /* print ($fh @args */ { $$ = op_convert_list($FUNC, OPf_STACKED, op_prepend_elem(OP_LIST, newGVREF($FUNC,$indirob), $expr) ); diff --git a/toke.c b/toke.c index dc2239e76a..87ddc05b08 100644 --- a/toke.c +++ b/toke.c @@ -2183,6 +2183,34 @@ S_lop(pTHX_ I32 f, U8 x, char *s) } } +#define BLKLOP(f, x) return S_blklop(aTHX_ f, x, s) + +STATIC I32 +S_blklop(pTHX_ I32 f, U8 x, char *s) +{ + PERL_ARGS_ASSERT_LOP; + + pl_yylval.ival = f; + CLINE; + PL_bufptr = s; + PL_last_lop = PL_oldbufptr; + PL_last_lop_op = (OPCODE)f; + if (PL_nexttoke) + goto lstop; + PL_expect = x; + if (*s == '(') + return REPORT(FUNC); + s = skipspace(s); + if (*s == '(') + return REPORT(FUNC); + else { + lstop: + if (!PL_lex_allbrackets && PL_lex_fakeeof > LEX_FAKEEOF_LOWLOGIC) + PL_lex_fakeeof = LEX_FAKEEOF_LOWLOGIC; + return REPORT(BLKLSTOP); + } +} + /* * S_force_next * When the lexer realizes it knows the next token (for instance, @@ -8001,7 +8029,7 @@ yyl_word_or_keyword(pTHX_ char *s, STRLEN len, I32 key, I32 orig_keyword, struct case KEY_all: Perl_ck_warner_d(aTHX_ packWARN(WARN_EXPERIMENTAL__ANY_ALL), "all is experimental"); - LOP(OP_ALLSTART, XBLOCK); + BLKLOP(OP_ALLSTART, XBLOCK); case KEY_and: if (!PL_lex_allbrackets && PL_lex_fakeeof >= LEX_FAKEEOF_LOWLOGIC) @@ -8011,7 +8039,7 @@ yyl_word_or_keyword(pTHX_ char *s, STRLEN len, I32 key, I32 orig_keyword, struct case KEY_any: Perl_ck_warner_d(aTHX_ packWARN(WARN_EXPERIMENTAL__ANY_ALL), "any is experimental"); - LOP(OP_ANYSTART, XBLOCK); + BLKLOP(OP_ANYSTART, XBLOCK); case KEY_atan2: LOP(OP_ATAN2,XTERM);

This needs some clean up* but it does prevent parsing of all EXPR, LIST, and the tests for the message fail because we don't manage to actually parse it.

* combining S_lop and S_blklop mostly

Yes I did think of adding a new grammar token type. It seems a bit heavy for just this use-case but perhaps if we add more, like first() or reduce() or whatever, it won't seem so bad.

toke.c

tonycoz · 2024-11-27T03:36:50Z

Is the any(BLOCK LIST) form meant to be allowed? There's no tests either way.

As to copy or alias the topic, I'm inclined towards a read-only copy, a copy to prevent modification of the original list/container, read-only to make it obvious that modification isn't allowed.

Making it a copy doesn't match the behaviour of grep/map, nor ListUtil::any/all from a quick look, which may be a negative.

leonerd · 2024-11-27T14:00:20Z

Is the any(BLOCK LIST) form meant to be allowed? There's no tests either way.

Yes it is. Three times now I've opened the test file to go to write a test for it, then got distracted into fixing something else and forgetting. I'll do that now before I even finish writing this comment.

t/op/any_all.t lines 19+20.

As to copy or alias the topic, I'm inclined towards a read-only copy, a copy to prevent modification of the original list/container, read-only to make it obvious that modification isn't allowed.

Making it a copy doesn't match the behaviour of grep/map, nor ListUtil::any/all from a quick look, which may be a negative.

Yes; it's a deliberate deviation from the previous things, but I think it's justifable. Already we don't support deferred expressions, only blocks, so these new any/all are different from grep. Having a further difference in the behaviour of $_ seems OK as it's intending to trend towards a better design.

op.c

tonycoz · 2024-11-27T23:58:24Z

pp_ctl.c

+    SV *src = PL_stack_base[TOPMARK];
+    if (SvPADTMP(src)) {
+        SV *newsrc = sv_mortalcopy(src);
+         PL_stack_base[TOPMARK] = newsrc;


indentation nit

tonycoz · 2024-11-28T00:21:42Z

I assume the copy/alias thing is waiting on the PPC discussion in Perl/PPCs#60.

leonerd · 2024-11-28T13:45:13Z

I assume the copy/alias thing is waiting on the PPC discussion in Perl/PPCs#60.

Yeah; but it seems the decision there is "not currently". I've moved it to a "Future Scope" section to maybe think about for another time. For now then I suppose I ought to add a unit test of the current behaviour, alias-modifications and all.

leonerd · 2024-11-29T10:26:49Z

Now edited so each new operator is under its own named feature and its own warning category, as stated by Perl/PPCs#60 (comment)

leonerd · 2024-12-02T13:40:39Z

Awaiting resolution on the issue of the flag names before merging. I'm leaving the branch un-squashed for now as that makes it easier to change the flag names. It'll be squashed before merge, one way or another.

As these only permit `any BLOCK LIST` and not the deferred-expression form like `grep EXPR, LIST`, we create a whole new token type, BLKLSTOP, to represent these. The parser then only accepts the block form and not the expression form when parsing these.

leonerd added the squash-before-merge Author must squash the commits down before merging to blead label Nov 22, 2024

leonerd force-pushed the any_all branch from b2fc1ad to 973e8b2 Compare November 22, 2024 21:35

leonerd marked this pull request as ready for review November 22, 2024 23:16

github-actions bot added the hasConflicts label Nov 25, 2024

leonerd force-pushed the any_all branch from 973e8b2 to 9740356 Compare November 25, 2024 21:52

leonerd removed the hasConflicts label Nov 25, 2024

tonycoz reviewed Nov 25, 2024

View reviewed changes

leonerd force-pushed the any_all branch 2 times, most recently from 55c4f67 to e57e680 Compare November 26, 2024 14:48

tonycoz reviewed Nov 27, 2024

View reviewed changes

toke.c Show resolved Hide resolved

leonerd force-pushed the any_all branch from e57e680 to e9e50a0 Compare November 27, 2024 14:01

tonycoz reviewed Nov 27, 2024

View reviewed changes

op.c Show resolved Hide resolved

tonycoz reviewed Nov 27, 2024

View reviewed changes

leonerd force-pushed the any_all branch 2 times, most recently from e8296c2 to 893bcfc Compare November 29, 2024 10:25

leonerd force-pushed the any_all branch from a8f85e3 to 05c6a07 Compare November 29, 2024 13:30

tonycoz approved these changes Dec 1, 2024

View reviewed changes

leonerd force-pushed the any_all branch from 05c6a07 to 7d42639 Compare December 2, 2024 12:07

leonerd mentioned this pull request Dec 2, 2024

Clarifications on PPC0027 Perl/PPCs#60

Open

leonerd force-pushed the any_all branch from 7d42639 to dd6d80c Compare December 4, 2024 12:45

leonerd added 2 commits December 4, 2024 15:22

perldelta for any() + all()

fd64c88

leonerd force-pushed the any_all branch from dd6d80c to fd64c88 Compare December 4, 2024 15:22

leonerd removed the squash-before-merge Author must squash the commits down before merging to blead label Dec 4, 2024

leonerd merged commit fcc9d7e into Perl:blead Dec 4, 2024
33 checks passed

leonerd deleted the any_all branch December 4, 2024 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PPC0027: any() and all() tests #22773

Implement PPC0027: any() and all() tests #22773

leonerd commented Nov 22, 2024 •

edited

Loading

scottchiefbaker commented Nov 22, 2024

Grinnz commented Nov 22, 2024

leonerd commented Nov 22, 2024

tonycoz commented Nov 25, 2024

tonycoz commented Nov 25, 2024

tonycoz commented Nov 25, 2024

tonycoz Nov 25, 2024

leonerd Nov 26, 2024

tonycoz commented Nov 27, 2024

leonerd commented Nov 27, 2024

tonycoz Nov 27, 2024

leonerd Nov 28, 2024

tonycoz commented Nov 28, 2024

leonerd commented Nov 28, 2024

leonerd commented Nov 29, 2024

leonerd commented Dec 2, 2024

Implement PPC0027: any() and all() tests #22773

Implement PPC0027: any() and all() tests #22773

Conversation

leonerd commented Nov 22, 2024 • edited Loading

scottchiefbaker commented Nov 22, 2024

Grinnz commented Nov 22, 2024

leonerd commented Nov 22, 2024

tonycoz commented Nov 25, 2024

tonycoz commented Nov 25, 2024

tonycoz commented Nov 25, 2024

tonycoz Nov 25, 2024

Choose a reason for hiding this comment

leonerd Nov 26, 2024

Choose a reason for hiding this comment

tonycoz commented Nov 27, 2024

leonerd commented Nov 27, 2024

tonycoz Nov 27, 2024

Choose a reason for hiding this comment

leonerd Nov 28, 2024

Choose a reason for hiding this comment

tonycoz commented Nov 28, 2024

leonerd commented Nov 28, 2024

leonerd commented Nov 29, 2024

leonerd commented Dec 2, 2024

leonerd commented Nov 22, 2024 •

edited

Loading