Get_class <namespace> confuses NSes that have same final name.

[Austin_Hastings]

Having already created a class "Matcher", I was trying in NQP to create a related class Mimus::CallSignature::Matcher. (Note the last name.) Far too many hours later, it appears that the get_class op on a namespace apparently returns invalid results when the namespace shares a last name with an already-extant class.

This code, inspired by the get_parrotclass method in P6object.pir, looks up (and tries to create, if needed) two classes with different namespace paths but the same class name (last name in the nsp), one called P and the other N::S::P :

.sub foo :main
	.local pmc class_p
	.local int addr_p

	.local pmc class_nsp
	.local int addr_nsp

	class_p = newclass [ 'P' ]
	addr_p = get_addr class_p
	

	$S0 = 'N::S::P'		# Change to ..Q and it changes
	$P0 = split '::', $S0

	$P1 = get_hll_namespace $P0
	unless null $P1 goto have_ns
	
	die "Namespace was null"
have_ns:
	
	class_nsp = get_class $P1
	$P2 = class_nsp
	addr_nsp = get_addr class_nsp

	print "Addr of class 'P' is: "
	say addr_p

	print "Addr of 'N::S::P' is: "
	say addr_nsp

	unless addr_p == addr_nsp goto end
	say "*** The addrs are the same. I think that's wrong."

	goto end

got_class:
	say "Got the class. I didn't see that coming."
end:
.end

.namespace ['N';'S';'P']

.sub bar :method
	.return (1)
.end


.namespace ['N';'S';'Q']

.sub bar :method
	.return (1)
.end

The effect of this seems to be that any code which relies on P6object cannot create classes that have the same name.

The workaround is, trivially, to make sure that class names are distinct. Anyone who has ever coded C89 should be okay.

[whiteknight]

Okay, here are some preliminary digging results. This code prints what we expect:

.sub foo :main
	.local pmc class_p
	.local pmc class_nsp
	class_p = newclass [ 'P' ]
	class_nsp = newclass ["N";'S';'P']

        say class_p
        say class_nsp

        $P0 = new ['P']
        $P0.'bar'()

        $P1 = new ['N';'S';'P']
        $P1.'bar'()
.end

.namespace ['N';'S';'P']

.sub bar :method
	say "NSP"
.end

.namespace ['P']

.sub bar :method
	say "P"
.end

This correctly says:

P
N;S;P
P
NSP

However, when we look up using the namespace, we have a problem:

.sub foo :main
	.local pmc class_p
	.local pmc class_nsp

	class_p = newclass [ 'P' ]
	$P1 = get_hll_namespace ['N';'S';'P']
	class_nsp = get_class $P1

        say class_p
        say class_nsp

        $P0 = new ['P']
        $P0.'bar'()

        $P1 = new ['N';'S';'P']
        $P1.'bar'()
.end

.namespace ['N';'S';'P']

.sub bar :method
	say "NSP"
.end


.namespace ['P']

.sub bar :method
	say "P"
.end

Which outputs something like this (I've ommitted line numbers since they are different in my test file:

P
P
P
Class '[ 'N' ; 'S' ; 'P' ]' not found

[whiteknight]

Next piece of the puzzle. In src/oo.c:Parrot_oo_get_class, when we call it with the NSP namespace PMC from my example above, we hit this piece of code:

switch (key->vtable->base_type) {
  case enum_class_NameSpace:
    classobj = VTABLE_get_class(interp, key);
    break;

However, at this point there is no NSP class object, VTABLE_get_class returns PMCNULL, and we enter a branch further down in the function that is intended to handle lazy creation of a PMCProxy. Here's the codepath it takes:

type = Parrot_pmc_get_type_str(interp, VTABLE_get_string(interp, key));
classobj = get_pmc_proxy(interp, type);

...And when stringified, the namespace apparently is just "P". get_pmc_proxy, despite the name, doesn't get a PMCProxy for the type if a class is already defined for it. Instead, it returns the "P" class object, which is where Austin's code got lost.

I think the problem is calling get_class to lookup a Class object that doesn't exist yet. In this case, I think we should return PMCNULL from get_class until newclass is created. I'm going to hack that solution together now, test it, and commit it if things look good.

[whiteknight]

After looking through this some more, I can't think of a good way to fix this as-is. Here's the situation:

When we call get_class<namespace> and the requested class does not currently exist, one of two things could plausibly happen: (1) we return PMCNULL, or (2) we create the necessary class object on the fly and return that.

Confusing this issue is the fact that we don't always get back a Class, sometimes for built-in types we would get a PMCProxy instead. In case (1) above the get_class opcode would only need minimal changes but calling newclass is obviously wrong unless we make newclass smart enough to detect a built-in type name and create the PMCProxy instead of Class. In case (2) above, get_class would need to detect the built-in type name and then create the PMCProxy or Class object appropriately.

Since the arguments we're talking about here are NameSpaces, it's not a simple matter of calling NameSpace's get_string VTABLE to get the class name, since each namespace only contains the "last name". We would need to walk the entire ->parent chain to assemble the "full name" of the namespace, and then use that to determine if the type is built-in or not. No current functions in Parrot are suitable for this operation, and I suspect it will be difficult because C-based dynpmcs could be loaded into a different HLL namespace.

Also we need to consider what to do in the case of an HLL override, whether we would want get_class to return the class of the override instead of the built-in type (this is a design decision that I think we can delay making for now).

This ticket becomes much easier to resolve if PMCProxies for C-based PMCs are created more eagerly at VM startup time, or dynpmc load time. In that case, the NameSpace's get_class VTABLE always returns a non-null PMCProxy for C-based types, and the behavior of get_class<namespace> op becomes substantially easier. In case (2) above, get_class<namespace> either returns the existing proxy, returns an existing class, or creates a new Class (not needing to ever worry about creating a PMCProxy or conflicting with an existing C-based PMC type name). In case (1) above, newclass would need to lookup the NameSpace and ensure the ->class slot isn't already filled with a PMCProxy or existing Class, and then create a new one. Of course, if we're being passed a NameSpace as an argument, the lookup is already done.

I think chromatic's current plan for #389 is to create PMCProxies eagerly as I describe here. If that's the case, we can fix this ticket shortly thereafter. If this is not his current intention or the final result of that work, I don't think this ticket can be reasonably fixed.

[pmichaud]

In case (2) above, get_class<namespace> either returns the existing proxy, returns an existing class, or creates a new Class (not needing to ever worry about creating a PMCProxy or conflicting with an existing C-based PMC type name).

IMO, get_class<namespace> should never automatically create a new Class PMC object. It should return PMCNULL for non-existing classes. (Built-in PMC types should be assumed to have a class object of some sort.)

Pm

[whiteknight]

I like that idea, Patrick. However, if I make that change the build fails horribly.

./parrot runtime/parrot/library/PGE/Perl6Grammar.pbc --output=compilers/tge/TGE/Parser.pir compilers/tge/TGE/Parser.pg
Parent isn't a Class.
current instr.: 'parrot;P6metaclass;add_parent' pc 278 (runtime/parrot/library/P6object.pir:243)
called from Sub 'parrot;P6metaclass;register' pc 708 (runtime/parrot/library/P6object.pir:465)
called from Sub 'parrot;PGE;Match;' pc 18 (compilers/pge/PGE/Match.pir:20)
called from Sub 'parrot;PGE;Perl6Grammar;Compiler;__onload' pc 24 (runtime/parrot/library/PGE/Perl6Grammar.pir:74)
... call repeated 1 times

So at the very least we are going to need a deprecation cycle and some non-trivial fixes to several areas of our standard runtime library. A quick review shows that TGE, PGE, and P6object are all going to need modifications in order to follow this rule. If this is okay for you, I can create a branch to start the work, and add in a deprecation notice.