Strong mode should disallow implicit downcasts of ternary operators

[leafpetersen]

This program passes strong mode because the LUB of the arms of the ternary operator is Object, which is allowed to be downcast to int. If we disallow implicit downcasts we'd catch this, but even in the absence of this I think we could do better. In a typed context, we should always check the arms of the ternary operator against the type from the context.

void main() {
  bool b;
  int test = b ? "123" : 123;
}

[lrhn]

Ob-pedantry: There are two ternary operators in Dart: The conditional operator (o1?o2:o3) and the index-set operator (o1[o2]=o3), so just saying "ternary operator" isn't uniquely identifying the conditional operator by its arity the same way it does in some other language.

[leafpetersen]

Ob-pedantry acknowledged... :)

I think this applies to the ?? operator as well. In general, any where we do a LUB we have the potential to lose all checking if the LUB is top.

[leafpetersen]

Note: pushing types down from the context when present instead of relying on LUB could avoid other unfortunate examples as well. Consider this example (abstracted from real code encountered in the wild):

class A{}
class B{}
class C extends B{}

class E extends C implements A {}
class F extends C implements A {}
void main() {
  bool a = true;
  A y = a ? new E() : new F();
}

This is currently an error, because the LUB of E and F is C, which is unrelated to A. But both arms of the conditional can be typed as A.

[jmesserly]

@sethladd marked this Flutter and bumped pri for you

[sethladd]

To be fair, I'm not sure the Flutter codebase ran into this, or that is is specifically strategic to Flutter's roadmap. That said, sounds like a general nice feature that any Dart developer would love.

[jmesserly]

ah okay, i misunderstood that because it was on the flutter issue tracker

[eernstg]

Maybe we could detect the unusually blatant loss of type information? Considering the original example b ? "123" : 123, the branches of the conditional operator have types with a non-trivial relationship to the type expectation associated with an enclosing expression (the initializing expression as a whole). The reason why the upcast-then-downcast is "unreasonable" is that we seem to forget, very quickly, that one branch is guaranteed to work and the other one is likely to fail (it's actually guaranteed to fail in this particular case, because the type String is known to be exact).

I think, however, that it would be dangerous to say "an implicit downcast cannot be applied to a conditional expression". The problem is that this is a syntactic criterion, and we can easily come up with syntactically different forms where the same kind of problem arises, and hence the same solution should work.

But we have a way to avoid forgetting the typings for the two branches: Just make the type of the conditional expression the union of the types of its branches.

That union type may now propagate through various constructs; in the given example it's directly the type of the whole initializer. We may now special-case downcasts from union types (which is a local rule, concerned with initializing expressions), rather than special-casing downcasts from Object (or whatever it might be), based on the syntactic shape of some subexpression, and that would work in a larger (and more natural) set of situations.

[leafpetersen]

Also encountered here: dart-lang/language#2216

[lrhn]

In Strong Mode we can rely on types, so if we have an expected context type, we can check (and require) that both branches are assignable to that type. If we don't have a context type, but we are inferring the type from the conditional expression, then getting Object is probably a good thing (you could say it's the default context type) - it means that you can't use it for anything.

This issue is the first case: int x = ... ? ... : ...; - we know that we need to assign the result to int, so we should check that both branches are assignable to int.

We can use union types, but if they get into the type system, they are also propagated, and we need to find places to stop propagating them. This approach instead just rejects more programs up front, if they are known to generate a failing down-cast for at least one of the branches. It solves just this one problem.

That's a change in type checking from being entirely bottom-up to ... not being entirely that (read: I haven't considered all the details yet).

We (sadly) still need an upper-bound-algorithm for the actual static type of the expression when there is no context expectation. This would just allow us to give up and not even try if we know that the result will be a downcast that will definitely fail for one of the branches - instead of generating Object and then failing at runtime. I'd be OK with disallowing such programs.

[eernstg]

This is exactly what we will get if we consider the type of the conditional expression to be the union of the types of the branches. Checking that union type for the context where the data flow goes amounts to the same two checks (or more checks if we have some nested conditional expressions).

This is a case where the "overkill" generalization of taking a loose upper bound (rather than a proper least upper bound) produces a real issue.

[lrhn]

Yes, it is exactly the same in this particular case, but if we introduce the union type as the actual static type of the conditional expression, we also need to handle it being propagated (through parentheses? await? cast? what else?) and eliminated appropriately (we don't want a variable being inferred to have a union type). Instead I'm suggesting to not create a union type, but to type-check the conditional expression in-place with a context provided type. Same effect locally and without the union types.

[floitschG]

I really think we want to push the type context down the two branches. Instantiations, tear-offs and (eventually) number literals benefit from type-contexts and without pushing the type down we would miss out on some convenient patterns:

double x = someBool ? 0 : 1;
List<int> listify(int? x) {
  return x == null ? [] : [x];
}
int Function(int, int) maxOrMin(bool pickMax) {
  return pickMax ? max : min;
}

[leafpetersen]

Yes, it's a bit of a change from the way the spec is currently defined, but we need a notion of typed context anyway for inference, and once you have it, it works extremely well for this and related constructs. The expression e1 ? e2 : e3 is valid in a context of type T and has static type T, if both e2 and e3 are valid in a context of type T.

Similarly, e1 ?? e2 is valid in a context of type T and has static type T if both e1 and e2 are valid in a context of type T.

Probably some other related constructs fall into this as well.

[eernstg]

Issue #29607 reports on the issue discussed here for the case e1 ?? e2. I've closed it as a duplicate of this one.

[eernstg]

Maybe we could try another approach in this kind of situation (where we get an extraordinary loss of typing information due to a detour to a top type): Flag upcasts to a top type when it's caused by a LUB computation.

We could also flag implicit downcasts from a top type differently from those from other types, because there's a difference in nature rather than a difference in degree between the huge subtype fan-out for top types and the tiny fan-out for any other (at least: user-defined) types. (A quick check in a large repository seems to support an estimate that Object has at the very least hundreds of times more subclasses than the next highest; hence, downcasts from a top type is "a wilder guess" than downcasts from any other type).

Both of these (1: LUB yielded top type from two non-top types; 2: implicit downcast from top type) could be lints.

If that is sufficiently helpful then it appears to me to be less disruptive than the other ideas.

[DanTup]

I don't know if this is adding anything, but I just hit an issue that I presume is this. I was calling a constructor that expected an arg of type Either2<bool, CodeActionOptions> but I'd written:

codeActionLiteralSupport != null
    ? Either2<bool, CodeActionOptions>.t2(new CodeActionOptions(DartCodeActionKind.serverSupportedKinds))
    : true,

The true here is not valid, it should've been Either2<bool, CodeActionOptions>.t1(true). I was surprised that it didn't generate any warnings. While testing I also noticed that even String b = false ? "test" : true doesn't warn and its failure isn't even conditional.

Are there any workarounds (eg. lints) that would catch mistakes like this?

[leafpetersen]

Currently, specifying no-implicit-casts in analysis_options.yaml or on the command line is the best solution.

[srawlins]

Null safety also bans code like in the original example. I'm going to close this issue as null safety has us covered.

In terms of changing LUB, in an analyzer mode, or as a language feature, I think that is a broader issue.