Don't call dinvoke on Object methods

Don't call dinvoke on Object methods

There is the separate, but related question of how to invoke Object
methods when the receiver is Object or a JS primitive.  That should be
handled in the JS backend.  I can tackle that next if no one else is.

See #98.

R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/1b48ba1ea1d61a199c9ef8baf0413e4706011391

[vsm, 2015-04-02 17:59:05]

vsm@google.com changed reviewers:
+ jacobr@google.com, jmesserly@google.com

[Jacob, 2015-04-02 18:29:49]

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:526: name != 'hashCode' &&
Add TODO: get all object properties from the AST.

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:540: // Special case Object.toString().  This is
always safe to invoke.
add a TODO to lookup all methods on Object instead of hardcoding.

[Jennifer Messerly, 2015-04-02 18:56:53]

so, JS objects aren't core.Object. If we want hashCode/runtimeType/toString/nSM
to work on JS objects, how do we support that? I think dart2js makes these work
(e.g. htmlElement.hashCode ... see code in js_helper). it seems like Jacob's
pattern for List would work here.

You're right that it's strange we were handling "dynamic" and "Object"
differently... I think this stems from resolver, treating "obj.toString()"
differently depending on whether the static type of obj is "dynamic" or
"Object".

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:540: // Special case Object.toString().  This is
always safe to invoke.
On 2015/04/02 18:29:49, Jacob wrote:
> add a TODO to lookup all methods on Object instead of hardcoding.

Agreed. It's not too hard to look up the methodelement:
   provider.objectType.element.getMethod('toString')

I guess the analyzer has not resolved `call.element` to point at it? A quick
test with:
```dart
    Object foo;
    foo.toString();

    var bar;
    bar.toString();
```

"go to definition" for the former will jump to Object.toString(), for the latter
it says "no definition to go to"

I wonder if we should implement this at resolution time. It feels a bit off to
add a special cases like this to isDynamicCall. (also, instead of isDynamicCall
we should probably just check if resolution failed to find an element. That
means dynamic. And it's simpler.)

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:541: if (call is SimpleIdentifier && call.name ==
'toString') {
also, what about noSuchMethod? :)

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:544: if (parent is MethodInvocation &&
getting the parent here feels a bit odd. we have to do it in the codegen
sometimes (no way to pass that down) but here we could just pass in the
MethodInvocation, or move this logic into visitMethodInvocation.

also: would this work for tear offs?

var f = obj.toString;
f();

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:547: // Verify that the target is an object (and not
a library prefix).
this can't happen right? calls to library prefixes are never dynamic... they're
statically resolved top level methods.

[vsm, 2015-04-02 19:21:56]

Thanks - I need to iterate on this a little more, but replies....

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:540: // Special case Object.toString().  This is
always safe to invoke.
On 2015/04/02 18:56:53, John Messerly wrote:
> On 2015/04/02 18:29:49, Jacob wrote:
> > add a TODO to lookup all methods on Object instead of hardcoding.
> 
> Agreed. It's not too hard to look up the methodelement:
>    provider.objectType.element.getMethod('toString')
>

Good point.  I'll add the lookup.
 
> I guess the analyzer has not resolved `call.element` to point at it? A quick
> test with:
> ```dart
>     Object foo;
>     foo.toString();
> 
>     var bar;
>     bar.toString();
> ```
> 
> "go to definition" for the former will jump to Object.toString(), for the
latter
> it says "no definition to go to"
> 
> I wonder if we should implement this at resolution time. It feels a bit off to
> add a special cases like this to isDynamicCall. (also, instead of
isDynamicCall
> we should probably just check if resolution failed to find an element. That
> means dynamic. And it's simpler.)

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:541: if (call is SimpleIdentifier && call.name ==
'toString') {
On 2015/04/02 18:56:53, John Messerly wrote:
> also, what about noSuchMethod? :)

We need the dinvoke to verify the type of the argument.  Actually, something
lightweight would do - but we  need something.

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:544: if (parent is MethodInvocation &&
On 2015/04/02 18:56:53, John Messerly wrote:
> getting the parent here feels a bit odd. we have to do it in the codegen
> sometimes (no way to pass that down) but here we could just pass in the
> MethodInvocation, or move this logic into visitMethodInvocation.
> 
> also: would this work for tear offs?
> 
> var f = obj.toString;
> f();

Interesting thought.

If we infer that f is ()->*, we won't need the dinvoke.  If we only infer that f
is dynamic or Function, we need it. 

Perhaps we should be handling this earlier.

https://codereview.chromium.org/1055923002/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:547: // Verify that the target is an object (and not
a library prefix).
On 2015/04/02 18:56:53, John Messerly wrote:
> this can't happen right? calls to library prefixes are never dynamic...
they're
> statically resolved top level methods.

It can ... see toString in my test.

[vsm, 2015-04-07 18:32:35]

vsm@google.com changed reviewers:
+ sigmund@google.com

[vsm, 2015-04-07 18:32:35]

PTAL

I moved this logic to the resolver.  I'm conservative about rejecting the result
of an Object access so we don't reject something like:

class A {
  String toString([int x]) => $x;
}
...
dynamic a = new A();
var toString = a.toString;
toString(42);

[Jennifer Messerly, 2015-04-07 19:05:50]

At a high level this new version is really nice. Just a few comments regarding
some of the details.

https://codereview.chromium.org/1055923002/diff/80001/lib/src/checker/resolve...
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1055923002/diff/80001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:706: return !_nonExtendableTypes.contains(t);
whenever you're calling _isExtendable it's being inverted, and it's inverted
here. Maybe try these renames:


_isExtendable -> _isSealed (or _isFinalType)
_nonExtendableTypes -> _sealedTypes (or _finalTypes)

https://codereview.chromium.org/1055923002/diff/80001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:720: if (target is SimpleIdentifier &&
why does target need to be a SimpleIdentifier? does the code handle this case:

dynamic foo() { ... }

main() {
  print(foo().toString());
}

https://codereview.chromium.org/1055923002/diff/80001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:721: target.staticElement is! PrefixElement) {
follow up to above suggestion, I wonder if something like this would help:

/// Returns true if the expression is `dynamic`.
/// This excludes library prefixes, which pretend to be `dynamic` but really are
not.
bool _isDynamicExpression(Expression e) {
  return e.staticType.isDynamic && !(e is SimpleIdentifier && e.staticElement is
PrefixElement);
}


it seems like we're mostly trying to avoid the fake-dynamic for library
prefixes, would that address the issue?

https://codereview.chromium.org/1055923002/diff/80001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:727: // it may be too strict.
can this actually happen? I thought we didn't allow loosening the return type.

class Derived extends Object {
  Object get toString() => 42; // I hope this is an error?
}


dynamic d = ...;
var x = d.toString(); // we can infer `x` is a String, right?

https://codereview.chromium.org/1055923002/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1055923002/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1869: if (rules.isDynamicGet(target, name)) {
ideally we'd only need to check one of these. Or alternatively, we could check
for PrefixElement before calling? Just trying to brainstorm a way out of this
library-prefix-is-dynamic madness. It's ironic one of the most statically known
things in the system is treated as if it's the most dynamic =)

[vsm, 2015-04-07 19:48:47]

Thanks, PTAL

https://codereview.chromium.org/1055923002/diff/80001/lib/src/checker/resolve...
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1055923002/diff/80001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:706: return !_nonExtendableTypes.contains(t);
On 2015/04/07 19:05:49, John Messerly wrote:
> whenever you're calling _isExtendable it's being inverted, and it's inverted
> here. Maybe try these renames:
> 
> 
> _isExtendable -> _isSealed (or _isFinalType)
> _nonExtendableTypes -> _sealedTypes (or _finalTypes)

Done.

https://codereview.chromium.org/1055923002/diff/80001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:720: if (target is SimpleIdentifier &&
On 2015/04/07 19:05:49, John Messerly wrote:
> why does target need to be a SimpleIdentifier? does the code handle this case:
> 
> dynamic foo() { ... }
> 
> main() {
>   print(foo().toString());
> }

Good point.  Fixed and test added.

https://codereview.chromium.org/1055923002/diff/80001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:721: target.staticElement is! PrefixElement) {
On 2015/04/07 19:05:49, John Messerly wrote:
> follow up to above suggestion, I wonder if something like this would help:
> 
> /// Returns true if the expression is `dynamic`.
> /// This excludes library prefixes, which pretend to be `dynamic` but really
are
> not.
> bool _isDynamicExpression(Expression e) {
>   return e.staticType.isDynamic && !(e is SimpleIdentifier && e.staticElement
is
> PrefixElement);
> }
> 
> 
> it seems like we're mostly trying to avoid the fake-dynamic for library
> prefixes, would that address the issue?

It cleans it up.  Done.

https://codereview.chromium.org/1055923002/diff/80001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:727: // it may be too strict.
On 2015/04/07 19:05:49, John Messerly wrote:
> can this actually happen? I thought we didn't allow loosening the return type.
> 
> class Derived extends Object {
>   Object get toString() => 42; // I hope this is an error?
> }
> 
> 
> dynamic d = ...;
> var x = d.toString(); // we can infer `x` is a String, right?
> 
> 

Note: my comment was in a confusing place - fixed it.

Your case is fine.  This is problematic:

class Derived extends Object {
  String toString([int x]) => '$x';
}

dynamic d = ...
var x = d.toString;
var y = x(42);

If we infer x as ()->String, we produce a static error on the above even though
it'd work at runtime.

https://codereview.chromium.org/1055923002/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1055923002/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1869: if (rules.isDynamicGet(target, name)) {
On 2015/04/07 19:05:49, John Messerly wrote:
> ideally we'd only need to check one of these. Or alternatively, we could check
> for PrefixElement before calling? Just trying to brainstorm a way out of this
> library-prefix-is-dynamic madness. It's ironic one of the most statically
known
> things in the system is treated as if it's the most dynamic =)

This particular one doesn't bother me.  A get is a based on the target and the
property, so it makes sense to pass in both.  Note, isDynamicGet doesn't check
for PrefixElement.  That logic moved to the resolver.

For that in general, perhaps the cleanest solution is to replace
PrefixedIdentifier with PropertyAccess in the AST once resolved?

[Jennifer Messerly, 2015-04-07 21:49:23]

Forgot upload maybe? Anyway your comments sound good, so LGTM based on that :)

https://codereview.chromium.org/1055923002/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1055923002/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1869: if (rules.isDynamicGet(target, name)) {
On 2015/04/07 19:48:46, vsm wrote:
> This particular one doesn't bother me.  A get is a based on the target and the
> property, so it makes sense to pass in both. 

yeah, that's a good point. I just would expect that to be handled by resolution.
Resolution combines target+name and figures out if it could resolve the thing or
not. Whenever we need to do resolution-y stuff in js_codegen it makes me wonder
if it's in the right place or not.

Maybe we could set the static type of "target" to be Object so it doesn't match
"dynamic" anymore? Since we did bind it to an Object method.

> For that in general, perhaps the cleanest solution is to replace
> PrefixedIdentifier with PropertyAccess in the AST once resolved?

yeah, that's a good idea.

[vsm, 2015-04-07 22:40:41]

Hmm, I probably just forgot to upload.  :-)

Anyway, updated now with a little more cleanup.

PTAL

https://codereview.chromium.org/1055923002/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1055923002/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1869: if (rules.isDynamicGet(target, name)) {
On 2015/04/07 21:49:23, John Messerly wrote:
> On 2015/04/07 19:48:46, vsm wrote:
> > This particular one doesn't bother me.  A get is a based on the target and
the
> > property, so it makes sense to pass in both. 
> 
> yeah, that's a good point. I just would expect that to be handled by
resolution.
> Resolution combines target+name and figures out if it could resolve the thing
or
> not. Whenever we need to do resolution-y stuff in js_codegen it makes me
wonder
> if it's in the right place or not.
> 
> Maybe we could set the static type of "target" to be Object so it doesn't
match
> "dynamic" anymore? Since we did bind it to an Object method.

I went ahead and did this and did some corresponding cleanup.

It does leave us in a weird (?) situation where a and x can have non-fuzzy
static types but a.x is dynamic.  But perhaps that is not too bad.  It means
that a.x doesn't require a dinvoke, but that we do not exactly know the result
type.

> 
> > For that in general, perhaps the cleanest solution is to replace
> > PrefixedIdentifier with PropertyAccess in the AST once resolved?
> 
> yeah, that's a good idea.

[Jennifer Messerly, 2015-04-07 23:00:21]

LGTM

https://codereview.chromium.org/1055923002/diff/120001/lib/src/checker/rules....
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1055923002/diff/120001/lib/src/checker/rules....
lib/src/checker/rules.dart:532: bool isDynamicTarget(Expression target) =>
nice!!!

[vsm, 2015-04-07 23:28:08]

Committed patchset #7 (id:120001) manually as
1b48ba1ea1d61a199c9ef8baf0413e4706011391 (presubmit successful).