Prototype supertype property proxy fix

This was a nasty bug that I discovered when working on a project at work
probably over a year ago. I had worked around it, but ease.js was largely
stalled at the time; with it revitalized by GNU, it's finally getting fixed.

See test case comments for more information.

lib/ClassBuilder.js

@@ -360,6 +360,14 @@ exports.prototype.build = function extend( _, __ )
     // increment class identifier
     this._classId++;
 
+    // if we are inheriting from a prototype, we must make sure that all
+    // properties initialized by the ctor are implicitly public; otherwise,
+    // proxying will fail to take place
+    if ( !( prototype instanceof exports.ClassBase ) )
+    {
+        this._discoverProtoProps( prototype, prop_init );
+    }
+
     // build the various class components (XXX: this is temporary; needs
     // refactoring)
     try
@@ -465,6 +473,55 @@ exports.prototype._getBase = function( base )
 };
 
 
+/**
+ * Discovers public properties on the given object and create an associated
+ * property
+ *
+ * This allows inheriting from a prototype that uses properties by ensuring
+ * that we properly proxy to that property. Otherwise, assigning the value
+ * on the private visibilit object would mask the underlying value rather
+ * than modifying it, leading to an inconsistent and incorrect state.
+ *
+ * This assumes that the object has already been initialized with all the
+ * properties. This may not be the case if the prototype constructor does
+ * not do so, in which case there is nothing we can do.
+ *
+ * This does not recurse on the prototype chian.
+ *
+ * For a more detailed description of this issue, see the interoperability
+ * test case for classes.
+ *
+ * @param  {Object}  obj        object from which to gather properties
+ * @param  {Object}  prop_init  destination property object
+ *
+ * @return  {undefined}
+ */
+exports.prototype._discoverProtoProps = function( obj, prop_init )
+{
+    var hasOwn = Object.hasOwnProperty,
+        pub    = prop_init[ 'public' ];
+
+    for ( var field in obj )
+    {
+        var value = obj[ field ];
+
+        // we are not interested in the objtype chain, nor are we
+        // interested in functions (which are methods and need not be
+        // proxied)
+        if ( !( hasOwn.call( obj, field ) )
+            || typeof value === 'function'
+        )
+        {
+            continue;
+        }
+
+        this._memberBuilder.buildProp(
+            prop_init, null, field, value, {}
+        );
+    }
+};
+
+
 exports.prototype.buildMembers = function buildMembers(
     props, class_id, base, prop_init, memberdest, staticInstLookup
 )

test/Class/InteropTest.js

@@ -29,6 +29,7 @@ require( 'common' ).testCase(
     caseSetUp: function()
     {
         this.Class    = this.require( 'class' );
+        this.fallback = this.require( 'util' ).definePropertyFallback();
     },
 
 
@@ -98,5 +99,194 @@ require( 'common' ).testCase(
             } );
         } );
     },
+
+
+    /**
+     * This was a subtle bug that creeped up in a class that was derived
+     * from a prototype: the prototype was setting its property values
+     * (which are of course public), which the class was also manipulating.
+     * Unfortunately, the class was manipulating a property of a same name
+     * on the private visibility object, whereas the prototype instance was
+     * manipulating it on the public. Therefore, the value of the property
+     * varied depending on whether you asked the class instance or the
+     * prototype instance that it inherited. Yikes.
+     *
+     * The root issue of this was even more subtle: the parent method (that
+     * does the manipulation) was invoked, meaning that it was executed
+     * within the context of the private visibility object, which is what
+     * caused the issue. However, this issue is still valid regardless of
+     * whether a parent method is called.
+     *
+     * Mitigating this is difficult, so we settle for a combination of good
+     * guessing and user education. We assume that all non-function fields
+     * set on the object (its own fields---not the prototype chain) by the
+     * constructor  are public and therefore need to be proxied, and so
+     * implicitly declare them as such. Any remaining properties that are
+     * set on the object (e.g. set by methods but not initialized in the
+     * ctor) will need to be manually handled by declaring them as public in
+     * the class. We test the first case here.
+     */
+    'Recognizes and proxies prototype properties as public': function()
+    {
+        var expected  = 'baz',
+            expected2 = 'buzz';
+
+        // ctor initializes a single property, which is clearly public (as
+        // all fields on an object are)
+        var P = function()
+        {
+            this.foo = 'bar';
+
+            this.updateFoo = function( val )
+            {
+                this.foo = val;
+            };
+        };
+
+        var inst = this.Class.extend( P,
+        {
+            // since updateField is invoked within the context of the
+            // instance's private visibility object (unless falling back),
+            // we need to ensure that the set of foo is properly proxied
+            // back to the public property
+            'override updateFoo': function( val )
+            {
+                // consider that we're now invoking the parent updateFoo
+                // within the context of the private visibility object,
+                // *not* the public visibility object that it is accustomed
+                // to
+                this.__super( val );
+                return this;
+            },
+
+            ownUpdateFoo: function( val )
+            {
+                this.foo = val;
+                return this;
+            }
+        } )();
+
+        // if detection failed, then the value of foo will still be "bar"
+        this.assertEqual( inst.ownUpdateFoo( expected ).foo, expected );
+
+        // another interesting case; they should be mutual, but it's still
+        // worth demonstrating (see docblock comments)
+        this.assertEqual( inst.updateFoo( expected2 ).foo, expected2 );
+    },
+
+
+    /**
+     * This demonstrates what happens if ease.js is not aware of a
+     * particular property. This test ensures that the result is as
+     * expected.
+     *
+     * This does not apply in the case of a fallback, because there are not
+     * separate visibility objects in that case.
+     */
+    'Does not recognize non-ctor-initialized properties as public':
+    function()
+    {
+        if ( this.fallback )
+        {
+            // no separate visibility layers; does not apply
+            return;
+        }
+
+        var expected = 'bar';
+
+        var P = function()
+        {
+            this.init = function( val )
+            {
+                // this was not initialized in the ctor
+                this.foo = val;
+                return this;
+            };
+        };
+
+        var inst = this.Class.extend( P,
+        {
+            rmfoo: function()
+            {
+                // this is not proxied
+                this.foo = undefined;
+                return this;
+            },
+
+            getFoo: function()
+            {
+                return this.foo;
+            }
+        } )();
+
+        // the public foo and the foo visible inside the class are two
+        // different references, so rmfoo() will have had no effect on the
+        // public API
+        this.assertEqual(
+            inst.init( expected ).rmfoo().foo,
+            expected
+        );
+
+        // but it will be visible internally
+        this.assertEqual( inst.getFoo(), undefined );
+    },
+
+
+    /**
+     * In the case where ease.js is unable to do so automatically, we should
+     * be able to correct the proxy situation ourselves. This is where the
+     * aforementioned "education" part comes in; it will be documented in
+     * the manual.
+     */
+    'Declaring non-ctor-initialized properties as public resolves proxy':
+    function()
+    {
+        var expected = 'bar';
+
+        var P = function()
+        {
+            this.init = function()
+            {
+                // this was not initialized in the ctor
+                this.foo = null;
+                return this;
+            };
+        };
+
+        var inst = this.Class.extend( P,
+        {
+            // the magic
+            'public foo': null,
+
+            setFoo: function( val )
+            {
+                this.foo = val;
+                return this;
+            }
+        } )();
+
+        this.assertEqual( inst.init().setFoo( expected ).foo, expected );
+    },
+
+
+    /**
+     * While this should follow as a conseuqence of the above, let's be
+     * certain, since it would re-introduce the problems that we are trying
+     * to avoid (not to mention it'd be inconsistent with OOP conventions).
+     */
+    'Cannot de-escalate visibility of prototype properties': function()
+    {
+        var P = function() { this.foo = 'bar'; };
+
+        var Class = this.Class;
+        this.assertThrows( function()
+        {
+            Class.extend( P,
+            {
+                // de-escalate from public to protected
+                'protected foo': '',
+            } );
+        } );
+    },
 } );