From 63a4f95f6544888d0e6d274add69642d795f144f Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Wed, 13 Apr 2011 14:48:20 -0400 Subject: [PATCH] Fix for overriding protected with protected - A better solution may be explored --- lib/member_builder.js | 3 +++ lib/propobj.js | 26 ++++++++++++++++++------ test/test-class-visibility.js | 38 +++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 6 deletions(-) diff --git a/lib/member_builder.js b/lib/member_builder.js index 035146c..0dec7c8 100644 --- a/lib/member_builder.js +++ b/lib/member_builder.js @@ -145,6 +145,9 @@ exports.buildMethod = function( // to ensure privileged calls will work properly dest[ name ] = overrideMethod( value, null, instCallback, cid ); } + + // store keywords for later reference (needed for pre-ES5 fallback) + dest[ name ].___$$keywords$$ = keywords; }; diff --git a/lib/propobj.js b/lib/propobj.js index 561e48c..33c81b2 100644 --- a/lib/propobj.js +++ b/lib/propobj.js @@ -71,7 +71,11 @@ exports.setup = function( dest, properties, methods ) // public. The reason for this is because the property object is laid /atop/ // of the public members, meaning that a parent's protected members will // take precedence over a subtype's overriding /public/ members. Uh oh. - doSetup( dest, properties[ 'protected' ], methods[ 'protected' ], true ); + doSetup( dest, + properties[ 'protected' ], + methods[ 'protected' ], + 'public' + ); // then add the private parts doSetup( obj, properties[ 'private' ], methods[ 'private' ] ); @@ -90,11 +94,10 @@ exports.setup = function( dest, properties, methods ) * * @return {undefined} */ -function doSetup( dest, properties, methods, unless_exists ) +function doSetup( dest, properties, methods, unless_keyword ) { - unless_exists = !!unless_exists; - - var hasOwn = Array.prototype.hasOwnProperty; + var hasOwn = Array.prototype.hasOwnProperty, + pre = null; // copy over the methods if ( methods !== undefined ) @@ -103,13 +106,24 @@ function doSetup( dest, properties, methods, unless_exists ) { if ( hasOwn.call( methods, method_name ) ) { + pre = dest[ method_name ]; + // If requested, do not copy the method over if it already // exists in the destination object. Don't use hasOwn here; // unnecessary overhead and we want to traverse any prototype // chains. We do not check the public object directly, for // example, because we need a solution that will work if a proxy // is unsupported by the engine. - if ( !unless_exists || ( dest[ method_name ] === undefined ) ) + // + // Also note that we need to allow overriding if it exists in + // the protected object (we can override protected with + // protected). This is the *last* check to ensure a performance + // hit is incured *only* if we're overriding protected with + // protected. + if ( !unless_keyword + || ( pre === undefined ) + || !( pre.___$$keywords$$[ unless_keyword ] ) + ) { dest[ method_name ] = methods[ method_name ]; } diff --git a/test/test-class-visibility.js b/test/test-class-visibility.js index 0d6fa69..326122b 100644 --- a/test/test-class-visibility.js +++ b/test/test-class-visibility.js @@ -725,3 +725,41 @@ var common = require( './common' ), ); } )(); + +/** + * There was a bug introduced when we prevented protected members from + * overriding public (since in the prototype chain, protected members are laid + * atop public, and this cannot change). This bug would disallow protected + * members from being overridden by other protected members. + * + * This test is both a proof and a regression test. + */ +( function testCanProperlyOverrideProtectedWithProtected() +{ + var val = 'foobar', + result = Class( + { + 'protected foo': function() {}, + } ).extend( + { + // provide concrete implementation + 'protected foo': function() + { + return val; + }, + + 'public doFoo': function() + { + return this.foo(); + }, + } )().doFoo(); + ; + + // if everything worked as expected, the value of 'val' will have been + // returned and stored in 'result' + assert.equal( result, val, + "Protected methods can properly be overriden by another protected " + + "method" + ); +} )(); +