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

A method in a role masks a public attribute in a class doing that role #3382

Closed
lizmat opened this issue Dec 25, 2019 · 9 comments · Fixed by #3397
Closed

A method in a role masks a public attribute in a class doing that role #3382

lizmat opened this issue Dec 25, 2019 · 9 comments · Fixed by #3397

Comments

@lizmat
Copy link
Contributor

@lizmat lizmat commented Dec 25, 2019

role A {
    method a() { 666 }
}
class B does A {
    has $.a = 42;
}
dd B.new.a     # 666 rather than 42

I think this is due to the accessor creation logic looking at whether the class is already providing a method, but disregarding the fact that that method is provided by the role.

Or is this a case of DIHWIDT?

@vrurg

This comment has been minimized.

Copy link
Member

@vrurg vrurg commented Dec 26, 2019

It looks like it's by design. Incorporated methods are the same first-class citizens and class' own. A role is being applied as early as possible during class composition (is it actually documented somewhere?) whereas accessors are children of a BUILD constructor which is invoked much later.

I think a warning could be issued here if BUILD finds out that a pre-existing method is actually originating from a role. But even in this case I wouldn't report this without an additional pragma like use warnings :extra;.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

@lizmat lizmat commented Dec 26, 2019

So we're leaning towards DIHWIDT?

Thing is that the role could be imported from an external module, so one could be using that and not notice that the accessor is never made.

In any case, this is a compilation error:

role A {
    method a() { 666 }
}
class B does A {
    has $.a = 42;
}
# Attribute '$!a' already exists in the class 'B', but a role also wishes to compose it

So maybe we shouldn't allow a public attribute like that in a class either when there is a method in the role that would mask the attribute's accessor ?

@vrurg

This comment has been minimized.

Copy link
Member

@vrurg vrurg commented Dec 27, 2019

Is it a real compilation error or imaginary one? For me the code always compiles.

Anyway, I would rather agree to the statement that the situation has to be reported somehow. Either as a warning, or error, but we must not leave it unnoticed. Looks like a @jnthn word on this is needed.

BTW, can't quickly locate how exactly accessors are generated. Must be a part of BUILDPLAN but brief overlook didn't come up with precise location.

@jonathanstowe

This comment has been minimized.

Copy link
Contributor

@jonathanstowe jonathanstowe commented Dec 27, 2019

It's in the compose method of Attribute

@vrurg

This comment has been minimized.

Copy link
Member

@vrurg vrurg commented Dec 31, 2019

@lizmat You could try the initial solution with #3390. It currently has some limits:

  • A warning is only issued at compile time, when $*LANG is available. There're two reasons for this: I don't know how to issue a standard warning from MOP code at run time; I don't know if there is a way to have access to pragmas at run-time to die if fatal is set.

  • I only warn if attribute originates from the current object (i.e. most likely – a class). Otherwise core fails compiling DateTime because Datish defines $.daycount and has method daycount. fatal is on for the CORE, so $*LANG.worry just dies. It could be solved with replacing $.daycount with $!daycount which is anyway the right way to have it, perhaps.

@jnthn

This comment has been minimized.

Copy link
Member

@jnthn jnthn commented Dec 31, 2019

I think this ultimately boils down to attribute composition taking place after role composition (during the overall composition process of a class; I don't think BUILD comes into this at all, since that's a "runtime" thing). Thus, the method that would be generated for the accessor is not present at the time the role composer considers whether to install the one from the role.

This ordering is deeply tied into the attribute composition process itself. Role attributes are not actually composed at the point the role's closing } is hit. Rather, they undergo generic instantiation and are then composed (including accessor generation) once they are in the class itself. So this can't really be flipped around. (Were it to be, we could not rely an attribute being non-generic by the time it has compose called on it.)

So that's why things are the way they are, and kind of need to be, but it's still somewhat displeasing that the "declarations in the class win" design goal is being obviously violated here. It's also inconsistent with this:

role R {
    method attr() { ... }
}
class C does R {
    has $.attr;
}

Where the attribute accessor that will be generated in the future clearly is considered by role composer! Well, sort of. What actually happens is the requirements are checked after the attribute composition. Which also, annoyingly, means that we probably can make a case like that @lizmat started this issue with using handles too, since those methods are also generated in attribute composition.

I guess one way out is to be able to ask an Attribute what methods it would produce during composition, and have the role composer consider those names as existing when it does its has_method check.

I think that in turn means we could move the role requirements check code from ClassHOW, where it currently lives (and thus feels hacky), into the role to class applier, where it feels more natural. So arguably we end up with things in their righter places, as the cost of an extra method on Attribute to ask it what it plans to do when composed, which isn't really delightful either, but perhaps the lesser evil, and also somewhat extensible for those extending what takes place at attribute composition time.

@vrurg

This comment has been minimized.

Copy link
Member

@vrurg vrurg commented Dec 31, 2019

Not sure I will have much time to implement it the right way, but will see what'd come out.

Initially, I have a feeling than rather than adding a method to Attribute it'd be better to split RoleToClassApplier.apply into few methods, implementing different stages. Perhaps it would require instantiating the class to do the job.

Besides, I'm thinking of providing RoleToClassApplier class via configuration to let 3rd party modules to override it and implement extra functionality.

@vrurg

This comment has been minimized.

Copy link
Member

@vrurg vrurg commented Jan 1, 2020

A bit of experimenting reveals that the fix could actually be simpler than it seemed. For now what I got is:

  • made Attribute::compose check wether attribute is already composed and skip composition if it is
  • compose class attributes first
  • apply roles to the class
  • do the rest including second pass of attribute composition which would compose attributes from roles

This actually results in correct output of @lizmat code. Where it fails:

  • when there is a multi in a class named after an attribute. Should not be hard to get it fixed, Attribute::compose must also check for multi method candidates
  • when enum used as a mixin

Otherwise the experiment looks promising.

vrurg added a commit to vrurg/roast that referenced this issue Jan 2, 2020
Also make typeobjects from a previous test lexically-scoped.

In support of rakudo/rakudo#3382 and rakudo/rakudo#3397
vrurg added a commit to vrurg/roast that referenced this issue Jan 2, 2020
Also make typeobjects from a previous test lexically-scoped.

In support of rakudo/rakudo#3382 and rakudo/rakudo#3397
@vrurg

This comment has been minimized.

Copy link
Member

@vrurg vrurg commented Jan 2, 2020

I think, #3397 got things sorted out.

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