Super/Aliasing Fix

[wied03]

Fixes #1384. Another weird quirk of the Ruby language (if it's a weird corner, RSpec will find it). It still needs the following:

• Rubyspec built from the anon_mod.rb file done
• This is probably incomplete as it really needs to be something that also fixes the Rubyspec Module#alias_method can call a method with super aliased twice save this for another PR

[wied03]

@meh - any thoughts here?

[elia]

This rings an alarm for me, [body] will convert the function to a string and use that as the key, which means that will use the actual source code of the function to identify it. That, to me, is a problem because:

• two methods in the inheritance chain can have the same implementation this defeating this technique (even if I'm aware it's surely not common)
• I'm not sure how standard is to have Function.prototype.toString() converto the function to its source code, what if the implementation is changed?
• looks like a hack
• could mislead a future me reading this code

Even if I recognize it's practical, I'd rather prefer to have a more robust system, that actually checks a prototype chain for the particular method or alternatively assign an Opal.uid() to each function and use that instead of the function body.

[wied03]

What about the first way/first commit?

sent from my mobile device
On Mar 20, 2016 7:24 PM, "Elia Schito" notifications@github.com wrote:

In opal/corelib/runtime.js
#1385 (comment):

@@ -1347,6 +1356,8 @@
//
Opal.defn = function(obj, jsid, body) {
obj.$$proto[jsid] = body;

• // super dispatcher find, etc. might need to lookup by function body
• obj.$$funcbodies[body] = jsid;

This rings an alarm for me, [body] will convert the function to a string
and use that as the key, which means that will use the actual source code
of the function to identify it. That, to me, is a problem because:

• two methods in the inheritance chain can have the same
  implementation this defeating this technique (even if I'm aware it's surely
  not common)
• I'm not sure how standard is to have Function.prototype.toString()
  converto the function to its source code, what if the implementation is
  changed?
• looks like a hack
• could mislead a future me reading this code

Even if I recognize it's practical, I'd rather prefer to have a more
robust system, that actually checks a prototype chain for the particular
method or alternatively assign an Opal.uid() to each function and use
that instead of the function body.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/opal/opal/pull/1385/files/479badbfb12bc716720a7cb34ffebc7588cca44b#r56777155

[wied03]

rubyspecs - ruby/spec#212

[coveralls]

Coverage remained the same at 87.643% when pulling 37e6cc5 on wied03:super_method_fix into 4964053 on opal:master.

[coveralls]

Coverage remained the same at 87.643% when pulling 159ea8f on wied03:super_method_fix into 4964053 on opal:master.

[wied03]

@elia - Thanks for the earlier feedback. I think with this 2nd commit, I got the nested loop eliminated and have something that can work OK.

[coveralls]

Coverage remained the same at 87.643% when pulling e9cb107 on wied03:super_method_fix into 4964053 on opal:master.

[coveralls]

Coverage increased (+0.09%) to 87.733% when pulling b454d66 on wied03:super_method_fix into 4964053 on opal:master.

[coveralls]

Coverage remained the same at 87.621% when pulling 61d3e2c on wied03:super_method_fix into 0815306 on opal:master.

[elia]

👍 :)