stevan / p5-mop-redux

A(nother) MOP for Perl 5
139 stars 36 forks source link

attributes: arrays and hashes #122

Open maettu opened 10 years ago

maettu commented 10 years ago

Please do some trickery that attributes can be hashes and arrays.

Using scalars to act as references to arrays and hashes is .. awkward(?) :-)

stevan commented 10 years ago

So in an email discussion with @maettu I proposed the following possibility:

class Foo {
    has @!bar = ( 1, 2, 3 );
}

would be transformed to:

class Foo {
    has @!bar = [ 1, 2, 3 ];
}

so that syntactically this "looks" right, but behaves properly behind the scenes.

doy commented 10 years ago

I'm kinda wondering if our reasons for not having array and hash attributes are no longer valid now that we don't have separate 'undef' and 'unset' states. We may not need to do any transformations at all. Need to think about it some more though.

stevan commented 10 years ago

Yes, I agree, this discussion needs to happen again in the new version.

The only reason I wanted to transform is because () is an array where [] is an array ref, and ultimately we have to store the ref on the instance.

On Oct 17, 2013, at 5:39 PM, Jesse Luehrs notifications@github.com wrote:

I'm kinda wondering if our reasons for not having array and hash attributes are no longer valid now that we don't have separate 'undef' and 'unset' states. We may not need to do any transformations at all. Need to think about it some more though.

— Reply to this email directly or view it on GitHub.

forwardever commented 10 years ago

wondering if/how p5mop works with autobox::Core

stevan commented 10 years ago

@forwardever it should work fine, but you probably need to just try it and see.

forwardever commented 10 years ago

@stevan: hope twigils work on windows soon, so I can try

regarding autobox::core -> was wondering whether the automatic transformation you were thinking about would be done by autobox::Core (transformation to objects, not array ref), there might also be conflicts with autobox if you would add some additional transformation magic

(just some thoughts, hope it makes sense)

stevan commented 10 years ago

@forwardever no, autobox::Core would not play any role in this, the array and hash variables would work just like normal Perl array and hash variables.

As for twigils working on windows, @doy and @rafl are currently working on an xs branch that integrates twigils and improves the parser, etc. This will ultimately be 0.02 and should work fine on windows.

doy commented 10 years ago

So I think the main issue is, how should has @foo is rw work? It can't differentiate between calling it as a reader and calling it with an empty array. Passing arrayrefs/hashrefs seems ugly and inconsistent. We could just make is rw an error on array and hash attributes, perhaps? Typically for array and hash accessors, setting the entire array or hash isn't a common operation anyway.

maettu commented 10 years ago

Or just default to the reader function whereas calling it with an empty array to clear was not possible? So, if you wanted to empty @foo or %foo you would have to write your own accessor?

You do not want to have auto generated "get_foo" and "set_foo" by design, I guess.

doy commented 10 years ago

@maettu I'm not sure what you mean by "default to the reader function", but the point is that has $!foo is rw lets you write $obj->foo for a reader and $obj->foo($new_foo) for a writer. Therefore, has @!foo is rw should let you write $obj->foo for a reader (returning a list) and $obj->foo(@new_foo) for a writer. Unfortunately, there is no way in perl to tell the difference between the case where you call foo as a reader and the case where you call foo as a writer, but @new_foo is empty.

forwardever commented 10 years ago

according to stevan, this is not an option #126 (lvalue accessor), but it would solve the problem?

$obj->foo = (1, 2, 3);
$obj->foo; # (1, 2, 3)
$obj->foo = ();
doy commented 10 years ago

No, because list context lvalue subs in perl need to be written as ($obj->foo) = (1, 2, 3), since there's no other way to indicate context (yet another reason why lvalue accessors are a bad idea).

stevan commented 10 years ago

I am going to suggest something that y'all probably won't like, but I wonder if it is not sensible.

We should return and accept only array refs.

The logic behind this is that there is often many times when it makes sense to return the actual array ref, such as:

my $foo = $self->foo;
push @$foo => 'bar';

where you want people to alter the array ref (or hash ref as the case may be). I think that this pattern is FAR more common then wanting to return an array (essentially a copy of all the values you had in your array internally).

The fact that you access it like a regular array within the methods is fine since the twigils are clearly special values and so special behavior is almost expected. The twigils have no meaning outside of the method, so having things behave differently on the other side of that is potentially okay.

Also, while this might seem ugly at first, it is insanely easy to understand.

Just a thought.

doy commented 10 years ago

The problem here is that it becomes impossible to control your object's data (see also: native delegations in Moose). If the default behavior is to allow direct access into the object's internals, then that's what people will tend to do by default, and then you can't do things like enforce type constraints on the contents of the array attribute. Returning the actual arrayref like this breaks encapsulation pretty badly.

doy commented 10 years ago

Also, as a data point, I haven't ever (in the last two or three years at least) written an accessor method which returned the actual arrayref or hashref stored in an attribute. I always write has foos => (traits => ['Array'], isa => 'ArrayRef[Foo]', handles => { elements => 'foos' }), and then if I want to allow people to alter the arrayref, I'll add things like add_foo => 'push' to the handles list. I don't think that the "returning an arrayref" pattern is more common.

forwardever commented 10 years ago

as some perl functions also work on array refs http://search.cpan.org/~jesse/perl-5.14.0/pod/perldelta.pod#Array_and_hash_container_functions_accept_references (is this still up to date ???)

e.g.

my $array = [1, 2, 3];
push $array, 4;
print "@$array";

it might make sense to just store array refs and also return array refs within methods and outside of methods

stevan commented 10 years ago

@doy - point taken, although I think it is more common then you might think, especially for those who don't use (or have access to (ENOMOOSE)) native traits.

For the record, I am not so much endorsing this as I am saying that this is just one way this might work for the default rw trait, it certainly doesn't box you in such that you can't do it your way either. I mean, if we want to get fugly about it we could check wantarray and return ref or no-ref based on the context.

Anyway, I was just throwing it out there.

stevan commented 10 years ago

FWIW, this is an issue with the rw trait alone, it is not something that should prevent the implementation of @!foo, at least IMO anyway.

I would assume just ban the rw trait for @!foo and %!foo attributes and let people write their own.

doy commented 10 years ago

We are absolutely not reintroducing auto_deref(:

I know that that pattern may be common at the moment, but I think that is in large part because that's what the current OO design makes the easiest. I'm just saying that since we're designing this API from scratch, the easiest thing (i.e. huffman coding) should be the thing that is more correct - doesn't break encapsulation, etc.

And I'd be fine with just banning is rw.

stevan commented 10 years ago

@doy - hahaha :)

stevan commented 10 years ago

@forwardever - Honestly I never liked the "push works on refs" thing, it always made me feel dirty. And this won't work with twigils:

push @$!foo => $bar;

it has to be:

push @{$!foo} => $bar;

which gets noisy.

stevan commented 10 years ago

@doy - So I think the solution is to just not support rw for those attributes, we will need to explain it and error on it early as possible (compile time), but that should be reasonable and understandable. The benefits of being able to do push @!foo => $bar and such inside methods far outweigh the lack of rw support.

forwardever commented 10 years ago

@stevan just wanted to provide some thoughts, not sure if I really like it

rather thought of

push @!foo => $bar;