Can yield do an implicit cast from dynamic under NNBD?

[munificent]

NNBD gets rid of implicit downcasts but, as I understand it, it still allows implicit casts from dynamic. My intuition is that yielding a value behaves the same as argument binding or assignment where an implicit cast from dynamic is allowed.

But if I take this:

Stream<int> stream() async* {
  dynamic d = 1;
  yield d;
}

Iterable<int> iterable() sync* {
  dynamic d = 1;
  yield d;
}

And analyze it with:

$ dartanalyzer --enable-experiment=non-nullable temp.dart

I get:

Analyzing /Users/rnystrom/Documents/temp.dart...
  error • The type 'Stream<dynamic>' implied by the 'yield' expression must be assignable to 'Stream<int>'. • /Users/rnystrom/Documents/temp.dart:3:9 • yield_of_invalid_type
  error • The type 'Iterable<dynamic>' implied by the 'yield' expression must be assignable to 'Iterable<int>'. • /Users/rnystrom/Documents/temp.dart:8:9 • yield_of_invalid_type
2 errors found.

@leafpetersen @lrhn @eernstg, is that working as intended? I'm tentatively assigning this as an analyzer bug with the assumption that a dynamic cast should be allowed here, but if not, feel free to close this.

[eernstg]

It is working as specified, but the fact that downcasts under nnbd are only allowed from dynamic makes a difference: The cast from Stream<dynamic> to Stream<T> used to be allowed for all T, but with nnbd in is not.

I agree that it appears inconsistent to make it an error to yield an expression of type dynamic.

We have a similar issue with yield*: It used to be allowed to yield* e where e had static type Iterable<T>/Stream<T> for any T such that this type was assignable to the declared return type, and the dynamic semantics is specified to obtain each element from the yield*'ed entity and possibly fail at that point. The approach under nnbd which is consistent with this would allow us to yield* an expression of type dynamic as well as an expression of type Iterable<dynamic>/Stream<dynamic>.

The nnbd spec has the following:

It is an error if the object being iterated over by a for-in loop has a static
type which is not dynamic, and is not a subtype of Iterable<dynamic>.

I can't see any other rules about for-in statements in the nnbd spec. This means that we allow the per-element downcast from dynamic in a for-in statement (cf. the desugared code of a for-in), so allowing a per-element downcast from dynamic in a yield* again seems to be the consistent choice.

Note that this is very much about the ability to work with data structures whose "element type" is dynamic (the element type would be the type argument of an Iterable/Stream, but could also be the second type argument of a Map, etc.). This may be important in cases like JSON or protobuf processing, where we may encounter dynamically typed data structures containing objects whose types can reasonably be assumed to live up to certain static expectations. In that case it may be both convenient and reasonable to have support for the per-element cast from dynamic.

[lrhn]

NNBD gets rid of implicit downcasts but, as I understand it, it still allows implicit casts from dynamic. My intuition is that yielding a value behaves the same as argument binding or assignment where an implicit cast from dynamic is allowed.

I agree that it should work.

Inside an async* function with return type Stream<T>, the context type of the expression of a yield should be T. It always should have been that, and checking now in dartpad, it seems to be the case for both dart2js and the analyzer.

In any place where Dart 2.0 would do an implicit downcast from dynamic to the context type, Dart 2.<NNBD> should do the same thing. The only difference is that it should now do it only for the static type dynamic, and make all other required downcasts errors instead.

We might have cheated in the spec of the static type and said that yield e is fine if e has type S and Stream<S> is assignable to Stream<T> (the return type), but that's just one more place where the change to assignable means we have to respecify the semantics to still mean the same thing in the new world.

[eernstg]

Cf. dart-lang/language#829.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/136712

[leafpetersen]

We discussed this in December, and in the notes, we indicated that all of the following constructs are (and should continue to be) treated compositionally (by which I roughly mean that the downcasts should be done per-element): for-in, spreads, collection elements, and await.

We noted that yield* is not currently (i.e. in Dart 2.7) treated compositionally. I believe we had left it as an open question as to whether it should be treated compositionally. I seem to recall that there is a concern that treating it compositionally makes it harder to efficiently implement (and it's already not very efficient). At a minimum, an efficient implementation will need to duplicate the relevant helper method, since the cast cannot be done up front.

Example:

Iterable<int> iterable() sync* {
  Iterable<dynamic> d = <dynamic>[1];
  yield* d;
}
void main() {
  iterable(); // Currently fails with a cast error.
}

cc @rakudrama

[lrhn]

It's only the dynamic case that is an issue, and that can be detected statically. So, we can do a special rewrite for that one case. The question is to what?

One reason this is tricky is that yield* forwards errors, so what should it do on an element of the wrong type. Should it throw at the yield* or enter an error into the yielded stream?

An implementation could be yield* (expression).cast<ElementType>().
However that would introduce the cast error into the stream, which does not seem like what we want.

A rewrite like for (var tmp in e) yield e; (with an implicit downcast at the yield) would not forward errors correctly.

So, we'd need a helper function which forwards valid values and errors, and which throws at the yield* if the value has a wrong type.

Keeping it illegal is the simplest option, and we can always make it better in the future. That argues against any rushes changes.

So, if we keep yield* requiring a Stream<ElementType> then the throwing is unavoidable in Dart 2.0 because of implicit downcasts. It will then be a compile-time error in Dart 2.10, and the user can choose how to handle it (cast, unroll, something else).

So, let's not change yield* now because I'm not sure to what, and we don't really have time for it.

[scheglov]

Analyzer was updated to comply with the spec change.

[leafpetersen]

@scheglov per the comment from @lrhn above, I don't think we want to change the yield* behavior yet.

[scheglov]

Sad. Will the language spec change reverted?

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/136814 will partially revert changes to analyzer, so that it continues checking using assignability of Iterable<T> or Stream<T>, and disallow yield* someIterable.

[leafpetersen]

@scheglov thanks. Sorry for the wasted work. Yes, I think we'll want to do a similar partial revert on the spec changes.

[eernstg]

Sorry about the wasted work indeed. Spec changes required to make yield* non-compositional are proposed in PR dart-lang/language#856.