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

Type check failures throw StrongModeError more eagerly than needed #296

Closed
vsmenon opened this issue Aug 19, 2015 · 10 comments
Closed

Type check failures throw StrongModeError more eagerly than needed #296

vsmenon opened this issue Aug 19, 2015 · 10 comments

Comments

@vsmenon
Copy link
Contributor

vsmenon commented Aug 19, 2015

Now that we're statically allowing more is and as checks (#236), we instead throw a StrongModeError at runtime in problematic cases.

We throw these in cases we don't really need to. E.g.,

<int>[] is List<String>

can safely return false even though:

x is List<String>

may be problematic in general.

@jmesserly
Copy link
Contributor

discussed solution here, essentially:

(3) reject at runtime is, but only if results differ from standard Dart is check

The nice thing about that fix: if you avoid creating objects with a reified dynamic type argument (e.g. List<dynamic>), then you can guarantee you'll never see a StrongModeError. So it encourages the right thing. You'll probably be avoiding reified dynamic anyway, because it can't be cast to List<T> for most T.

@vsmenon
Copy link
Contributor Author

vsmenon commented Aug 19, 2015

On a very related note, after #236, strong mode and standard Dart print different values for:

class Foo<T> {
  bool bar(x) => x is T;
}

void main() {
  Foo<int> foo = new Foo();
  print(foo.bar("hello"));
}

We used to reject this. @leafpetersen - maybe we should split this issue out?

@vsmenon
Copy link
Contributor Author

vsmenon commented Aug 19, 2015

Though I added that example here as @jmesserly 's solution (3) should require a StrongModeError on the call to bar.

@jmesserly
Copy link
Contributor

ah, because of inference... eeeep.

class Foo<T> {
  bool bar(x) => x is T;
}

void main() {
  Foo<int> foo = new Foo();
  var foo2 = new Foo<int>();
  print(foo.bar("hello"));
  print(foo2.bar("hello"));
}

it seems really nice if foo2 could work, even if foo doesn't.

@jmesserly
Copy link
Contributor

I guess we could track "inferred type T" at runtime, not sure if that's way too crazy...

@leafpetersen
Copy link
Contributor

Yes, the coherence of generic parameter inference relative to the regular Dart semantics was based on the following reasoning:

  1. Improving generic types from dynamic to something concrete can only cause more is/as checks to fail
  2. If we reject non-ground type is checks then we can't observe a different result on is checks
  3. If we throw a strong mode error on non-ground type as checks that fail then we can't observe different result on as checks

With this change, we no longer statically reject non-ground is checks, so we lose property 2. Property 3 also gets a bit hinky in that we may throw a CastError instead of a StrongModeError, but that strikes me as less problematic.

Tracking at runtime which generic parameters and function type components were originally dynamic and got improved via inference doesn't seem too hard to me, and should resolve this. If the "inferred" bit for a type T is set, and F[T] is a type involving T (e.g. List<T>, or T -> T), then:

  • if F[T] is a strong mode subtype of S then it should be the case that F[dynamic] is a weak mode subtype of S, so we're ok
  • if F[T] is not a strong mode subtype of S and F[dynamic] is not a weak mode subtype of S, we're also ok
  • if F[T] is not a strong mode subtype of S and F[dynamic] is a weak mode subtype of S, we're not ok and need to throw a StrongModeError

So we would have that:

List<String> inferred = []
List<String> annotated = <String>[]
(inferred is List<String);  // true
(inferred is List<int>); // StrongModeError
(inferred is List; // true
(inferred is Future<int>); // false
(annotated is List<String); // true
(annotated is List<int>); // false
(annotated is List); // true
(annotated is Future<int>); // false

@jmesserly
Copy link
Contributor

I think @leafpetersen may have fixed this :)

@leafpetersen
Copy link
Contributor

I fixed the original reported issue: we now only throw when we can't be sure that spec mode would have given the same answer, given the types in question. However, the inference issue is still live: inference can change what types we're asking the question about, and we're not tracking that yet. We can either leave this bug live for that, or open a new one, either way.

@jmesserly
Copy link
Contributor

Yeah I'll split that. Thanks!

@jmesserly
Copy link
Contributor

#567 for the other bug

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

3 participants