More issues with "+" and other binary operators

[dgrove]

svn r1844

This is along the lines of issue #576.

f() => 77;

main() { print(f() + "88"); }

frog produces "7788", while the VM throws: NoSuchMethodException - receiver: '88' function name: 'addFromInteger' arguments: [77]]  0. Function: 'Object.noSuchMethod' url: 'bootstrap' line:315 col:3  1. Function: 'IntegerImplementation.+' url: 'bootstrap_impl' line:1463 col:32  2. Function: '::.main' url: '/Users/dgrove/repo/dart-bleeding/dart/frog/y.dart' line:4 col:11

You can see similar results with

print(f() & "88") print(f() - "88")

I see a few other cases where frog is pushing onto the JS operator incorrectly. for instance,

print(foo() >> (-1)); // frog: 0, VM: unhandled exception print(foo() << (-1));

In general, these issues are appearing because frog is being overly aggressive in pushing down onto the JS operators.

[dgrove]

Added this to the FrogEditor milestone.

[DartBot]

This comment was originally written by jimhug@google.com

---

Set owner to jimhug@google.com. Added Started label.

[jmesserly]

I suspect there are two issues here:

• we aren't dispatching operators dynamically, even though we should in this case (because return type of "f" is Dynamic)
• we are somehow bypassing the type check code for operators (the " TODO(jimhug): This fails in bad ways when argsCode[1] is not num." in member.dart? -- although we should've converted arguments already by this point, so I'm not sure)

[jmesserly]

FWIW, I think these results can be okay, as long as it properly fails in --enable_type_checks mode.

[DartBot]

This comment was originally written by jimhug@google.com

---

This bug can be easily fixed by doing the dynamic dispatch. The reason we weren't doing that dispatch currently was actually a faulty detection that num was the only type that implemented '+'. This same semi-brittle optimization will continue to work for most of the other operators - but I'm inclined to remove it for now due to the excessive brittleness. FYI - The brittleness is that adding a type in a completely unrelated part of the code that overrides one of these operators could have a surprisingly large impact on performance. With this trivial fix, most benchmarks are unchanged, except that meteor and quicksort each take a 25-33% performance hit. Analyzing that perf hit shows that it is caused by a separate bug in handling of generic types. I plan to try to fix both the generics bug and this one at the same time for a perf neutral change. However, if I can't get that finished this week I'll just check in the simple fix.

[jmesserly]

I'm surprised those benchmarks are doing dynamic dispatch. I thought the benchmarks had type annotations everywhere?

[jmesserly]

Marked this as blocking #595.

[DartBot]

This comment was originally written by jimhug@google.com

---

I am removing the >> and << cases and putting them into a new bug. I will assign the bug to Kasper to decide if it is in this Milestone or not. Frog's current behavior matches dartc's (with good reason) and the precise semantics of this operation in a JS implementation is not well defined.

[DartBot]

This comment was originally written by jimhug@google.com

---

One more comment for the record. I just ran these tests on dartc and it behaves exactly the same as frog for your examples, i.e. they are both broken for this case (unless I'm doing something really stupid this holiday morning). We will fix this in frog for the editor milestone, but it is this kind of discrepancy that makes it hard for me to understand which tests belong in this milestone and which ones don't.

jimhug-macbookpro:frog$ cat t2.dart f() => 77;

main() { print(f() + "88"); print(f() &"88"); print(f() - "88");

} jimhug-macbookpro:frog$ ../xcodebuild/Release_ia32/dartc --out t2.js t2.dart jimhug-macbookpro:frog$../xcodebuild/Release_ia32/d8 t2.js 7788 72 -11

---

cc @kasperl.

[kasperl]

Thanks for investigating this, Jim. It does look like the dartc implementations of the numeric operations fail to check that the right hand side is indeed a number. I'll get this fixed.

[DartBot]

This comment was originally written by jimhug@google.com

---

Fix in r3505.

---

Added Fixed label.