Added private member name conflict validations

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,

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
+    // do not permit visibility de-escalation
-    if ( typeof prev === 'function' )
+    if ( prev )
     {
-        throw new TypeError(
+        // perform this check first, as it will make more sense than those that
-            "Cannot override method '" + name + "' with property"
+        // 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.

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' );
+        } );
+    },
 } );

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' );
+        } );
+    },
 } );
 

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' );
+        } );
+    },
 } );
 

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'    ],
-        [ 'protected', 'public' ],
+        [ 'public',    '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
     {