From e03c081cfdc65e9c09761c09cc61e5bbc16b5206 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Sun, 13 Mar 2011 04:51:00 -0400 Subject: [PATCH] Fixed bug that made private members of a supertype inaccessible to methods that have not been overridden by a subtype - In doing so, abandoned the super identifier (sid) for a more elegant solution with class ids (cid's) - This permits fast and easy private member swapping via getMethodInstance() in class.js --- lib/class.js | 49 +++++++++++++-------- lib/member_builder.js | 14 +++--- test/test-class-visibility.js | 82 ++++++++++++++++++++++++++++++++++- 3 files changed, 119 insertions(+), 26 deletions(-) diff --git a/lib/class.js b/lib/class.js index 16e5f44..3aae42e 100644 --- a/lib/class.js +++ b/lib/class.js @@ -457,6 +457,9 @@ var extend = ( function( extending ) } } + // increment class identifier + class_id++; + util.propParse( props, { each: function( name, value, keywords ) { @@ -498,7 +501,8 @@ var extend = ( function( extending ) method: function( name, func, is_abstract, keywords ) { member_builder.buildMethod( - members, null, name, func, keywords, getMethodInstance + members, null, name, func, keywords, getMethodInstance, + class_id ); if ( is_abstract ) @@ -531,13 +535,13 @@ var extend = ( function( extending ) // set up the new class var new_class = createCtor( cname, abstract_methods, members ); - attachPropInit( prototype, prop_init, members ); + attachPropInit( prototype, prop_init, members, class_id ); new_class.prototype = prototype; new_class.constructor = new_class; // important: call after setting prototype - setupProps( new_class, abstract_methods, ++class_id ); + setupProps( new_class, abstract_methods, class_id ); // lock down the new class (if supported) to ensure that we can't add // members at runtime @@ -761,24 +765,24 @@ function initInstance( iid, instance ) * ensuring that their data is not shared with other instances (this is not a * problem with primitive data types). * - * The __initProps() method will also initialize any parent properties - * (recursive) to ensure that subtypes do not have a referencing issue, and - * subtype properties take precedence over those of the parent. + * The method will also initialize any parent properties (recursive) to ensure + * that subtypes do not have a referencing issue, and subtype properties take + * precedence over those of the parent. * * @param {Object} prototype prototype to attach method to * @param {Object} properties properties to initialize + * @param {number} cid class id * * @param {{public: Object, protected: Object, private: Object}} members * * @return {undefined} */ -function attachPropInit( prototype, properties, members ) +function attachPropInit( prototype, properties, members, cid ) { - util.defineSecureProp( prototype, '__initProps', function( inherit, sid ) + util.defineSecureProp( prototype, '__initProps', function( inherit ) { // defaults to false, sid = super identifier inherit = !!inherit; - sid = ( sid ) ? +sid : 0; var iid = this.__iid; @@ -790,7 +794,7 @@ function attachPropInit( prototype, properties, members ) // call the parent prop_init, letting it know that it's been // inherited so that it does not initialize private members or // perform other unnecessary tasks - parent_init.call( this, true, ( sid + 1 ) ); + parent_init.call( this, true ); } // this will return our property proxy, if supported by our environment, @@ -801,8 +805,8 @@ function attachPropInit( prototype, properties, members ) // if we're inheriting, perform a setup that doesn't include everything // that we don't want (e.g. private properties) - class_instance[ iid ][ sid ] = propobj.setup( - inst_props, properties, members, sid + class_instance[ iid ][ cid ] = propobj.setup( + inst_props, properties, members ); }); } @@ -980,21 +984,28 @@ function getMeta( id ) /** * Returns the instance object associated with the given method * - * The instance object contains the protected and private members. This object - * can be passed as the context when calling a method in order to give that - * method access to those members. + * The instance object contains the protected members. This object can be passed + * as the context when calling a method in order to give that method access to + * those members. + * + * One level above the instance object on the prototype chain is the object + * containing the private members. This is swappable, depending on the class id + * associated with the provided method call. This allows methods that were not + * overridden by the subtype to continue to use the private members of the + * supertype. * * @param {function()} method method to look up instance object for + * @param {number} cid class id * * @return {Object,null} instance object if found, otherwise null */ -function getMethodInstance( method ) +function getMethodInstance( inst, cid ) { - var iid = method.__iid, - data = class_instance[ method.__iid ]; + var iid = inst.__iid, + data = class_instance[ iid ]; return ( iid && data ) - ? data[ 0 ] + ? data[ cid ] : null ; } diff --git a/lib/member_builder.js b/lib/member_builder.js index 5daf630..3df3f4e 100644 --- a/lib/member_builder.js +++ b/lib/member_builder.js @@ -62,11 +62,12 @@ exports.initMembers = function( mpublic, mprotected, mprivate ) * @param {Object=} instCallback function to call in order to retrieve * object to bind 'this' keyword to + * @param {number} cid class id * * @return {undefined} */ exports.buildMethod = function( - members, meta, name, value, keywords, instCallback + members, meta, name, value, keywords, instCallback, cid ) { var prev; @@ -109,7 +110,7 @@ exports.buildMethod = function( if ( prev ) { // override the method - dest[ name ] = overrideMethod( prev, value, instCallback ); + dest[ name ] = overrideMethod( prev, value, instCallback, cid ); } else if ( keywords[ 'abstract' ] ) { @@ -120,7 +121,7 @@ exports.buildMethod = function( { // we are not overriding the method, so simply copy it over, wrapping it // to ensure privileged calls will work properly - dest[ name ] = overrideMethod( value, null, instCallback ); + dest[ name ] = overrideMethod( value, null, instCallback, cid ); } }; @@ -303,10 +304,11 @@ function scanMembers( members, name, cmp ) * * @param {Object=} instCallback function to call in order to retrieve * object to bind 'this' keyword to + * @param {number} cid class id * * @return {function()} override method */ -function overrideMethod( super_method, new_method, instCallback ) +function overrideMethod( super_method, new_method, instCallback, cid ) { instCallback = instCallback || function() {}; @@ -319,7 +321,7 @@ function overrideMethod( super_method, new_method, instCallback ) { override = function() { - var context = instCallback( this ) || this, + var context = instCallback( this, cid ) || this, retval = undefined ; @@ -344,7 +346,7 @@ function overrideMethod( super_method, new_method, instCallback ) // we are defining a new method override = function() { - var context = instCallback( this ) || this, + var context = instCallback( this, cid ) || this, retval = undefined ; diff --git a/test/test-class-visibility.js b/test/test-class-visibility.js index 61bf5c9..5c82bf2 100644 --- a/test/test-class-visibility.js +++ b/test/test-class-visibility.js @@ -73,6 +73,24 @@ var common = require( './common' ), { // override me }, + + + 'public getPrivProp': function() + { + return this.parts; + }, + + + 'public invokePriv': function() + { + return this._priv(); + }, + + + 'private _priv': function() + { + return priv; + }, }), // instance of Foo @@ -80,13 +98,28 @@ var common = require( './common' ), // subtype SubFoo = Foo.extend({ + 'private _pfoo': 'baz', + 'public getSelfOverride': function() { // return this from overridden method return this; }, + + + /** + * We have to override this so that 'this' is not bound to the supertype + */ + 'public getProp': function( name ) + { + // return property, allowing us to break encapsulation for + // protected/private properties (for testing purposes) + return this[ name ]; + }, }), - sub_foo = SubFoo() + sub_foo = SubFoo(), + + sub_sub_foo = SubFoo.extend( {} )() ; @@ -362,3 +395,50 @@ var common = require( './common' ), ); } )(); + +/** + * This one's a particularly nasty bug that snuck up on me. Private members + * should not be accessible to subtypes; that's a given. However, they need to + * be accessible to the parent methods. For example, let's say class Foo + * contains public method bar(), which invokes private method _baz(). This is + * perfectly legal. Then SubFoo extends Foo, but does not override method bar(). + * Invoking method bar() should still be able to invoke private method _baz(), + * because, from the perspective of the parent class, that operation is + * perfectly legal. + * + * The resolution of this bug required a slight system redesign. The short-term + * fix was to declare any needed private members are protected, so that they + * were accessible by the subtype. + */ +( function testParentMethodsCanAccessPrivateMembersOfParent() +{ + // properties + assert.equal( + sub_foo.getPrivProp(), + priv, + "Parent methods should have access to the private properties of the " + + "parent" + ); + + // methods + assert.equal( + sub_foo.invokePriv(), + priv, + "Parent methods should have access to the private methods of the parent" + ); + + // should apply to super-supertypes too + assert.equal( + sub_sub_foo.getPrivProp(), + priv, + "Parent methods should have access to the private properties of the " + + "parent (2)" + ); + assert.equal( + sub_sub_foo.invokePriv(), + priv, + "Parent methods should have access to the private methods of the " + + "parent (2)" + ); +} )(); +