dart2js strong: issue with subtyping checks on js-interop types

[sigmundch]

repro:

@JS()
library foo;

import 'package:js/js.dart';

@JS()
@anonymous
class A<T> {
  external factory A({T foo});
  external T get foo;
}

main() {
  var x = new A<int>(foo: 1);
  print(x is A<int>);
}

This program prints true in legacy mode, but false in strong mode.

(example extracted from testing strong mode in customer apps)

[matanlurey]

/sub: What is the expectation here?

Last time I checked this doesn't work for DDC at all (it causes weird/bad behavior).

/cc @jmesserly

[johnniwinther]

I thought we didn't support generic jsinterop classes. At runtime an A<int> will be a A<dynamic> (actually a JavaScriptObject which implements A<dynamic>) which is not a subtype of A<int> in strong mode.

[sigmundch]

You are correct. I talked with the DDC folks and they also don't reify the generic type arguments: type checks against these terms work as if you are checking against the raw types.

@rakudrama and I brainstormed some ideas:

• option 1: do something special for interop types anywhere we implement type checks. This includes the runtime subtyping logic, but also several optimizations that simplify checks in SSA.
• option 2 (preferred): erase type arguments very early in our compilation pipeline. If we can do it as early as we first generate our DartType terms, then all the existing codegen and checks could work out of the box, our RTI-need analysis can also benefit and not include type parameters that were only required because of a js-interop erased type argument.

Option 1 has a disadvantage that it makes subtyping checks more expensive overall, while option 2 deals with this statically and keeps the runtime as lean as we can.

WDYT?

[jmesserly]

You are correct. I talked with the DDC folks and they also don't reify the generic type arguments: type checks against these terms work as if you are checking against the raw types.

Yeah, it may happen to work. I don't know if JS interop really supports generic classes or if it's an accident of how things are implemented (the RTTI for JS interop classes completely ignores any type parameters/arguments).

FWIW, there's also no static checks in Analyzer to ensure @JS is used correctly: https://github.com/dart-lang/sdk/issues/32929, and DDC has a lot of inconsistencies in how it decides if a class/member/procedure should use JS interop or not. There's a lot of ways it can be used incorrectly, which will be silently ignored or lead to undefined behavior.

[johnniwinther]

I prefer option 2: This is consistent with the current (Dart 1) behavior and the only scheme that we can consistently support at runtime. We should additionally warn about generic jsinterop types and maybe in time ban them.

[johnniwinther]

The problem also occurs if a jsinterop class implements a regular generic class:

class A<T> {}
@JS()
class B implements A<int> {}
main() {
  print(new B() is A<int>); // This prints false regardless of option 1/2 solution.
}

[johnniwinther]

A third option is to add new special RTI value that represents "any" type argument (i.e. the Dart 1 dynamic semantics) and use this for jsinterop uses of generic types. With this we could support a lax semantics of jsinterop generic that makes all cases "pass":

@JS()
class A<T> {}
class B<T> {}
class C implements B<int> {}
main() {
  dynamic a = new A<int>(); // An `A<any>` instance at runtime.
  print(a is A<int>); // Prints true because A<any> <: A<int>
  print(a is A<String>); // Also prints true because A<any> <: A<String>

  dynamic c = new C(); // A `C` instance that implements `B<any>` at runtime.
  print(c is B<int>); // Prints true because B<any> <: B<int>
  print(c is B<String>); // Also prints true because B<any> <: B<String>
}

Note that we're already lax in type-tests on jsinterop classes. In the example above we have

  print(a is C); // Prints true because we can't tell an A from a C at runtime.
  print(c is A); // Prints true because we can't tell a C from an A at runtime.
  print(a is B); // Prints true because we can't tell an A from
                 // a C at runtime so A also implements B.

We should probably warn about tests against jsinterop types.

[sigmundch]

How do you suggest we replace any on interop types? In particular, what happens in indirect generic interfaces such as:

class A<T> {  T get value; }
class B implements A<int> {}
@JS
class C implements B {}

Personally, I'd be OK with option-2 for now and not deal with the interface cases (I'd have to validate this, but I believe they are not used today)

[johnniwinther]

C cannot inherit its A-ness from B so it needs its own is$A and as$A properties. Since C is a jsinterop class these properties will be put on JavaScriptObject.

[johnniwinther]

I think option 3 is actually easier to implement than option 2.

[jmesserly]

Is implementing a Dart interface with a JS interop class actually supported? It will not work in DDC in general (simple examples may happen to work, but there are lots of ways it can go wrong). Especially if the Dart interface is generic, it seems very problematic. I added it to #32929 (we need errors for invalid use of @JS)

CC @jacob314 ... not sure if we have a spec somewhere of what JS interop is intended to support?

[jacob314]

I don't think it is crucial if the goal of JS interop is limited to supporting creating good Dart facades for JS interfaces which I think it is at this point. There is no up to date spec of what we intend to support and what we don't. We need to ground up evaluate what we intend to support and what we don't intend to support based on the new world of what is easy and hard to support given Dart2. Specifically as you mentioned, generic methods make this much harder.