Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

is / as checks unnecessarily fail at runtime #524

Closed
vsmenon opened this issue Apr 25, 2016 · 7 comments
Closed

is / as checks unnecessarily fail at runtime #524

vsmenon opened this issue Apr 25, 2016 · 7 comments

Comments

@vsmenon
Copy link
Contributor

vsmenon commented Apr 25, 2016

This test throws a strong mode error at runtime. No static errors or warnings.

typedef String Predicate(x);

String foo(x) {
  if (x is Predicate) {
    return x("hello");
  } if (x is String) {
    return x;
  } else {
    return "unexpected value";
  }
}

void main() {
  print(foo("test"));
}
@vsmenon
Copy link
Contributor Author

vsmenon commented Apr 25, 2016

@leafpetersen thoughts on the right behavior here?

@jmesserly
Copy link
Contributor

jmesserly commented Apr 25, 2016

Oh shoot, we need a to do something here. Here's my sense.

Most consistent/easiest to implement in the short term: it should be a warning like DownCastComposite.

Medium-to-long term, maybe we can make progress on "no reified dynamic" direction, and we can have VM subtype rule consistent with DDC, making all of these "composite casts" a non issue.

@vsmenon
Copy link
Contributor Author

vsmenon commented Apr 25, 2016

This is coming up in package:unittest. @nex3 fixed a strong mode error, but the new code (see util.dart) introduces _Predicate and triggers the runtime issue above. See:

dart-lang/test@7beae72

@jmesserly
Copy link
Contributor

Oh: we should fix this in DDC for sure, if Dart 1 and 2 checks would agree, we shouldn't throw.

@leafpetersen
Copy link
Contributor

If we give a warning, it should only be on doing an is check with something which returns non-dynamic. All the other cases should be safe, I think. For casts, we let people do them without a warning if they make them explicit. I'm a little worried about the fact that we're basically leaving people with no option here. What do I do with this code to make it strong mode clean then:

typedef String StringIt(x);
typedef bool BoolIt(x);

void test(f) {
  if (f is StringIt) {
     foo(f);
  } else if (f is BoolIt) {
    bar(f);
 }
}

@jmesserly
Copy link
Contributor

okay we chatted about this :). It sounds like Leaf may want to try attacking this from the Analyzer side. I'll assign to you Leaf for now.

@vsmenon -- is this a P1?

@leafpetersen
Copy link
Contributor

Just a note on this. The VM doesn't actually implement is checks correctly per the spec for function types. The code below passes checked mode and prints true: it should do neither per the spec.

typedef To Function2<From, To>(From x);
typedef To Function2Opt<From, To>([From x]);

int f(int g([int y])) => g(3);
int g(int q(int y)) => q(3);

void main() {
  Function2<Function2<int, int>, int> f2 = f;
  Function2<Function2Opt<int, int>, int> f3 = g;
  print(g is Function2<Function2Opt<int, int>, int>);
}

nex3 pushed a commit to dart-lang/sdk that referenced this issue Aug 31, 2016
the DDC runtime returns null when a difference from the spec mode
result is possible.

Fixes dart-archive/dev_compiler#524 .

BUG=
R=jmesserly@google.com, vsm@google.com

Review URL: https://codereview.chromium.org/1945113003 .
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants