[patch][cla]strange inheritance behavior

[ben hockey]

in the test file i've attached, i'm overriding the buildRendering in dijit._Widget and dijit._Templated prototypes. if you switch between 1.3 and 1.4 you'll see some different behavior.

dojo 1.3

• you'll see the console.log showing that the new buildRendering is being used

dojo 1.4

• the new buildRendering is not used unless i give my.foo a buildRendering method which calls this.inherited

i've marked this as critical because it's not quite a blocker since there's a workaround.

i'd also be happy to be told that there's something wrong with my example.

[Eugene Lazutkin]

Methods are decorated by dojo.declare(), yet you use raw functions and construct prototypes (as far as buildRendering() is concerned) manually. Here you go.

Most probably you should use Class.extend() to replace those methods. I'll post a snippet later.

[ben hockey]

i've updated my test file but still have the same issues. declaring a buildRendering in my.foo and calling this.inherited is still a workaround.

[ben hockey]

updated to use Class.extend

[ben hockey]

i'm not sure if this patch is exactly how you would like to solve the issue but it works and should hopefully at least point to where the issue is coming from - see declare.diff attached.

[ben hockey]

patch for declare to update meta data in constructor

[ben hockey]

oh... and do you think this bug would be appropriate for inclusion in the 1.4.2 release? i feel like it is quite important.

[Eugene Lazutkin]

The patch appears to be correct in general, but I need to see if it doesn't violate anything else. For example, right off the bat: _meta should be used only if it is defined (it is not always the case).

[James Burke]

elazutkin: This ticket has milestone set for 1.4.2, is this something that should go in the 1.4.2 release? Adam just cut an RC for 1.4.2, but if we should wait for this ticket, that would be good to know, so we can adjust the 1.4.2 release according to when it is fixed.

[Adam Peller]

I should have noted that I had a chat with Eugene about this yesterday and he didn't think it would make the 1.4.2 release. We should make that decision final by changing the milestone... going once, twice?

[James Burke]

Oh, OK. Looks like there was also a possibly related issue mentioned in the dojo-interest list today: ​http://mail.dojotoolkit.org/pipermail/dojo-interest/2010-March/043804.html

although it is light on reproducible details.

As this seems like a regression from 1.3, it would be good to address if not in a 1.4.2 release, maybe a 1.4.3.

If we need to take another week to get this one sorted out for a 1.4.2 release, would that be OK (depending on what Eugene says)? AFAIK, there is nothing pressing the 1.4.2 release except for the xss fixes for test files, but that can wait another week if that helps resolve the issue.

[ben hockey]

behavior has definitely changed since 1.3 and can be seen in the attached test case. even using the preferred Class.extend rather than changing the prototype directly still does not give the expected behavior. the attached files show use cases, which i assume would be typical, where changes to a superclass will not be inherited by subclasses (unless the workaround shown in the test case is applied).

[Eugene Lazutkin]

Ben forgot to add a link to his test case: ​http://neonstalwart.dojotoolkit.org/test/declare.html

Again it is not clear if his patch fixes it.

[Eugene Lazutkin]

One more note: dojo.declare() uses the mixin model to emulate the multiple inheritance. It means that only one class can be a true parent (in the single inheritance sense), the rest are going to be copied. If anybody changes a class, which was already copied, the change would not be reflected in copies for obvious reasons. It was true for the original dojo.declare(), it is true for the current implementation.

[ben hockey]

Replying to elazutkin:

One more note: dojo.declare() uses the mixin model to emulate the multiple inheritance. It means that only one class can be a true parent (in the single inheritance sense), the rest are going to be copied. If anybody changes a class, which was already copied, the change would not be reflected in copies for obvious reasons. It was true for the original dojo.declare(), it is true for the current implementation.

the test case at ​http://neonstalwart.dojotoolkit.org/test/declare.html demonstrates that my "patch" is not sufficient. please just consider the patch to be only indicative - it is not truly a complete patch. these 2 links are similar to the first proto.html file attached to this ticket - ​http://neonstalwart.dojotoolkit.org/test/proto1.3.html uses dojo 1.3 and ​http://neonstalwart.dojotoolkit.org/test/proto1.4.html uses dojo 1.4

[Eugene Lazutkin]

I looked at declare.html example and it should not work because only A is inherited, and B and C are copied. Obviously it doesn't work with Dojo 1.3 either.

Having said that the _Widget/_Templated example should work => there is a bug, which should be fixed.

[ben hockey]

i have a couple more test files to attach - these ones show a difference in behavior when you call this.inherited with the name of the function you want to call

			dojo.declare('my.Base', null, {
				foo: function(){
					console.log('Base::foo');
				}
			});
	    	
			dojo.declare('my.Thing', my.Base, {
				foo: function(){
					console.log('Thing::foo');
				},
				bar: function(){
					console.log('Thing::bar');
					this.inherited('foo', arguments);
				}
			});

i'll attach the files to the ticket but here are links to live copies of these files ​http://neonstalwart.dojotoolkit.org/test/inherit1.4.html ​http://neonstalwart.dojotoolkit.org/test/inherit1.3.html

[Eugene Lazutkin]

Related to #10709.

[Eugene Lazutkin]

Replying to neonstalwart:

i have a couple more test files to attach - these ones show a difference in behavior when you call this.inherited with the name of the function you want to call

This one is explainable: the method name overrides the annotated name. It should not be used to call a superclass' method with a different name.

The algorithm of "inherited" works like that:

1. arguments.caller gives us a method, which was used to call "inherited". We need to find it in our hierarchy in order to find next method to call.

2. The "right" way to do it is to inspect all properties of a prototype in order to find the match. The process should be repeated for all objects in the prototype chain until the method is found. If it was not found, the other possibility is that it was dynamically attached to the instance => we need to inspect all properties of the instance checking for our method. If it was not found there, we give up.

3. Obviously the previous algorithm while exhausting is very too slow: for...in is very slow, and the amount of checks is too big.

4. So we use a shortcut --- we need to know the method's name in order to eliminate for...in loops. Now our search is linear. It is proportional to the length of the inheritance chain. Using caching we can eliminate loops in most cases.

5. The source of method's name is a property on the method itself called "nom". If the method was added manually, it is not decorated, so the name attribute of "inherited" is used. It overrides "nom" to cover another case: then already decorated method is added to an object with a different name.

6. So the name override is used for dynamic methods to indicate their name to "inherited" rather than who we call next.

Is it different from 1.3? I think it is different as your examples demonstrate. Does it warrant the change? Only if it doesn't increase the code and doesn't break anything.

[Eugene Lazutkin]

Minimal example that demonstrates the bug found by Ben.

[Eugene Lazutkin]

I corrected the bug and explained the behavior in ​http://docs.dojocampus.org/dojo/declare

[Eugene Lazutkin]

(In [21793]) Using own properties instead of a shadow, !strict, fixes #10709, fixes #10788.

[Eugene Lazutkin]

See ​http://docs.dojocampus.org/dojo/declare#extend