From 8480d8f92c45a1c63d3f4f140619aff48cbf2666 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Tue, 4 Mar 2014 00:19:39 -0500 Subject: [PATCH] Added support for abstract overrides --- lib/MemberBuilder.js | 51 ++++++++++++++++++++++++++++-- lib/MemberBuilderValidator.js | 19 +++++++++-- lib/Trait.js | 16 ++++++---- lib/util.js | 5 ++- test/Trait/ClassVirtualTest.js | 33 ++++++++++++++++++- test/Trait/LinearizationTest.js | 47 +++++++++++++++++++++++++-- test/Util/PropParseKeywordsTest.js | 27 ++++++++++++++++ 7 files changed, 183 insertions(+), 15 deletions(-) diff --git a/lib/MemberBuilder.js b/lib/MemberBuilder.js index b9012be..ab568e6 100644 --- a/lib/MemberBuilder.js +++ b/lib/MemberBuilder.js @@ -146,7 +146,7 @@ exports.buildMethod = function( } else if ( prev ) { - if ( keywords.weak ) + if ( keywords.weak && !( prev_keywords[ 'abstract' ] ) ) { // another member of the same name has been found; discard the // weak declaration @@ -154,9 +154,15 @@ exports.buildMethod = function( } else if ( keywords[ 'override' ] || prev_keywords[ 'abstract' ] ) { + // if we have the `abstract' keyword at this point, then we are + // an abstract override + var override = ( keywords[ 'abstract' ] ) + ? aoverride( name ) + : prev; + // override the method dest[ name ] = this._overrideMethod( - prev, value, instCallback, cid + override, value, instCallback, cid ); } else @@ -185,6 +191,47 @@ exports.buildMethod = function( }; +/** + * Creates an abstract override super method proxy to NAME + * + * This is a fairly abstract concept that is disastrously confusing without + * having been put into the proper context: This function is intended to be + * used as a super method for a method override in the case of abstract + * overrides. It only makes sense to be used, at least at this time, with + * mixins. + * + * When called, the bound context (`this') will be the private member object + * of the caller, which should contain a reference to the protected member + * object of the supertype to proxy to. It is further assumed that the + * protected member object (pmo) defines NAME such that it proxies to a + * mixin; this means that invoking it could result in an infinite loop. We + * therefore skip directly to the super-super method, which will be the + * method we are interested in proxying to. + * + * There is one additional consideration: If this super method is proxying + * from a mixin instance into a class, then it is important that we bind the + * calling context to the pmo instaed of our own context; otherwise, we'll + * be executing within the context of the trait, without access to the + * members of the supertype that we are proxying to! The pmo will be used by + * the ease.js method wrapper to look up the proper private member object, + * so it is not a problem that the pmo is being passed in. + * + * That's a lot of text for such a small amount of code. + * + * @param {string} name name of method to proxy to + * + * @return {Function} abstract override super method proxy + */ +function aoverride( name ) +{ + return function() + { + return this.___$$pmo$$.___$$parent$$[ name ] + .apply( this.___$$pmo$$, arguments ); + }; +} + + /** * Copies a property to the appropriate member prototype, depending on * visibility, and assigns necessary metadata from keywords diff --git a/lib/MemberBuilderValidator.js b/lib/MemberBuilderValidator.js index 3beec53..e6e60cf 100644 --- a/lib/MemberBuilderValidator.js +++ b/lib/MemberBuilderValidator.js @@ -213,9 +213,22 @@ exports.prototype.validateMethod = function( // disallow overriding non-virtual methods if ( keywords[ 'override' ] && !( prev_keywords[ 'virtual' ] ) ) { - throw TypeError( - "Cannot override non-virtual method '" + name + "'" - ); + if ( !( keywords[ 'abstract' ] ) ) + { + throw TypeError( + "Cannot override non-virtual method '" + name + "'" + ); + } + + // at this point, we have `abstract override' + if ( !( prev_keywords[ 'abstract' ] ) ) + { + // TODO: test me + throw TypeError( + "Cannot perform abstract override on non-abstract " + + "method '" + name + "'" + ); + } } // do not allow overriding concrete methods with abstract unless the diff --git a/lib/Trait.js b/lib/Trait.js index e7544de..27da48d 100644 --- a/lib/Trait.js +++ b/lib/Trait.js @@ -175,8 +175,10 @@ function createConcrete( acls ) var dfn = { 'protected ___$$trait$$': function() {}, - // protected member object - 'private ___$$pmo$$': null, + // protected member object (we define this as protected so that the + // parent ACLS has access to it (!), which is not prohibited since + // JS does not provide a strict typing mechanism...this is a kluge) + 'protected ___$$pmo$$': null, __construct: function( pmo ) { this.___$$pmo$$ = pmo; @@ -393,7 +395,7 @@ function mixMethods( src, dest, vis, iname ) // if abstract, then we are expected to provide the implementation; // otherwise, we proxy to the trait's implementation - if ( keywords['abstract'] ) + if ( keywords[ 'abstract' ] && !( keywords[ 'override' ] ) ) { // copy the abstract definition (N.B. this does not copy the // param names, since that is not [yet] important); the @@ -403,9 +405,10 @@ function mixMethods( src, dest, vis, iname ) } else { - var vk = keywords['virtual'], + var vk = keywords['virtual'], virt = vk ? 'weak virtual ' : '', - pname = ( vk ? '' : 'proxy ' ) + virt + vis + ' ' + f; + ovr = ( keywords['override'] ) ? 'override ' : '', + pname = ( vk ? '' : 'proxy ' ) + virt + ovr + vis + ' ' + f; // if we have already set up a proxy for a field of this name, // then multiple traits have defined the same concrete member @@ -416,7 +419,7 @@ function mixMethods( src, dest, vis, iname ) } // if non-virtual, a normal proxy should do - if ( !( keywords['virtual'] ) ) + if ( !( keywords[ 'virtual' ] ) ) { dest[ pname ] = iname; continue; @@ -546,3 +549,4 @@ function createTctor( tc ) module.exports = Trait; + diff --git a/lib/util.js b/lib/util.js index eb5b274..2cc46f5 100644 --- a/lib/util.js +++ b/lib/util.js @@ -305,7 +305,10 @@ exports.propParse = function( data, options ) name = parse_data.name || prop; keywords = parse_data.keywords || {}; - if ( options.assumeAbstract || keywords[ 'abstract' ] ) + // note the exception for abstract overrides + if ( options.assumeAbstract + || ( keywords[ 'abstract' ] && !( keywords[ 'override' ] ) ) + ) { // may not be set if assumeAbstract is given keywords[ 'abstract' ] = true; diff --git a/test/Trait/ClassVirtualTest.js b/test/Trait/ClassVirtualTest.js index 7cdcb5b..36e1613 100644 --- a/test/Trait/ClassVirtualTest.js +++ b/test/Trait/ClassVirtualTest.js @@ -177,8 +177,39 @@ require( 'common' ).testCase( * otherwise, override does not make sense, because I.M is clearly * abstract and there is nothing to override. */ - 'Trait can override virtual concrete interface methods at mixin': + 'Mixin can override virtual concrete method defined by interface': function() { + var called = false, + I = this.Interface( { foo: [] } ); + + var T = this.Sut.implement( I ).extend( + { + // the keyword combination `abstract override' indicates that we + // should override whatever concrete implementation was defined + // before our having been mixed in + 'abstract override foo': function() + { + called = true; + }, + } ); + + var _self = this; + var C = this.Class.implement( I ).extend( + { + // this should be overridden by the mixin and should therefore + // never be called (for __super tests, see LinearizationTest) + 'virtual foo': function() + { + _self.fail( false, true, + "Concrete class method was not overridden by mixin" + ); + }, + } ); + + // mixing in a trait atop of C should yield the results described + // above due to the `abstract override' keyword combination + C.use( T )().foo(); + this.assertOk( called ); }, } ); diff --git a/test/Trait/LinearizationTest.js b/test/Trait/LinearizationTest.js index 88d0728..3906bd1 100644 --- a/test/Trait/LinearizationTest.js +++ b/test/Trait/LinearizationTest.js @@ -28,8 +28,9 @@ require( 'common' ).testCase( { caseSetUp: function() { - this.Sut = this.require( 'Trait' ); - this.Class = this.require( 'class' ); + this.Sut = this.require( 'Trait' ); + this.Class = this.require( 'class' ); + this.Interface = this.require( 'interface' ); }, @@ -76,5 +77,47 @@ require( 'common' ).testCase( this.assertOk( scalled ); }, + + + /** + * If a trait overrides a method of a class that it is mixed into, then + * super calls within the trait method should resolve to the class + * method. + */ + 'Mixin overriding class method has class method as super method': + function() + { + var _self = this; + + var expected = {}, + I = this.Interface( { foo: [] } ); + + var T = this.Sut.implement( I ).extend( + { + // see ClassVirtualTest case for details on this + 'abstract override foo': function() + { + // should reference C.foo + return this.__super( expected ); + }, + } ); + + var priv_expected = Math.random(); + + var C = this.Class.implement( I ).extend( + { + // asserting on this value will ensure that the below method is + // invoked in the proper context + 'private _priv': priv_expected, + + 'virtual foo': function( given ) + { + _self.assertEqual( priv_expected, this._priv ); + return given; + }, + } ); + + this.assertStrictEqual( C.use( T )().foo(), expected ); + }, } ); diff --git a/test/Util/PropParseKeywordsTest.js b/test/Util/PropParseKeywordsTest.js index 5f8e45a..d6378ec 100644 --- a/test/Util/PropParseKeywordsTest.js +++ b/test/Util/PropParseKeywordsTest.js @@ -55,6 +55,33 @@ require( 'common' ).testCase( }, + /** + * As an exception to the above rule, a method shall not considered to be + * abstract if the `override' keyword is too provided (an abstract + * override---see the trait tests for more information). + */ + 'Not considered abstract when `override\' also provided': function() + { + var _self = this; + + var data = { 'abstract override foo': function() {} }, + found = null; + + this.Sut.propParse( data, { + method: function ( name, func, is_abstract ) + { + _self.assertOk( is_abstract === false ); + _self.assertEqual( typeof func, 'function' ); + _self.assertOk( _self.Sut.isAbstractMethod( func ) === false ); + + found = name; + }, + } ); + + this.assertEqual( found, 'foo' ); + }, + + /** * The idea behind supporting this functionality---which is unsued at * the time of writing this test---is to allow eventual customization of