From f9b951ddb23ff856201d7f81794f4287ac3ce2a5 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Sat, 22 Oct 2011 01:00:17 -0400 Subject: [PATCH] [#25] [#25] Began moving MemberBuilder validation rules into MemberBuilderValidator (moved method rules) --- lib/MemberBuilder.js | 133 +-------- lib/MemberBuilderValidator.js | 191 ++++++++++++ test/MemberBuilderValidator/MethodTest.js | 347 ++++++++++++++++++++++ 3 files changed, 547 insertions(+), 124 deletions(-) create mode 100644 lib/MemberBuilderValidator.js create mode 100644 test/MemberBuilderValidator/MethodTest.js diff --git a/lib/MemberBuilder.js b/lib/MemberBuilder.js index f7891d3..20f032a 100644 --- a/lib/MemberBuilder.js +++ b/lib/MemberBuilder.js @@ -30,7 +30,9 @@ var util = require( __dirname + '/util' ), Warning = require( __dirname + '/warn' ).Warning, - visibility = [ 'public', 'protected', 'private' ] + visibility = [ 'public', 'protected', 'private' ], + + validate = require( __dirname + '/MemberBuilderValidator' )() ; @@ -47,6 +49,9 @@ module.exports = function MemberBuilder( wrap_method, wrap_override ) this._wrapMethod = wrap_method; this._wrapOverride = wrap_override; + + // XXX: temporarily tightly coupled + this._validate = validate; }; @@ -108,7 +113,9 @@ exports.buildMethod = function( // ensure that the declaration is valid (keywords make sense, argument // length, etc) - this._validateMethod( keywords, prev_data, prev_keywords, value, name ); + this._validate.validateMethod( + name, value, keywords, prev_data, prev_keywords + ); // we might be overriding an existing method if ( prev ) @@ -147,128 +154,6 @@ exports.buildMethod = function( }; -/** - * Validates a method declaration, ensuring that keywords are valid, overrides - * make sense, etc. - * - * @param {Object.} keywords parsed keywords - * - * @param {Object} prev_data data of member being overridden - * @param {Object} prev_keywords keywords of member being overridden - * @param {*} value property value - * @param {string} name property name - */ -exports._validateMethod = function( - keywords, prev_data, prev_keywords, value, name ) -{ - var prev = ( prev_data ) ? prev_data.member : null; - - if ( keywords[ 'abstract' ] ) - { - // do not permit private abstract methods (doesn't make sense, since - // they cannot be inherited/overridden) - if ( keywords[ 'private' ] ) - { - throw TypeError( - "Method '" + name + "' cannot be both private and abstract" - ); - } - } - - // const doesn't make sense for methods; they're always immutable - if ( keywords[ 'const' ] ) - { - throw TypeError( - "Cannot declare method '" + name + "' as constant; keyword is " + - "redundant" - ); - } - - // virtual static does not make sense, as static methods cannot be - // overridden - if ( keywords[ 'virtual' ] && ( keywords[ 'static' ] ) ) - { - throw TypeError( - "Cannot declare static method '" + name + "' as virtual" - ); - } - - // do not allow overriding getters/setters - if ( prev_data && ( prev_data.get || prev_data.set ) ) - { - throw TypeError( - "Cannot override getter/setter '" + name + "' with method" - ); - } - - // search for any previous instances of this member - if ( prev ) - { - // disallow overriding properties with methods - if ( !( prev instanceof Function ) ) - { - throw TypeError( - "Cannot override property '" + name + "' with method" - ); - } - - // disallow overriding non-virtual methods - if ( keywords[ 'override' ] && !( prev_keywords[ 'virtual' ] ) ) - { - throw TypeError( - "Cannot override non-virtual method '" + name + "'" - ); - } - - // do not allow overriding concrete methods with abstract - if ( keywords[ 'abstract' ] && !( prev_keywords[ 'abstract' ] ) ) - { - throw TypeError( - "Cannot override concrete method '" + name + "' with " + - "abstract method" - ); - } - - // ensure parameter list is at least the length of its supertype - if ( ( value.__length || value.length ) - < ( prev.__length || prev.length ) - ) - { - throw TypeError( - "Declaration of method '" + name + "' must be compatiable " + - "with that of its supertype" - ); - } - - // do not permit visibility deescalation - if ( this._getVisibilityValue( prev_keywords ) < - this._getVisibilityValue( keywords ) - ) - { - throw TypeError( - "Cannot de-escalate visibility of method '" + name + "'" - ); - } - - // if redefining a method that has already been implemented in the - // supertype, the default behavior is to "hide" the method of the - // supertype, unless otherwise specified - // - // IMPORTANT: do this last, to ensure we throw errors before warnings - if ( !( keywords[ 'new' ] || keywords[ 'override' ] ) ) - { - if ( !( prev_keywords[ 'abstract' ] ) ) - { - throw Warning( Error( - "Hiding method '" + name + "'; " + - "use 'new' if intended, or 'override' to override instead" - ) ); - } - } - } -} - - /** * 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 new file mode 100644 index 0000000..a8cc668 --- /dev/null +++ b/lib/MemberBuilderValidator.js @@ -0,0 +1,191 @@ +/** + * Validation rules for members + * + * Copyright (C) 2010 Mike Gerwitz + * + * This file is part of ease.js. + * + * ease.js is free software: you can redistribute it and/or modify it under the + * terms of the GNU Lesser General Public License as published by the Free + * Software Foundation, either version 3 of the License, or (at your option) + * any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License + * for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program. If not, see . + * + * @author Mike Gerwitz + * @package core + */ + +// XXX: remove dependency +var Warning = require( __dirname + '/warn' ).Warning; + + +module.exports = exports = function MemberBuilderValidator() +{ + // permit omitting 'new' keyword + if ( !( this instanceof module.exports ) ) + { + return new module.exports(); + } +}; + + +/** + * Validates a method declaration, ensuring that keywords are valid, overrides + * make sense, etc. + * + * Throws exception on validation failure + * + * @param {string} name method name + * @param {*} value method value + * + * @param {Object.} keywords parsed keywords + * + * @param {Object} prev_data data of member being overridden + * @param {Object} prev_keywords keywords of member being overridden + * + * @return {undefined} + */ +exports.prototype.validateMethod = function( + name, value, keywords, prev_data, prev_keywords +) +{ + var prev = ( prev_data ) ? prev_data.member : null; + + if ( keywords[ 'abstract' ] ) + { + // do not permit private abstract methods (doesn't make sense, since + // they cannot be inherited/overridden) + if ( keywords[ 'private' ] ) + { + throw TypeError( + "Method '" + name + "' cannot be both private and abstract" + ); + } + } + + // const doesn't make sense for methods; they're always immutable + if ( keywords[ 'const' ] ) + { + throw TypeError( + "Cannot declare method '" + name + "' as constant; keyword is " + + "redundant" + ); + } + + // virtual static does not make sense, as static methods cannot be + // overridden + if ( keywords[ 'virtual' ] && ( keywords[ 'static' ] ) ) + { + throw TypeError( + "Cannot declare static method '" + name + "' as virtual" + ); + } + + // do not allow overriding getters/setters + if ( prev_data && ( prev_data.get || prev_data.set ) ) + { + throw TypeError( + "Cannot override getter/setter '" + name + "' with method" + ); + } + + // search for any previous instances of this member + if ( prev ) + { + // disallow overriding properties with methods + if ( !( typeof prev === 'function' ) ) + { + throw TypeError( + "Cannot override property '" + name + "' with method" + ); + } + + // disallow overriding non-virtual methods + if ( keywords[ 'override' ] && !( prev_keywords[ 'virtual' ] ) ) + { + throw TypeError( + "Cannot override non-virtual method '" + name + "'" + ); + } + + // do not allow overriding concrete methods with abstract + if ( keywords[ 'abstract' ] && !( prev_keywords[ 'abstract' ] ) ) + { + throw TypeError( + "Cannot override concrete method '" + name + "' with " + + "abstract method" + ); + } + + // ensure parameter list is at least the length of its supertype + if ( ( value.__length || value.length ) + < ( prev.__length || prev.length ) + ) + { + throw TypeError( + "Declaration of method '" + name + "' must be compatible " + + "with that of its supertype" + ); + } + + // do not permit visibility deescalation + if ( this._getVisibilityValue( prev_keywords ) < + this._getVisibilityValue( keywords ) + ) + { + throw TypeError( + "Cannot de-escalate visibility of method '" + name + "'" + ); + } + + // if redefining a method that has already been implemented in the + // supertype, the default behavior is to "hide" the method of the + // supertype, unless otherwise specified + // + // IMPORTANT: do this last, to ensure we throw errors before warnings + if ( !( keywords[ 'new' ] || keywords[ 'override' ] ) ) + { + if ( !( prev_keywords[ 'abstract' ] ) ) + { + throw Warning( Error( + "Hiding method '" + name + "'; " + + "use 'new' if intended, or 'override' to override instead" + ) ); + } + } + } +}; + + +/** + * Return the visibility level as a numeric value, where 0 is public and 2 is + * private + * + * @param {Object} keywords keywords to scan for visibility level + * + * @return {number} visibility level as a numeric value + */ +exports.prototype._getVisibilityValue = function( keywords ) +{ + if ( keywords[ 'protected' ] ) + { + return 1; + } + else if ( keywords[ 'private' ] ) + { + return 2; + } + else + { + // default is public + return 0; + } +} + diff --git a/test/MemberBuilderValidator/MethodTest.js b/test/MemberBuilderValidator/MethodTest.js new file mode 100644 index 0000000..d4413a7 --- /dev/null +++ b/test/MemberBuilderValidator/MethodTest.js @@ -0,0 +1,347 @@ +/** + * Tests member builder validation rules + * + * Copyright (C) 2010 Mike Gerwitz + * + * This file is part of ease.js. + * + * ease.js is free software: you can redistribute it and/or modify it under the + * terms of the GNU Lesser General Public License as published by the Free + * Software Foundation, either version 3 of the License, or (at your option) + * any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License + * for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program. If not, see . + * + * @author Mike Gerwitz + * @package test + */ + +require( 'common' ).testCase( +{ + setUp: function() + { + var _self = this; + + this.sut = this.require( 'MemberBuilderValidator' )(); + + /** + * Tests to ensure that a method with the given keywords fails + * validation with an error message partially matching the provided + * identifier + */ + this.quickKeywordMethodTest = function( keywords, identifier ) + { + var keyword_obj = {}, + name = 'fooBar'; + + // convert our convenient array into a keyword obj + for ( var i = 0, len = keywords.length; i < len; i++ ) + { + keyword_obj[ keywords[ i ] ] = true; + } + + _self.quickFailureTest( name, identifier, function() + { + _self.sut.validateMethod( + name, function() {}, keyword_obj, {}, {} + ); + } ); + }; + + + this.quickFailureTest = function( name, identifier, action ) + { + _self.incAssertCount(); + + try + { + action(); + } + catch ( e ) + { + // using the identifier, ensure the error string makes sense + _self.assertOk( ( e.message.search( identifier ) !== -1 ), + "Incorrect error; expected identifier '" + identifier + + "', but received: " + e.message + ); + + // to aid in debugging, the error message should contain the + // name of the method + _self.assertOk( ( e.message.search( name ) !== -1 ), + 'Error message should contain method name' + ); + + return; + } + + _self.fail( "Expected failure" ); + }; + + + this.quickVisChangeTest = function( start, override, failtest ) + { + var name = 'foo', + + startobj = { 'virtual': true }, + overrideobj = { 'override': true } + ; + + startobj[ start ] = true; + overrideobj[ override ] = true; + + var testfun = function() + { + _self.sut.validateMethod( + name, + function() {}, + overrideobj, + { member: function() {} }, + startobj + ); + }; + + if ( failtest ) + { + this.quickFailureTest( name, 'de-escalate', testfun ); + } + else + { + _self.assertDoesNotThrow( testfun, Error ); + } + }; + }, + + + /** + * Private, abstract methods do not make sense. Private methods cannot be + * overridden. + */ + 'Method cannot be both private and abstract': function() + { + this.quickKeywordMethodTest( [ 'private', 'abstract' ], + 'private and abstract' + ); + }, + + + /** + * Methods (in terms of a class) are always immutable. As such, `const' + * would be redundant. + */ + 'Methods cannot be declared const': function() + { + this.quickKeywordMethodTest( [ 'const' ], 'const' ); + }, + + + /** + * Virtual static methods do not make sense because static methods can only + * be hidden, not overridden. + */ + 'Method cannot be both virtual and static': function() + { + this.quickKeywordMethodTest( [ 'virtual', 'static' ], 'static' ); + }, + + + /** + * Getters/setters are treated as properties and should not be able to be + * overridden with methods. + */ + 'Cannot override getter/setter with method': function() + { + var name = 'foo', + _self = this; + + // test getter + this.quickFailureTest( name, 'getter/setter', function() + { + _self.sut.validateMethod( + name, function() {}, {}, + { get: function() {} }, + {} + ); + } ); + + // test setter + this.quickFailureTest( name, 'getter/setter', function() + { + _self.sut.validateMethod( + name, function() {}, {}, + { set: function() {} }, + {} + ); + } ); + }, + + + /** + * Although a function can certainly be assigned to a property, we cannot + * allow /declaring/ a method in place of a parent property, as that alters + * the interface. + */ + 'Cannot override property with method': function() + { + var name = 'foo', + _self = this; + + this.quickFailureTest( name, 'property', function() + { + // attempt to override a property + _self.sut.validateMethod( + name, function() {}, {}, + { member: 'immaprop' }, + {} + ); + } ); + }, + + + /** + * The `virtual' keyword denotes a method that may be overridden. Without + * it, we should not allow overriding. + */ + 'Cannot override non-virtual methods': function() + { + var name = 'foo', + _self = this; + + this.quickFailureTest( name, 'non-virtual', function() + { + // override provided, but not 'virtual' + _self.sut.validateMethod( + name, + function() {}, + { override: true }, + { member: function() {} }, + {} + ); + } ); + }, + + + /** + * Abstract methods act as a sort of placeholder, requiring an + * implementation. Once an implementation has been defined, it does not make + * sense (in the context of inheritance) to remove it entirely by reverting + * back to an abstract method. + */ + 'Cannot override concrete method with abstract method': function() + { + var name = 'foo', + _self = this; + + this.quickFailureTest( name, 'concrete', function() + { + // attempting to override concrete (non-abstract) method + _self.sut.validateMethod( + name, + function() {}, + { 'abstract': true }, + { member: function() {} }, + {} + ); + } ); + }, + + + /** + * The parameter list is part of the class interface. Changing the length + * will make the interface incompatible with that of its parent and make + * polymorphism difficult. However, since all parameters in JS are + * technically optional, we can permit extending the parameter list (which + * itself has its dangers since the compiler cannot detect type errors). + */ + 'Override parameter list must match or exceed parent length': function() + { + var name = 'foo', + _self = this; + + // check with parent with three params + this.quickFailureTest( name, 'compatible', function() + { + _self.sut.validateMethod( + name, + function() {}, + { 'override': true }, + { member: function( a, b, c ) {} }, + { 'virtual': true } + ); + } ); + + // also check with __length property (XXX: testing too closely to the + // implementation; provide abstraction) + this.quickFailureTest( name, 'compatible', function() + { + var parent_method = function() {}; + parent_method.__length = 3; + + _self.sut.validateMethod( + name, + function() {}, + { 'override': true }, + { member: parent_method }, + { 'virtual': true } + ); + } ); + + // finally, check __length of override will actually work (no error) + this.assertDoesNotThrow( function() + { + var method = function() {}; + method.__length = 3; + + _self.sut.validateMethod( + name, + method, + { 'override': true }, + { member: function( a, b, c ) {} }, + { 'virtual': true } + ); + }, Error ); + }, + + + /** + * One should not be able to, for example, declare a private method it had + * previously been declared protected, or declare it as protected if it has + * previously been declared public. Again - the reason being interface + * consistency. Otherwise the concept of polymorphism doesn't work. + */ + 'Methods do not support visibiliy de-escalation': function() + { + this.quickVisChangeTest( 'public', 'protected', true ); + this.quickVisChangeTest( 'protected', 'private', true ); + }, + + + /** + * To ensure we don't have a bug in our validation, let's also test the + * reverse - ensure that we support escalation and staying at the same + * level. + */ + 'Methods support visibility escalation or equality': function() + { + var tests = [ + [ 'private', 'protected' ], + [ 'protected', 'public' ], + + [ 'public', 'public' ], + [ 'protected', 'protected' ], + [ 'private', 'private' ] + ]; + + for ( var i = 0, len = tests.length; i < len; i++ ) + { + var cur = tests[ i ]; + this.quickVisChangeTest( cur[ 0 ], cur[ 1 ], false ); + } + }, +} ); +