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

DDC strong mode and exact types #30546

Closed
mkj-gram opened this issue Aug 25, 2017 · 9 comments
Closed

DDC strong mode and exact types #30546

mkj-gram opened this issue Aug 25, 2017 · 9 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. language-strong-mode-polish P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Milestone

Comments

@mkj-gram
Copy link
Contributor

In strong-mode the following test fails (from compile_time_constant_static5_test):

class A {
const A();
}
class Test3<U extends A, V extends B> {
final U x = const A(); //# 11: ok
}

because the result is a compile-time warning.

This is changed behavior from earlier. Is the current behavior expected, then the test should be updated.

@eernstg
Copy link
Member

eernstg commented Aug 25, 2017

Adding a bit of context: We need to specify if/when the strong mode static analysis determines that a given expression has an exact type, and what that knowledge is used for. A typical case would be constant expressions, where the exact type is known at compile-time, but new A() could also be given an exact type when it is known statically that it invokes a generative constructor.

In the given context, a downcast from "exactly A" to B could be rejected because it is guaranteed to fail at run time. A downcast from "exactly A" to U would not be guaranteed to fail at run time (the value of U could be A), so it would be consistent with the approach taken elsewhere in Dart to allow that (except when --no-implicit-casts is specified, of course).

When we have a clear model for which expressions have exact types, and when that's used for what, we can adjust the tests to have the correct expectations.

@leafpetersen leafpetersen added analyzer-strong-mode area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. language-strong-mode-polish and removed web-dev-compiler labels Aug 25, 2017
@leafpetersen
Copy link
Member

The intention is that the error only be issued on casts that are guaranteed to fail. A reasonable approximation of this is to only issue this error when the target type of the implicit cast is a ground type. That would allow this cast.

@bwilkerson bwilkerson added P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Aug 28, 2017
@kmillikin
Copy link

kmillikin commented Nov 27, 2017

Related:

class A {
  const A();
}

class B extends A {
  const B();
}

// Snip...

// Will be instantiated with U=dynamic and V=dynamic.
class Test5<U extends A, V extends B> {
  final U x = const A(); //# 21: ok
  final U x = const B(); //# 22: compile-time error
  final V x = const A(); //# 23: ok
  final V x = const C(); //# 24: compile-time error
  final V x = const C.d(); //# 25: compile-time error
  const Test5();
}

use(x) => x;

main() {
  use(const Test1());
  use(const Test2<A, B>());
  use(const Test3<A, B>());
  use(const Test4<A, B>());
  use(const Test5());
}

#23 is a compile-time error in DDC, but a run-time error in DDK (DDC with the new common front end).

@lrhn
Copy link
Member

lrhn commented Nov 27, 2017

// Will be instantiated with U=dynamic and V=dynamic.
class Test5<U extends A, V extends B> {

This comment is incorrect for Dart 2, dynamic is not a valid type argument for U or V.
Most likely the class will be instantated to bounds: Test5<A, B>, otherwise the instantiation itself will be an error.

The #23 test should still be a compile-time error.

The assignment final V x = const A(); should be a valid implicit down-cast. The static type of const A() is A, which is a supertype of the parameterized type V with bound B. A type variable is a subtype of its bounds, so V <: B, and B is a subclass of A.
(The static type system does not have a notion of "exact type", so it can't say for sure that const A() is not assignable to V - so it must follow the usual implicit downcast rules, and A is a supertype of V).

Since the assignment of an A to a B variable would fail at run time (due to the run-time check introduced by the implicit downcast), and because it's a const object construction, it'll cause a compile-time error. So the current DDC is correct.

@eernstg
Copy link
Member

eernstg commented Nov 27, 2017

Trying to actually run it, I can see that the use of exact types in DDC does play a role in Kevin's example after all, because DDC emits a compile-time error for subtest 21 (which is marked ok):

[error] The constructor returns type 'A' that isn't of expected type 'U'. (out/ReleaseX64/generated_tests/language_2/scratch_test_21.dart, line 13, col 15)

We may check the initialization of final U x ... //# 21.. locally (that is, in a way that should be valid for both const and new invocations of the constructor): The initializer has type A, and the variable has a type U which is less-equal-than A according to the bound. So that's a plain downcast, and the only way we can make it a compile-time error is by considering const A() to have an exact type and then concluding that the downcast to U "will never succeed". That, again, is not quite true because U could be A (as it actually is in the invocation of const Test5() in main).

So this means that Kevin's example shows an issue with DDK in relation to const expressions (23), and it again shows the issue with DDC and exact types that gave rise to this issue in the first place.

@lrhn
Copy link
Member

lrhn commented Nov 28, 2017

I agree that #21 should not be a compile-time error.
The static types are U extends A and A, which is an implicit downcast, and the run-time types are A and A, so assignment succeeds.

And #22, #24 and #25 are strong-mode compile-time errors because the static types do not form a valid up- or down-cast. The 22 case is the most surprising because U extends A and B are unrelated subtypes of A (statically, the parameterized type U is not the type A, it's an unknown subtype of A).
Likewise the static type C is not a related to V extends B.

@jmesserly jmesserly self-assigned this Jun 27, 2018
@jmesserly
Copy link

this still reproduces. should be a quick fix

@jmesserly
Copy link

https://dart-review.googlesource.com/c/sdk/+/62663 ... does not fix CFE tho, which appears to be expecting these as failures (perhaps ported the bug from Analyzer?)

@bwilkerson bwilkerson added this to the Dart2.1 milestone Sep 4, 2018
@bwilkerson bwilkerson modified the milestones: Dart2.1, PostDart2.1 Sep 4, 2018
@aadilmaan aadilmaan modified the milestones: Future, D25 Release Jun 4, 2019
@fishythefish
Copy link
Member

Closing since both CFE and analyzer seem to agree on disallowing these casts: https://dart-review.googlesource.com/c/sdk/+/115778

Feel free to re-open if this is premature.

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. language-strong-mode-polish P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

10 participants