Ticket #1473 (closed bug: fixed)

Opened 4 years ago

Last modified 4 years ago

Handle PMC type is broken

Reported by: Austin_Hastings Owned by:
Priority: normal Milestone:
Component: core Version: 2.1.0
Severity: medium Keywords:
Cc: Language:
Patch status: Platform:

Description

Despite being an abstract class, there are reasons to talk to the Handle PMC type. For example, to ask for a list of (inherited) methods or attributes, or to display it when using data::dumper on a (sub) class that includes Handle as a parent. Sadly, queries via the 'does' opcode are fatal:

austin@andLinux:~/kakapo$ cat test.pir
.sub foo
	$P0 = get_class 'Handle'
	$I0 = does $P0, 'hash'
.end
austin@andLinux:~/kakapo$ parrot test.pir
Handle cannot be instantiated directly.
current instr.: 'foo' pc 3 (test.pir:4)

Note that the "does $P0, 'hash'" query is straight from Data::Dumper::Base - you can't dump the FileHandle or StringHandle class PMCs because of this.

Attachments

tt1473.patch Download (5.6 KB) - added by whiteknight 4 years ago.
proposed fix

Change History

follow-up: ↓ 2   Changed 4 years ago by whiteknight

Notice that this problem should manifest with any built-in c-based type that has guards in VTABLE_init to prevent direct instantiation. For instance, doing the same operation on "Object" should produce the same result (I'll test that to be sure in a minute).

The problem is this function from PMCProxy:

VTABLE INTVAL does(STRING *role_name) {
    Parrot_Class_attributes *_class  = PARROT_CLASS(SELF);
    INTVAL                   id      = _class->id;
    PMC                     *proxied = Parrot_pmc_new(interp, id);
    if (VTABLE_does(interp, proxied, role_name))
        return 1;
    return VTABLE_isa(interp, proxied, role_name);
}

You'll notice that it attempts to instantiate a new pmc of that type to call VTABLE_does on directly. Some options:

  • Change Parrot_pmc_new to Parrot_pmc_new_init, though this might create new problems for types that require attribute initialization to determine it's "does" result
  • Add an exception handler in Data::Dumper around the does call, though this really seems like a hackaround of a solution that would need to be duplicated any place that wants to try this same operation.
  • Improve handling of abstract types. PMCProxy could detect when the type is marked as "abstract" and do something special. What "special" here means is open to debate. Maybe we could try instantiating a pre-existing subclass, attempting to fabricate a new temporary subclass to instantiate, read the provides string directly (ignoring any VTABLE), etc.
  • Create and enforce some kind of guarantee that built-in abstract types cannot have a VTABLE_does that depends on attribute initialization. This seems the least possible alternative.
  • Declare that C-based PMCs cannot ever override VTABLE_does, they must specify all their roles as constants in the pmclass declaration. This sounds limiting at first but actually makes a certain amount of good sense from several perspectives.

We definitely need to improve Parrot's handling of abstract types. I don't think that PMCProxy should be instantiating and initializing PMCs for this kind of operation, but I can't think of a good way to avoid that when the result of an overridden VTABLE_does may depend on that types attributes being initialized. The last option, forbidding C-based types from overloading VTABLE seems the best in this regard, though we would need to hard-code some exceptions to the rule (PMCProxy itself, Class, Object, default, Null). Currently the only "non-special" type which implements VTABLE_does is Socket, and I'm working to change that right now.

in reply to: ↑ 1   Changed 4 years ago by Austin_Hastings

Is there some reason that we shouldn't just rewrite the does code to look more like the isa code - check the local roles, then iterate the parents, asking each one?

Changed 4 years ago by whiteknight

proposed fix

  Changed 4 years ago by whiteknight

I've attached a patch that I think resolves this ticket. Includes fix and test case to prove it works.

I can't commit it right now because I'm seeing unrelated test failures in trunk. Once those failures get fixed and I can prove this patch still does what I want it to do, I'll commit it.

  Changed 4 years ago by whiteknight

  • status changed from new to closed
  • resolution set to fixed

Fixed in r45261.

Note: See TracTickets for help on using tickets.