Opened 10 years ago

Closed 10 years ago

#9795 closed defect (fixed)

Preamble inappropriately invoked

Reported by: Stefan Bird Owned by: Eugene Lazutkin
Priority: high Milestone: 1.4
Component: Core Version: 1.3.2
Keywords: Cc: Eugene Lazutkin
Blocked By: Blocking:

Description

Under certain situations, a class's preamble may be inappropriately invoked. This will happen under the following conditions:

ClassB has a superclass ClassB has a preamble method, which returns an instance of ClassC

ClassC's preamble method will be invoked (correctly) during its initialisation. However once the object is returned by ClassB's preamble, ClassC's preamble will be invoked again, but in the context of ClassB which is incorrect.

The following test illustrates this (note in the console logs that ClassC's preamble is called twice, even though only one instance is created):

dojo.provide("TestPreamble");

dojo.declare("ClassA", [], { });

dojo.declare("ClassB", [ClassA], 
{
	preamble: function()
	{
		console.log("ClassB preamble invoked: " + this.declaredClass);
		return [new ClassC()];
	}
});

dojo.declare("ClassC", null, 
{
	preamble: function()
	{
		console.log("ClassC preamble invoked: " + this.declaredClass);
		doh.assertEqual("ClassC", this.declaredClass);
	}
});

doh.register("Preamble",
[
 	{
		name: "Preamble",
 		runTest: function(t) 
 		{
 			new ClassB();
 		}
 	}
]);

Change History (12)

comment:1 Changed 10 years ago by bill

Cc: Eugene Lazutkin added

I'm hoping Eugene's dojo.declare() replacement solves this?

comment:2 Changed 10 years ago by Stefan Bird

I've also noticed the problem happens when you pass another class (that has a preamble) as an argument when calling new Class(...), so it doesn't seem to be caused by the preamble returning a new argument set.

Until it's fixed, the simple workaround is to check this.declaredClass is what you expect it to be at the start of the preamble.

comment:3 Changed 10 years ago by Eugene Lazutkin

Milestone: tbdfuture
Owner: changed from anonymous to Eugene Lazutkin
Status: newassigned

Frankly I would prefer if we don't support preamble() at all for many reasons. IIRC Dojo doesn't use it anywhere (unlike postscript()), and it was never documented in any way...

comment:4 Changed 10 years ago by Stefan Bird

Can't comment on whether it's used within Dojo or not, but it has been documented in high-profile places, even if they're not considered official. See

comment:5 in reply to:  4 ; Changed 10 years ago by Eugene Lazutkin

Replying to scbird:

Can't comment on whether it's used within Dojo or not, but it has been documented in high-profile places...

Unfortunately the information on preamble() in those links is incomplete/incorrect. But back to the ticket.

I am not sure if it is a bug. It is how dojo.declare() coded. In the absence of the documentation and without knowing the implementor's intent we are left wondering:

  • If our working hypothesis is preamble() is used to modify parameters for base constructors than yes, it looks weird: it is called without invoking base constructors.
  • But if our hypothesis is preamble() is a callback, which is invoked before initializing an instance it makes sense: it is used to signal that the instance of ClassA is about to be created.

We may have code in the wild, which relies on this hideous buguseful feature. Do we really want to brake it?

This ticket illustrates yet another side of preamble(), which I believe should be removed completely.

I am keeping this ticket around to implement this bugfeature in the new code. :-( After that I plan to close it is "invalid".

comment:6 Changed 10 years ago by Eugene Lazutkin

(In [20040]) lang.oo: minor clean up of the test, adding a test for new feature (refs #9795), !strict.

comment:7 in reply to:  5 Changed 10 years ago by Eugene Lazutkin

Replying to elazutkin:

Do we really want to brake it?

Heh. Should be "break it".

comment:8 Changed 10 years ago by Eugene Lazutkin

(In [20089]) lang.oo: clean up and fixing the preamble bug, !strict, refs #9862, refs #9795.

comment:9 Changed 10 years ago by Eugene Lazutkin

(In [20126]) declare: committing the new version with updated inline documentation, and getInherited(), !strict, refs #9862, refs #9795.

comment:10 Changed 10 years ago by matthw

I too would like to see dojo.declare get a bit of a revamp, for this reason and others.

To my mind it's really important to have a well-thought-out, theoretically sound inheritance model on which to base everything else.

At the moment dojo.declare is quite dense, cryptic code, whose intentions and design goals aren't clearly enough documented. It implements an inheritance/mixin scheme which seems to have peculiar characteristics, and which doesn't correspond to the inheritance model of any other mainstream language I'm aware of.

The presence of a number of ominous looking 'FIXMEs' in dojo.declare doesn't exactly inspire confidence either, especially when they seem to indicate that some important design decisions with respect to mixins are still up in the air.

My main issues are:

  • At the moment 'constructor' is special and different to all other methods in that superclass constructors are automatically called before you. I don't think it should be special like this; I would much prefer to call superclass constructor(s) explicitly, which gives flexibility in what arguments are passed to them, and involves less 'magic'. The current solution (use 'preamble') seems an inflexible and poorly-documented hack.
  • There are some serious flaws in the way superclass method resolution (via inherited) works in combination with mixins, which makes it hard to write real composable mixin classes. See how other mixin-based languages linearize the inheritance hierarchy, eg: http://jim-mcbeath.blogspot.com/2009/08/scala-class-linearization.html
  • Calls to 'constructor' happen in an order which isn't consistent with the superclass method resolution ordering
  • Calls to constructor also suffer from the 'diamond problem' (although this isn't an issue if you disallow constructors on mixins)

comment:11 Changed 10 years ago by matthw

Apologies, just noticed the work on dojox.lang.oo.declare. Will check that out!

comment:12 Changed 10 years ago by Eugene Lazutkin

Milestone: future1.4
Resolution: fixed
Status: assignedclosed

Fixed in #9862.

Note: See TracTickets for help on using tickets.