Corrrected virtual non-overridden trait methods
See the introduced test cases for great detail; there was a problem with the implementation where only the public API of the abstract trait object was being checked, meaning that protected virtual methods were not found when peforming the call. This was not a problem on override, because that proxies to the protected member object (PMO), which includes protected members.protolib
commit
ff4b9c856b
11
lib/Trait.js
11
lib/Trait.js
|
@ -398,11 +398,12 @@ function createVirtProxy( acls, dfn )
|
||||||
? 'public'
|
? 'public'
|
||||||
: 'protected';
|
: '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
|
// this is the aforementioned proxy method; see the docblock for
|
||||||
// more information
|
// more information
|
||||||
dfn[ vis + ' virtual override ' + f ] = ( function()
|
dfn[ vis + ' virtual override ' + f ] = ( function( f )
|
||||||
{
|
{
|
||||||
var retf = function()
|
var retf = function()
|
||||||
{
|
{
|
||||||
|
@ -422,16 +423,16 @@ function createVirtProxy( acls, dfn )
|
||||||
|
|
||||||
// this guy bypasses the above virtual override check, which is
|
// this guy bypasses the above virtual override check, which is
|
||||||
// necessary in certain cases to prevent infinte recursion
|
// necessary in certain cases to prevent infinte recursion
|
||||||
dfn[ vis + ' virtual __$$' + f ] = ( function( f )
|
dfn[ vis + ' virtual __$$' + f ] = ( function( method )
|
||||||
{
|
{
|
||||||
var retf = function()
|
var retf = function()
|
||||||
{
|
{
|
||||||
return this.___$$parent$$[ f ].apply( this, arguments );
|
return method.apply( this, arguments );
|
||||||
};
|
};
|
||||||
|
|
||||||
retf.__length = plen;
|
retf.__length = plen;
|
||||||
return retf;
|
return retf;
|
||||||
} )( f );
|
} )( srcmethod );
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -212,4 +212,48 @@ require( 'common' ).testCase(
|
||||||
C.use( T )().foo();
|
C.use( T )().foo();
|
||||||
this.assertOk( called );
|
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 ) {} } )();
|
||||||
|
} );
|
||||||
|
},
|
||||||
} );
|
} );
|
||||||
|
|
|
@ -191,46 +191,48 @@ require( 'common' ).testCase(
|
||||||
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Virtual methods for traits are handled via a series of proxy methods
|
* This test unfortunately requires knowledge of implementation details
|
||||||
* that determine, at runtime (as opposed to when the class is created),
|
* to explain; it is a regression test covering a rather obnoxious bug,
|
||||||
* where the call should go. (At least that was the implementation at
|
* especially when the author was away from the implementation for a
|
||||||
* the time this test was written.) This test relies on the proper
|
* couple months.
|
||||||
* 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
|
* Proxying to an overridden protected method was not a problem because
|
||||||
* catch it because the virtual methods had no arguments. The initial
|
* it proxies to the protected member object (PMO) which is passed into
|
||||||
* problem was that, since __length was not defined on the generated
|
* the ctor and, as is evident by its name, provides both the public and
|
||||||
* method that was recognized as the override, it was always zero, which
|
* protected API. However, when not overridden, we fall back to having
|
||||||
* always failed if there were any arguments on the virtual method. The
|
* to invoke our original method, which is on our supertype---the
|
||||||
* reverse case was also a problem, but it didn't manifest as an
|
* abstract trait class. The problem there is that the stored supertype
|
||||||
* error---rather, it did *not* error when it should have.
|
* prototype provides only the public API.
|
||||||
*
|
*
|
||||||
* Note the instantiation in these cases: this is because the trait
|
* This test ensures that we properly access the protected API of our
|
||||||
* implementation lazily performs the mixin on first use.
|
* 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()
|
function()
|
||||||
{
|
{
|
||||||
var _self = this;
|
var expecteda = { a: true },
|
||||||
|
expectedb = { b: true };
|
||||||
|
|
||||||
var C = this.Class.use(
|
var T = this.Sut(
|
||||||
this.Sut( { 'virtual foo': function( a, b ) {} } )
|
|
||||||
);
|
|
||||||
|
|
||||||
this.assertThrows( function()
|
|
||||||
{
|
{
|
||||||
// does not meet param requirements (note the
|
pub: function() { return this.vpub(); },
|
||||||
// instantiation---traits defer processing until they are used)
|
prot: function() { return this.vprot(); },
|
||||||
C.extend( { 'override foo': function( a ) {} } )();
|
|
||||||
|
'virtual public vpub': function() { return expecteda; },
|
||||||
|
'virtual protected vprot': function() { return expectedb; }
|
||||||
} );
|
} );
|
||||||
|
|
||||||
this.assertDoesNotThrow( function()
|
var inst = this.Class.use( T ).extend( {} )();
|
||||||
{
|
|
||||||
// does not meet param requirements (note the
|
this.assertStrictEqual( inst.pub(), expecteda );
|
||||||
// instantiation---traits defer processing until they are used)
|
this.assertStrictEqual( inst.prot(), expectedb );
|
||||||
C.extend( { 'override foo': function( a, b ) {} } )();
|
|
||||||
} );
|
|
||||||
},
|
},
|
||||||
|
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue