Perl / perl5

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

Allow `can()` to take a list of methods #22405

Closed Ovid closed 1 month ago

Ovid commented 1 month ago

Not very familiar with core hacking, so feedback needed to see if this is sane at all.

Based on this discussion on P5P, this draft PR attempts to extend can() to accept a list of methods. If all of the methods are available, returns a list of coderefs matching those methods, in order. If any of the methods is not available, it returns undef.

Notes

  1. Code is in universal.c
  2. Tests are in t/universal.t
  3. No docs yet written because I want to know if this is sane
  4. Happy to write a PPC first
  5. It returns a list, so in scalar context, we get a coderef, not a number.

I wasn't sure how we felt about checking context in the core, so I punted on the last issue, but returning a coderef feels wrong.

rwp0 commented 1 month ago

Great idea and work, thanks a lot!

My minor feedback: Can format can() in the PR title with backticks to avoid a possible confusion/difficulty (albeit there's parentheses already) while reading.

Also, can remove the DRAFT: part and mark the PR as draft more properly by (temporarily) converting it as below (top right under Reviewers) so that tests won't run on it (if undesirable):

image

image

Ovid commented 1 month ago

@rwp0 Thank you. Done!

RandalSchwartz commented 1 month ago

As I recall, we discussed something like this 30 years ago. :)

mauke commented 1 month ago

I don't like this code for two reasons:

  1. The interface is counterintuitive. For example, the first added test ok( my @methods = Child->can(qw(foo bar baz)) ); never fails regardless of what arguments are passed to can. This is because can() returns a non-empty list on failure, so if (my @methods = $obj->can(qw(a b c))) is always true (as @methods = (undef) yields 1 in scalar context).

    Also, I'm not sure what these tests are about:

    ok( !Child->can(qw(foo baz no_such_method)), 'can(@methods) with non-existent methods should already return nothing' );
    ok( !scalar Child->can(qw(foo baz no_such_method)), 'can(@methods) with non-existent methods should return false in scalar context' );

    To me it looks like the same test repeated twice: ! already forces scalar context on its operand.

    I'd much prefer a separate method, named can_all or similar, that always returns a list (and an empty list on failure).

  2. The code was taken from an LLM and is thus copyright infringement.

EvanCarroll commented 1 month ago
  1. The interface is counterintuitive. For example, the first added test ok( my @methods = Child->can(qw(foo bar baz)) ); never fails regardless of what arguments are passed to can. This is because can() returns a non-empty list on failure, so if (my @methods = $obj->can(qw(a b c))) is always true

What do you mean a non-empty list on failure? I don't get that. It returns undef on failure. In list context undef is (undef). I agree that's very silly, but it's also perl. And it's the current behavior.

I wonder if it's possible to enable boolean tracking for lists, such that.

my @foo = undef

Instead of constructing (undef) construct () which evals to sv_no in scalar context. I think this would be a huge improvement for anything that returns lists.

Other than that, it seems that by requesting that ->can() return () rather than (undef) in list context you're asking for a change in behavior (as am I, but that's because this is stoopid).

Other options may be to make ->can('method1', 'method2', 'does_not_exist') return (ref, ref, undef). I would find that to be more intuitive than (undef); but, to each their own.

  1. The code was taken from an LLM and is thus copyright infringement.

Agreed. This puts the whole project at risk.

Grinnz commented 1 month ago
  1. The interface is counterintuitive. For example, the first added test ok( my @methods = Child->can(qw(foo bar baz)) ); never fails regardless of what arguments are passed to can. This is because can() returns a non-empty list on failure, so if (my @methods = $obj->can(qw(a b c))) is always true

What do you mean a non-empty list on failure? I don't get that. It returns undef on failure. In list context undef is (undef). I agree that's very silly, but it's also perl. And it's the current behavior.

I wonder if it's possible to enable boolean tracking for lists, such that.

my @foo = undef

Instead of constructing (undef) construct () which evals to sv_no in scalar context. I think this would be a huge improvement for anything that returns lists.

Other than that, it seems that by requesting that ->can() return () rather than (undef) in list context you're asking for a change in behavior (as am I, but that's because this is stoopid).

That would additionally be context sensitivity, which has led to many high profile bugs due to list context occurring unintentionally such as when constructing hashes or passing arguments to functions, thus my earlier objection to such on the list.

Other options may be to make ->can('method1', 'method2', 'does_not_exist') return (ref, ref, undef). I would find that to be more intuitive than (undef); but, to each their own.

Unfortunately, while this seems reasonable to me for informational purposes, it makes it very annoying to check the results for success.

haarg commented 1 month ago

The only return value that would make sense to me for this would be a list of subrefs the same length as its input. But that isn't useful as a boolean check as either an any or an all check.

I don't think this is a good idea.

tonycoz commented 1 month ago

The main problem I see is can() is overrideable, and perlobj even suggests overriding can() if you're using AUTOLOAD for methods.

Such existing overrides are unlikely to implement this new signature.

rwp0 commented 1 month ago

I'd much prefer a separate method, named can_all or similar, that always returns a list (and an empty list on failure).

I think this might be the way to go about it

Ovid commented 1 month ago

I think there's enough objection to this that it's OK to close. It was more an experiment with XS, anyway. Thanks for the feedback, all!