From 7427958ec063945972f53dd7939369458f1418b2 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Thu, 10 Mar 2011 12:19:39 -0500 Subject: [PATCH 1/3] Returning 'this' from a method will now return the object instance, not the internal property object --- lib/member_builder.js | 21 ++++++++++++++++++--- test/test-class-visibility.js | 28 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/lib/member_builder.js b/lib/member_builder.js index 40dcd69..f467674 100644 --- a/lib/member_builder.js +++ b/lib/member_builder.js @@ -314,6 +314,7 @@ function overrideMethod( super_method, new_method, instCallback ) // __super property var override = null; + // are we overriding? if ( new_method ) { override = function() @@ -330,11 +331,25 @@ function overrideMethod( super_method, new_method, instCallback ) } else { + // we are defining a new method override = function() { - return super_method.apply( - ( instCallback( this ) || this ), arguments - ); + var context = instCallback( this ) || this, + retval = undefined + ; + + // invoke the method + retval = super_method.apply( context, arguments ); + + // if the value returned from the method was the context that we + // passed in, return the actual instance (to ensure we do not break + // encapsulation) + if ( retval === context ) + { + return this; + } + + return retval; }; } diff --git a/test/test-class-visibility.js b/test/test-class-visibility.js index be5df66..67f07e6 100644 --- a/test/test-class-visibility.js +++ b/test/test-class-visibility.js @@ -45,6 +45,7 @@ var common = require( './common' ), 'protected protf': protf, 'private privf': privf, + 'public getProp': function( name ) { // return property, allowing us to break encapsulation for @@ -60,6 +61,12 @@ var common = require( './common' ), { this[ name ] = value; }, + + + 'public getSelf': function() + { + return this; + }, }), // instance of Foo @@ -308,3 +315,24 @@ var common = require( './common' ), ); } )(); + +/** + * When a method is called, 'this' is bound to the property object containing + * private and protected members. Returning 'this' would therefore be a very bad + * thing. Not only would it break encapsulation, but it would likely have other + * problems down the road. + * + * Therefore, we have to check the return value of the method. If the return + * value is the property object that it was bound to, we need to replace the + * return value with the actual class instance. This allows us to transparently + * enforce encapsulation. How sweet is that? + */ +( function testReturningSelfFromMethodShouldReturnInstanceNotPropObj() +{ + assert.deepEqual( + foo.getSelf(), + foo, + "Returning 'this' from a method should return instance of self" + ); +} )(); + From 84363aca4531af8c9a1154b13a21ad3c5bc999b8 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Thu, 10 Mar 2011 12:24:59 -0500 Subject: [PATCH 2/3] Added test to ensure correct instance is returned when returning 'this' from a parent method --- test/test-class-visibility.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/test-class-visibility.js b/test/test-class-visibility.js index 67f07e6..a02fb71 100644 --- a/test/test-class-visibility.js +++ b/test/test-class-visibility.js @@ -334,5 +334,12 @@ var common = require( './common' ), foo, "Returning 'this' from a method should return instance of self" ); + + // what happens in the case of inheritance? + assert.deepEqual( + sub_foo.getSelf(), + sub_foo, + "Returning 'this' from a super method should return the subtype" + ); } )(); From e0fb37daa06a1fc71681222a80b37a340b0deee6 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Thu, 10 Mar 2011 12:40:55 -0500 Subject: [PATCH 3/3] Overridden methods now property return instance when returning 'this' - There may be a cleaner way to do this. This is a quick fix. --- lib/member_builder.js | 16 +++++++++++++--- test/test-class-visibility.js | 21 ++++++++++++++++++++- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/lib/member_builder.js b/lib/member_builder.js index f467674..5daf630 100644 --- a/lib/member_builder.js +++ b/lib/member_builder.js @@ -319,12 +319,22 @@ function overrideMethod( super_method, new_method, instCallback ) { override = function() { + var context = instCallback( this ) || this, + retval = undefined + ; + // the _super property will contain the parent method this.__super = super_method; - var retval = new_method.apply( - ( instCallback( this ) || this ), arguments - ); + retval = new_method.apply( context, arguments ); + + // if the value returned from the method was the context that we + // passed in, return the actual instance (to ensure we do not break + // encapsulation) + if ( retval === context ) + { + return this; + } return retval; }; diff --git a/test/test-class-visibility.js b/test/test-class-visibility.js index a02fb71..61bf5c9 100644 --- a/test/test-class-visibility.js +++ b/test/test-class-visibility.js @@ -67,13 +67,25 @@ var common = require( './common' ), { return this; }, + + + 'public getSelfOverride': function() + { + // override me + }, }), // instance of Foo foo = Foo(), // subtype - SubFoo = Foo.extend( {} ), + SubFoo = Foo.extend({ + 'public getSelfOverride': function() + { + // return this from overridden method + return this; + }, + }), sub_foo = SubFoo() ; @@ -341,5 +353,12 @@ var common = require( './common' ), sub_foo, "Returning 'this' from a super method should return the subtype" ); + + // finally, overridden methods should still return the instance + assert.deepEqual( + sub_foo.getSelfOverride(), + sub_foo, + "Returning 'this' from a overridden method should return the subtype" + ); } )();