From bcada872408a8616da831fd6d9f960f82f43e8c0 Mon Sep 17 00:00:00 2001
From: Mike Gerwitz <mikegerwitz@gnu.org>
Date: Tue, 4 Feb 2014 23:48:40 -0500
Subject: [PATCH] [bug fix] Corrected bug with implicit visibility escalation

See test case comments for details. This wasted way too much of my time.
---
 lib/VisibilityObjectFactory.js      | 26 +++++++++++++------------
 test/VisibilityObjectFactoryTest.js | 30 +++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/lib/VisibilityObjectFactory.js b/lib/VisibilityObjectFactory.js
index cf2ab5c..dab7feb 100644
--- a/lib/VisibilityObjectFactory.js
+++ b/lib/VisibilityObjectFactory.js
@@ -80,7 +80,7 @@ exports.prototype.setup = function setup( dest, properties, methods )
     this._doSetup( dest,
         properties[ 'protected' ],
         methods[ 'protected' ],
-        'public'
+        true
     );
 
     // then add the private parts
@@ -127,20 +127,21 @@ exports.prototype._createPrivateLayer = function( atop_of, properties )
 /**
  * Set up destination object by copying over properties and methods
  *
- * @param  {Object}   dest            destination object
- * @param  {Object}   properties      properties to copy
- * @param  {Object}   methods         methods to copy
- * @param  {boolean}  unless_keyword  do not set if keyword is set on existing
- *                                    method
+ * The prot_priv parameter can be used to ignore both explicitly and
+ * implicitly public methods.
+ *
+ * @param  {Object}   dest        destination object
+ * @param  {Object}   properties  properties to copy
+ * @param  {Object}   methods     methods to copy
+ * @param  {boolean}  prot_priv   do not set unless protected or private
  *
  * @return  {undefined}
  */
 exports.prototype._doSetup = function(
-    dest, properties, methods, unless_keyword
+    dest, properties, methods, prot_priv
 )
 {
-    var hasOwn = Array.prototype.hasOwnProperty,
-        pre    = null;
+    var hasOwn = Array.prototype.hasOwnProperty;
 
     // copy over the methods
     if ( methods !== undefined )
@@ -149,7 +150,8 @@ exports.prototype._doSetup = function(
         {
             if ( hasOwn.call( methods, method_name ) )
             {
-                pre = dest[ method_name ];
+                var pre = dest[ method_name ],
+                    kw  = pre && pre.___$$keywords$$;
 
                 // If requested, do not copy the method over if it already
                 // exists in the destination object. Don't use hasOwn here;
@@ -163,9 +165,9 @@ exports.prototype._doSetup = function(
                 // protected). This is the *last* check to ensure a performance
                 // hit is incured *only* if we're overriding protected with
                 // protected.
-                if ( !unless_keyword
+                if ( !prot_priv
                     || ( pre === undefined )
-                    || !( pre.___$$keywords$$[ unless_keyword ] )
+                    || ( kw[ 'private' ] || kw[ 'protected' ] )
                 )
                 {
                     dest[ method_name ] = methods[ method_name ];
diff --git a/test/VisibilityObjectFactoryTest.js b/test/VisibilityObjectFactoryTest.js
index 9f7e437..745ab44 100644
--- a/test/VisibilityObjectFactoryTest.js
+++ b/test/VisibilityObjectFactoryTest.js
@@ -267,6 +267,36 @@ require( 'common' ).testCase(
     },
 
 
+    /**
+     * This test addresses a particularily nasty bug that wasted hours of
+     * development time: When a visibility modifier keyword is omitted, then
+     * it should be implicitly public. In this case, however, the keyword is
+     * not automatically added to the keyword list (maybe one day it will
+     * be, but for now we'll maintain the distinction); therefore, we should
+     * not be checking for the `public' keyword when determining if we
+     * should write to the protected member object.
+     */
+    'Public methods are not overwritten when keyword is omitted': function()
+    {
+        var f = function() {};
+        f.___$$keywords$$ = {};
+
+        // no keywords; should be implicitly public
+        var dest = { fpub: f };
+
+        // add duplicate method to protected
+        this.methods[ 'protected' ].fpub = function() {};
+
+        this.sut.setup( dest, this.props, this.methods );
+
+        // ensure our public method is still referenced
+        this.assertStrictEqual( dest.fpub, f,
+            "Public methods should not be overwritten by protected methods"
+        );
+    },
+
+
+
     /**
      * Same situation with private members as protected, with the exception that
      * we do not need to worry about the overlay problem (in regards to