Skip to content

Add formal unchecked cast support #31142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
matanlurey opened this issue Oct 18, 2017 · 29 comments
Closed

Add formal unchecked cast support #31142

matanlurey opened this issue Oct 18, 2017 · 29 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language).

Comments

@matanlurey
Copy link
Contributor

matanlurey commented Oct 18, 2017

Sometimes the type system is not expressive enough, and especially in Dart 2.x, knowing the explicit static type is important (and in many cases, required - for example, invoking methods, accessors, etc).

Prior art in other nominal type-system languages:

(This seems free in most structural ones)

Language Support Notes
Swift No It has as!, but that is similar to Dart's as
Kotlin/Java No JVM has various unchecked modes, though.
C# No
C++ Yes, with reinterpret_cast

Current Support

There are three ways to "cast" that I know of:

Implicit Casts

void printSum(int a, int b) => print(a + b);

void foo(dynamic a, dynamic b) {
  printSum(a, b);
}

or

void foo(dynamic a) {
  TypeA actualA = a;
  a.doThing();
}

Explicit Casts

void foo(dynamic a) {
  var actualA = a as TypeA;
  a.doThing();
}

Type Promotion

void foo(dynamic a) {
  if (a is TypeA) {
    a.doThing();
  } else {
    throw new UnsupportedError('Not expected: $a');
  }
}

However, all of these fall short:

  • Implicit casts hide sources of (real) bugs in code:
List<String> firstThree(List<String> names) {
 return names.subList(0, 2);
}

void foo(List<String> allNames) {
  // RUNTIME ERROR: Iterable<String> is not List<String>
  var firstThree = firstThree(allNames.where((name) => name != 'Matan'));
}

... and is a warning with implicit-casts: false turned on.

  • Explicit casts, in both Dart2JS, DDC, and the Dart VM, have a cost:

Here is dart2js:

void main(dynamic input) {
  print((input as String).toUpperCase());
}
// Code shared by all dart2js compilations omitted.

main: [function(input) {
  P.print(H.stringTypeCast(input).toUpperCase());
}, "call$1", "main__main$closure", 2, 0, 7]
  • Type promotion forces awkward if-else handling code:

... as well as having a runtime cost for the is check.

String getBestProprtyOf(dynamic a) {
  if (a is Dog) {
    return a.tail.description;
  } else if (a is Cat) {
    return a.paws.description;
  } else {
    // Uh... I have to fill this in.
    throw new UnsupportedError('Neither a Cat or Dog');
  }
}

Proposal

Add a as! operator (or similar) - someone can think of a better syntax than me.

void foo(dynamic a) {
  var actualA = a as! _TypeIKnowForSure;
  print(actualA.propertyOnType);
}

Optionally, when assertions are enabled, I'd be OK for this use the normal as.

/cc @yjbanov @Hixie who I know have had similar requests.

@matanlurey
Copy link
Contributor Author

Potentially related (old) issue: #5031.

@Hixie
Copy link
Contributor

Hixie commented Oct 18, 2017

How does this differ from as?

@ferhatb
Copy link

ferhatb commented Oct 18, 2017

+1 , would let us remove implicit downcasts for innerloopy hot code.

@matanlurey
Copy link
Contributor Author

matanlurey commented Oct 18, 2017

@Hixie:

How does this differ from as?

As I mentioned above, as has a runtime cost:

void main(dynamic input) {
  print((input as String).toUpperCase());
}
// Code shared by all dart2js compilations omitted.

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

EDIT: I'd personally be fine with making as free, though, with assertions disabled.

@leafpetersen
Copy link
Member

C# does not support this, as far as I know. The keyword that you link to only covers overflow checking, not casting.

@ds84182
Copy link
Contributor

ds84182 commented Oct 18, 2017

What about type promotions from asserts?

Instead of

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

you can do

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

@leafpetersen
Copy link
Member

The issue with doing any of this (unchecked casts, promotion from asserts, etc) in the language is that the implication on native is that arbitrary memory corruption results if you get it wrong (a la C++). We can certainly go that route, but my understanding is that this is not the semantics that our native compilation customers want? @Hixie ?

Doing this on web is easier: you still get arbitrary app corruption, but at least your machine doesn't get pwnd.

It seems to me that this is probably something that should be provided at the tooling level for individual platforms as appropriate.

@leafpetersen
Copy link
Member

I don't understand this example:

String getBestProprtyOf(dynamic a) {
  if (a is Dog) {
    return a.tail.description;
  } else if (a is Cat) {
    return a.paws.description;
  } else {
    // Uh... I have to fill this in.
    throw new UnsupportedError('Neither a Cat or Dog');
  }
}

How does unchecked cast help you write this code?

@yjbanov
Copy link

yjbanov commented Oct 18, 2017

Yeah, on the web your worst case is "Undefined is not a function" and the app usually keeps limping along. In Flutter it would be a segfault, which is much less pleasant.

Perhaps dart2js should simply implement as via assert, if the performance benefits outweigh the risk of undefineds?

@Hixie
Copy link
Contributor

Hixie commented Oct 18, 2017

I wouldn't want this to cause the Dart VM to segfault, even in release mode:

void main() {
  int x = 1;
  (x as! String).trim();
}

It should throw.

I don't really care what it throws, if there's a way to have a really low-level type check that throws an uninformative error like "VMError" in release builds in exchange for making the type check super fast, that's fine by me. (In checked mode I'd like more detail, to aid debugging.)

@matanlurey
Copy link
Contributor Author

matanlurey commented Oct 18, 2017

@leafpetersen:

C# does not support this, as far as I know. The keyword that you link to only covers overflow checking, not casting.

Thanks, I updated the table.

The issue with doing any of this (unchecked casts, promotion from asserts, etc) in the language is that the implication on native is that arbitrary memory corruption results if you get it wrong (a la C++). We can certainly go that route, but my understanding is that this is not the semantics that our native compilation customers want? @Hixie ?

Upon talking to @Hixie in person, seemed fine with @ds84182's idea:

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

This works for me too FWIW.

Doing this on web is easier: you still get arbitrary app corruption, but at least your machine doesn't get pwnd. It seems to me that this is probably something that should be provided at the tooling level for individual platforms as appropriate.

Fine with me, though perhaps it is just showing more a hole in the language.

If I know the following:

void feed(dynamic /* Cat | Dog */ pet) {
  if (pet is Dog) {
    pet.feed();
    return;
  }
  // pet is definitively 'Cat' here, but I can't express it statically.
  (pet as Cat).feed();
}

... then the as! or assert(<var> is <Type>) should safe to do.

(Though to be fair, lack of non-nullable types does make this harder)

@matanlurey matanlurey changed the title Add an unchecked cast operator Add an unchecked cast support Oct 18, 2017
@matanlurey matanlurey changed the title Add an unchecked cast support Add formal unchecked cast support Oct 18, 2017
@Hixie
Copy link
Contributor

Hixie commented Oct 18, 2017

The problem is that you might "know" it but maybe someone one day passes in "null" or a dynamic that happens to actually be a string or something.

I think this expresses the semantics you want:

void feed(dynamic pet) {
  assert(pet != null);
  if (pet is Dog) {
    pet.feed();
    return;
  }
  assert(pet is Cat);
  pet.feed();
}

Static analysis and type inference should determine that on that last line, pet is a Cat, but there still needs to be a last-ditch verification somewhere in that method to make sure that that is an actual invariant.

@leafpetersen
Copy link
Member

Yeah, on the web your worst case is "Undefined is not a function" and the app usually keeps limping along. In Flutter it would be a segfault, which is much less pleasant.

Just to be clear, neither of these are the worst case.

The worst case on the web is that you begin executing arbitrary (or attacker controlled) unexpected javascript.

The worst case on flutter is that you begin executing arbitrary (or attacker controlled) unexpected bytes.

@Hixie
Copy link
Contributor

Hixie commented Oct 18, 2017

In particular, consider similar cases like:

void _feed(dynamic pet) {
  assert(pet != null);
  if (pet is Dog) {
    pet._feed();
    return;
  }
  assert(pet is Cat);
  pet._feed();
}

...what if someone (from another module) indirectly passed you an object that implements Cat? There's no way that object can implement _feed, since it's private. At some point before the jump to _feed, the VM or the compiler or someone has to verify that we're dealing with an actual instance of Cat that has _feed on it. Type checking isn't enough.

@matanlurey
Copy link
Contributor Author

Totally fair, hence why I posted this thread here so I could get tons of reasons why it is bad :)

To be clear though, it already works this way, on the web, with implicit casts:

void foo(dynamic x) {
  Map mapX = x;
  print(mapX.keys);
}

... I just want to live in a world eventually where implicit-casts: false is enabled. I guess I could just use // ignore: implicit_cast, but something in the language, or a convention (assert) or otherwise would make me happier.

@Hixie
Copy link
Contributor

Hixie commented Oct 18, 2017

What's wrong with

void foo(dynamic x) {
  Map mapX = x as Map;
  print(mapX.keys);
}

@matanlurey
Copy link
Contributor Author

The fact that it generates:

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

... instead of P.print(input.get$keys()).

@leafpetersen
Copy link
Member

To be clear though, it already works this way, on the web, with implicit casts:

With implicit casts + --trust-type-annotations --trust-primitives presumably?

@leafpetersen
Copy link
Member

Totally fair, hence why I posted this thread here so I could get tons of reasons why it is bad :)

Also to be totally fair, pretty much every language that I know of has some way to escape the constraints of the core language (inline assembly, foreign function interface, JS interop, etc).

I'm not necessarily opposed to providing a way to drop out of the safe subset (particularly on web): you can already do so (on web) via JS interop. But I do personally see a lot of value in keeping those sharp bits off to the side of the language where most people aren't likely to grab the pointy end by mistake.

I'd actually much prefer us to be in a state where @ferhatb can unsafely optimize his inner loops to his heart's content but the rest of the app runs with runtime checks, over being in a state where everyone just turns off all checking as a matter of course. I'm a huge fan of giving the programmer close control over performance where they need it.

@Hixie
Copy link
Contributor

Hixie commented Oct 19, 2017

@matanlurey But you have to have that check regardless, as discussed above. Implicit casts involve the same check.

@matanlurey
Copy link
Contributor Author

matanlurey commented Oct 19, 2017

@leafpetersen:

With implicit casts + --trust-type-annotations --trust-primitives presumably?

Yup!

@leafpetersen:

I'm not necessarily opposed to providing a way to drop out of the safe subset (particularly on web): you can already do so (on web) via JS interop. But I do personally see a lot of value in keeping those sharp bits off to the side of the language where most people aren't likely to grab the pointy end by mistake.

This would be totally fine. If we could write some sort of (my syntax is totally made up):

void foo(dynamic x) {
  final keys = native```
    return ${(x as Map).keys};
  ```;
  print(keys);
}

(If it allowed inline-C/Assembly for the VM, maybe that would be great too for some folks)

I agree this definitely shouldn't be a misused part of the language, though. I could see it even being linted/not allowed outside of very carefully created and maintained code, i.e. framework-level.

@Hixie

But you have to have that check regardless, as discussed above.
Implicit casts involve the same check.

Not really :) In Dart 1.x, type annotations mean nothing outside of checked mode.

So this:

void foo(dynamic x) {
  Map mapX = x;
}

Is basically a no-op, or:

void foo(dynamic x) {
  dynamic mapX = x;
}

in production mode. However, literally every web user of dart2js uses two flags:

  • --trust-type-annotations
  • --trust-primitives

They basically let you "cheat", and use otherwise ignored type annotations to drive optimization. They are also dangerous, and basically mean "I've tested all of my code, in checked mode, and the type annotations all work!".

Dart2JS then interprets:

void foo(dynamic x) {
  var mapX = x /* infer: Map */;
  // At this point, mapX is a Map.
}

I know a lot of people on the Dart team weren't super happy about adding this feature, especially when in Dart 1.x type annotations were supposed to be for static tooling only and not driving optimizations.

In Dart 2.x we are sort of in murky water where now they are very important for optimization, but in AOT-runtimes (like Dart2JS), adding a is Map check to something called 1000s of times on a hot loop really hurts code size and runtime performance (these patterns are not always inline-able by V8).

@Hixie
Copy link
Contributor

Hixie commented Oct 19, 2017

@matanlurey You still have the type check, it's just in the JavaScript VM instead of in the JavaScript code.

@matanlurey
Copy link
Contributor Author

Implementation detail :). We don't generate an extra type-check :)

@leafpetersen
Copy link
Member

@matanlurey You still have the type check, it's just in the JavaScript VM instead of in the JavaScript code.

I think that is essentially the point that @matanlurey is getting at (and why I am more sympathetic to these arguments on web). On web, we pay the type check twice: once in the generated JavaScript, and once in the JavaScript VM. The latter check doesn't strictly dominate the conditions ensured by the former check, but it is enough to ensure memory safety, and so when combined with good test coverage, our web customers are currently "happy enough" relying on it.

@matanlurey
Copy link
Contributor Author

Exactly that. I guess I should change to "don't emit a language-level check".

@Hixie
Copy link
Contributor

Hixie commented Oct 19, 2017

In that case I think the trick with assert is definitely the way to go. It's not a language thing, it's a compiler implementation optimisation.

@kasperl kasperl added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label Oct 25, 2017
@matanlurey
Copy link
Contributor Author

Hey @hterkelsen @rakudrama - how does the CFE + Dart2JS handle this today?

@rakudrama
Copy link
Member

@matanlurey Not sure what you mean by 'handle this'. Perhaps you mean assert.
dart2js learns nothing from asserts. assert is completely ignored without --checked or --enable-assert.
The rarely used option of using a function as a condition is too hard to analyze and I hope we remove it.

@matanlurey
Copy link
Contributor Author

I'm closing this issue as I think it is confusing, and I want to track it as a dart2js feature.

Related: #31410

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language).
Projects
None yet
Development

No branches or pull requests

8 participants