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

Creating accessors for private attributes without adding them to the BUILDPLAN #145

Closed
lizmat opened this issue Jan 4, 2020 · 34 comments
Closed
Assignees
Labels

Comments

@lizmat
Copy link
Contributor

@lizmat lizmat commented Jan 4, 2020

This came up in https://stackoverflow.com/questions/59568073/using-automatically-generated-methods-for-public-attributes-vs-creating-methods but I have encountered the issue many times myself:

class A {
    has $!a;
    # other code that sets $!a
    method a() { $!a } 
}

So I was thinking of adding an Attribute trait is accessible:

class A {
    has $!a is accessible;
    # other code that sets $!a
}

And the implementation was, I thought, pretty straightforward:

multi sub trait_mod:<is>(Attribute:D $a, :$accessible!) {
    nqp::bindattr_i($a,Attribute,'$!has_accessor',1)
}

However, that also adds the attribute to the BUILDPLAN.

If we agree on that this is a useful feature to have, I would either propose adding an has int $!build-allowed attribute to Attribute, indicating whether an attribute should be part of the BUILDPLAN or not. Or use different values for the has_accessor attribute, to indicate an attribute that is private, but which can accessed via an accessor.

Or whatever other way we can come up with this functionality.

@lizmat lizmat added the language label Jan 4, 2020
@lizmat lizmat changed the title Creating accessor for private attributes without adding them to the BUILDPLAN Creating accessors for private attributes without adding them to the BUILDPLAN Jan 4, 2020
@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 4, 2020

Implementation provided by rakudo/rakudo#3404 . I've used the name "is_settable" rather than "build-allowed" for the new Attribute attribute.

@vrurg

This comment has been minimized.

Copy link
Contributor

@vrurg vrurg commented Jan 4, 2020

Do I get it right that the purpose is to have accessors but don't allow setting from constructor parameters?

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 4, 2020

Yes, it's basically private attributes with accessors. See rakudo/rakudo#3404 for an implementation.

@vrurg

This comment has been minimized.

Copy link
Contributor

@vrurg vrurg commented Jan 4, 2020

It doesn't feel right to me as it dissolves edges between private and public attributes. To my view something like has $.a is unsettable would be much cleaner solution.

I would even consider adding not or no trait_mod as has $.a not settable would be ideal.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 4, 2020

Adding additional trait_mod's has been denied in the past on account of grammar slowdown, if I remember correctly. But is unsettable could be easily be added, or renamed from is accesible.

Anyways, I don't think this dissolves the edge between private and public attributes: it separates functionality.

@vrurg

This comment has been minimized.

Copy link
Contributor

@vrurg vrurg commented Jan 4, 2020

My point here is that ! twigil is the indication of a private attribute. That's it, it's not visible for the outer world. is accessible changes this situation half-implicitly.

BTW, is unsettable could as well be applied to rw attributes two. This would even extend possible applications of the functionality.

@jnthn

This comment has been minimized.

Copy link
Contributor

@jnthn jnthn commented Jan 4, 2020

We should probably consider all the things that . does.

The use of . instead of ! has no less than four consequences:

  1. The generation of an accessor method
  2. Automatic mapping of a named parameter to bless to the attribute of the same name
  3. The inclusion of the attribute in the output produced by .perl
  4. The inclusion of the attribute in the default .Capture, which is significant in so far as it controls how the object destructures

It's possible today to use ! and control all of these individually by (same order as above):

  1. Writing an accessor method (trivial, but boring)
  2. Writing a BUILD or TWEAK (not so bad, given :$!foo parameter syntax exists)
  3. Overriding .perl (potentially quite annoying if many attributes)
  4. Overriding .Capture (not so bad, again especially thanks to the :$!foo forms of colonpairs reducing named repetition)

So the problem really isn't missing capabilities, but rather whether there's some combination(s) of these that are common enough to deserve a shortcut. Most probably 2 and 3 go together.

If anything, it feels to me like a trait controlling the initialization semantics of attributes is perhaps the most useful thing to provide, so in that sense I'm kind of agreeing with @vrurg - however, settable is a quite unfortunate name (given "setter" is common OO terminology, and so it'd be quite easily confused with the semantics of is rw).

Potentially an is init(...) trait, whose arguments determine what to do, could work out.

has $!foo is init;           # Private, but you can initialize it, and have it in .perl
has $!foo is init(True);     # Same as the above
has $!foo is init(False);    # Same as the default semantics
has $.foo is init;           # Same as the default semantics
has $.foo is init(True);     # Same as the default semantics
has $.foo is init(False);    # Have a getter, but disable initialization, and it won't appear in .perl

This also gives us a peg to hang other declarative initialization things off in the future. For example, I've sometimes - in performance-sensitive code - wanted to have my array and hash attributes bound rather than assigned. We could potentially offer:

has @.foo is init(:bind);

To make that declaratively possible.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 4, 2020

ok, will adapt the PR accordingly

lizmat added a commit to rakudo/rakudo that referenced this issue Jan 4, 2020
As discussed in Raku/problem-solving#145
@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 4, 2020

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 4, 2020

Relatedly, I wonder whether it would be an idea to allow is !init as syntax for specifying a trait and passing the value False, rather than True ? Then the syntax for 3 and 6 would be:

has $.foo is !init;
@vrurg

This comment has been minimized.

Copy link
Contributor

@vrurg vrurg commented Jan 4, 2020

What about future possibility to have also something like:

has $.foo is init(:as<bar baz>);

for aliasing the attribute: Foo.new(bar => 42)? Though if sounds as a bad idea, my AttrX::Mooish implements aliasing anyway. :)

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 4, 2020

@jnthn For an implementation of :bind, would it be enough to skip the codegen of QAST::Var to something else?

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 4, 2020

on second thought, shouldn't the :bind functionality be better implemented as a is bound trait?

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 4, 2020

or maybe better, showing what it is: is immutable ?

@vrurg

This comment has been minimized.

Copy link
Contributor

@vrurg vrurg commented Jan 5, 2020

@lizmat Binding doesn't automatically means immutability.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 5, 2020

True. But it would mean that from the outside of the class. If you, as a module developer, decide to break that contract, well, then you're breaking that contract :-)

@vrurg

This comment has been minimized.

Copy link
Contributor

@vrurg vrurg commented Jan 5, 2020

What I'm not sure about is what'd be the semantics of the following:

class Foo {
    has $.foo is init(:bind);
}
my $v = 42;
my $inst = Foo.new: :foo($v);

Since attribute initialization is currently an assignment, it doesn't really matter how $v is used. But with binding we have to decide wether $v is deconted or not. In the first case treating it as immutable is ok for scalars. In the second – it is not.

But in either case Array and Hash break the immutable term as a decent user would expect them to become Listy/Mapy things.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 5, 2020

But in either case Array and Hash break the immutable term as a decent user would expect them to become Listy/Mapy things.

Why would that? They're already codegenned differently, so why not codegen them as decontainerized Lists and Maps?

@vrurg

This comment has been minimized.

Copy link
Contributor

@vrurg vrurg commented Jan 5, 2020

Reasonable... But does additional time to do the conversion worth it? For big ones it could be quite considerable amount. So, instead of quick binding one gets rather slow assignment with coercion.

Perhaps it makes sense to have both bind and immutable? It's a quick thought, I haven't considered it well enough yet.

@raiph

This comment has been minimized.

Copy link

@raiph raiph commented Jan 14, 2020

Isn't this will stuff rather than is?

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 14, 2020

hmmm... good point:

 has $.a will bind;

I like it. :-)

@jnthn

This comment has been minimized.

Copy link
Contributor

@jnthn jnthn commented Jan 14, 2020

Replies to various comments:

Isn't this will stuff rather than is?

No, because the grammar of will is to take an identifier followed by a block, and we don't want the block. That said, I agree is init doesn't sound so nice. I'm not sure why it took me until today to think of it, but is built not only sounds better, but also is a very good clue as to what phase that initialization takes place in, and also that a custom BUILD overrides such a declaration.

on second thought, shouldn't the :bind functionality be better implemented as a is bound trait?

I don't think so, because then there's a question of when it is bound and so forth, whereas is built(:bind) makes it clear it's bound at build time.

or maybe better, showing what it is: is immutable ?

Actually binding often means more action at a distance, not less, so we certainly shouldn't be using words that suggest you're safer using it. The main reasons I've wanted such a think were performance related.

aliasing the attribute

Yes, it had occurred to me that is built(:args<q quiet>) or similar could be a way to offer ways to look at a different - or more than one - incoming named argument.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 14, 2020

Coming back to the original proposal: so that would become is built(:with-accessor) ?

@jnthn

This comment has been minimized.

Copy link
Contributor

@jnthn jnthn commented Jan 14, 2020

@lizmat No, accessor generation is controlled by . or not. So:

has $!foo is built;           # Private, but you can initialize it, and have it in .perl
has $!foo is built(True);     # Same as the above
has $!foo is built(False);    # Same as the default semantics
has $.foo is built;           # Same as the default semantics
has $.foo is built(True);     # Same as the default semantics
has $.foo is built(False);    # Have a getter, but disable initialization, and it won't appear in .perl
@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 14, 2020

Ok, will adapt PR accordingly

@Kaiepi

This comment has been minimized.

Copy link
Collaborator

@Kaiepi Kaiepi commented Jan 14, 2020

I'm not sure how I feel about the trait being called is built; "build" is already used in the names of methods of Attribute related to default values, which could be confusing. is scant is the best name for it I can think of at the moment, but describing it like that doesn't feel right either...

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 14, 2020

?? build is referred to if an attribute needs to be part of the buildplan, which is what this is about, no?

@Kaiepi

This comment has been minimized.

Copy link
Collaborator

@Kaiepi Kaiepi commented Jan 14, 2020

...right 🤦. Would Attribute's build and set_build methods being unrelated to the is built trait pose enough of a problem for it to be worthwhile to rename them?

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 14, 2020

$ 6 'dd Attribute.^methods.map(*.name).grep(* ne "<anon>").sort'
("Str", "WHY", "apply_handles", "compose", "container", "dimensions", "get_value", "gist", "inlined", "package", "readonly", "set_value", "set_why").Seq

I'm not sure what methods you are referring to?

@Kaiepi

This comment has been minimized.

Copy link
Collaborator

@Kaiepi Kaiepi commented Jan 14, 2020

They're named <anon>, which is why they weren't showing up:

class Foo {
    has Int:D $.foo = 0;
    BEGIN {
        my Attribute:D $foo = $?CLASS.^get_attribute_for_usage: '$!foo';
        say $foo.build; # OUTPUT: 0
        $foo.set_build: 42;
    }
}

say Foo.new.foo; # OUTPUT: 42

I was forgetting these were methods that are mainly intended for internal use though. Their name probably isn't as big a deal as I was thinking earlier.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 14, 2020

I see. Then maybe we need to get back to the is settable idea?

@jnthn

This comment has been minimized.

Copy link
Contributor

@jnthn jnthn commented Jan 14, 2020

I don't see the existence of will build { } being an impediment to having an is built.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 14, 2020

Ok, I've implemented this now in rakudo/rakudo@e76f8e4 .

There is however one problem caught by spectests: True.HOW.say fails with X::Method::NotFound without specifying a method name. Further research reveals it is actually dieing when trying to create an error message. Looking further, it looks like it is trying to call the is_settable method on an NQPAttribute, which doesn't have that method.

Suggestions welcome! @Kaiepi @jnthn

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Jan 14, 2020

The PR got merged, thanks for your attention :-)

@lizmat lizmat closed this Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.