From e41495c0d1b675612428448a59c71e56825f59fd Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Sat, 3 Dec 2011 00:30:22 -0500 Subject: [PATCH] Added private member name conflict validations --- lib/MemberBuilder.js | 7 ++- lib/MemberBuilderValidator.js | 62 ++++++++++++++----- .../GetterSetterTest.js | 21 ++++++- test/MemberBuilderValidator/MethodTest.js | 24 ++++++- test/MemberBuilderValidator/PropertyTest.js | 24 ++++++- test/MemberBuilderValidator/inc-common.js | 36 ++++++++--- 6 files changed, 142 insertions(+), 32 deletions(-) diff --git a/lib/MemberBuilder.js b/lib/MemberBuilder.js index 216e0ab..76d6136 100644 --- a/lib/MemberBuilder.js +++ b/lib/MemberBuilder.js @@ -209,13 +209,18 @@ exports.buildGetterSetter = function( ) { var prev_data = scanMembers( members, name, base ), - prev_keywords = null + prev_keywords = ( ( prev_data && prev_data.get ) + ? prev_data.get.___$$keywords$$ + : null + ) ; this._validate.validateGetterSetter( name, {}, keywords, prev_data, prev_keywords ); + get.___$$keywords$$ = keywords; + Object.defineProperty( getMemberVisibility( members, keywords, name ), name, diff --git a/lib/MemberBuilderValidator.js b/lib/MemberBuilderValidator.js index 6bc0829..f618a28 100644 --- a/lib/MemberBuilderValidator.js +++ b/lib/MemberBuilderValidator.js @@ -98,6 +98,15 @@ exports.prototype.validateMethod = function( // search for any previous instances of this member if ( prev ) { + // perform this check first, as it will make more sense than those that + // follow, should this condition be satisfied + if ( prev_keywords[ 'private' ] ) + { + throw TypeError( + "Private member name '" + name + "' conflicts with supertype" + ); + } + // disallow overriding properties with methods if ( !( typeof prev === 'function' ) ) { @@ -191,12 +200,34 @@ exports.prototype.validateProperty = function( { var prev = ( prev_data ) ? prev_data.member : null; - // disallow overriding methods with properties - if ( typeof prev === 'function' ) + // do not permit visibility de-escalation + if ( prev ) { - throw new TypeError( - "Cannot override method '" + name + "' with property" - ); + // perform this check first, as it will make more sense than those that + // follow, should this condition be satisfied + if ( prev_keywords[ 'private' ] ) + { + throw TypeError( + "Private member name '" + name + "' conflicts with supertype" + ); + } + + // disallow overriding methods with properties + if ( typeof prev === 'function' ) + { + throw new TypeError( + "Cannot override method '" + name + "' with property" + ); + } + + if ( this._getVisibilityValue( prev_keywords ) + < this._getVisibilityValue( keywords ) + ) + { + throw TypeError( + "Cannot de-escalate visibility of property '" + name + "'" + ); + } } // do not allow overriding getters/setters @@ -207,18 +238,6 @@ exports.prototype.validateProperty = function( ); } - // do not permit visibility de-escalation - if ( prev && - ( this._getVisibilityValue( prev_keywords ) - < this._getVisibilityValue( keywords ) - ) - ) - { - throw TypeError( - "Cannot de-escalate visibility of property '" + name + "'" - ); - } - // abstract properties do not make sense if ( keywords[ 'abstract' ] ) { @@ -267,6 +286,15 @@ exports.prototype.validateGetterSetter = function( if ( prev || prev_gs ) { + // perform this check first, as it will make more sense than those that + // follow, should this condition be satisfied + if ( prev_keywords && prev_keywords[ 'private' ] ) + { + throw TypeError( + "Private member name '" + name + "' conflicts with supertype" + ); + } + // To speed up the system we'll simply check for a getter/setter, rather // than checking separately for methods/properties. This is at the // expense of more detailed error messages. They'll live. diff --git a/test/MemberBuilderValidator/GetterSetterTest.js b/test/MemberBuilderValidator/GetterSetterTest.js index 072f4c6..7493b9c 100644 --- a/test/MemberBuilderValidator/GetterSetterTest.js +++ b/test/MemberBuilderValidator/GetterSetterTest.js @@ -35,7 +35,7 @@ require( 'common' ).testCase( this.quickFailureTest = shared.quickFailureTest; - this.quickVisChangeTest = function( start, override, failtest ) + this.quickVisChangeTest = function( start, override, failtest, failstr ) { shared.quickVisChangeTest.call( _self, start, override, failtest, function( name, startobj, overrideobj ) @@ -45,7 +45,8 @@ require( 'common' ).testCase( { get: function() {}, set: function() {} }, startobj ); - } + }, + failstr ); }; }, @@ -117,4 +118,20 @@ require( 'common' ).testCase( _self.quickVisChangeTest( cur[ 0 ], cur[ 1 ], false ); } ); }, + + + /** + * See property/method tests for more information. This is not strictly + * necessary (since getters/setters can exist only in an ES5+ environment), + * but it's provided for consistency. It's also easy to remove this feature + * without breaking BC. The reverse is untrue. + */ + 'Cannot redeclare private getters/setters in subtypes': function() + { + var _self = this; + shared.privateNamingConflictTest( function( cur ) + { + _self.quickVisChangeTest( cur[ 0 ], cur[ 1 ], true, 'conflict' ); + } ); + }, } ); diff --git a/test/MemberBuilderValidator/MethodTest.js b/test/MemberBuilderValidator/MethodTest.js index fc4b683..bdc3f9f 100644 --- a/test/MemberBuilderValidator/MethodTest.js +++ b/test/MemberBuilderValidator/MethodTest.js @@ -42,7 +42,7 @@ require( 'common' ).testCase( this.quickFailureTest = shared.quickFailureTest; - this.quickVisChangeTest = function( start, override, failtest ) + this.quickVisChangeTest = function( start, override, failtest, failstr ) { shared.quickVisChangeTest.call( _self, start, override, failtest, function( name, startobj, overrideobj ) @@ -57,7 +57,8 @@ require( 'common' ).testCase( { member: function() {} }, startobj ); - } + }, + failstr ); }; }, @@ -354,5 +355,24 @@ require( 'common' ).testCase( 'Override warning should contain method name' ); }, + + + /** + * Wait - what? That doesn't make sense from an OOP perspective, now does + * it! Unfortunately, we're forced into this restriction in order to + * properly support fallback to pre-ES5 environments where the visibility + * object is a single layer, rather than three. With this impl, all members + * are public and private name conflicts would result in supertypes and + * subtypes altering eachothers' private members (see manual for more + * information). + */ + 'Cannot redeclare private members in subtypes': function() + { + var _self = this; + shared.privateNamingConflictTest( function( cur ) + { + _self.quickVisChangeTest( cur[ 0 ], cur[ 1 ], true, 'conflict' ); + } ); + }, } ); diff --git a/test/MemberBuilderValidator/PropertyTest.js b/test/MemberBuilderValidator/PropertyTest.js index 6af3675..22f3f96 100644 --- a/test/MemberBuilderValidator/PropertyTest.js +++ b/test/MemberBuilderValidator/PropertyTest.js @@ -40,7 +40,7 @@ require( 'common' ).testCase( ); }; - this.quickVisChangeTest = function( start, override, failtest ) + this.quickVisChangeTest = function( start, override, failtest, failstr ) { shared.quickVisChangeTest.call( _self, start, override, failtest, function( name, startobj, overrideobj ) @@ -50,7 +50,8 @@ require( 'common' ).testCase( { member: 'foo' }, startobj ); - } + }, + failstr ); }; }, @@ -169,5 +170,24 @@ require( 'common' ).testCase( _self.quickVisChangeTest( cur[ 0 ], cur[ 1 ], false ); } ); }, + + + /** + * Wait - what? That doesn't make sense from an OOP perspective, now does + * it! Unfortunately, we're forced into this restriction in order to + * properly support fallback to pre-ES5 environments where the visibility + * object is a single layer, rather than three. With this impl, all members + * are public and private name conflicts would result in supertypes and + * subtypes altering eachothers' private members (see manual for more + * information). + */ + 'Cannot redeclare private properties in subtypes': function() + { + var _self = this; + shared.privateNamingConflictTest( function( cur ) + { + _self.quickVisChangeTest( cur[ 0 ], cur[ 1 ], true, 'conflict' ); + } ); + }, } ); diff --git a/test/MemberBuilderValidator/inc-common.js b/test/MemberBuilderValidator/inc-common.js index 240a414..305a9e2 100644 --- a/test/MemberBuilderValidator/inc-common.js +++ b/test/MemberBuilderValidator/inc-common.js @@ -66,7 +66,7 @@ exports.quickFailureTest = function( name, identifier, action ) // to aid in debugging, the error message should contain the // name of the method _self.assertOk( ( e.message.search( name ) !== -1 ), - 'Error message should contain method name' + 'Error message should contain member name' ); return; @@ -139,12 +139,12 @@ exports.quickKeywordTest = function( type, keywords, identifier, prev ) exports.visEscalationTest = function( test ) { var tests = [ - [ 'private', 'protected' ], - [ 'protected', 'public' ], - - [ 'public', 'public' ], + [ 'protected', 'public' ], + [ 'public', 'public' ], [ 'protected', 'protected' ], - [ 'private', 'private' ] + + // note: private/private is intentionally omitted; see private naming + // conflict test ]; for ( var i = 0, len = tests.length; i < len; i++ ) @@ -155,6 +155,22 @@ exports.visEscalationTest = function( test ) }; +exports.privateNamingConflictTest = function( test ) +{ + var tests = [ + [ 'private', 'private' ], + [ 'private', 'protected' ], + [ 'private',' public' ], + ]; + + var i = tests.length; + while ( i-- ) + { + test( tests[ i ] ); + } +}; + + /** * Performs a simple visibility change test using access modifiers * @@ -166,9 +182,13 @@ exports.visEscalationTest = function( test ) * * @param {function()} func test function * + * @param {string} failstr string to check for in failure string + * * @return {undefined} */ -exports.quickVisChangeTest = function( start, override, failtest, func ) +exports.quickVisChangeTest = function( + start, override, failtest, func, failstr +) { var _self = this, name = 'foo', @@ -187,7 +207,7 @@ exports.quickVisChangeTest = function( start, override, failtest, func ) if ( failtest ) { - this.quickFailureTest( name, 'de-escalate', testfun ); + this.quickFailureTest( name, ( failstr || 'de-escalate' ), testfun ); } else {