diff --git a/lib/Trait.js b/lib/Trait.js index 8ceb5ce..3825454 100644 --- a/lib/Trait.js +++ b/lib/Trait.js @@ -398,11 +398,12 @@ function createVirtProxy( acls, dfn ) ? 'public' : 'protected'; - var plen = acls.___$$methods$$[ vis ][ f ].__length; + var srcmethod = acls.___$$methods$$[ vis ][ f ], + plen = srcmethod.__length; // this is the aforementioned proxy method; see the docblock for // more information - dfn[ vis + ' virtual override ' + f ] = ( function() + dfn[ vis + ' virtual override ' + f ] = ( function( f ) { var retf = function() { @@ -422,16 +423,16 @@ function createVirtProxy( acls, dfn ) // this guy bypasses the above virtual override check, which is // necessary in certain cases to prevent infinte recursion - dfn[ vis + ' virtual __$$' + f ] = ( function( f ) + dfn[ vis + ' virtual __$$' + f ] = ( function( method ) { var retf = function() { - return this.___$$parent$$[ f ].apply( this, arguments ); + return method.apply( this, arguments ); }; retf.__length = plen; return retf; - } )( f ); + } )( srcmethod ); } } diff --git a/test/Trait/ClassVirtualTest.js b/test/Trait/ClassVirtualTest.js index 84a2e94..d745732 100644 --- a/test/Trait/ClassVirtualTest.js +++ b/test/Trait/ClassVirtualTest.js @@ -212,4 +212,48 @@ require( 'common' ).testCase( C.use( T )().foo(); this.assertOk( called ); }, + + + /** + * Virtual methods for traits are handled via a series of proxy methods + * that determine, at runtime (as opposed to when the class is created), + * where the call should go. (At least that was the implementation at + * the time this test was written.) This test relies on the proper + * parameter metadata being set on those proxy methods so that the + * necessary length requirements can be validated. + * + * This was a bug in the initial implemenation: the above tests did not + * catch it because the virtual methods had no arguments. The initial + * problem was that, since __length was not defined on the generated + * method that was recognized as the override, it was always zero, which + * always failed if there were any arguments on the virtual method. The + * reverse case was also a problem, but it didn't manifest as an + * error---rather, it did *not* error when it should have. + * + * Note the instantiation in these cases: this is because the trait + * implementation lazily performs the mixin on first use. + */ + 'Subtype must meet compatibility requirements of virtual trait method': + function() + { + var _self = this; + + var C = this.Class.use( + this.Sut( { 'virtual foo': function( a, b ) {} } ) + ); + + this.assertThrows( function() + { + // does not meet param requirements (note the + // instantiation---traits defer processing until they are used) + C.extend( { 'override foo': function( a ) {} } )(); + } ); + + this.assertDoesNotThrow( function() + { + // does not meet param requirements (note the + // instantiation---traits defer processing until they are used) + C.extend( { 'override foo': function( a, b ) {} } )(); + } ); + }, } ); diff --git a/test/Trait/VirtualTest.js b/test/Trait/VirtualTest.js index 36cecae..4dcf068 100644 --- a/test/Trait/VirtualTest.js +++ b/test/Trait/VirtualTest.js @@ -191,46 +191,48 @@ require( 'common' ).testCase( /** - * Virtual methods for traits are handled via a series of proxy methods - * that determine, at runtime (as opposed to when the class is created), - * where the call should go. (At least that was the implementation at - * the time this test was written.) This test relies on the proper - * parameter metadata being set on those proxy methods so that the - * necessary length requirements can be validated. + * This test unfortunately requires knowledge of implementation details + * to explain; it is a regression test covering a rather obnoxious bug, + * especially when the author was away from the implementation for a + * couple months. * - * This was a bug in the initial implemenation: the above tests did not - * catch it because the virtual methods had no arguments. The initial - * problem was that, since __length was not defined on the generated - * method that was recognized as the override, it was always zero, which - * always failed if there were any arguments on the virtual method. The - * reverse case was also a problem, but it didn't manifest as an - * error---rather, it did *not* error when it should have. + * Proxying to an overridden protected method was not a problem because + * it proxies to the protected member object (PMO) which is passed into + * the ctor and, as is evident by its name, provides both the public and + * protected API. However, when not overridden, we fall back to having + * to invoke our original method, which is on our supertype---the + * abstract trait class. The problem there is that the stored supertype + * prototype provides only the public API. * - * Note the instantiation in these cases: this is because the trait - * implementation lazily performs the mixin on first use. + * This test ensures that we properly access the protected API of our + * supertype. This problem existed before any general solution to this + * problem for all subtypes. We test public as well to produce a more + * general test case. + * + * The second part of this test is implicit---we're testing multiple + * virtual methods to ensure that they return distinct results, ensuring + * that we don't have any variable reassignment issues in the loop that + * generates the closures. */ - 'Subtype must meet compatibility requirements of virtual trait method': + 'Properly invokes non-overridden virtual trait methods': function() { - var _self = this; + var expecteda = { a: true }, + expectedb = { b: true }; - var C = this.Class.use( - this.Sut( { 'virtual foo': function( a, b ) {} } ) - ); - - this.assertThrows( function() + var T = this.Sut( { - // does not meet param requirements (note the - // instantiation---traits defer processing until they are used) - C.extend( { 'override foo': function( a ) {} } )(); + pub: function() { return this.vpub(); }, + prot: function() { return this.vprot(); }, + + 'virtual public vpub': function() { return expecteda; }, + 'virtual protected vprot': function() { return expectedb; } } ); - this.assertDoesNotThrow( function() - { - // does not meet param requirements (note the - // instantiation---traits defer processing until they are used) - C.extend( { 'override foo': function( a, b ) {} } )(); - } ); + var inst = this.Class.use( T ).extend( {} )(); + + this.assertStrictEqual( inst.pub(), expecteda ); + this.assertStrictEqual( inst.prot(), expectedb ); },