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

alias references incorrectly bomb out on empty array refs #69

Open
bitbugprime opened this issue Mar 9, 2013 · 14 comments
Open

alias references incorrectly bomb out on empty array refs #69

bitbugprime opened this issue Mar 9, 2013 · 14 comments

Comments

@bitbugprime
Copy link

use MooseX::Declare;
use Method::Signatures::Modifiers;
use v5.10;

class A {
method x (Str @args) {
    say for @args;
}
method y (Str \@args) {
    say for @args;
}
}

my @x = qw<foo bar baz>;
my @y = ();

my $a = A->new;
$a->x(@x);
$a->x(@y);

$a->y(\@x);
$a->y(\@y);

On the second invocation of method y, I get the error:
Use of uninitialized value $name in concatenation (.) or string at /usr/local/lib/perl5/site_perl/5.16.2/Method/Signatures.pm line 1272
In call to A::y(), the '' parameter ("args") is not of type Str

Basically, "Str @Args" doesn't care if it's passed a zero element array, while "Str @Args" bombs out when passed an arrayref with zero elements. That doesn't seem right, arrayrefs should work just like arrays in this case.

You can, of course, do "ArrayRef[Str] $args" instead of "Str @Args" and have no problems receiving an empty array ref, but you'd lose the sugar you get by using the reference alias syntax.

@barefootcoder
Copy link
Contributor

You can, of course, do "ArrayRef[Str] $args" instead of "Str @Args" ...

Actually, what you should do is ArraryRef[Str] \@args. I guess that's counter-intuitive, but that's the way it works. The \@ syntax is telling MS to create a local variable which is an array, even though the parameter is an arrayref. The type checking is on the parameter passed, not on the local variable. Therefore, you have to use the type of the parameter you're passed, not the local variable you're going to utilize.

Does that make it make better sense?

@bitbugprime
Copy link
Author

Arrays with zero elements are perfectly valid things in Perl. And aliases of zero-element arrays happen all the time: see @_ in subroutines. So why should an alias of a zero-element arrayref fail type checking?

@schwern
Copy link
Contributor

schwern commented Mar 11, 2013

On 3/10/13 11:18 PM, Buddy Burden wrote:

Actually, what you /should/ do is |ArraryRef[Str] @Args|. I guess
that's counter-intuitive, but that's the way it works.

It is counter-intuitive, and fortunately we control how it works!

Can we automatically change Str \@args into an ArrayRef[Str] check?

@barefootcoder
Copy link
Contributor

Arrays with zero elements are perfectly valid things in Perl. And aliases of zero-element arrays happen all the time: see @_ in subroutines. So why should an alias of a zero-element arrayref fail type checking?

The problem really has nothing to do with your array being empty. The problem is that the type checking is applied to the parameter passed in, which is not a Str but rather an ArrayRef. I certainly hope that check is failing regardless of whether there's anything in the arrayref.

Can we automatically change Str \@args into an ArrayRef[Str] check?

Sure, we could ... I guess the question is, do we want to? :-D Saying that it's "counter-intuitive" really just means that it's counter-intuitive to @Randian. It wasn't counter-intuitive to me, for instance, although I certainly have an unfair advantage by knowing a lot of the internal implementation. I'm not sure whether it's counter-intuitive to other users or not, but I'm betting that if we fix that issue for some users, we'll create counter-intuition for others.

My general rule-of-thumb is that I try to imagine how to document it. If it seems like it's going to be difficult to explain how it works, I'm wary of trying to make it work like that. In this case, if I pass in \@foo and I refer to it in my function as @args, then I have to note that neither @foo nor @args (nor \@foo, even) is a Str, so I wonder how we'd explain exactly what was going on there. :-/

@schwern
Copy link
Contributor

schwern commented Mar 12, 2013

FWIW its counter intuitive to me, too. Because of the aliasing, from
the POV of what's being passed in and what's being used, its an array.

OTOH does a type check of "Str @array" even make sense? Its not a
string, its an array of strings. We may be addressing the wrong end of
the problem. Moose doesn't have a way of saying "an array of strings"
AFAIK.

@bitbugprime
Copy link
Author

Running some code, "Str @array" does make sure that the first element is a Str, but the rest can be anything. In other words, the sigil is not part of the type of the variable for type checking purposes. Indeed, all the @ sigil seems to signify is "one or more", rather than "exactly one".

here's an interesting test case.

use v5.10;
use Method::Signatures;

my $a = [qw<a b c>];

func x (ArrayRef[Str] $a) {
    for my $string (@{$a}) {
        say $string;
    }
}
x($a);

func z (ArrayRef[Str] \@a) {
    for my $string (@a) {
        say $string;
    }
}
z($a);

[bin]$ perl zzz
a
b
c
In call to main::z(), the 'b' parameter ("a") is not of type ArrayRef[Str] at zzz line 19.

@barefootcoder
Copy link
Contributor

OTOH does a type check of "Str @array" even make sense? Its not a string, its an array of strings. We may be addressing the wrong end of the problem. Moose doesn't have a way of saying "an array of strings" AFAIK.

Nope, it doesn't. That's because you can't really pass an array to a function. Since arrays (and hashes) are always flattened, there's no concept of an array (or a hash) in the parameter list. You can typecheck the individual parameters, but there's no way to typecheck an arbitrary group of them.

In call to main::z(), the 'b' parameter ("a") is not of type ArrayRef[Str] at zzz line 19.

Hunh. So we are trying to typecheck the local variable and not the parameter passed in. I'd consider that a bug, actually--you can't typecheck an array or hash (see above). Maybe we just have to state a rule that you can't typecheck slurpy parameters .... ? (See, that would be easy to document. ;-> )

I think we can do it however we like, mostly. We are sort of stuck with how Moose (or Mouse or Moo or whatever) does types, and there is no non-scalar typechecking. But we could make it so that the type of a slurpy parameter was enforced across the elements of the container. But it would be a bit of work, and, as I say, difficult to describe. (OTOH, smart matching is horrific to describe, but that's still useful. So there are exceptions to my rule-of-thumb, I suppose.)

@bitbugprime
Copy link
Author

Type checking of an "@name" parameter only happens against the first element. The rest are ignored. HashRef[Type] and ArrayRef[Type] both already check every element against Type. For example,

my $x;
my $a = [qw<a b c>];
my $b = [qw<a b>, \$x];

$a passes an ArrayRef[Str] check, but $b doesn't.

Which gets me back to: shouldn't

ArrayRef[Str] $a, and
ArrayRef[Str] \@a

mean exactly the same thing (ignoring aliasing)?

@schwern
Copy link
Contributor

schwern commented Mar 12, 2013

Seems like there's two different problems here.

First is what to do about type checking arrays and hashes. It seems to
me this can be adapted so that "Str @array" means "ArrayRef[Str]" on
@array. It would seem backwards to make the user say "ArrayRef[Str]
@array".

Second is what to do about type checking aliased arrays and hashes.
Once "Str @array" is worked out its clearer to me what to do about this,
the same thing. "Str @array" means "ArrayRef[Str] @array".

Both would seem to involve some sort of adapting the type declaration to
what the user means and to work around the lack of array and hash types.

@thoughtstream
Copy link
Contributor

Schwern concluded:

It seems to me this can be adapted so that "Str @array" means
"ArrayRef[Str]" on @array. It would seem backwards to make the user
say "ArrayRef[Str] @array".

I strongly agree. This approach would be (as) consistent (as possible)
with Perl 6.

"Str @array" means "ArrayRef[Str] @array".

Again, I strongly agree. This too would be consistent (or at least
comprehensible) from a Perl 6 perspective.

FWIW, the Perl 6 principle here is that the type specifier on a
container variable (i.e. on an array or hash) specifies the type
of each individual value stored in the container (the variable's sigil
is already specifying the type of the container itself).

For M::S, you could document the proposed semantics like so:

* The '@' sigil is read:          "...is an Array assigned a list"
* The '\@' sigil is read:         "...is an Array aliased to an ArrayRef"
* An explicit typename X is read: "...of X"

Giving us:

              @foo  --->  "foo is an Array assigned a list
          Bar @foo  --->  "foo is an Array assigned a list of Bar"
ArrayRef[Bar] @foo  --->  "foo is an Array assigned a list of

ArrayRefs of Bar"

              \@foo  --->  "foo is an Array aliased to an ArrayRef
          Bar \@foo  --->  "foo is an Array aliased to an ArrayRef of Bar"
ArrayRef[Bar] \@foo  --->  "foo is an Array aliased to an ArrayRef

of ArrayRefs of Bar"

Likewise:

* The '%' sigil is read:   "...is a Hash assigned a list"
* The '\%' sigil is read:  "...is a Hash aliased to a HashRef"

* The '$' sigil is read:   "...is a Scalar assigned a value"
* The '\$' sigil is read:  "...is a Scalar aliased to an ScalarRef"

Damian

@barefootcoder
Copy link
Contributor

Type checking of an "@name" parameter only happens against the first element. The rest are ignored. HashRef[Type] and ArrayRef[Type] both already check every element against Type.

I wouldn't put it that way. I would say instead that when you say Str @name, then you are requesting that the next parameter in the list be a string. The next parameter in the list is not @name; it is in fact the first element of @name, which is exactly what is checked.

Contrariwise, when you say ArrayRef[Str] $name, then you are requesting that the next parameter in the list be an array ref of strings. The next parameter in the list is $name, which is exactly what is checked.

Which gets me back to: shouldn't

ArrayRef[Str] $a, and
ArrayRef[Str] \@a

mean exactly the same thing (ignoring aliasing)?

I think so, yes. Which is why I said I'd consider that a bug. But that's very different from what you were originally suggesting (and what @schwern and @thoughtstream suggest afterwards), which is that you should write instead Str \@a.

It seems to me this can be adapted so that "Str @array" means "ArrayRef[Str]" on @array.

I think so, yes. Assuming that it's at the end, which it would have to be, since it's a slurpy parameter. I don't think it's particularly easy, but it should be possible, yes.

It would seem backwards to make the user say "ArrayRef[Str] @array".

Oh, I definitely wouldn't suggest that. I was suggesting that you can't type check slurpy parameters at all. In fact, realistically, I'm having difficulty imaginging a non-contrived case where you'd actually want to.

Both would seem to involve some sort of adapting the type declaration to what the user means and to work around the lack of array and hash types.Both would seem to involve some sort of adapting the type declaration to what the user means and to work around the lack of array and hash types.

Welll ... I'm a bit hesitant conflating the two too much. @array means you pass your function an array. But \@array means you pass your function an arrayref. The user really needs to understand this (elsewise they can't call their own function properly). I worry that treating @array and \@array too similarly would confuse the issue.

Now, if we had a @array is alias syntax, then I'd be agreeing with you totally. But that's a whole different beast, which involves getting prototypes into the mix (similar to the email discussion you (@schwern) and I had recently), and maybe (probably) isn't too relevant here.

FWIW, the Perl 6 principle here is that the type specifier on a container variable (i.e. on an array or hash) specifies the type of each individual value stored in the container (the variable's sigil is already specifying the type of the container itself).

I'm also a bit leery of trying to conflate with Perl 6 too much here, because this is one of those specific instances where Perl 5 and Perl 6 really are different. That is, when you pass @name in Perl 6, you really are passing an array (or at least that's my understanding). When you pass @name in Perl 5, you're passing each element of the array separately. It's a lot closer to a signature of *@name in Perl 6 (at least that was the syntax last time I checked). Can you say things like Str *@name in Perl 6? And, if so, is the type checking done before the parameter list is built? 'Cause we can't do it that way, ya know. :-D

So, point being, if we're going to try to parallel P6, I think we should parallel what we're closest to, which in this case would be flattened slurpy params.

@bitbugprime
Copy link
Author

Which is why I said I'd consider that a bug. But that's very different from what you were originally suggesting.

I'm now thinking less is more. A comparable %hash syntax for aliasing hashrefs would be neat, though.

@barefootcoder
Copy link
Contributor

A comparable %hash syntax for aliasing hashrefs would be neat, though.

Oh, does \ not work on hashes? Yeah, it definitely should. Let me look at getting that added in.

@schwern
Copy link
Contributor

schwern commented Mar 13, 2013

On 3/12/13 4:29 PM, Buddy Burden wrote:

Type checking of an "@name <https://github.com/name>" parameter only
happens against the first element. The rest are ignored.
HashRef[Type] and ArrayRef[Type] both already check every element
against Type.

I wouldn't put it that way. I would say instead that when you say |Str
@name|, then you are requesting that the next parameter in the list be a
string. The next parameter in the list is /not/ |@name|; it is in fact
the first element of |@name|, which is exactly what is checked.

@barefootcoder I'm not sure if you're advocating this or not. Either
way, I think you've hit on something very important and I want to
amplify it and maybe discuss it more clearly.

While I agree that right now Perl is interpreting Str @name as "type
check $name[0]", I disagree its what the user means and that its how it
should work. Part of the point of Method::Signatures is to move away
from "arguments are just a list" paradigm towards a declarative
approach. Each variable in the argument list is an atom, they cannot be
split. @name is not a list of scalars, it is an atom. Type @name
is acting on @name because it is an atom.

To put it another way, what should @name is ro do? If Str @name
only checks $name[0] then it would make just as much sense to have
@name is ro only make $name[0] read only? That doesn't have much
utility. I'm pretty sure its not what the user meant.

Yet another way, if the user wants to treat the first element of a list
differently then it should not be part of the list. It should be its
own element. If you really want the type and traits to act on the first
element, you should write that. (Str $header is ro, @names)

In short, the next parameter on the list is @name. What you see
is what you get, not how its implemented.

While I agree this should be tempered with a healthy respect for how far
we can diverge from the implementation, I hope we can agree that it is
part of our design goal to be as declarative as possible. Treating
variables in the signature as atoms works towards that.

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

No branches or pull requests

4 participants