Skip to content
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

StrongTypeSystemImpl.isAssignableTo returns wrong result in one case #35300

Closed
bwilkerson opened this issue Nov 30, 2018 · 6 comments
Closed
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P1 A high priority bug; for example, a single project is unusable or has many test failures type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@bwilkerson
Copy link
Member

The type system implementation of isAssignableTo is broken in the case where the first argument is Set<Null> and the second argument is a type parameter T whose bound is something like Set<int>. (The class Set can be replaced by anything, and the type int can be replaced by anything other than Null.)

This is blocking #35120.

@bwilkerson bwilkerson added P1 A high priority bug; for example, a single project is unusable or has many test failures area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Nov 30, 2018
@bwilkerson bwilkerson self-assigned this Nov 30, 2018
@MichaelRFairhurst
Copy link
Contributor

Assuming I'm creating a correct a code example to this...

class C<T extends Set<int>> {
  T t;
  f() {
    t = Set<Null>(); // This looks like it should be valid at first...but its not!
  }
}

This looks like it should be valid at first...but its not!

Proof by contradiction:

C<HashSet<int>> set;
set.f();

In this case, set.t will be a Set<Null>, but that is not a subtype of HashSet<int>. So the only types assignable to T in this case are T and Null.

@bwilkerson
Copy link
Member Author

@lrhn I originally opened this issue because I thought this was a bug in analyzer that was preventing some of the set literal tests from passing (set_literal_test.dart and const_set_literal_test.dart). If Mike is right, then I think that some portions of those two tests are invalid.

@eernstg
Copy link
Member

eernstg commented Dec 3, 2018

@MichaelRFairhurst is right, knowing only T <: Set<int> we cannot prove Set<Null> <: T. You could think of the subtype universe as a layered structure: Set<Null> is at the bottom of the layer which contains all the types of the form Set<S>, but we may have additional layers below, such as HashSet<_>. Of course, you could also use a layer which is always available: C<Null>; with that, you'd need Set<Null> <: Null and that won't fly, either.

You could have other subtypes with a bound: <S extends T>() {/* S is a subtype of T here */}, but otherwise we have no other lower bounds for a type variable than Null (and bottom).

So maybe this issue should be retargeted to correct

those two tests

@lrhn
Copy link
Member

lrhn commented Dec 3, 2018

I agree that there is a problem with the tests. Similar tests worked for double literals, so I copied them, but those tests didn't get into generics.

It's true that S declared as <S extends Set<num>> does not allow Set<Null> to be assigned to it, and hence it does not introduce a set literal.
That's probably a good thing. The expression S x = {}; has no good element type that we can infer for the set. If we use num, then it's a run-time error if S is actually Set<int> at run-time (which it very well may be). So, we can't safely make it a set, so it should be an error. It's annoying that it becomes a " Map<dynamic,dynamic> is not assignable to S extends Set<num> error, but for now I think we will live with that.

We could change the rules to, say, make the empty-set/map-literal a set if there exists some type T so that Set<T> is assignable to the context type (and no types K and V so that Map<K, V> is).
That would allow this case because Set<num> is assignable to S by downcast, but it will also likely break if we try to run it because of that downcast. Not much saved, and forcing the compiler into a searching for a solution.

I'll fix the tests: https://dart-review.googlesource.com/c/sdk/+/85740

dart-bot pushed a commit that referenced this issue Dec 3, 2018
See #35300

Bug: http://dartbug.com/35300
Change-Id: I00610ce9eadc51499fc5f35d43095dbf29d07543
Reviewed-on: https://dart-review.googlesource.com/c/85740
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Lasse R.H. Nielsen <lrn@google.com>
@lrhn
Copy link
Member

lrhn commented Dec 4, 2018

I'm fixing the test now, but keeping the door open for changing the specification if the current one is too restrictive.

@bwilkerson
Copy link
Member Author

Given that the analyzer's implementation was right and the tests were wrong, I'm closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P1 A high priority bug; for example, a single project is unusable or has many test failures type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants