From 1b323ed80bee53ef4328ab75ff8edd15567dfd75 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Thu, 30 Jan 2014 23:13:52 -0500 Subject: [PATCH] Validation warnings now stored in state object This will allow for additional processing before actually triggering the warnings. For the sake of this commit, though, we just keep with existing functionality. --- lib/ClassBuilder.js | 8 ++- lib/MemberBuilder.js | 26 +++++++- lib/MemberBuilderValidator.js | 75 ++++++++++++++++++++-- lib/interface.js | 6 +- test/ClassBuilder/MemberRestrictionTest.js | 38 ++++++++--- test/MemberBuilder/GetterSetterTest.js | 44 +++++++------ test/MemberBuilder/MethodTest.js | 50 ++++++++------- test/MemberBuilder/PropTest.js | 44 +++++++------ test/MemberBuilder/inc-common.js | 25 +++++--- test/MemberBuilderValidator/MethodTest.js | 31 +++++---- test/MemberBuilderValidator/inc-common.js | 5 +- 11 files changed, 247 insertions(+), 105 deletions(-) diff --git a/lib/ClassBuilder.js b/lib/ClassBuilder.js index 65ce8aa..f72c3b1 100644 --- a/lib/ClassBuilder.js +++ b/lib/ClassBuilder.js @@ -451,6 +451,9 @@ exports.prototype.buildMembers = function buildMembers( smethods = static_members.methods, sprops = static_members.props, + // holds member builder state + state = {}, + _self = this ; @@ -527,7 +530,7 @@ exports.prototype.buildMembers = function buildMembers( var used = _self._memberBuilder.buildMethod( dest, null, name, func, keywords, instLookup, - class_id, base + class_id, base, state ); // do nothing more if we didn't end up using this definition @@ -556,6 +559,9 @@ exports.prototype.buildMembers = function buildMembers( } }, } ); + + // process accumulated member state + this._memberBuilder.end( state ); } diff --git a/lib/MemberBuilder.js b/lib/MemberBuilder.js index ce7883e..6d80a71 100644 --- a/lib/MemberBuilder.js +++ b/lib/MemberBuilder.js @@ -95,6 +95,10 @@ exports.initMembers = function( mpublic, mprotected, mprivate ) * Copies a method to the appropriate member prototype, depending on * visibility, and assigns necessary metadata from keywords * + * The provided ``member run'' state object is required and will be + * initialized automatically if it has not been already. For the first + * member of a run, the object should be empty. + * * @param {__visobj} members * @param {!Object} meta metadata container * @param {string} name property name @@ -108,10 +112,12 @@ exports.initMembers = function( mpublic, mprotected, mprivate ) * @param {number} cid class id * @param {Object=} base optional base object to scan * + * @param {Object} state member run state object + * * @return {undefined} */ exports.buildMethod = function( - members, meta, name, value, keywords, instCallback, cid, base + members, meta, name, value, keywords, instCallback, cid, base, state ) { // TODO: We can improve performance by not scanning each one individually @@ -125,7 +131,7 @@ exports.buildMethod = function( // ensure that the declaration is valid (keywords make sense, argument // length, etc) this._validate.validateMethod( - name, value, keywords, prev_data, prev_keywords + name, value, keywords, prev_data, prev_keywords, state ); // we might be overriding an existing method @@ -492,3 +498,19 @@ exports._getVisibilityValue = function( keywords ) } } + +/** + * End member run and perform post-processing on state data + * + * A ``member run'' should consist of the members required for a particular + * object (class/interface/etc). This action will perform validation + * post-processing if a validator is available. + * + * @param {Object} state member run state + * + * @return {undefined} + */ +exports.end = function( state ) +{ + this._validate && this._validate.end( state ); +}; diff --git a/lib/MemberBuilderValidator.js b/lib/MemberBuilderValidator.js index ffe657c..d0a0af9 100644 --- a/lib/MemberBuilderValidator.js +++ b/lib/MemberBuilderValidator.js @@ -32,10 +32,71 @@ module.exports = exports = function MemberBuilderValidator( warn_handler ) /** - * Validates a method declaration, ensuring that keywords are valid, overrides - * make sense, etc. + * Initialize validation state if not already done * - * Throws exception on validation failure + * @param {Object} state validation state + * + * @return {Object} provided state object STATE + */ +exports.prototype._initState = function( state ) +{ + if ( state.__vready ) return state; + + state.warn = {}; + state.__vready = true; + return state; +}; + + +/** + * Perform post-processing on and invalidate validation state + * + * All queued warnings will be triggered. + * + * @param {Object} state validation state + * + * @return {undefined} + */ +exports.prototype.end = function( state ) +{ + // trigger warnings + for ( var f in state.warn ) + { + var warns = state.warn[ f ]; + for ( var id in warns ) + { + this._warningHandler( warns[ id ] ); + } + } + + state.__vready = false; +}; + + +/** + * Enqueue warning within validation state + * + * @param {Object} state validation state + * @param {string} member member name + * @param {string} id warning identifier + * @param {Warning} warn warning + * + * @return {undefined} + */ +function _addWarn( state, member, id, warn ) +{ + ( state.warn[ member ] = state.warn[ member ] || {} )[ id ] = warn; +} + + +/** + * Validates a method declaration, ensuring that keywords are valid, + * overrides make sense, etc. + * + * Throws exception on validation failure. Warnings are stored in the state + * object for later processing. The state object will be initialized if it + * has not been already; for the initial validation, the state object should + * be empty. * * @param {string} name method name * @param {*} value method value @@ -45,12 +106,16 @@ module.exports = exports = function MemberBuilderValidator( warn_handler ) * @param {Object} prev_data data of member being overridden * @param {Object} prev_keywords keywords of member being overridden * + * @param {Object} state pre-initialized state object + * * @return {undefined} */ exports.prototype.validateMethod = function( - name, value, keywords, prev_data, prev_keywords + name, value, keywords, prev_data, prev_keywords, state ) { + this._initState( state ); + var prev = ( prev_data ) ? prev_data.member : null; if ( keywords[ 'abstract' ] ) @@ -202,7 +267,7 @@ exports.prototype.validateMethod = function( // but it shouldn't stop the class definition (it doesn't adversely // affect the functionality of the class, unless of course the method // attempts to reference a supertype) - this._warningHandler( Error( + _addWarn( state, name, 'no', Error( "Method '" + name + "' using 'override' keyword without super method" ) ); diff --git a/lib/interface.js b/lib/interface.js index dcb6beb..b2661f2 100644 --- a/lib/interface.js +++ b/lib/interface.js @@ -190,6 +190,9 @@ var extend = ( function( extending ) prototype = new base(), iname = '', + // holds validation state + vstate = {}, + members = member_builder.initMembers( prototype, prototype, prototype ) @@ -235,7 +238,8 @@ var extend = ( function( extending ) } member_builder.buildMethod( - members, null, name, value, keywords + members, null, name, value, keywords, + null, 0, {}, vstate ); }, } ); diff --git a/test/ClassBuilder/MemberRestrictionTest.js b/test/ClassBuilder/MemberRestrictionTest.js index 5cf7020..dd79fd1 100644 --- a/test/ClassBuilder/MemberRestrictionTest.js +++ b/test/ClassBuilder/MemberRestrictionTest.js @@ -38,16 +38,6 @@ require( 'common' ).testCase( }, - setUp: function() - { - this.builder = this.Sut( - this.require( 'MemberBuilder' )(), - this.require( 'VisibilityObjectFactoryFactory' ) - .fromEnvironment() - ); - }, - - /** * It's always useful to be able to quickly reference a list of reserved * members so that an implementer can programatically handle runtime @@ -288,4 +278,32 @@ require( 'common' ).testCase( _self.AbstractClass( dfn ); } ); }, + + + /** + * During the course of processing, certain data are accumulated into + * the member builder state; this state must be post-processed to + * complete anything that may be pending. + */ + 'Member builder state is ended after processing': function() + { + var _self = this, + build = this.require( 'MemberBuilder' )(); + + var sut = this.Sut( + build, + this.require( 'VisibilityObjectFactoryFactory' ) + .fromEnvironment() + ); + + // TODO: test that we're passed the right state + var called = false; + build.end = function( state ) + { + called = true; + }; + + sut.build( {} ); + this.assertOk( called ); + }, } ); diff --git a/test/MemberBuilder/GetterSetterTest.js b/test/MemberBuilder/GetterSetterTest.js index e09e89f..297016a 100644 --- a/test/MemberBuilder/GetterSetterTest.js +++ b/test/MemberBuilder/GetterSetterTest.js @@ -30,30 +30,32 @@ require( 'common' ).testCase( { var _self = this; - this.testArgs = function( args, name, value, keywords ) + this.testArgs = function( args, name, value, keywords, state ) { - shared.testArgs( _self, args, name, value, keywords, function( - prev_default, pval_given, pkey_given - ) - { - var expected = _self.members[ 'public' ][ name ]; - - if ( !expected ) + shared.testArgs( _self, args, name, value, keywords, state, + function( + prev_default, pval_given, pkey_given + ) { - return prev_default; - } + var expected = _self.members[ 'public' ][ name ]; - return { - value: { - expected: expected, - given: pval_given.member, - }, - keywords: { - expected: null, // XXX - given: pkey_given, - }, - }; - } ); + if ( !expected ) + { + return prev_default; + } + + return { + value: { + expected: expected, + given: pval_given.member, + }, + keywords: { + expected: null, // XXX + given: pkey_given, + }, + }; + } + ); }; }, diff --git a/test/MemberBuilder/MethodTest.js b/test/MemberBuilder/MethodTest.js index ca2dcb7..b88fc0b 100644 --- a/test/MemberBuilder/MethodTest.js +++ b/test/MemberBuilder/MethodTest.js @@ -27,30 +27,32 @@ require( 'common' ).testCase( { var _self = this; - this.testArgs = function( args, name, value, keywords ) + this.testArgs = function( args, name, value, keywords, state ) { - shared.testArgs( _self, args, name, value, keywords, function( - prev_default, pval_given, pkey_given - ) - { - var expected = _self.members[ 'public' ][ name ]; - - if ( !expected ) + shared.testArgs( _self, args, name, value, keywords, state, + function( + prev_default, pval_given, pkey_given + ) { - return prev_default; - } + var expected = _self.members[ 'public' ][ name ]; - return { - value: { - expected: expected, - given: pval_given.member, - }, - keywords: { - expected: expected.___$$keywords$$, // XXX - given: pkey_given, - }, - }; - } ); + if ( !expected ) + { + return prev_default; + } + + return { + value: { + expected: expected, + given: pval_given.member, + }, + keywords: { + expected: expected.___$$keywords$$, // XXX + given: pkey_given, + }, + }; + } + ); }; // simply intended to execute test two two perspectives @@ -100,17 +102,19 @@ require( 'common' ).testCase( name = 'foo', value = function() {}, + state = {}, keywords = {} ; this.mockValidate.validateMethod = function() { called = true; - _self.testArgs( arguments, name, value, keywords ); + _self.testArgs( arguments, name, value, keywords, state ); }; this.assertOk( this.sut.buildMethod( - this.members, {}, name, value, keywords, function() {}, 1, {} + this.members, {}, name, value, keywords, function() {}, 1, {}, + state ) ); this.assertEqual( true, called, 'validateMethod() was not called' ); diff --git a/test/MemberBuilder/PropTest.js b/test/MemberBuilder/PropTest.js index fbb530b..8b8ffd2 100644 --- a/test/MemberBuilder/PropTest.js +++ b/test/MemberBuilder/PropTest.js @@ -27,30 +27,32 @@ require( 'common' ).testCase( { var _self = this; - this.testArgs = function( args, name, value, keywords ) + this.testArgs = function( args, name, value, keywords, state ) { - shared.testArgs( _self, args, name, value, keywords, function( - prev_default, pval_given, pkey_given - ) - { - var expected = _self.members[ 'public' ][ name ]; - - if ( !expected ) + shared.testArgs( _self, args, name, value, keywords, state, + function( + prev_default, pval_given, pkey_given + ) { - return prev_default; - } + var expected = _self.members[ 'public' ][ name ]; - return { - value: { - expected: expected[ 0 ], - given: pval_given.member[ 0 ], - }, - keywords: { - expected: expected[ 1 ], - given: pkey_given, - }, - }; - } ); + if ( !expected ) + { + return prev_default; + } + + return { + value: { + expected: expected[ 0 ], + given: pval_given.member[ 0 ], + }, + keywords: { + expected: expected[ 1 ], + given: pkey_given, + }, + }; + } + ); }; }, diff --git a/test/MemberBuilder/inc-common.js b/test/MemberBuilder/inc-common.js index 7a92e19..1dd23be 100644 --- a/test/MemberBuilder/inc-common.js +++ b/test/MemberBuilder/inc-common.js @@ -27,11 +27,14 @@ * @param {string} name member name * @param {*} value expected value * @param {Object} keywords expected keywords - * @param {function()} prevLookup function to use to look up prev member data + * @param {Object} state validation state + * @param {function()} prevLookup function to look up prev member data * * @return {undefined} */ -exports.testArgs = function( testcase, args, name, value, keywords, prevLookup ) +exports.testArgs = function( + testcase, args, name, value, keywords, state, prevLookup +) { var prev = { value: { expected: null, given: args[ 3 ] }, @@ -41,24 +44,28 @@ exports.testArgs = function( testcase, args, name, value, keywords, prevLookup ) prev = prevLookup( prev, prev.value.given, prev.keywords.given ); testcase.assertEqual( name, args[ 0 ], - 'Incorrect name passed to validator' + "Incorrect name passed to validator" ); testcase.assertDeepEqual( value, args[ 1 ], - 'Incorrect value passed to validator' + "Incorrect value passed to validator" ); testcase.assertStrictEqual( keywords, args[ 2 ], - 'Incorrect keywords passed to validator' + "Incorrect keywords passed to validator" ); testcase.assertStrictEqual( prev.value.expected, prev.value.given, - 'Previous data should contain prev value if overriding, ' + - 'otherwise null' + "Previous data should contain prev value if overriding, " + + "otherwise null" ); testcase.assertDeepEqual( prev.keywords.expected, prev.keywords.given, - 'Previous keywords should contain prev keyword if ' + - 'overriding, otherwise null' + "Previous keywords should contain prev keyword if " + + "overriding, otherwise null" + ); + + testcase.assertStrictEqual( state, args[ 5 ], + "State object was not passed to validator" ); }; diff --git a/test/MemberBuilderValidator/MethodTest.js b/test/MemberBuilderValidator/MethodTest.js index 9fe6a00..58c71bd 100644 --- a/test/MemberBuilderValidator/MethodTest.js +++ b/test/MemberBuilderValidator/MethodTest.js @@ -51,13 +51,18 @@ require( 'common' ).testCase( startobj.virtual = true; overrideobj.override = true; + var state = {}; + _self.sut.validateMethod( name, function() {}, overrideobj, { member: function() {} }, - startobj + startobj, + state ); + + _self.sut.end( state ); }, failstr ); @@ -128,7 +133,7 @@ require( 'common' ).testCase( _self.sut.validateMethod( name, function() {}, {}, { get: function() {} }, - {} + {}, {} ); } ); @@ -138,7 +143,7 @@ require( 'common' ).testCase( _self.sut.validateMethod( name, function() {}, {}, { set: function() {} }, - {} + {}, {} ); } ); }, @@ -161,7 +166,7 @@ require( 'common' ).testCase( _self.sut.validateMethod( name, function() {}, {}, { member: 'immaprop' }, - {} + {}, {} ); } ); }, @@ -246,7 +251,8 @@ require( 'common' ).testCase( // this function returns each of its arguments, otherwise // they'll be optimized away by Closure Compiler. { member: function( a, b, c ) { return [a,b,c]; } }, - { 'virtual': true } + { 'virtual': true }, + {} ); } ); @@ -262,7 +268,8 @@ require( 'common' ).testCase( function() {}, { 'override': true }, { member: parent_method }, - { 'virtual': true } + { 'virtual': true }, + {} ); } ); @@ -277,7 +284,8 @@ require( 'common' ).testCase( method, { 'override': true }, { member: function( a, b, c ) {} }, - { 'virtual': true } + { 'virtual': true }, + {} ); }, Error ); }, @@ -304,7 +312,8 @@ require( 'common' ).testCase( function() {}, {}, { member: amethod }, - { 'weak': true, 'abstract': true } + { 'weak': true, 'abstract': true }, + {} ); } ); @@ -316,7 +325,7 @@ require( 'common' ).testCase( amethod, { 'weak': true, 'abstract': true }, { member: function() {} }, - {} + {}, {} ); } ); }, @@ -448,7 +457,7 @@ require( 'common' ).testCase( { // provide function instead of string _self.sut.validateMethod( - name, function() {}, { 'proxy': true }, {}, {} + name, function() {}, { 'proxy': true }, {}, {}, {} ); } ); }, @@ -464,7 +473,7 @@ require( 'common' ).testCase( this.assertDoesNotThrow( function() { _self.sut.validateMethod( - 'foo', 'dest', { 'proxy': true }, {}, {} + 'foo', 'dest', { 'proxy': true }, {}, {}, {} ); }, TypeError ); }, diff --git a/test/MemberBuilderValidator/inc-common.js b/test/MemberBuilderValidator/inc-common.js index e6e7233..052286f 100644 --- a/test/MemberBuilderValidator/inc-common.js +++ b/test/MemberBuilderValidator/inc-common.js @@ -87,6 +87,7 @@ exports.quickKeywordTest = function( prev_obj = {}, prev_data = prev_data || {}, name = exports.testName, + state = {}, _self = this; // convert our convenient array into a keyword obj @@ -114,7 +115,7 @@ exports.quickKeywordTest = function( var val = ( keyword_obj[ 'proxy' ] ) ? 'proxyDest': function() {}; _self.sut[ type ]( - name, val, keyword_obj, prev_data, prev_obj + name, val, keyword_obj, prev_data, prev_obj, state ); }; @@ -126,6 +127,8 @@ exports.quickKeywordTest = function( { this.assertDoesNotThrow( testfunc ); } + + this.sut.end( state ); };