Need a way to avoid some type checks without implicit downcasts

[matanlurey]

I closed #31142 to open this instead.

EDIT: This is considered a blocking dependency of working on #31410.

In Dart 1.x + Dart2JS + --trust-type-annotations, the following was "free":

import 'dart:html';

var _map = <String, dynamic/*Element|OtherClass*/>{};

Element readElement(String key) {
  Element value = _map[key];
  return value;
}

In Dart 2.x, this roughly translates to:

import 'dart:html';

var _map = <String, dynamic/*Element|OtherClass*/>{};

Element readElement(String key) {
  var value = _map[key] as Element;
  return value;
}

... where as has both a runtime and code-size cost:

void main(dynamic input) {
  print((input as String).toUpperCase());
}

main: [function(input) {
  P.print(H.stringTypeCast(input).toUpperCase());
}, "call$1", "main__main$closure", 2, 0, 7]

Proposal

Add unsafeCase (or similar) to package:meta/dart2js.dart or package:js.

/// Returns [any] casted to the type [T].
///
/// ```dart
/// import 'package:meta/dart2js.dart';
///
/// void main(List<dynamic> input) {
///   // I know the first element of this is list is _always_ a String.
///   var element = unsafeCast<String>(input)
/// }
/// ```
///
/// Optimizing compilers, such as dart2js, may choose to omit the explicit
/// cast, substituting safety for a bit faster runtime performance and less
/// omitted code. Like `--trust-type-annotations` from Dart 1.x, this should
/// be used sparingly as it can cause your application to fail in undefined ways
/// in release mode.
T unsafeCast<T>(Object any) => any/* as T*/;

At a later point of time (before 2.0-on-by-default in google3, for example), Dart2JS would have a compiler intrinsic specifically for this function call, where-as the body of the function is not used, and instead it would similarly to --trust-type-annotations in Dart 1.x.

So, the above example would be re-written as:

import 'dart:html';

import 'package:meta/dart2js.dart';

var _map = <String, dynamic/*Element|OtherClass*/>{};

Element readElement(String key) {
  var value = unsafeCast<Element>(_map[key]);
  return value;
}

DDC would continue to just use the function as-is so tests and dev-mode is protected.
/cc @leafpetersen @vsmenon

@hterkelsen @sigmundch @rakudrama WDUT? Happy to send a CL to add to pkg/meta.

[ferhatb]

+1. Straightforward to implement/execute migration. 'unsafe' prefix will prevent overuse.

[leafpetersen]

Is the intention that dart2js always treat this cast as a no-op, or only when the Dart 2.0 equivalent of --trust-type-annotations is on? The former is a little concerning, since it means that any of my dependencies can opt me in to undefined behavior without my knowing it, and without any way for me to opt out.

[sigmundch]

I believe you can achieve the behavior you want as follows:

T unsafeCast<T>(dynamic any) {
   T anyAsT = any;
   return anyAsT;
}

Doing so will have the following behavior:

• FE will inject an implicit downcast
• The downcast will have a bit indicating that it was an implicit cast
• Dart2js will generate the cast with running in strong mode, but will omit the cast when running with some special "--trust" flag.

I don't believe we need to put this function in package:meta though, dart2js is not treating it specially in any way, so this can live directly in a private library of your package instead, or you can use that pattern in general in regular code. In your original example, ths might look like this:

import 'dart:html';

var _map = <String, dynamic/*Element|OtherClass*/>{};

Element readElement(String key) {
  Element value = _map[key] as dynamic;
  return value;
}

The only difference with the 1.0 code is that we added as dynamic. This will force the FE (and DDC) to inject an implicit downcast. Dart2js with --trust with not inject it, and the as dynamic is optimized away.

[sigmundch]

*edit: we don't even need the as dynamic I mentioned earlier. My main reason I wanted to bring it up is that there is a way to do this kind of implicit cast in an assignment alone, like T x = y as dynamic, without having to define a new function for it.

[rakudrama]

If --trust-type-annotations was acceptable, I don't think this is necessary.
The Dart 2.0 equivalent to --trust-type-annotations is to trust implicit downcasts.
dart2js could have a command-line flag for that, maybe --trust-implicit-casts.

Is there is some other reason you want unsafeCast?
Do you plan to make implicit casts (--no-implicit-casts) a build error?
Do you need to document the places that are unsafe?
Is --trust-type-annotations too broad in scope and you need something more fine-grained than the whole-program level?

It seems like package/meta is the wrong place for something that is not an annotation.

[matanlurey]

@leafpetersen:

Is the intention that dart2js always treat this cast as a no-op, or only when the Dart 2.0 equivalent of --trust-type-annotations is on?

Always. Or at least by default (start on the "golden path").

The former is a little concerning, since it means that any of my dependencies can opt me in to undefined behavior without my knowing it, and without any way for me to opt out.

I'm not convinced it's an objective of Dart4Web to prevent any undefined behavior. We already use dart:html and package:js in ways that can easily cause undefined behavior if the JS API changes/is polyfilled/etc, and we just hope it works. We use something like unsafeCast today to get the same type of behavior that is obtainable via package:js.

@sigmundch:

... Dart2js will generate the cast with running in strong mode, but will omit the cast when running with some special "--trust" flag.

I'm trying to avoid that. There is no reason to need --trust-XXX in Dart4Web. We don't test or run any of our tests or applications internally without that flag, and virtually every customer both externally and internally uses it. If someone feels strongly and wants to add --force-spec-compliance, that's another option, but I think otherwise we should just include it by default.

@rakudrama:

If --trust-type-annotations was acceptable, I don't think this is necessary.

Coupled with my extremely popular proposal to remove implicit downcasts, I don't want to rely on Dart2JS disabling functionality of the language to get a performance boost.

Do you plan to make implicit casts (--no-implicit-casts) a build error?

Yes.

Do you need to document the places that are unsafe?

No. We don't document usage of dart:html or package:js today.

It seems like package/meta is the wrong place for something that is not an annotation.

Fine to add to package:js. Was just an idea.

[leafpetersen]

There is no reason to need --trust-XXX in Dart4Web. We don't test or run any of our tests or applications internally without that flag, and virtually every customer both externally and internally uses it. If someone feels strongly and wants to add --force-spec-compliance, that's another option, but I think otherwise we should just include it by default.

I'm not sure this is a good idea. The concern I would have here is that we are giving customers no indications that they are not getting Dart semantics, and that code which they have developed and tested using dart2js will not run on other platforms.

[matanlurey]

I'm not sure this is a good idea. The concern I would have here is that we are giving customers no indications that they are not getting Dart semantics, and that code which they have developed and tested using dart2js will not run on other platforms.

They already don't get Dart semantics if they use int or double (all of them do), or if they use dart:html or package:js (all of them do). I'm not sure how this is meaningfully different. In this particular case, they'd have to import and use package:js, so it wouldn't run on other platforms anyway?

[leafpetersen]

I'm lost. I understood the piece of text that I quoted to be suggesting that dart2js should ship with the 2.0 equivalent of --trust-type-annotations on by default. This means that with no package:js, no dart:html, no nothing, the can write and ship code that is not portable to DDC, nor to the VM, just by using implicit casts. Is that not what you mean to say by the text that I quote?

[matanlurey]

I'm lost. I understood the piece of text that I quoted to be suggesting that dart2js should ship with the 2.0 equivalent of --trust-type-annotations on by default.

Sorry I didn't make this clear enough, specifically:

• Implicit casts used for performance optimizations is a dangerous pattern.
• Almost every cast should be checked.
• I'd like to see a new function to perform an "unsafe cast" instead of relying on implicit casts.

I had a nice chat with @kevmoo about this. When you look in AngularDart and see:

void doThing() {
  _doThing();
  return null;
  return null;
  return null;
}

What does that mean? Well, it means "don't inline". We'd prefer to write:

import 'package:meta/dart2js.dart';

@noInline
void doThing() {
  _doThing();
}

Similarly, if you see the following:

void doThing(dynamic /*A|B*/ element) {
  if (element is A) {
    // ...
  } else (element == null) {
    // ...
  } else {
    // We guarantee this is always B, but we can't prove it statically.
    B bElement = element;
  }
}

We'd prefer to write:

import 'package:js/js.dart';

void doThing(dynamic /*A|B*/ element) {
  if (element is A) {
    // ...
  } else (element == null) {
    // ...
  } else {
    // We guarantee this is always B, but we can't prove it statically.
    var bElement = unsafeCast<B>(element);
  }
}

This is clear to code reviewers, and people browsing the source code that we are making an explicit choice to either opt-out of inlining or perform an unsafe cast for performance reasons. In large companies like Google code conventions pass by word-of-mouth as much as they do official style guides. We now have folks telling each-other to "use implicit casts because it is faster". This is terrible advice, goes against the style guide, and is completely wrong.

Let me know if we should VC to chat in person and explain it further.

[leafpetersen]

Ok, I think we're on the same page then.

[matanlurey]

This has come up again in user code.

An internal user using a combination of GWT/JS/Dart has an "extension manager", which relies on reading/writing between GWT, JS, and Dart. An abstract example is the following JavaScript code:

// May be read/write/replaced by either a JS App, GWT App, or Dart App.
window.extensions = [
  'foo',
  'bar',
];

In both GWT, JS, and Dart 1.x (and early DDC) this is fine.

In Dart 2.x (and newer DDC) this throws JSArray is not a List<String>. The issue here is that the user will have to read this file and cast it (via .map/.cast) on every read, because it could have been changed by another (non-Dart) application.

In this case (and honestly String.split too), it would be great to be able to "force" the cast and/or "ignore" type arguments - that is, pretend this is a List<String> even though it is likely a JSArray<dynamic>.

[rakudrama]

There are two separate use cases here: 1. Doing a cast, but have the cast be compiled to a no-op. Something has the static type Object but you know it is really a String. This is what unsafeCast would do, and could be made safe by doing the check, for example, DDC might do this to keep the unsafeCast honest. We can support this. 2. Pretending the type is something that can't be verified by a type check (i.e. a lie).

On Fri, Feb 16, 2018 at 9:24 AM, Matan Lurey ***@***.***> wrote: This has come up again in user code. An internal user using a combination of GWT/JS/Dart has an "extension manager", which relies on reading/writing between GWT, JS, and Dart. An abstract example is the following JavaScript code: // May be read/write/replaced by either a JS App, GWT App, or Dart App.window.extensions = [ 'foo', 'bar', ]; In both GWT, JS, and Dart 1.x (and early DDC) this is fine. In Dart 2.x (and newer DDC) this throws JSArray is not a List<String>. The issue here is that the user will have to read this file and cast it (via .map/.cast) on *every* read, because it could have been changed by another (non-Dart) application. In this case (and honestly String.split too), it would be great to be able to "force" the cast and/or "ignore" type arguments - that is, pretend this is a List<String> even though it is likely a JSArray<dynamic>.

This is the second case, telling a lie. Pretending the type is List<String> will be hard to make work because there is independent code that can check the type and catch you out in the lie. For example, if you call a method on the list and the method inspects the class type parameter, it will still be dynamic. You will have to unsafeCast the elements as you take them out of the list. Regarding String.split, I recently committed an optimization to leave off the type argument on the result if we can see that the type is unused. A little over a third of cases get optimized. If all you do is for-in or index the list in the same function as the .split() call, it works great. After Dart 2, we can consider a more general form of this optimization.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31428 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACAtlVYeJfhZI3J5MAIoie1oxSMaU156ks5tVbmwgaJpZM4QmSbG> .

[matanlurey]

Doing a cast, but have the cast be compiled to a no-op. Something has
the static type Object but you know it is really a String. This is what
unsafeCast would do, and could be made safe by doing the check, for
example, DDC might do this to keep the unsafeCast honest. We can support
this.

Yup, we already do this here speculatively:

https://github.com/dart-lang/angular/blob/37612b46ed820d4682d798e4a65eed973426e0f9/angular/lib/src/runtime/optimizations.dart#L25-L39

This is the second case, telling a lie. Pretending the type is List
will be hard to make work because there is independent code that can check
the type and catch you out in the lie. For example, if you call a method on
the list and the method inspects the class type parameter, it will still be
dynamic. You will have to unsafeCast the elements as you take them out of
the list.

I guess what I (might?) be asking for is a "secret" type, that will always pass for the following check:

if (list is List<AnythingICheck>) {
  // Always true if list is a List, regardless of List<T>.
}

You will have to unsafeCast the elements as you take them out of
the list.

Yeah, we might do that for now, but that isn't really ergonomic for users.

Regarding String.split, I recently committed an optimization to leave off
the type argument on the result if we can see that the type is unused. A
little over a third of cases get optimized.

Not a bad start, but I'm worried we will still need an "opt-out" for cases such as the GWT/JS interop ones I listed above.

[matanlurey]

Any thoughts here in the last few months?

We're already using this pattern, ourselves:
https://github.com/dart-lang/angular/search?utf8=%E2%9C%93&q=unsafeCast&type=

In conjunction with the various --strict-x lints discussed (which would enforce implicit casts), we'd like to stop relying on --omit-implicit-casts in the future and instead require the use of this method (or a better named one) for the same effect. It will also grok a lot better.