Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarifications on PPC0027 #60

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

leonerd
Copy link
Contributor

@leonerd leonerd commented Nov 25, 2024

Closes the existing "open issues" with regard to naming of the feature flag.

Also opens one new issue to do with aliasing vs. copy of $_.

@Grinnz
Copy link

Grinnz commented Nov 25, 2024

Should it be called list_utils if it only includes any and all? I would say they are the most commonly used that require special syntax, but what if we want to add first or reduce in the future? Or would we consider whether to add them as part of this PPC?

@book
Copy link
Contributor

book commented Nov 26, 2024

Should it be called list_utils if it only includes any and all?

Why not simply any_all?

Possibly first (as a short-circuiting version of any) could be part of the lot without needing to expand the name.

what if we want to add first or reduce in the future?

How different is reduce? Should it be part of the same batch?

Do we have a better name under which put all these list-processing operations?

I would hate to end up with list_util, list_util2, ...

If we do multiple batches, we can have any_all, reduce, etc. The point to is to include them in version bundles, anyway.

@leonerd
Copy link
Contributor Author

leonerd commented Nov 26, 2024

I was hoping we'd add all the functions in the same release, even if not the same actual commit. It won't matter too much if we add a few at 5.41.7, .8, .9... as long as the set of them is fixed by the time we make the next release (of whatever version number it has). I think that would be better than lots of single-function named features.

@Grinnz
Copy link

Grinnz commented Nov 26, 2024

Well consider this my vote for adding 'first' at least. reduce I think needs a better way than $a and $b but if that can be resolved that would be nice to have too. none and notall are simple inversions of any and all so I'm not sure they're worth the feature bundle keywords.

Original PR for discussion: #50

@guest20
Copy link

guest20 commented Nov 26, 2024

There's a couple of fun options under "Deprecated and removed variables" in perlvar, I like $* best, though $# is pretty tempting too:

my $sum     = reduce { $* += $_ } @numbers;
my $grouped = reduce { $#->{ $_ } ++ } @numbers 

@rabbiveesh
Copy link
Contributor

rabbiveesh commented Nov 26, 2024

There's a couple of fun options under "Deprecated and removed variables" in perlvar, I like $* best, though $# is pretty tempting too:

my $sum     = reduce { $* += $_ } @numbers;
my $grouped = reduce { $#->{ $_ } ++ } @numbers 

I don't think that giving a new meaning to a deprecated variable is the right approach. If anything, we should have a keyword for the accumulator (acc?) and $_ for the list item.
My only annoyance with reduce when the default for the accumulator is a hashref is that it ends up looking weird as reduce { ... } {}, @the_thing_we_care_about; maybe there's a better syntax for that too (attributes, perhaps?)

@guest20
Copy link

guest20 commented Nov 26, 2024

I don't think that giving a new meaning to a deprecated variable is the right approach.

We know they're already valid syntax and they've been available since 5.10 (perlvar)

Flavour-wise, I'd like it to be a single character variable so I don't have to repeat myself like I do in:

my $sum; $sum += $_ for @numbers; 

annoyance with reduce when the default for the accumulator is a hashref

Can't one trust autoviv deal with the collector becoming a hash-ref?

@book
Copy link
Contributor

book commented Nov 27, 2024

What about $^a and $^b? They have a the benefit of an obvious identification to the left and right side of an operation. And all the $^ variables are already reserved.

@leonerd
Copy link
Contributor Author

leonerd commented Nov 27, 2024

If we want a first I can consider that, but lets do that in its own discussion.

I wanted to clarify here first what the feature/warning name would be, and also the alias/copy/readonly-copy behaviour of $_ in the blocks. Can we discuss that, so I can finish and merge the work so far, before I consider the next bit? ;)

@haarg
Copy link
Contributor

haarg commented Nov 27, 2024

Differing in behavior from grep/map would be extremely surprising. If map/grep use aliases, any/all should as well.

@book
Copy link
Contributor

book commented Nov 27, 2024

Differing in behavior from grep/map would be extremely surprising. If map/grep use aliases, any/all should as well.

My second-hand memory of this is that one is using aliases and not the other.

@haarg
Copy link
Contributor

haarg commented Nov 27, 2024

$_ is an alias inside both grep and map. grep returns a list which contains aliases to the input list elements. map returns copies.

any and all return booleans, so the aliased returns are not relevant.

@book
Copy link
Contributor

book commented Nov 27, 2024

$_ is an alias inside both grep and map. grep returns a list which contains aliases to the input list elements. map returns copies.

OK, that's what my memory was about: the returned values.

If these are meant to be analogues of map/grep, they should probably have the same behaviour indeed.

@leonerd
Copy link
Contributor Author

leonerd commented Nov 27, 2024

My thinking here was that the aliasing behaviour is basically never used intentionally and any time it comes up it's always been a bug that has caused accidental modification. The fact that these new operators already don't support the op EXPR, LIST deferred expression syntax makes them feel a little different, so I thought that a second safety-oriented difference would be OK.

Alternatively, this could be something we fix later if/when we get around to adding named lexicals to the blocks. Perhaps a :ro attribute?

if(any my $x :ro { $x is now readonly copy of list } @things) { ... }

but at that point, much like the long history of things like use strict, it would feel a bit odd that the safer version is something you have to ask extra for. If you have to ask for it, most people won't bother and will lack the intended safety.

@book
Copy link
Contributor

book commented Nov 27, 2024

The fact that these new operators already don't support the op EXPR, LIST deferred expression syntax

Why not? Many people love to use the EXPR version of map and grep.

Also, should these eventually support the proposed topicalization behaviour for map and grep (PPC 0023)?

Meaning:

op EXPR, LIST
op BLOCK LIST
op my VAR BLOCK LIST

@leonerd
Copy link
Contributor Author

leonerd commented Nov 27, 2024

Why not? Many people love to use the EXPR version of map and grep.

This was already mentioned in the original PPC0027.

Avoiding that syntax avoids a bunch of parser ambiguities that makes the-below easier to handle.

Also, should these eventually support the proposed topicalization behaviour for map and grep (PPC 0023)?

Meaning:

op EXPR, LIST
op BLOCK LIST
op my VAR BLOCK LIST

Yes - the intention is that these can easily support that final idea. By not supporting the deferred expression form, those become easier to handle.

@guest20
Copy link

guest20 commented Nov 27, 2024

@book reduce doesn't have left/right, it has aggregate/the thing... $_ is already "the thing", so the only question is what aggregate is to be named.

If you want to rename $a and $b why would you pick "$a and $b, but with extra typing"? It really is a shame $< and $> are taken, they would be way more memorable for sort BLOCK.

@leonerd we already have a tool for that, copy the list yourself:

if(any my $x { $x is now readonly copy of the item } (my @scratch=@things) ) { ... }

To echo @haarg, I agree that It's more important that the list aliasing semantics be consistent than "nice".

I'd vote for individual responsibility, in the form of a lexical pragma to make the list copying opt-in, so one can keep the faster (not copying) up until they need the slower, safer version (IE when they discover the people they work with keep submitting "oops I overwrote the thing on accident" patches). Perhaps:

use more 'list copies'; # -or-
use Readonly qw[ grep map ];

... like that, but not that
__
†. ... though first seems like a sketchy name, since last SAME_BLOCK, SAME_LIST isn't going to give you the last match

@leonerd
Copy link
Contributor Author

leonerd commented Nov 28, 2024

†. ... though first seems like a sketchy name, since last SAME_BLOCK, SAME_LIST isn't going to give you the last match

first is already very-well established as the name for this:

https://metacpan.org/pod/List::Util#first

https://metacpan.org/pod/List::Keywords#first

@leonerd leonerd force-pushed the ppc0027-issue-answers branch from ce3a7c9 to 5888878 Compare November 28, 2024 13:27
@leonerd
Copy link
Contributor Author

leonerd commented Nov 28, 2024

As the idea for read-only copies of data in $_ seemed unpopular, I've moved it into a suggestion for Future Scope; we can come back and think about that as another feature at another time.

What is the decision on feature flag name for these operators? Are we sticking with any_all for this, and accepting that first or reduce or whatever would each get their own flags? That does feel like a lot of new flags but perhaps that's OK?

@book
Copy link
Contributor

book commented Nov 28, 2024

What is the decision on feature flag name for these operators? Are we sticking with any_all for this, and accepting that first or reduce or whatever would each get their own flags? That does feel like a lot of new flags but perhaps that's OK?

We discussed this during the PSC meeting today. Assuming we're not constrained in the ever-growing number of feature flags, there should simply be one flag per symbol (so any, all, first, and reduce). This will make it easier to add more symbols of this kind in the future.

We also looked at the policy, and we want to make it possible to progress quickly on this, so that the features can be marked as experimental (because of the change in behavior from the List::Util versions) in the next stable release, and taken out of experimental in the following one.

@leonerd
Copy link
Contributor Author

leonerd commented Nov 29, 2024

We discussed this during the PSC meeting today. Assuming we're not constrained in the ever-growing number of feature flags, there should simply be one flag per symbol (so any, all, first, and reduce).

OK; I can do that. Though perhaps we should add some specific wording in the docs pointing out the case for all, that it is one specific symbol called "all", and not all of the features/warnings:

use feature 'all';
no warnings 'experimental::all';

@book
Copy link
Contributor

book commented Nov 29, 2024

Though perhaps we should add some specific wording in the docs pointing out the case for all, that it is one specific symbol called "all", and not all of the features/warnings:

This makes me realize that all is unfortunate as a feature name... Maybe that's a good argument for any_all, first, reduce...?

@haarg, @ap: what do you think?

@leonerd
Copy link
Contributor Author

leonerd commented Nov 29, 2024

My personal thought is that I think it's fine.

People really shouldn't be doing use feature ':all' anyway, because that's just a footgun waiting to happen. Yes it is a bit confusing that "all" doesn't mean all the features, it means one feature called "all", but I think that's less confusing overall for the consistency of the fact that it enables an operator called "all".

I've added a small extra detail to the docs in feature.pm:

B<Note:> remember that this enables one specific feature whose name is C<all>;
it does not enable all of the features.  This is not C<use feature ':all'>.
For that, see the section below.

@guest20
Copy link

guest20 commented Nov 29, 2024

To me it feels like being defensive in some POD doesn't really address the usability issue of calling a single feature "all"

@leonerd
Copy link
Contributor Author

leonerd commented Dec 2, 2024

PR Perl/perl5#22773 is currently paused awaiting a resolution on this question. At present it's implemented with two separate feature flags, named all and any. Despite the potential for confusion, I think this is the best choice as it nicely relates named feature flags with the names of the operators each enables.

@Grinnz
Copy link

Grinnz commented Dec 2, 2024

I don't particularly like the conflation with 'all' but at the same time, I've never seen the ':all' flag used and frankly it should be deprecated (in both the use and no case, given our negative features).

@leonerd
Copy link
Contributor Author

leonerd commented Dec 3, 2024

@Grinnz Indeed - I think especially in the case of feature.pm, it's very unlikely anyone would actually use :all the features, precisely because of those negative ones. I think the chance of a collision of semantics here is low enough not to worry.

book added a commit to Perl/perl5 that referenced this pull request Dec 3, 2024
As discussed in Perl/PPCs#60, `:all` is very unlikely to be useful.

Since it will enable all features a future Perl might support, it will
also enable some that might be incompatible with the code as written
(see `bitwise`, for example, which changes the meaning of the existing
bitwise operators).

With the addition of "negative" features in v5.32, `:all` became even
less useful, since it would re-enable features deemed undesirable in
modern Perl.

As this point in time, `:all` is effectively a footgun.

If the keyword `all` (from PPC0027) is added as a feature, there's an
extra risk of confusion between `use feature 'all'` and `use feature ':all'`.

This patch makes `:all` warn. We should consider removing that bundle
entirely in the future.
book added a commit to Perl/perl5 that referenced this pull request Dec 4, 2024
As discussed in Perl/PPCs#60, `:all` is very unlikely to be useful.

Since it will enable all features a future Perl might support, it will
also enable some that might be incompatible with the code as written
(see `bitwise`, for example, which changes the meaning of the existing
bitwise operators).

With the addition of "negative" features in v5.32, `:all` became even
less useful, since it would re-enable features deemed undesirable in
modern Perl.

As this point in time, `:all` is effectively a footgun.

If the keyword `all` (from PPC0027) is added as a feature, there's an
extra risk of confusion between `use feature 'all'` and `use feature ':all'`.

This patch makes `:all` warn. We should consider removing that bundle
entirely in the future.
@ap
Copy link
Contributor

ap commented Dec 4, 2024

My thinking here was that the aliasing behaviour is basically never used intentionally and any time it comes up it’s always been a bug that has caused accidental modification.

I use it all the time. And I would absolutely expect it from list ops that are essentially specializations of grep. Sometimes the aliasing is undesirable and requires making a copy explicitly, which is slightly inconvenient, but an easy minor alteration of the code – whereas if aliasing is needed but copying were the only available behavior, the only way out would be switching to an entirely different approach (e.g. iterating over indexes instead of values).

@leonerd leonerd force-pushed the ppc0027-issue-answers branch from 5888878 to 5baf2c9 Compare December 4, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants