Analyzer does not promote in closures (WAI), but no warning/hint

[matanlurey]

FutureOr<R> run<R>(FutureOr<R> callback()) {
  FutureOr<R> result = callback();
  if (result is Future<R>) {
    // error: The method 'then' isn't defined for the class 'FutureOr<R>'.
    result.then((value) => ...);
  }
}

This works as a workaround:

FutureOr<R> run<R>(FutureOr<R> callback()) {
  FutureOr<R> result = callback();
  if (result is Future<R>) {
    final Future<R> resultFuture = result;
    resultFuture.then((value) => ...);
  }
}

[matanlurey]

I see a similar issue when result is LHS typed dynamic.

[leafpetersen]

Is that really the right repro? That code works for me with bleeding edge analyzer.

What version of the analyzer?

[matanlurey]

Dart VM version: 2.0.0-dev.23.0+google3-v2.0.0.dev.23.0

[leafpetersen]

I don't see that error with that code, and that analyzer version.

[matanlurey]

I'll try and write a tiny reproduction case internally and share with you.

[matanlurey]

I shared an internal case. This is the code that fails for me:

import 'dart:async';

// Created ~roughly from 'class ApplicationRef' from AngularDart.

class ApplicationRefWithFutureOr {
  run<R>(FutureOr<R> callback()) {
    FutureOr<R> result;
    var completer = new Completer<R>();
    Zone.current.run(() {
      try {
        result = callback();
        if (result is Future<R>) {
          // error: The method 'then' isn't defined for the class 'FutureOr<R>'.
          result.then((ref) {
            completer.complete(ref);
          });
        }
      } catch (e) {
        rethrow;
      }
    });
    return result is Future ? completer.future : result;
  }
}

class ApplicationRefWithDynamic {
  run<R>(FutureOr<R> callback()) {
    dynamic result;
    var completer = new Completer<R>();
    Zone.current.run(() {
      try {
        result = callback();
        if (result is Future<R>) {
          // No error, but note in IDEs that 'result' is inferred as dynamic.
          result.then((ref) {
            completer.complete(ref);
          });
        }
      } catch (e) {
        rethrow;
      }
    });
    return result is Future ? completer.future : result;
  }
}

class ApplicationRefWithObject {
  run<R>(FutureOr<R> callback()) {
    Object result;
    var completer = new Completer<R>();
    Zone.current.run(() {
      try {
        result = callback();
        if (result is Future<R>) {
          // error: The method 'then' isn't defined for the class 'Object'.
          result.then((ref) {
            completer.complete(ref);
          });
        }
      } catch (e) {
        rethrow;
      }
    });
    return result is Future ? completer.future : result;
  }
}

[leafpetersen]

Ok, in that code, result is assigned to within a closure, and that disables promotion. You need to use a local variable inside of the closure to get promotion.

class ApplicationRefWithFutureOr {
  run<R>(FutureOr<R> callback()) {
    FutureOr<R> result;
    var completer = new Completer<R>();
    Zone.current.run(() {
      try {
        FutureOr<R> localResult = callback();
        result = localResult;
        if (localResult is Future<R>) {
          // error: The method 'then' isn't defined for the class 'FutureOr<R>'.
          localResult.then((ref) {
            completer.complete(ref);
          });
        }
      } catch (e) {
        rethrow;
      }
    });
    return result is Future ? completer.future : result;
  }
}

[matanlurey]

Ok, in that code, result is assigned to within a closure, and that disables promotion.

Oh, well, I guess... Why? Should the analyzer help here?

[leafpetersen]

In general, because closures can hide mutations to variables that can invalidate promotion.

The code below calls .then on a String. If we allowed promotion of result, this would break the guarantees that the type system is supposed to provide.

 void test(FutureOr<int> callback()) {
  var result;
  var a;
  a = () {
    if (result == null) {
      result = callback();
      if (result is Future<int>) {
        // error: The method 'then' isn't defined for the class 'FutureOr<R>'.
        a();
        result.then((value) => null);
      }
    }
    result = "hello";
  };
  a();
}

void main() {
  test(() => new Future.value(3));
}

[bwilkerson]

Should the analyzer help here?

I'd love for analyzer to be able to help. Making the type system's inference and promotion more visible (less mysterious) is a very desirable goal. I can think of a couple of ways to do that in this case, but both have drawbacks.

We could add a lint when a local variable is not being promoted, but I suspect that there would be a lot of false positives, so I'm not sure anyone would find it useful enough to enable it.

We could provide such information in a less obtrusive way, such as in the hover text you get when you hover over the variable, but (a) that limits the feature to only being useful to users of clients of server, and (b) makes the feature very hard to discover.

If you (or anyone) has other ideas for ways we could support this or related needs, we'd love to discuss them.

[leafpetersen]

One thing that we could consider in the language is to add an "always promote if possible" "is" check (x ISNOW T)?, which would error out if the promotion couldn't be soundly allowed.

The other thing we've discussed is allowing unsound promotion but adding dynamic checks when we can't prove it sound. I don't love this though since it can silently add more runtime checks that you don't expect.

[munificent]

One thing that we could consider in the language is to add an "always promote if possible" "is" check (x ISNOW T)?, which would error out if the promotion couldn't be soundly allowed.

Right. Another way to spin that is to add a flow control structure whose express purpose is type testing + promotion. In other words, something along the lines of pattern matching. (And, if it bound a new variable, that would avoid the issues with promoting a mutated variable.)

[matanlurey]

I guess in this particular case, I would have "done it right" by putting the variable in the closure if I knew that is what I needed to do (it just seems like a bug in the analyzer without any indication otherwise).

[leafpetersen]

This is a difficult one. I've seen an amazing number of bugs filed because people were expecting promotion to happen where it wasn't. It's enough to make me wonder whether the analyzer should just issue a hint whenever promotion gets disabled. But... that could get annoying if you're trying to be hint free. It certainly at least feels like something that we could have an opt-in lint for. cc @pq

[eernstg]

We have discussed a feature where promotion is made explicit by binding a new name:

class ApplicationRefWithFutureOr {
  run<R>(FutureOr<R> callback()) {
    FutureOr<R> result;
    var completer = new Completer<R>();
    Zone.current.run(() {
      try {
        result = callback();
        if (result is Future<R> future) {   // <-- Declare `future`, init. with `result`.
          future.then((ref) {
            completer.complete(ref);
          });
        }
      } catch (e) {
        rethrow;
      }
    });
    return result is Future ? completer.future : result;
  }
}

This e is T id construct declares a new final variable and initializes it to the value of e. It only makes sense for id to be in scope when this construct evaluates to true, so it probably has to be constrained to occur only in if (...HERE...) { .. } and ...HERE... ? e1_which_may_use_id : e2, and maybe a few more places.

The point is that the named typeTest only makes sense when promotion takes place, which means that it will enable developers to explicitly request promotion (so it's an error when promotion cannot take place, but that will probably never happen because this is a final variable, and T can be allowed to be an arbitrary type, not just a subtype of the static type of e).

Also note that the ability to test the value of an arbitrary expression e may allow developers to avoid declaring some other local variables, which means that the id will replace a declaration rather than being an additional one.

This looks like a sensible way to address some of the issues raised here.

[lrhn]

I would personally support "optimistic promotion" where an if (x is Foo) would promote x to Foo in the branch guarded by the test no matter what x refers to (global or instance variable included) and whether there might be assignments to x somewhere. It just have to be a promotion (to a subtype).
Effectively, it's promoting variables in the static type system, not values.

If the variable x is used in a way that requires the promotion (any operation where the validity or static typing depends on the promotion), then the system must check that it still has that type, but the user obviously thought so. That's just a completely normal implicit downcast at runtime (well, on the receiver in this case, but still no new functionality).

If the compiler can analyze that the test isn't necessary, it can omit it. That's the case here - there is no unknown code executed between the test and the use. This will allow the compiler to be arbitrarily smart in removing the runtime check, instead of only allowing promotion when we can specify up-front in the spec that it's certainly safe.
At the least, the test can start by checking that the variable hasn't changed value since the test (a single identical check that will almost always succeed) and skip the type check on success.

(I'd also love to make assignment and promote the variable to the static type of the left-hand side in the following code, but that does require a notion of continuation flow).

[srawlins]

CC @stereotype441 who is working on "not promoted because ..." messaging.