Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#10788 closed defect (fixed)

[patch][cla]strange inheritance behavior

Reported by: ben hockey Owned by: Eugene Lazutkin
Priority: high Milestone: 1.5
Component: Core Version: 1.4.0
Keywords: Cc:
Blocked By: Blocking:

Description

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.

Attachments (6)

proto.html (4.2 KB) - added by ben hockey 9 years ago.
proto.2.html (3.5 KB) - added by ben hockey 9 years ago.
updated to use Class.extend
declare.diff (448 bytes) - added by ben hockey 9 years ago.
patch for declare to update meta data in constructor
inherit1.3.html (800 bytes) - added by ben hockey 9 years ago.
inherit1.4.html (800 bytes) - added by ben hockey 9 years ago.
ben_declare.html (1.6 KB) - added by Eugene Lazutkin 9 years ago.
Minimal example that demonstrates the bug found by Ben.

Download all attachments as: .zip

Change History (28)

Changed 9 years ago by ben hockey

Attachment: proto.html added

comment:1 Changed 9 years ago by Eugene Lazutkin

Owner: changed from anonymous to Eugene Lazutkin
Status: newassigned

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.

comment:2 Changed 9 years ago by 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.

Changed 9 years ago by ben hockey

Attachment: proto.2.html added

updated to use Class.extend

comment:3 Changed 9 years ago by 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.

Changed 9 years ago by ben hockey

Attachment: declare.diff added

patch for declare to update meta data in constructor

comment:4 Changed 9 years ago by 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.

comment:5 Changed 9 years ago by Eugene Lazutkin

Milestone: tbd1.4.2
severity: criticalnormal

comment:6 Changed 9 years ago by 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).

comment:7 Changed 9 years ago by Adam Peller

Summary: strange inheritance behavior[patch][cla]strange inheritance behavior

comment:8 Changed 9 years ago by 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.

comment:9 Changed 9 years ago by 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?

comment:10 Changed 9 years ago by 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.

comment:11 Changed 9 years ago by 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).

comment:12 Changed 9 years ago by 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.

comment:13 Changed 9 years ago by 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.

comment:14 in reply to:  13 Changed 9 years ago by 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

comment:15 Changed 9 years ago by 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.

comment:16 Changed 9 years ago by 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

Changed 9 years ago by ben hockey

Attachment: inherit1.3.html added

Changed 9 years ago by ben hockey

Attachment: inherit1.4.html added

comment:17 Changed 9 years ago by Adam Peller

Milestone: 1.4.21.5

comment:18 Changed 9 years ago by Eugene Lazutkin

Related to #10709.

comment:19 in reply to:  16 Changed 9 years ago by 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.
  1. 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.
  1. Obviously the previous algorithm while exhausting is very too slow: for...in is very slow, and the amount of checks is too big.
  1. 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.
  1. 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.
  1. 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.

Changed 9 years ago by Eugene Lazutkin

Attachment: ben_declare.html added

Minimal example that demonstrates the bug found by Ben.

comment:20 Changed 9 years ago by Eugene Lazutkin

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

comment:21 Changed 9 years ago by Eugene Lazutkin

Resolution: fixed
Status: assignedclosed

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

Note: See TracTickets for help on using tickets.