kentnl / Devel-Isa-Explainer

Pretty Print Function Hierarchies of Classes
Other
1 stars 0 forks source link

Truly convoluted inheritance confuses splainer #10

Closed ribasushi closed 8 years ago

ribasushi commented 8 years ago

I ran into this while working on a related feature, and tested D::I::E - it gets tricked in the same manner. DBICTest::Schema::Track does not define a delete(), it inherits it from elsewhere. The confusion happens because in the middle of the ISA there is DBIx::Class::Relationship::CascadeActions which does not inherit from anything while providing a method for the consuming chain.

The correct output of my (upcoming) tooling is:

perl -Ilib -It/lib -MDBICTest -MDevel::Dwarn -e 'Dwarn [ DBIx::Class::_Util::describe_class_methods("DBICTest::Schema::Track")->{methods}{delete} ]'
[
  {
    atributes => {},
    name => "delete",
    shadowing => [
      {
        atributes => {},
        name => "delete",
        via_class => "DBIx::Class::Relationship::CascadeActions"
      },
      {
        atributes => {},
        name => "delete",
        via_class => "DBIx::Class::Row"
      }
    ],
    via_class => "DBIx::Class::Ordered"
  }
]

Compare to:

rabbit@Ahasver:~/devel/dbic$ git rev-parse HEAD
5ab7259324b6e3d0feea533239b6d77db0b28c9c
rabbit@Ahasver:~/devel/dbic$ ANSI_COLORS_DISABLED=1 perl -Ilib -It/lib -MANFANG -S isa-splain DBICTest::Schema::Track | grep -P '^\w|delete'
Key:
DBICTest::Schema::Track:
    cd_single, cd_title, check_customcond_args, delete(^), deliberately_broken_all_cd_tracks, 
DBICTest::DeployComponent:
DBIx::Class::InflateColumn::DateTime:
    delete(^), insert(^), register_column(^), update(^)
DBIx::Class::Ordered:
    delete(^), first_sibling, grouping_column, insert(^), last_sibling, move_first, move_last, 
DBICTest::BaseResult:
DBICTest::Base:
DBIx::Class::Core: ()
DBIx::Class::Relationship: ()
DBIx::Class::Relationship::Helpers: ()
DBIx::Class::Relationship::HasMany:
DBIx::Class::Relationship::HasOne:
DBIx::Class::Relationship::BelongsTo:
DBIx::Class::Relationship::ManyToMany:
DBIx::Class::Relationship::Accessor:
DBIx::Class::Relationship::CascadeActions:
    delete(^), update(^)
DBIx::Class::Relationship::ProxyMethods:
DBIx::Class::Relationship::Base:
    count_related, create_related, delete_related, find_or_create_related, find_or_new_related, 
DBIx::Class::InflateColumn:
DBIx::Class::PK::Auto: ()
DBIx::Class::PK:
DBIx::Class::Row:
    copy, delete, discard_changes, get_column, get_columns, get_dirty_columns, get_from_storage, 
DBIx::Class::ResultSourceProxy::Table:
DBIx::Class::ResultSourceProxy:
DBIx::Class:
DBIx::Class::Componentised:
Class::C3::Componentised:
DBIx::Class::AccessorGroup:
DBIx::Class::MethodAttributes:
Class::Accessor::Grouped:
kentfredric commented 8 years ago

Isn't this the kind of "Ugh, MRO being mystical and spicy" the sort of thing I was worried about earlier?

This is pretty much a direct result of the "use a linearized bottom-up refaddr check" logic you recommended.

perl -Ilib -It/lib -MDBICTest::Schema::Track -MScalar::Util=refaddr -E' say refaddr \&DBICTest::Schema::Track::delete; say refaddr \&DBIx::Class::InflateColumn::DateTime'
29246656
29247232

I guess what you want here is not a "refaddr matches previous" but "refaddr previously seen" thing ... but... that's going to lead to some false assumptions.

x -> isa y -> isa z
y::foo = sub {};
x::foo = z::foo = sub {};

Here reporting that x shadows y is "right".

However, if y is in isa like that due to tree flattening, then x does not shadow y.

I don't know how to resolve this ambiguity.

ribasushi commented 8 years ago

"refaddr matches previous" but "refaddr previously seen" thing

This is exactly it, yes: https://github.com/dbsrgits/dbix-class/blob/296248c3/lib/DBIx/Class/_Util.pm#L731-L735

However, if y is in isa like that due to tree flattening, then x does not shadow y.

Explain?

kentfredric commented 8 years ago

However, if y is in isa like that due to tree flattening, then x does not shadow y.

In your example:

I want to know what exactly is happening in the inheritance graph to make Perl have this happen.

perl -Ilib -It/lib -MANFANG -MDBICTest::Schema::Track -MScalar::Util=refaddr -E' printf "$_ => %s\n", $_->can(q[delete]) for qw( DBICTest::Schema::Track DBIx::Class::InflateColumn::DateTime DBIx::Class::Relationship::CascadeActions DBIx::Class::Ordered DBIx::Class::Row)'
DBICTest::Schema::Track => CODE(0x13ca5f0)
DBIx::Class::InflateColumn::DateTime => CODE(0x101fbe0)
DBIx::Class::Relationship::CascadeActions => CODE(0xec11c8)
DBIx::Class::Ordered => CODE(0x13ca5f0)
DBIx::Class::Row => CODE(0x101fbe0)

Because ::Track is comming from ::Ordered, but the fact all the inbetween entries are completely different seem to be ignored.

Obviously, this means "linearised isa is broken and meaningless".

ribasushi commented 8 years ago

On 06/07/2016 09:39 PM, Kent Fredric wrote:

Obviously, this means "linearised isa is broken and meaningless".

No, everything is correct as far as perl's behavior is concerned. I will write a more detailed explanation when my internet comes back.

kentfredric commented 8 years ago

Reduced subgraph of affected leaves in this interaction.

dbic_bitch2

kentfredric commented 8 years ago

Now the same graph in MRO linear order is ...

WTF?

dbic_bitch3

kentfredric commented 8 years ago

PS: I can see why you hate life now :P

dbic_bitch

ribasushi commented 8 years ago

Won't be able to get to you on this today, something else came up. Sorry for the delay.

kentfredric commented 8 years ago

The simplest way to explain my error that anyone else reading along can understand:

     DBICTest::Schema::Track  -> can("delete") --> \&DBIx::Class::Ordered::delete
      | 
     DBIx::Class::InflateColumn::DateTime -> can("delete") --> \&DBIx::Class::Row::delete
      | 
     DBIx::Class::Ordered -> can("delete")  -> \&DBIx::Class::Ordered::delete

The real issue is using can on ::DateTime brings the sub from DBIx::Class::Row forward to that point.

However, Perl itself does not work this way tracing methods from DBICTest::Schema::Track.

Instead, perl sees only this.

     DBICTest::Schema::Track::delete = undef
      | 
     DBIx::Class::InflateColumn::DateTime::delete = undef
      | 
     DBIx::Class::Ordered::delete = sub {  };

and resolves down that way, so that

DBICTest::Schema::Track directly resolves to DBIx::Class::Ordered.

This means ::DateTime does not shadow DBIx::Class::Ordered, and DBICTest::Schema::Track does not shadow ::DateTime

Essentially this means using can to determine whether or not a parent class provides a sub that shadows a lower sub is right out.

Because ->can() on ::DateTime would result in:

     DBIx::Class::InflateColumn::DateTime::delete = undef
      | 
     DBIx::Class::InflateColumn::Row::delete = undef
      | 
     DBIx::Class::Row::delete = sub { };

Making DBIx::Class::Row::delete return instead of undef

How we respect user defined "can" here is problematic.

I think the most "right" logic would be:

As for how method resolution works if a parent class defines can .... anybodies guess really.