From db18a61d3094f2d527803c17c68fe9bf2c552ebe Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Wed, 6 Jul 2011 19:34:35 -0400 Subject: [PATCH] [#19] Began implementing method hiding (added warning for implicit hiding) --- lib/class_builder.js | 38 ++++++++--- lib/member_builder.js | 27 ++++---- test/test-class-visibility.js | 6 +- test/test-class_builder-method-hiding.js | 80 ++++++++++++++++++++++++ test/test-member_builder-method.js | 52 ++++----------- 5 files changed, 139 insertions(+), 64 deletions(-) create mode 100644 test/test-class_builder-method-hiding.js diff --git a/lib/class_builder.js b/lib/class_builder.js index 5a63dc1..73ab67b 100644 --- a/lib/class_builder.js +++ b/lib/class_builder.js @@ -28,9 +28,12 @@ */ var util = require( __dirname + '/util' ), + warn = require( __dirname + '/warn' ), member_builder = require( __dirname + '/member_builder' ), propobj = require( __dirname + '/propobj' ), + Warning = warn.Warning, + /** * Class id counter, to be increment on each new definition * @type {number} @@ -236,18 +239,33 @@ exports.build = function extend() // build the various class components (xxx: this is temporary; needs // refactoring) - buildMembers( props, - class_id, - base, - prop_init, - abstract_methods, - members, - static_members, - function( inst ) + try + { + buildMembers( props, + class_id, + base, + prop_init, + abstract_methods, + members, + static_members, + function( inst ) + { + return new_class.___$$svis$$; + } + ); + } + catch ( e ) + { + // intercept warnings /only/ + if ( e instanceof Warning ) { - return new_class.___$$svis$$; + warn.handle( e ); } - ); + else + { + throw e; + } + } // reference to the parent prototype (for more experienced users) prototype.___$$parent$$ = base.prototype; diff --git a/lib/member_builder.js b/lib/member_builder.js index 7653769..e79c001 100644 --- a/lib/member_builder.js +++ b/lib/member_builder.js @@ -24,6 +24,8 @@ var util = require( __dirname + '/util' ), + Warning = require( __dirname + '/warn' ).Warning, + fallback = util.definePropertyFallback(), visibility = [ 'public', 'protected', 'private' ]; @@ -173,15 +175,6 @@ function validateMethod( keywords, prev_data, value, name ) ); } - // Can only override virtual methods. Abstract methods are considered to - // be virtual. - if ( !( prev_keywords[ 'virtual' ] || prev_keywords[ 'abstract' ] ) ) - { - throw TypeError( - "Cannot override non-virtual method '" + name + "'" - ); - } - // do not allow overriding concrete methods with abstract if ( keywords[ 'abstract' ] && !( util.isAbstractMethod( prev ) ) ) { @@ -202,13 +195,25 @@ function validateMethod( keywords, prev_data, value, name ) ); } - // do not permit visibility de-escalation + // do not permit visibility deescalation if ( prev_data.visibility < getVisibilityValue( keywords ) ) { throw TypeError( "Cannot de-escalate visibility of method '" + name + "'" ); } + + // if redefining non-virtual method, the new method will "hide" the + // parent's + // + // IMPORTANT: do this last, to ensure we throw errors before warnings + if ( !( prev_keywords[ 'virtual' ] || prev_keywords[ 'abstract' ] ) ) + { + throw Warning( Error( + "Hiding method '" + name + "'; " + + "use 'new' if intended, or 'override' to override instead" + ) ); + } } } @@ -246,7 +251,7 @@ exports.buildProp = function( members, meta, name, value, keywords, base ) // do not allow overriding getters/setters if ( prev_data && ( prev_data.get || prev_data.set ) ) - { + { throw TypeError( "Cannot override getter/setter '" + name + "' with property" ); diff --git a/test/test-class-visibility.js b/test/test-class-visibility.js index c0d7ddf..f15100a 100644 --- a/test/test-class-visibility.js +++ b/test/test-class-visibility.js @@ -559,7 +559,7 @@ var common = require( './common' ), { Class( { - 'public baz': function() {}, + 'virtual public baz': function() {}, } ).extend( { 'protected baz': function() {}, } ); @@ -581,7 +581,7 @@ var common = require( './common' ), { Class( { - 'public baz': function() {}, + 'virtual public baz': function() {}, } ).extend( { 'private baz': function() {}, } ); @@ -603,7 +603,7 @@ var common = require( './common' ), { Class( { - 'protected baz': function() {}, + 'virtual protected baz': function() {}, } ).extend( { 'private baz': function() {}, } ); diff --git a/test/test-class_builder-method-hiding.js b/test/test-class_builder-method-hiding.js new file mode 100644 index 0000000..b165447 --- /dev/null +++ b/test/test-class_builder-method-hiding.js @@ -0,0 +1,80 @@ +/** + * Tests hidden methods + * + * 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 + */ + +var common = require( './common' ), + assert = require( 'assert' ), + builder = common.require( 'class_builder' ), + warn = common.require( 'warn' ) +; + + +/** + * Restores warning handler to the default + */ +function restoreWarningHandler() +{ + warn.setHandler( warn.handlers.log ); +} + + +/** + * If a non-virtual method is implicitly hidden (redefined without the 'new' + * keyword), a warning should be provided. This will ensure that, should a + * parent introduce a method that is already defined by a supertype, the + * developer of the subtype is made aware of the issue. + */ +( function testThrowsWarningWhenHidingSuperMethod() +{ + var thrown = false; + + // mock the warning handler to ensure a warning is thrown + warn.setHandler( function( e ) + { + thrown = true; + + assert.ok( + ( e.message.search( 'foo' ) !== -1 ), + "Method hiding warning should contain method name" + ); + } ); + + var Foo = builder.build( + { + // non-virtual method + 'public foo': function() {}, + } ); + + // hide the non-virtual method + builder.build( Foo, + { + 'public foo': function() {}, + } ); + + assert.equal( thrown, true, "No warning was thrown" ); +} )(); + + +// important, otherwise tests in combined file may fail +restoreWarningHandler(); + diff --git a/test/test-member_builder-method.js b/test/test-member_builder-method.js index 30fc6c9..4542485 100644 --- a/test/test-member_builder-method.js +++ b/test/test-member_builder-method.js @@ -26,7 +26,10 @@ var common = require( './common' ), assert = require( 'assert' ), mb_common = require( __dirname + '/inc-member_builder-common' ), builder = common.require( 'member_builder' ), - util = common.require( 'util' ) + util = common.require( 'util' ), + + warn = common.require( 'warn' ), + Warning = warn.Warning ; mb_common.funcVal = 'foobar'; @@ -72,43 +75,6 @@ mb_common.assertCommon(); } )(); -/** - * Unlike Java, PHP, Python and similar languages, methods in ease.js are *not* - * virtual by default. In order to make them override-able, the 'virtual' - * keyword must be specified for that method in the supertype. - * - * Therefore, let's ensure that non-virtual methods cannot be overridden. - */ -( function testCannotOverrideNonVirtualMethod() -{ - mb_common.value = function() {}; - mb_common.buildMemberQuick(); - - try - { - // attempt to override (should throw exception; non-virtual) - mb_common.buildMemberQuick( {}, true ); - } - catch ( e ) - { - // ensure we have the correct error - assert.ok( - e.message.search( 'virtual' ) !== -1, - "Error message for non-virtual override should mention virtual" - ); - - assert.ok( - e.message.search( mb_common.name ) !== -1, - "Method name should be provided in non-virtual error message" - ); - - return; - } - - assert.fail( "Should not be permitted to override non-virtual methods" ); -} )(); - - /** * Working off of what was said in the test directly above, we *should* be able * to override virtual methods. @@ -143,10 +109,16 @@ mb_common.assertCommon(); mb_common.buildMemberQuick( {}, true ); // attempt to override again (should fail) - assert.throws( function() + try { mb_common.buildMemberQuick( {}, true ); - }, TypeError, "Overrides are not declared as virtual by default" ); + } + catch ( e ) + { + return; + } + + assert.fail( "Overrides should not be declared as virtual by default" ); } )();