From 34d84412e17c30dbf65886b68c106e1f22f8b48d Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Wed, 23 Apr 2014 02:01:02 -0400 Subject: [PATCH] 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 | 57 +++++++++++ test/Class/InteropTest.js | 192 +++++++++++++++++++++++++++++++++++++- 2 files changed, 248 insertions(+), 1 deletion(-) diff --git a/lib/ClassBuilder.js b/lib/ClassBuilder.js index c52558c..343f513 100644 --- a/lib/ClassBuilder.js +++ b/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 ) diff --git a/test/Class/InteropTest.js b/test/Class/InteropTest.js index b66f348..ee1ce29 100644 --- a/test/Class/InteropTest.js +++ b/test/Class/InteropTest.js @@ -28,7 +28,8 @@ require( 'common' ).testCase( { caseSetUp: function() { - this.Class = this.require( 'class' ); + 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': '', + } ); + } ); + }, } );