Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.99k stars 557 forks source link

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

Open leonerd opened 1 week ago

leonerd commented 1 week ago

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

scottchiefbaker commented 1 week ago

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

Grinnz commented 1 week ago

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 commented 1 week ago

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

tonycoz commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 6 days ago

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.

tonycoz commented 6 days ago

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

leonerd commented 5 days ago

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 commented 5 days ago

Now edited so each new operator is under its own named feature and its own warning category, as stated by https://github.com/Perl/PPCs/pull/60#issuecomment-2506579839

leonerd commented 1 day ago

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.