[#19] Began implementing method hiding (added warning for implicit hiding)

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,
+    try
-        class_id,
+    {
-        base,
+        buildMembers( props,
-        prop_init,
+            class_id,
-        abstract_methods,
+            base,
-        members,
+            prop_init,
-        static_members,
+            abstract_methods,
-        function( inst )
+            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;

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"
         );

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() {},
         } );

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 <http://www.gnu.org/licenses/>.
+ *
+ * @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();
+

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" );
 } )();