Skip to content
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

common developer bad coding style issue of nullable list iteration using for-loop #59567

Open
raymondmakz opened this issue Nov 12, 2024 · 9 comments
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request

Comments

@raymondmakz
Copy link

Hi everyone,
I am have encounter a problem about a common coding style which potential trigger runtime exception.
But I cannot really tell it is related to dart-core or linter issue.

My dart version is 3.5.1
I have read the 3.7.x lint rule and test case file for omit_obvious_local_variable_types and specify_nonobvious_local_variable_types.
Ref: #58773

Here is the code:

class Fruit
{
  final String name;
  const Fruit(String name) : this.name = name;
  
  @override
  String toString(){
    return '(Fruit name: $name)';
  }
}

List<Fruit> getFruitList(){
  return <Fruit>[Fruit('pear'), Fruit('apple')];
}

List<Fruit>? getNullableFruitList(){
  return <Fruit>[Fruit('pear'), Fruit('apple')];
}

Map<String, Fruit>? getNullableFruitMap(){
  var map = <String, Fruit>{
    'PEAR': Fruit('pear'), 
    'APPLE': Fruit('apple'),
  };
  return  map;
}

void main(){

  var fruitList = getFruitList();
  var nullableFruitList = getNullableFruitList();
  var nullableFruitMap = getNullableFruitMap();

  //Case 1
  for (var fruit in fruitList ?? []){      //$fruit inferenced as dynamic, even fruitList is not null
    print(fruit.name);
    print(fruit.non_exist_prop);     //NO LINT, But Unhandled exception: NoSuchMethodError: Class 'Fruit' has no instance getter 'non_exist_prop'.
  }

  //Case 2
  for (var fruit in nullableFruitList ?? <Fruit>[]){   //use implict typed list, $fruit inferenced as Fruit
    print(fruit.name);
    print(fruit.non_exist_prop);     //LINT, undefined_getter
  }

  //Case 3
  for (var fruit in nullableFruitList ?? []){      //$fruit is inferenced as dynamic
    print(fruit.name);
    print(fruit.non_exist_prop);   //NO LINT, But Unhandled exception: NoSuchMethodError: Class 'Fruit' has no instance getter 'non_exist_prop'.
  }

  //Case 4
  for (var fruit in nullableFruitList ?? []){      //$fruit is still inferenced as dynamic
    print((fruit as Fruit).name);
  }

  //Case 5
  for (Fruit fruit in nullableFruitList ?? []){
    print(fruit.name);
    print(fruit.non_exist_prop);   //LINT, undefined_getter
  }

  //Case 6
  var nonNullableFruitList = nullableFruitList ?? [];   //$nonNullableFruitList is inferenced as List<Fruit>
  for (var fruit in nonNullableFruitList){      //$fruit is inferenced as dynamic
    print(fruit.name);
    print(fruit.non_exist_prop);   //LINT, undefined_getter
  }

  //Case 7
  (nullableFruitList ?? []).forEach((f) {
    print(f.name);
    print(f.non_exist_prop);   //LINT, undefined_getter
  });

  for (var mapEn in nullableFruitMap?.entries ?? {}){}   //$mapEn is inferenced as Object? , because {} is Set<dynamic> 
  for (var mapEn in nullableFruitMap?.entries ?? []){}   //$mapEn is inferenced as Object? , because {} is List<dynamic>
  for (var mapEn in (nullableFruitMap ?? <String, Fruit>{}).entries){}   //$mapEn is inferenced as MapEntry<String, Fruit>

}

Look at case 1, case 3 and case 6.
It seems that the for-loop declaration have its own type inference on null-coalesce logic.

@eernstg
Copy link
Member

eernstg commented Nov 12, 2024

You need to consider the context type of various expressions. In particular, the context type for the iterable of a for-in statement is obtained from the declaration of the iteration variable (or from the existing iteration variable, if it is a for-in statement that doesn't declare this variable, as in for (existingVariable in myList) ...).

You can explore this context type using examples like these ones:

X whatever<X>() {
  print('X is $X');
  return <Never>[] as X;
}

void main() {
  for (int _ in whatever()) {} // 'X is Iterable<int>'.
  for (String _ in whatever()) {} // 'X is Iterable<String>'.
  for (var _ in whatever()) {} // 'X is Iterable<Object?>'.
}

This illustrates that if the type of the iteration variable is T then the context type for the iterable is Iterable<T>. With var there is no context type (it's the "unknown type" _), which means that whatever() is inferred as whatever<Iterable<Object?>>().

In 'Case 1', fruit is then inferred to have type dynamic, which means that you get a dynamic check rather than a compile-time error when you try to call non_exist_prop.

In 'Case 2', you're providing the type argument in <Fruit>[] which means that the type of the iterable as a whole is List<Fruit>.

'Case 3' is like case 1 (the left-hand operand of ?? is now nullable, but this makes no different for the typing of the ?? expression as a whole).

And so on.

The point is that the ?? expression gets a type which is determined in several steps, and some of them will yield an Iterable<dynamic> or List<dynamic>, and this will cause the iteration variable to get an inferred type (when it doesn't have a type annotation of its own) of dynamic, and then you won't get any errors for calling non_exist_prop. Conversely, when the type of the iteration variable isn't dynamic, you get static typing.

In general, if e1 ?? e2 has a context type T then the context type for e1 is T? and the context type for e2 is T. If there is no context type (that is, the context type is _, e.g., var v = e1 ?? e2;) then e1 does not have a context type, and e2 has the non-null variant of the type of e1 as its context type. In the end, the static type of e1 ?? e2 is the least upper bound of the type of e1, removing nullability if possible, and the type of e2. When you have determined the context type you'll know what terms like [] or {} mean, and then you can compute the type of the iterable or map.

It is somewhat confusing because inference of ?? is complicated, but it is generally working as intended.

One part is still surprising, though:

@stereotype441, can you explain why fruit gets the type dynamic rather than Object? in case 1, but whatever() gets inferred as whatever<Iterable<Object?>>() and not whatever<Iterable<dynamic>>()?

@eernstg
Copy link
Member

eernstg commented Nov 12, 2024

The lints shouldn't play a big role here: They're concerned with having or omitting type annotations on local variable declarations, and the interesting question for this code is which type the variable gets when there is no type annotation (so it's not relevant to add a type annotation, or remove it).

The lines marked //LINT are actually not locations where a lint message is emitted, they are locations where a compile-time error is encountered, which is true for all the cases where non_exist_prop is invoked, but the iteration variable doesn't have type dynamic.

@eernstg
Copy link
Member

eernstg commented Nov 12, 2024

By the way, you might want to use something like this in your analysis_options.yaml:

analyzer:
  language:
    strict-raw-types: true
    strict-inference: true
    strict-casts: true

linter:
  rules:
    - avoid_dynamic_calls

@raymondmakz
Copy link
Author

Thanks @eernstg for answering my question.
I gains more insight on the ?? type inference now.
Understand lint rules specify_nonobvious_local_variable_types not main concern this issue.
var xx = someLIst ?? [] is acting differently on function body and inside for-loop declaration trouble me most.

Wait to see any changes in dart 3.6/3.7 release, since i just come across this seem dart core team already had a glance on this matter:
#55436

Sadly, strict-raw-types, strict-inference , strict-casts and even avoid_dynamic_calls can make teammates who work on the project unhappy.

@devoncarew devoncarew added analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 20, 2024
@devoncarew devoncarew transferred this issue from dart-lang/linter Nov 20, 2024
@eernstg
Copy link
Member

eernstg commented Nov 20, 2024

var xx = someLIst ?? [] is acting differently on function body and inside for-loop declaration trouble me most.

Right, it is always a difficult trade off when a language is using implicit criteria to make semantic choices. It's convenient, concise, perhaps even readable in the cases where the implicit effect is as expected and desired. But in the case where it does something unexpected and/or unwanted, it may not be obvious why this "wrong" thing happens, or how the code could be modified in order to do the "right" thing.

In this case it's all about the context type. There is nothing special about being in a function body (which could mean any number of things, too) and being the iterable of a for loop, other than the fact that those situations give rise to different context types for the inference of the ?? expression.

The for statement is slightly special in that an iteration variable declared like var x or final x (with no type annotation) will provide the unknown context type _ which is used during inference of the iterable (in that it gets the context type Iterable<_>), and the element type of the iterable is subsequently used to infer the type of that variable. That is, the information floats in two directions, as opposed to a normal variable declaration where it only flows from the initializing expression into the variable declaration.

However, it is still true that the type inference of someList ?? [] is guided by the context type, and it makes no difference that the resulting type (let's say that it is inferred as someList ?? <Fruit>[] yielding static type List<Fruit>) may subsequently be used to infer the type of the iteration variable (so var f would be inferred as Fruit f).

So you don't have to be worried about magic for statements, but you do need to think about context types if you wish to understand how inference has made certain choices in your code.

@eernstg
Copy link
Member

eernstg commented Nov 20, 2024

I'll reopen this issue because of the confusion about dynamic and Object?.

@chloestefantsova and @stereotype441, can you explain why fruit gets the type dynamic rather than Object? in case 1 in the original post, but whatever() in the context for (var _ in whatever()) {} gets inferred, here, as whatever<Iterable<Object?>>() and not whatever<Iterable<dynamic>>()?

@eernstg eernstg reopened this Nov 20, 2024
@pq pq added the P3 A lower priority bug or feature request label Nov 20, 2024
@stereotype441
Copy link
Member

@eernstg

Sorry I missed this the first time around.

I dug into this in the analyzer implementation, and as far as I can tell, it looks like the spec text in https://github.com/dart-lang/language/blob/main/resources/type-system/inference.md does not precisely match what was implemented.

In particular, the spec says, in the section Grounded constraint solution for a type variable:

The grounded constraint solution for a type variable X with respect to a constraint set C is define as follows:

  • let Mb <: X <: Mt be the merge of C with respect to X.
  • If Mb is known (that is, it does not contain _) then the solution is Mb
  • Otherwise, if Mt is known (that is, it does not contain _) then the solution is Mt
  • Otherwise, if Mb is not _ then the solution is the least closure of Mb with respect to _
  • Otherwise the solution is the greatest closure of Mt with respect to _.

But the implementation does this (differences in bold):

The grounded constraint solution for a type variable X with respect to a constraint set C is define as follows:

  • let Mb <: X <: Mt be the merge of C with respect to X.
  • If Mb is known (that is, it does not contain _) then the solution is Mb
  • Otherwise, if Mt is known (that is, it does not contain _) then the solution is Mt
  • Otherwise, if Mb is not _ then the solution is the least closure of Mb with respect to _
  • Otherwise, if Mt is not _ then the solution is the greatest closure of Mt with respect to _
  • Otherwise the solution is Mb.

See

DartType _chooseTypeFromConstraints(

This behaves identically except in exactly one circumstance: when both Mb and Mt are _, the solution is _ rather than Object?.

Which is a problem because the grounded constraint solution for a type variable is supposed to be a type, not a type schema (meaning it's not supposed to be, or contain, _).

A later step in type inference (which I don't see documented anywhere in our spec documents), attempts to finish off type inference by running the "instantiate to bounds" algorithm to fill in any types that haven't been successfully inferred. That step is here:

var result = _typeSystem.instantiateTypeFormalsToBounds(_typeFormals,

The end result of all of this is that if a type parameter is completely unconstrained, type inference will give it a type based on its bound, and by default a type parameter's bound is dynamic.

So, considering the whatever example, which looks like this:

X whatever<X>() { ... }
...
for (var _ in whatever()) { ... }

The type inference context for whatever() is Iterable<_>. Since whatever has type X Function<X>(), type inference needs to come up with a type X such that X satisfies the context. So it matches X <# Iterable<_>. That produces the constraint X <: Iterable<_>. So Mb = _ and Mt = Iterable<_>, and that results in a grounded constraint solution of Iterable<Object?> (since the greatest closure of _ is Object?). Therefore _ gets an inferred type of Object?.

But in the fruit example, which looks like this:

var fruitList = ...; // Inferred type: `List<Fruit>`
...
for (var fruit in fruitList ?? []) { ... }

Things are slightly different. The type inference context for [] is still Iterable<_>, but now type inference needs to come up with a type E such that List<E> satisfies this context. So it matches List<E> <# Iterable<_>. That produces no constraints, so Mb = Mt = _, and we get into the weird case described above. The grounded constraint solution "type" is _, which then later gets replaced by the bound for E, which is dynamic. So [] gets interpreted as <dynamic>[].

@eernstg
Copy link
Member

eernstg commented Nov 21, 2024

Thanks, @stereotype441, that's a great analysis!

It would surely be a breaking change to adjust the implemented algorithm such that it matches the specified algorithm because some types used to be dynamic and they will now be Object?. However, this is a move in the direction of more static type safety, as well as a slightly simpler and more consistent approach to type inference. So in that sense it is "good breakage". Hopefully it will also fit the description "small breakage". ;-)

It would be possible to bundle this change with a couple of other changes in the same topic area (e.g., dart-lang/language#433). WDYT?

@lrhn
Copy link
Member

lrhn commented Nov 21, 2024

and by default a type parameter's bound is dynamic.

Nit: It's actually Object?, but instantiate to bounds still infers dynamic if there is no declared bound.

We haven't properly specified what a "default bound" means and does, and implementation treats "no bound" as a sepearate thing than defaulting to a default type, so the default bound might not matter.

A place where it does matter is inside the declaration, where member access on a type variable is based on the bound or default bound, not instantiate-to-bounds:

void foo<T, D extends dynamic>(T v1, D v2) {
  v2.arglebargle(); // sure thing.
  v1.arglebargle(); // Error: The method 'arglebargle' isn't defined for the class 'Object?'.
}

But not instantiate to bounds 😢.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

6 participants