1
0
Fork 0

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
closure/master
Mike Gerwitz 2011-03-13 04:51:00 -04:00
parent e4e8900a9f
commit e03c081cfd
3 changed files with 119 additions and 26 deletions

View File

@ -457,6 +457,9 @@ var extend = ( function( extending )
} }
} }
// increment class identifier
class_id++;
util.propParse( props, { util.propParse( props, {
each: function( name, value, keywords ) each: function( name, value, keywords )
{ {
@ -498,7 +501,8 @@ var extend = ( function( extending )
method: function( name, func, is_abstract, keywords ) method: function( name, func, is_abstract, keywords )
{ {
member_builder.buildMethod( member_builder.buildMethod(
members, null, name, func, keywords, getMethodInstance members, null, name, func, keywords, getMethodInstance,
class_id
); );
if ( is_abstract ) if ( is_abstract )
@ -531,13 +535,13 @@ var extend = ( function( extending )
// set up the new class // set up the new class
var new_class = createCtor( cname, abstract_methods, members ); 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.prototype = prototype;
new_class.constructor = new_class; new_class.constructor = new_class;
// important: call after setting prototype // 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 // lock down the new class (if supported) to ensure that we can't add
// members at runtime // 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 * ensuring that their data is not shared with other instances (this is not a
* problem with primitive data types). * problem with primitive data types).
* *
* The __initProps() method will also initialize any parent properties * The method will also initialize any parent properties (recursive) to ensure
* (recursive) to ensure that subtypes do not have a referencing issue, and * that subtypes do not have a referencing issue, and subtype properties take
* subtype properties take precedence over those of the parent. * precedence over those of the parent.
* *
* @param {Object} prototype prototype to attach method to * @param {Object} prototype prototype to attach method to
* @param {Object} properties properties to initialize * @param {Object} properties properties to initialize
* @param {number} cid class id
* *
* @param {{public: Object, protected: Object, private: Object}} members * @param {{public: Object, protected: Object, private: Object}} members
* *
* @return {undefined} * @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 // defaults to false, sid = super identifier
inherit = !!inherit; inherit = !!inherit;
sid = ( sid ) ? +sid : 0;
var iid = this.__iid; 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 // call the parent prop_init, letting it know that it's been
// inherited so that it does not initialize private members or // inherited so that it does not initialize private members or
// perform other unnecessary tasks // 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, // 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 // if we're inheriting, perform a setup that doesn't include everything
// that we don't want (e.g. private properties) // that we don't want (e.g. private properties)
class_instance[ iid ][ sid ] = propobj.setup( class_instance[ iid ][ cid ] = propobj.setup(
inst_props, properties, members, sid inst_props, properties, members
); );
}); });
} }
@ -980,21 +984,28 @@ function getMeta( id )
/** /**
* Returns the instance object associated with the given method * Returns the instance object associated with the given method
* *
* The instance object contains the protected and private members. This object * The instance object contains the protected members. This object can be passed
* can be passed as the context when calling a method in order to give that * as the context when calling a method in order to give that method access to
* method access to those members. * 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 {function()} method method to look up instance object for
* @param {number} cid class id
* *
* @return {Object,null} instance object if found, otherwise null * @return {Object,null} instance object if found, otherwise null
*/ */
function getMethodInstance( method ) function getMethodInstance( inst, cid )
{ {
var iid = method.__iid, var iid = inst.__iid,
data = class_instance[ method.__iid ]; data = class_instance[ iid ];
return ( iid && data ) return ( iid && data )
? data[ 0 ] ? data[ cid ]
: null : null
; ;
} }

View File

@ -62,11 +62,12 @@ exports.initMembers = function( mpublic, mprotected, mprivate )
* @param {Object=} instCallback function to call in order to retrieve * @param {Object=} instCallback function to call in order to retrieve
* object to bind 'this' keyword to * object to bind 'this' keyword to
* @param {number} cid class id
* *
* @return {undefined} * @return {undefined}
*/ */
exports.buildMethod = function( exports.buildMethod = function(
members, meta, name, value, keywords, instCallback members, meta, name, value, keywords, instCallback, cid
) )
{ {
var prev; var prev;
@ -109,7 +110,7 @@ exports.buildMethod = function(
if ( prev ) if ( prev )
{ {
// override the method // override the method
dest[ name ] = overrideMethod( prev, value, instCallback ); dest[ name ] = overrideMethod( prev, value, instCallback, cid );
} }
else if ( keywords[ 'abstract' ] ) else if ( keywords[ 'abstract' ] )
{ {
@ -120,7 +121,7 @@ exports.buildMethod = function(
{ {
// we are not overriding the method, so simply copy it over, wrapping it // we are not overriding the method, so simply copy it over, wrapping it
// to ensure privileged calls will work properly // 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 * @param {Object=} instCallback function to call in order to retrieve
* object to bind 'this' keyword to * object to bind 'this' keyword to
* @param {number} cid class id
* *
* @return {function()} override method * @return {function()} override method
*/ */
function overrideMethod( super_method, new_method, instCallback ) function overrideMethod( super_method, new_method, instCallback, cid )
{ {
instCallback = instCallback || function() {}; instCallback = instCallback || function() {};
@ -319,7 +321,7 @@ function overrideMethod( super_method, new_method, instCallback )
{ {
override = function() override = function()
{ {
var context = instCallback( this ) || this, var context = instCallback( this, cid ) || this,
retval = undefined retval = undefined
; ;
@ -344,7 +346,7 @@ function overrideMethod( super_method, new_method, instCallback )
// we are defining a new method // we are defining a new method
override = function() override = function()
{ {
var context = instCallback( this ) || this, var context = instCallback( this, cid ) || this,
retval = undefined retval = undefined
; ;

View File

@ -73,6 +73,24 @@ var common = require( './common' ),
{ {
// override me // override me
}, },
'public getPrivProp': function()
{
return this.parts;
},
'public invokePriv': function()
{
return this._priv();
},
'private _priv': function()
{
return priv;
},
}), }),
// instance of Foo // instance of Foo
@ -80,13 +98,28 @@ var common = require( './common' ),
// subtype // subtype
SubFoo = Foo.extend({ SubFoo = Foo.extend({
'private _pfoo': 'baz',
'public getSelfOverride': function() 'public getSelfOverride': function()
{ {
// return this from overridden method // return this from overridden method
return this; 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)"
);
} )();