dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.21k stars 1.57k forks source link

DDK failure on dsend #33293

Open kmillikin opened 6 years ago

kmillikin commented 6 years ago

I need some help figuring out what's going on here. I've tried to fix #32414 in the Dart front end. If we look at this from tests/language_2/assert_with_message_test.dart:

testAssertFails() {
  try {
    assert(false, "Oops");
    Expect.fail("Assert should throw.");
  } catch (e) {
    Expect.isTrue(e.toString().contains("Oops"));
  }
}

We had in Kernel before:

  on dynamic catch(final dynamic e) {
    exp::Expect::isTrue(e.toString().contains("Oops"));
  }

and now will have

  on dynamic catch(final dynamic e) {
    exp::Expect::isTrue(e.toString().{core::String::contains}("Oops"));
  }

DDK generated before:

    } catch (e) {
      expect$.Expect.isTrue(dart.dsend(dart.dsend(e, $toString, []), 'contains', ["Oops"]));
    }

and now will generate:

    } catch (e) {
      expect$.Expect.isTrue(dart.dsend(e, $toString, []).contains("Oops"));
    }

However, the new code fails with

Runtime error:
TypeError: dart.dsend(...).contains is not a function
at Object.assert_with_message_test.testAssertFails

Why?

jmesserly commented 6 years ago

My guess about what's going on: it's generating a typed call against a dynamically typed target:

exp::Expect::isTrue(e.toString().{core::String::contains}("Oops"));

I'm guessing the receiver e.toString() has type dynamic?

If so: DDK does not expect that combination. If the receiver type is only known to be dynamic, how can you call a method on String soundly? So it kinda doesn't make sense from a typing perspective. The fix in CFE would be to have e.toString() getStaticType return core::String. We could try to workaround this in DDK. But it would be better if the static types were consistent in the IR.

jmesserly commented 6 years ago

Confirmed the receiver type is dynamic:

--- a/pkg/dev_compiler/lib/src/kernel/compiler.dart
+++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart
@@ -3756,6 +3756,7 @@ class ProgramCompiler extends Object
     var jsReceiver = _visitExpression(receiver);
     var args = _emitArgumentList(arguments);
     var receiverType = receiver.getStaticType(types);
+    print('!!! receiver: `$receiver`, receiverType: $receiverType, target: $target');

     bool isCallingDynamicField = target is Member &&
         target.hasGetter &&

prints

!!! receiver: `e`, receiverType: dynamic, target: null
!!! receiver: `e.toString()`, receiverType: dynamic, target: dart.core::String::contains

I believe this is a CFE bug, because the spec says the static type of e.toString() should be String.

Also I realized dev compiler can't work around this in general. There are a bunch of places it uses getStaticType, and a target is not always available to carry the type info. For example: e.toString() as String ... we are unable to eliminate the cast because we think e.toString() has type dynamic.

jmesserly commented 6 years ago

Reassigning this to CFE, let me know if that seems right to you. I may be able to work around it in the meantime for some kinds of expressions. Interestingly type inference seems to understand the real type:

main() {
  Type f<T>(T t) => T;
  dynamic e = 'hi';
  print(f(e.toString())); // prints String
}

so at some point CFE is aware that e.toString() has type core::String ... perhaps it is just not getting updated on the kernel node?

jmesserly commented 6 years ago

here's a CL that works around most of the issues: https://dart-review.googlesource.com/c/sdk/+/57710

BTW, it would be helpful if Kernel output included the target for the e.toString() call:

  on dynamic catch(final dynamic e) {
    exp::Expect::isTrue(e.{core::Object::toString}().{core::String::contains}("Oops"));
  }

Since the target (core::Object::toString) was found and used for type inference, it would be nice if the target was recorded.

kmillikin commented 6 years ago

Thanks for taking a look.

The front end bug, which is the missing interface target, will be fixed by https://dart-review.googlesource.com/c/sdk/+/57820.

I'll fix the Kernel type system bug, which gives the wrong static type, separately.

kmillikin commented 6 years ago

The front-end bug was fixed in 6ac6baae8e5d65f8c619faa91b7e6cc4184750ff.

Kernel's type checker hasn't been fixed.