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

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

Closed
wants to merge 5 commits into from
Closed

Conversation

Ovid
Copy link
Contributor

@Ovid Ovid commented Jul 14, 2024

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
Copy link
Contributor

rwp0 commented Jul 14, 2024

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 Ovid changed the title DRAFT: allow can() to take a list of methods DRAFT: allow can() to take a list of methods Jul 14, 2024
@Ovid Ovid marked this pull request as draft July 14, 2024 15:23
@Ovid Ovid changed the title DRAFT: allow can() to take a list of methods Allow can() to take a list of methods Jul 14, 2024
@Ovid
Copy link
Contributor Author

Ovid commented Jul 14, 2024

@rwp0 Thank you. Done!

@RandalSchwartz
Copy link
Contributor

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

@mauke
Copy link
Contributor

mauke commented Jul 14, 2024

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
Copy link

EvanCarroll commented Jul 15, 2024

  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
Copy link
Contributor

Grinnz commented Jul 15, 2024

  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
Copy link
Contributor

haarg commented Jul 15, 2024

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.

universal.c Outdated
}

EXTEND(SP, items - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You moved the writes to the stack before the EXTEND(), which may write beyond the end of the stack, the EXTEND should be moved above the loop.

@tonycoz
Copy link
Contributor

tonycoz commented Jul 15, 2024

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
Copy link
Contributor

rwp0 commented Jul 15, 2024

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 Ovid force-pushed the ovid/can-isa-does branch from 799ce1d to f2b6d8f Compare July 15, 2024 10:47
@Ovid
Copy link
Contributor Author

Ovid commented Jul 15, 2024

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!

@Ovid Ovid closed this Jul 15, 2024
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.

8 participants