opal · Nov 27, 2014 · Nov 27, 2014 · Nov 27, 2014 · Nov 27, 2014 · Nov 27, 2014
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## edge (upcoming 0.7)
 
+*   Fix donating methods defined in modules. This ensures that if a class includes more than one module, then the methods defined on the class respect the order in which the modules are included.
+
 *   Improved support for recursive `Hash` for both `#inspect` and `#hash`.
 
 *   Optimized `Hash` implementation for `String` and `Symbol`, they have a separate hash-table in which they're used as both keys and hashes.

diff --git a/examples/rack/.gitignore b/examples/rack/.gitignore
@@ -0,0 +1 @@
+tmp
diff --git a/lib/opal/nodes/def.rb b/lib/opal/nodes/def.rb
@@ -99,14 +99,11 @@ def compile
         if recvr
           unshift 'Opal.defs(', recv(recvr), ", '$#{mid}', "
           push ')'
-        elsif scope.class? and %w(Object BasicObject).include?(scope.name)
+        elsif uses_defn?(scope)
           wrap "Opal.defn(self, '$#{mid}', ", ')'
-        elsif scope.class_scope?
-          scope.methods << "$#{mid}"
+        elsif scope.class?
           unshift "#{scope.proto}#{jsid} = "
-        elsif scope.iter?
-          wrap "Opal.defn(self, '$#{mid}', ", ')'
-        elsif scope.type == :sclass
+        elsif scope.sclass?
           unshift "self.$$proto#{jsid} = "
         elsif scope.top?
           unshift "Opal.Object.$$proto#{jsid} = "
@@ -117,6 +114,22 @@ def compile
         wrap '(', ", nil) && '#{mid}'" if expr?
       end
 
+      # Simple helper to check whether this method should be defined through
+      # `Opal.defn()` runtime helper.
+      #
+      # @param [Opal::Scope] scope
+      # @returns [Boolean]
+      #
+      def uses_defn?(scope)
+        if scope.iter? or scope.module?
+          true
+        elsif scope.class? and %w(Object BasicObject).include?(scope.name)
+          true
+        else
+          false
+        end
+      end
+
       # Returns code used in debug mode to check arity of method call
       def arity_check(args, opt, splat, block_name, mid)
         meth = mid.to_s.inspect

diff --git a/lib/opal/nodes/module.rb b/lib/opal/nodes/module.rb
@@ -24,7 +24,6 @@ def compile
 
           line scope.to_vars
           line body_code
-          line scope.to_donate_methods
         end
 
         line "})(", base, ")"

diff --git a/lib/opal/nodes/scope.rb b/lib/opal/nodes/scope.rb
@@ -110,14 +110,6 @@ def proto
         "def"
       end
 
-      # A scope donates its methods if it is a module, or the core Object
-      # class. Modules donate their methods to classes or objects they are
-      # included in. Object donates methods to bridged classes whose native
-      # prototypes do not actually inherit from Opal.Object.prototype.
-      def should_donate?
-        @type == :module
-      end
-
       ##
       # Vars to use inside each scope
       def to_vars
@@ -148,15 +140,6 @@ def to_vars
         fragment(result)
       end
 
-      # Generates code for this module to donate methods
-      def to_donate_methods
-        if should_donate? and !@methods.empty?
-          fragment("%s;Opal.donate(self, [%s]);" % [@compiler.parser_indent, @methods.map(&:inspect).join(', ')])
-        else
-          fragment("")
-        end
-      end
-
       def add_scope_ivar(ivar)
         if def_in_class?
           @parent.add_proto_ivar ivar

diff --git a/opal/corelib/module.rb b/opal/corelib/module.rb
@@ -47,11 +47,7 @@ def <(other)
 
   def alias_method(newname, oldname)
     %x{
-      self.$$proto['$' + newname] = self.$$proto['$' + oldname];
-
-      if (self.$$methods) {
-        Opal.donate(self, ['$' + newname ])
-      }
+      Opal.defn(self, '$' + newname, self.$$proto['$' + oldname]);
     }
     self
   end
@@ -88,18 +84,16 @@ def attr_accessor(*names)
 
   def attr_reader(*names)
     %x{
-      var proto = self.$$proto, cls = self;
       for (var i = 0, length = names.length; i < length; i++) {
         (function(name) {
-          proto[name] = nil;
+          self.$$proto[name] = nil;
           var func = function() { return this[name] };
 
-          if (cls.$$is_singleton) {
-            proto.constructor.prototype['$' + name] = func;
+          if (self.$$is_singleton) {
+            self.$$proto.constructor.prototype['$' + name] = func;
           }
           else {
-            proto['$' + name] = func;
-            Opal.donate(self, ['$' + name ]);
+            Opal.defn(self, '$' + name, func);
           }
         })(names[i]);
       }
@@ -110,18 +104,16 @@ def attr_reader(*names)
 
   def attr_writer(*names)
     %x{
-      var proto = self.$$proto, cls = self;
       for (var i = 0, length = names.length; i < length; i++) {
         (function(name) {
-          proto[name] = nil;
+          self.$$proto[name] = nil;
           var func = function(value) { return this[name] = value; };
 
-          if (cls.$$is_singleton) {
-            proto.constructor.prototype['$' + name + '='] = func;
+          if (self.$$is_singleton) {
+            self.$$proto.constructor.prototype['$' + name + '='] = func;
           }
           else {
-            proto['$' + name + '='] = func;
-            Opal.donate(self, ['$' + name + '=']);
+            Opal.defn(self, '$' + name + '=', func);
           }
         })(names[i]);
       }
@@ -241,8 +233,12 @@ def define_method(name, method = undefined, &block)
       block.$$s    = null;
       block.$$def  = block;
 
-      self.$$proto[jsid] = block;
-      Opal.donate(self, [jsid]);
+      if (self.$$is_singleton) {
+        self.$$proto[jsid] = block;
+      }
+      else {
+        Opal.defn(self, jsid, block);
+      }
 
       return name;
     }

diff --git a/opal/corelib/runtime.js b/opal/corelib/runtime.js
@@ -148,7 +148,7 @@
 
       // Copy all parent constants to child, unless parent is Object
       if (superklass !== ObjectClass && superklass !== BasicObjectClass) {
-        Opal.donate_constants(superklass, klass);
+        donate_constants(superklass, klass);
       }
 
       // call .inherited() hook with new class on the superclass
@@ -358,9 +358,25 @@
     return object.$$meta = meta;
   }
 
-  /*
-   * The actual inclusion of a module into a class.
-   */
+  /**
+    The actual inclusion of a module into a class.
+
+    ## Class `$$parent` and `iclass`
+
+    To handle `super` calls, every class has a `$$parent`. This parent is
+    used to resolve the next class for a super call. A normal class would
+    have this point to its superclass. However, if a class includes a module
+    then this would need to take into account the module. The module would
+    also have to then point its `$$parent` to the actual superclass. We
+    cannot modify modules like this, because it might be included in more
+    then one class. To fix this, we actually insert an `iclass` as the class'
+    `$$parent` which can then point to the superclass. The `iclass` acts as
+    a proxy to the actual module, so the `super` chain can then search it for
+    the required method.
+
+    @param [RubyModule] module the module to include
+    @param [RubyClass] klass the target class to include module into
+  */
   Opal.append_features = function(module, klass) {
     var included = klass.$$inc;
 
@@ -406,10 +422,10 @@
     }
 
     if (klass.$$dep) {
-      Opal.donate(klass, methods.slice(), true);
+      donate_methods(klass, methods.slice(), true);
     }
 
-    Opal.donate_constants(module, klass);
+    donate_constants(module, klass);
   };
 
   // Boot a base class (makes instances).
@@ -557,7 +573,7 @@
    * When a source module is included into the target module, we must also copy
    * its constants to the target.
    */
-  Opal.donate_constants = function(source_mod, target_mod) {
+  function donate_constants(source_mod, target_mod) {
     var source_constants = source_mod.$$scope.constants,
         target_scope     = target_mod.$$scope,
         target_constants = target_scope.constants;
@@ -885,7 +901,7 @@
   /*
    * Donate methods for a class/module
    */
-  Opal.donate = function(klass, defined, indirect) {
+  function donate_methods(klass, defined, indirect) {
     var methods = klass.$$methods, included_in = klass.$$dep;
 
     // if (!indirect) {
@@ -905,29 +921,129 @@
         }
 
         if (includee.$$dep) {
-          Opal.donate(includee, defined, true);
+          donate_methods(includee, defined, true);
         }
       }
     }
   };
 
-  Opal.defn = function(obj, jsid, body) {
-    if (obj.$$is_mod) {
-      obj.$$proto[jsid] = body;
-      Opal.donate(obj, [jsid]);
+  /**
+    Define the given method on the module.
+
+    This also handles donating methods to all classes that include this
+    module. Method conflicts are also handled here, where a class might already
+    have defined a method of the same name, or another included module defined
+    the same method.
+
+    @param [RubyModule] module the module method defined on
+    @param [String] jsid javascript friendly method name (e.g. "$foo")
+    @param [Function] body method body of actual function
+  */
+  function define_module_method(module, jsid, body) {
+    module.$$proto[jsid] = body;
+    body.$$owner = module;
+
+    module.$$methods.push(jsid);
+
+    if (module.$$module_function) {
+      module[jsid] = body;
+    }
+
+    var included_in = module.$$dep;
+
+    if (included_in) {
+      for (var i = 0, length = included_in.length; i < length; i++) {
+        var includee = included_in[i];
+        var dest = includee.$$proto;
+        var current = dest[jsid];
 
-      if (obj.$$module_function) {
-        obj[jsid] = body;
+
+        if (dest.hasOwnProperty(jsid) && !current.$$donated && !current.$$stub) {
+          // target class has already defined the same method name - do nothing
+        }
+        else if (dest.hasOwnProperty(jsid) && !current.$$stub) {
+          // target class includes another module that has defined this method
+          var klass_includees = includee.$$inc;
+
+          for (var j = 0, jj = klass_includees.length; j < jj; j++) {
+            if (klass_includees[j] === current.$$owner) {
+              var current_owner_index = j;
+            }
+            if (klass_includees[j] === module) {
+              var module_index = j;
+            }
+          }
+
+          // only redefine method on class if the module was included AFTER
+          // the module which defined the current method body. Also make sure
+          // a module can overwrite a method it defined before
+          if (current_owner_index <= module_index) {
+            dest[jsid] = body;
+            dest[jsid].$$donated = true;
+          }
+        }
+        else {
+          // neither a class, or module included by class, has defined method
+          dest[jsid] = body;
+          dest[jsid].$$donated = true;
+        }
+
+        if (includee.$$dep) {
+          donate_methods(includee, [jsid], true);
+        }
       }
     }
+  }
+
+  /**
+    Used to define methods on an object. This is a helper method, used by the
+    compiled source to define methods on special case objects when the compiler
+    can not determine the destination object, or the object is a Module
+    instance. This can get called by `Module#define_method` as well.
+
+    ## Modules
+
+    Any method defined on a module will come through this runtime helper.
+    The method is added to the module body, and the owner of the method is
+    set to be the module itself. This is used later when choosing which
+    method should show on a class if more than 1 included modules define
+    the same method. Finally, if the module is in `module_function` mode,
+    then the method is also defined onto the module itself.
+
+    ## Classes
+
+    This helper will only be called for classes when a method is being
+    defined indirectly; either through `Module#define_method`, or by a
+    literal `def` method inside an `instance_eval` or `class_eval` body. In
+    either case, the method is simply added to the class' prototype. A special
+    exception exists for `BasicObject` and `Object`. These two classes are
+    special because they are used in toll-free bridged classes. In each of
+    these two cases, extra work is required to define the methods on toll-free
+    bridged class' prototypes as well.
+
+    ## Objects
+
+    If a simple ruby object is the object, then the method is simply just
+    defined on the object as a singleton method. This would be the case when
+    a method is defined inside an `instance_eval` block.
+
+    @param [RubyObject or Class] obj the actual obj to define method for
+    @param [String] jsid the javascript friendly method name (e.g. '$foo')
+    @param [Function] body the literal javascript function used as method
+    @returns [null]
+  */
+  Opal.defn = function(obj, jsid, body) {
+    if (obj.$$is_mod) {
+      define_module_method(obj, jsid, body);
+    }
     else if (obj.$$is_class) {
       obj.$$proto[jsid] = body;
 
       if (obj === BasicObjectClass) {
         define_basic_object_method(jsid, body);
       }
       else if (obj === ObjectClass) {
-        Opal.donate(obj, [jsid]);
+        donate_methods(obj, [jsid]);
       }
     }
     else {

diff --git a/spec/filters/bugs/module.rb b/spec/filters/bugs/module.rb
@@ -24,4 +24,5 @@
   fails "Module#module_function with specific method names can make accessible private methods"
   fails "Module#module_function as a toggle (no arguments) in a Module body does not affect module_evaled method definitions also if outside the eval itself"
   fails "Module#module_function as a toggle (no arguments) in a Module body has no effect if inside a module_eval if the definitions are outside of it"
+  fails "Module#module_function with specific method names creates an independent copy of the method, not a redirect"
 end