Skip to content

Consider taking final classes into account for "impossible cast" warnings #29548

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
leafpetersen opened this issue May 3, 2017 · 6 comments
Closed
Assignees
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). language-strong-mode-polish

Comments

@leafpetersen
Copy link
Member

#29547 proposes extending strong mode impossible cast warnings to explicit casts as well as implicit casts. For implicit casts, we are at least guaranteed that the types are subtype related, but for explicit casts we allow casts between non-subtype related classes, since there may be a type somewhere which implements both. However, if either class is final and they are not subtype related, then the only way the cast can succeed is if the value is null, which is highly unlikely to be what the user intended. We should consider taking this into account and issuing an error.

void test() {
 int x = 3;
 bool y = (x as bool); // Make this an error
}

cc @Hixie @lrhn @munificent @eernstg @floitschG

@leafpetersen leafpetersen added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label May 3, 2017
@leafpetersen leafpetersen self-assigned this May 3, 2017
@lrhn
Copy link
Member

lrhn commented May 4, 2017

The final classes in Dart are int, double, bool, Null and String, so those are the only ones that benefit from this - and Null is special, so it's really just the other four.
And just for the record:

int x = null;
bool y = (x as bool);  

would "succeed".
That doesn't mean that it's not stupid code and we should warn about it, or make it an error, just stating that it's not a guaranteed runtime error. (The solution would be to change x as bool to x as Null, then you seem to know what you are doing, and we'll allow it).

With a whole-program analysis, we can warn if the cast is impossible in the program, even if it's not impossible in general, so I think the analyzer is the right place for such a warning.

So, is it important to handle the few cases of the four final classes earlier? Does it actually happen in practice?
Or does this generalize to generics?

List<int> x = ...;
List<bool> y = x as List<bool>;  /// Failure? (Except for List<Null>)?

@eernstg
Copy link
Member

eernstg commented May 4, 2017

I know that they've discussed this topic in the Scala community in terms of intersections: If S&T is the empty type then cast-from-S-to-T will fail, and if S&T is a non-empty type then we generally can't know for sure that it will fail. So the intersection criterion is a pretty accurate way to see it.

I believe we cannot hope to determine type emptiness in the general case (and certainly not during separate compilation), so this is inherently a "best effort" thing. With that in mind I think it's an OK policy to go ahead and pick the low-hanging fruit.

However, we might want to wait until we get support for non-null types (or at least a few non-null predefined types), such that the failure is actually guaranteed? Or maybe the error message could say "If you really want to test for null then do it directly"?

@leafpetersen
Copy link
Member Author

The final classes in Dart are int, double, bool, Null and String, so those are the only ones that benefit from this - and Null is special, so it's really just the other four.

Unless we add user defined final (sealed) classes, which I'd still like us to do.

With a whole-program analysis, we can warn if the cast is impossible in the program, even if it's not impossible in general, so I think the analyzer is the right place for such a warning.

Yes, we could do this as a lint, but I don't want to specify it as a language error since I want us to be able to do modular analysis.

Or does this generalize to generics?

I don't think so. Not without whole program analysis, or maybe unless we forbid implementing the same class twice. If you have a generic class A which only uses the type variable contravariantly, then you can implement both A<bool> and A<int> just fine.

@lrhn
Copy link
Member

lrhn commented Jun 25, 2018

We can no longer have classes that implement both A<bool> and A<int>, so it might actually generalize to generics now.

@eernstg
Copy link
Member

eernstg commented Jul 2, 2018

As Lasse mentioned here, we do have A<Null> which will play the role as a "lifted" version of null, and they may well arise in practice—constant expressions might give rise to such instances, esp. <Null>[], but the same trick could also be used in a developer-written foo([A<T> = const A<Null>()]), or foo([A<T> = const A()]) where the type argument Null is inferred.

So the example that we considered will succeed in some cases, and it seems overly strict to make it an error:

List<int> x = ...; // Actually a `List<Null>`.
List<bool> y = x as List<bool>;

After all, the whole point of using as is to say "I know what I'm doing". Maybe we should handle this with a lint instead?

It's another matter as soon as we have non-null types, because they will enable us to say for sure that these final classes (in various positions including type arguments) can provide a guarantee that a cast will fail at run time.

@matanlurey
Copy link
Contributor

Brought over from #33933 (duplicate):


We don't have user-defined sealed types yet, but they do exist. Consider:

void main() {
  int x = 5;
  if (x is String) {
    print('...');
  }
}

I'd expect x is String to be a hint similar to a dead code warning.

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). language-strong-mode-polish
Projects
None yet
Development

No branches or pull requests

4 participants