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

Attribute values should be set from constructor arguments prior to default values being applied #151

Open
Ovid opened this issue Nov 18, 2013 · 4 comments

Comments

@Ovid
Copy link
Contributor

Ovid commented Nov 18, 2013

I've wound up with a non-deterministic bug because I've set the value from an attribute in the constructor, but the default value of a different attribute relies on the first attribute. The following test demonstrates the error. Roughly half of these tests fail. If we're agree that this is an error, I'm happy to add a new test to the test suite for this.

use Test::More;
{
    use 5.18.0;
    use mop;
    class Attributes {
        has $!first is ro = do {
            ::ok($_->second, '$!second should be set') 
        };                                                                                                                                                                                                    
        has $!second is ro;
    }
}

for ( 1..100 ) {
    Attributes->new( second => 1 )->first;
}

done_testing;

And the summary:

Test Summary Report
-------------------
attribute_order.t (Wstat: 10752 Tests: 100 Failed: 42)
  Failed tests:  3-4, 6, 12-13, 16, 18-19, 22, 24-26, 28
                30, 32-36, 39, 47-48, 53, 59, 61, 63, 65
                67, 69-72, 76-77, 81-83, 92, 94, 96-97
                99
  Non-zero exit status: 42
Files=1, Tests=100,  0 wallclock secs ( 0.03 usr  0.01 sys +  0.04 cusr  0.00 csys =  0.08 CPU)
Result: FAIL

Cheers,
Ovid

@doy
Copy link
Collaborator

doy commented Nov 18, 2013

I agree that this is a problem, but I don't think that making constructor parameters different from defaults is the right answer. I think that if class Foo { has $!foo; has $!bar = $!foo }; Foo->new(foo => 1) works, then class Foo { has $!foo = 1; has $!bar = $!foo; }; Foo->new should also work. I kinda wonder if we do want to make attribute declarations respect ordering - we are making them resemble variable declarations as much as possible, and my $foo = 1; my $bar = $foo does work, so it seems like has $!foo = 1; has $!bar = $!foo should work for the same reason.

@doy
Copy link
Collaborator

doy commented Nov 18, 2013

(If all you care about is initializing via accessors, then adding is lazy to the attribute whose default is calling the accessor already works.)

@Ovid
Copy link
Contributor Author

Ovid commented Nov 18, 2013

I would suggest that the less dependencies we have on order, the more robust p5-mop is likely to be. We don't care what order subs/methods are declared, why should we for attributes? One of the lovely features of functional languages is that order generally does not matter (that said, I agree it's not a trivial problem).

@doy
Copy link
Collaborator

doy commented Nov 18, 2013

It is possible that we could provide a different magic implementation for attribute access in defaults vs attribute access in methods, but it's not entirely clear that that wouldn't cause other problems, especially when dealing with classes built through the mop rather than through an actual class declaration. It is something to consider, though.

Ovid added a commit to Ovid/test-class-mop that referenced this issue Nov 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants