Skip to content

Analyzer and compiler should check type mismatch error for if null operator #34710

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
jerrywell opened this issue Oct 8, 2018 · 5 comments
Closed
Labels
legacy-area-analyzer Use area-devexp instead.

Comments

@jerrywell
Copy link

int i = 0;
String a = "";
i = a ?? 10; //=> no analyzer error. have a runtime error
i = a; //=> analyzer error. have a runtime error

analyzer should show type mismatch error since it has enough type information.

already checked for two versions:

  1. Dart VM version: 2.0.0 (Fri Aug 3 10:53:23 2018 +0200) on "macos_x64"
  2. Flutter 0.9.5 • channel master • [email protected]:flutter/flutter.git Framework • revision 020fd59 (11 days ago) • 2018-09-26 14:28:26 -0700 Engine • revision 38a646e14c Tools • Dart 2.1.0-dev.5.0.flutter-4cf2d3990b
@eernstg
Copy link
Member

eernstg commented Oct 8, 2018

The missing analyzer error is caused by a "zigzag cast" from String and int to Object and then down to int. See some more details here. So that's not a bug, it is a consequence of having a least-upper-bound operator that will work on highly unrelated types (like int and String) and then having an implicit downcast to int.

You can avoid this issue by taking away the permission to do a downcast at all (e.g., passing --no-implicit-caststo the analyzer on the command line). For the general case (where implicit downcasts are allowed), my proposal is to single out the extreme types (Null at the bottom and Object, dynamic, etc. at the top) and prevent downcasts from there (upcasts from Null when it occurs contravariantly) on types that were obtained from such operations as least-upper-bound.

@mit-mit mit-mit added the legacy-area-analyzer Use area-devexp instead. label Oct 8, 2018
@mit-mit mit-mit closed this as completed Oct 8, 2018
@krisgiesing
Copy link

krisgiesing commented Dec 28, 2018

Why does a zigzag cast need to occur at all?

I would expect "T t = a ?? b" to be shorthand for "T t = a != null ? a : b", which in turn would be shorthand for this:

T t;
if (a != null)
  t = a;
else
  t = b;

The first two idioms involve this zigzag cast and the resulting lack of an analyzer error, but the third does not.

This language idiosyncrasy encourages developers to always use the more elaborate longhand structure, which is truly unfortunate.

(I'm currently in the middle of a lengthy refactoring effort, and it has become significantly more complicated because I can't rely on the analyzer to help me with type mismatches of this nature.)

@eernstg
Copy link
Member

eernstg commented Dec 29, 2018

As you mention, a ?? b is similar to a conditional expression (b ? e1 : e2), so I'll focus on that (the consequences for conditional expressions transfer directly back to the ?? expression as wall).

You can't eliminate the type of an expression like b ? e1 : e2 in general. For instance, with return b ? e1 : e2 you will have to decide on the type of that expression in order to decide whether it is allowed to return that expression.

This means that the idea you mention (which is to eliminate the conditional expression by transforming one assignment into several statements) may work in some contexts, but not all, and it would surely introduce a surprising and inhomogeneous static analysis if we were to treat conditional expressions like a shorthand for several statements in some cases, and like an expression in its own right in others.

@krisgiesing
Copy link

I don't think I'm suggesting the type of an expression be eliminated.

I had a bit of an insight while reading your response though. Perhaps this will explain my thinking more clearly, and maybe help me understand your response better.

In the last transformation I introduced above, the type of T is determined not by the types of the contents of the expression, but by the context in which the expression is evaluated. Taking your example of return b ? e1 : e2, the translated equivalent is:

T result;
if (b)
  result = e1;
else
  result = e2;
return result;

Here both e1 and e2 need to match the return type T of the function in question.

Equivalently, in the third example above, the type T of the variable t is set by conditions external to a and b, and both a and b need to convertible to that type.

If I understand correctly, what Dart does instead is find the common supertype of a and b, and determine whether that is convertible to T. Since Object is a common supertype of all types and it can be implicitly cast to anything, there is essentially no analysis-time type safety on ternary expressions (or if-null expressions).

My suggestion is that the type analysis be done on each branch of the expression independently rather than on the combination. I grant that it will get complicated with more complex expressions, but I believe it is deterministic, and seems strictly better than the current behavior that appears to me to be throwing away type safety for a very common language idiom. (It would also be an API breaking change, but my suspicion is that any code affected by this represents a bug anyway.)

While I was playing around with these concepts I ran into what seems like a similar issue. The following code gives no analyzer warnings but throws at runtime:

print(1 as String);

Does this behavior have the same underlying cause or is it a separate issue?

@eernstg
Copy link
Member

eernstg commented Dec 30, 2018

I grant that it will get complicated with more complex expressions

I think this is the core issue, and it's also what I referred to when I said that it will be complex (and, on top of that, we'll have to give up on that approach at some point, which makes the static analysis inhomogeneous in this respect).

So the exploration of this idea might be useful (and I do admit that we'd get better typings in some cases), but I tend to be sceptical about its scalability.

Also, I do think that developers will pay for language complexity in the sense that they'll have to understand more arcane rules and exceptions for every single expression or statement that they are writing, so even if we get better typings here and there it might not be a net positive if it makes the whole language more difficult to work with everywhere else.

there is essentially no analysis-time type safety on ternary expressions (or if-null expressions).

Cf. #25368 and #34058: The case where a conditional expression involves a least upper bound that yields a top type (like dynamic or Object) merits special attention here, because the top types have a lot more subtypes than any other type. So apart from the ongoing discussion about allowing implicit downcasts at all, I think it would make a lot of sense to flag downcasts from a computed top type (and similarly for upcasts from a computed bottom type when we consider contravariantly located types like the parameter types of functions).

1 as String

This, being an explicit cast, is exclusively the responsibility of the developer who wrote it. We could use exact types (#33307) to determine that this particular cast will definitely fail at run time. But I think it makes sense, as a general rule, to trust developers when they make the choice to explicitly claim that they know a certain expression has a certain type. After all, we know that no type system can understand the actual semantics of a program in any non-trivial language.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead.
Projects
None yet
Development

No branches or pull requests

4 participants