dart-lang / language

Design of the Dart language
Other
2.65k stars 202 forks source link

[Analyzer] Type inference for lists in for loops #2054

Open ciriousjoker opened 2 years ago

ciriousjoker commented 2 years ago

Consider the following code:

final a = ["b", "c"];
for (final letter in a) {
  // letter is a String 👍
}

List<String>? b;
final c = b ?? []; // c is a List<String> 👍
for (final letter in b ?? []) {
  // letter is a dynamic ❌
}
for (final letter in c) {
  // letter is a String 👍
}

Imo, letter should act the same in all for loops. Why does it make a difference for the inferred type if it's in a for loop or in a separate variable?

Is this a bug? If not, what am I missing here?

Dart SDK version: 2.15.1 (stable) (Tue Dec 14 13:32:21 2021 +0100) on "windows_x64"

ciriousjoker commented 2 years ago

Why should c not be a List<String>? []can be any list and if it's used just to make sure an existing list exists then I think it's pretty useful that the result type isn't botched. In your example, I view d as an error too.

Like, I get that [] is technically a List<dynamic> and therefore your examples make sense, I just assumed that the Dart team chose the above mentioned improved type inference on purpose and in that case the inconsistent behavior doesn't make sense.

I guess there is at least one inconsistency in your example. Either c and d should be List<String> (including the ?? version used in a for loop) or all of these should be List<dynamic>.

I just highly prefer List<String> for the aforementioned reasons.

vsmenon commented 2 years ago

The analyzer and cfe infer identically here AFAICT.

Transferring this to the language repo.

leafpetersen commented 2 years ago

This is, unfortunately, working as intended, and unfortunately I don't have any good ideas about how to do better with this. Technically, we're already using heuristics to infer List<String> in the first cases - it doesn't fall out naturally from the inference framework we're in. The issue is that nothing in the language says that given a ?? b, a and b have to have related types. So for example int? x = 3; var a = x ?? "hello" is a valid program. So given b ?? [] from the program above, the "natural" thing to infer would be List<dynamic>, because we would infer List<String> for the LHS, List<dynamic> for the RHS, take the upper bound, and end up with List<dynamic>. Bummer.

In practice, we observed patterns like the above a lot in real code when we were designing Dart 2. So we added a heuristic: if there is no context type for a ?? b, then we use the type of a as the context type when inferring b. This makes inference work as expected for final c = b ?? []. That is, we infer <String>[] for the RHS, and hence List<String> for the entire expression. However, in the for loop, we already have a context type, and we can't have more than one context type, so the heuristic doesn't kick in. That means that the context type for inferring the RHS is List<dynamic>, and the RHS becomes <dynamic>[], and hence the upper bound is List<dynamic>.

I don't think we can avoid this in general without substantially generalizing the inference mechanism.

@stereotype441 there is one thing here that I'm curious whether we could do better with however: specifically in my analysis above (from memory), I'm not sure why we infer with a downwards context of Iterable<dynamic> instead of Iterable<_>, but I think we must be since otherwise I think we would compute <Object?>[] for the RHS? Is this right? If so, I wonder whether we could change this to Iterable<_> without breaking too much code, for minor improvement in the outcome here.

@srawlins would strict inference catch this? It feels like it could/should.

srawlins commented 2 years ago

Unfortunately this is not triggered by strict-inference today, nor by strict-raw-types. I am actually surprised that its not caught by strict-raw-types, which does catch var a = [];. I can take this as a bug for strict-raw-types.

leafpetersen commented 2 years ago

I don't think this is really a raw type issue. It could reasonably be flagged by --no-implicit-dynamic case, and/or by --strict-inference though.

lrhn commented 2 years ago

Would this be solved if we could give the expression of a for-in loop, with no declared variable type, a context type of Iterable<?> instead of Iterable<dynamic>? How hard would that be? (Guessing hard, since we haven't done it for raw types otherwise).