report a warning for returning a value within a forEach() callback

[DartBot]

This issue was originally filed by bslesinsk...@gmail.com

---

The following code is wrong, but looks right. No warning is reported at compile time and no error happens at runtime:

String findFirstError(Map m) {
  m.forEach((k,v) {
    if (!validate(k,v)) {
      return "found a problem with ${k}";
    }
  });
  return "ok";
}

By analogy with regular for loops this looks like it should return early when it finds the first error, but actually the return value from the callback is ignored, so it always returns "ok".

It seems like we should be able to infer that the callback's return type is void, and therefore any return statements within the callback should not return a value.

[sethladd]

This reminds me of https://code.google.com/p/dart/issues/detail?id=73

Regardless, I wonder if we can issue a hint here.

---

Removed Type-Defect label.
Added Type-Enhancement, Area-Analyzer, Triaged labels.

[bwilkerson]

Added this to the Later milestone.

[clayberg]

Removed Priority-Unassigned label.
Added Priority-Medium label.

[bwilkerson]

I'm concerned about the number of false positives such a hint would produce, but it might make sense in some cases, so we might want to investigate.

---

Removed Priority-Medium label.
Added Priority-Low, Analyzer-Hint labels.

[DartBot]

This comment was originally written by bslesinsky...@gmail.com

---

But that's a silly use for forEach. Why doesn't it return void? This code is mistaken and should be fixed.

If you really want to call a function that returns an int in a void context, the warning can easily be suppressed:

void main() {
  Map m = {"a" : 1};
  m.forEach((k, v) { intFunction(k,v) });
}

This is similar to calling a function with a different number of parameters; you can't pass it directly, but must wrap it.

Regarding casting an int function to a void function, warnings are typically reported by the Dart Editor (or compiler) using static analysis. It should use the static type, and therefore no warning should be reported. That's okay; the warning is to detect simpler mistakes by beginners, not every case.

Dart's language spec has things to say about when warnings are reported, so perhaps this means changing the language spec. If it's a good idea then that should be okay.

[DartBot]

This comment was originally written by bsles...@gmail.com

---

That makes things more complicated. However, I don't think we should let language-lawyering get in the way of helping users. It's better to use language-lawyering for good, that is by figuring out how to carve out an exception so that we can warn about mistaken usage. Otherwise there's no way to make progress by doing better static analysis.

[kasperl]

Removed this from the Later milestone.
Added Oldschool-Milestone-Later label.

[kasperl]

Removed Oldschool-Milestone-Later label.

[srawlins]

I think this is a great request for the linter. Accidental return for new users.

[eernstg]

This is actually a tricky case, because the inferred return type of a function literal will be based on what is actually returned in the case where this is a subtype of the return type which is required by the context:

void main() {
  void f(void Function() g) => print(g.runtimeType);
  f(() => 42); // Prints '() => int'.
}

So we won't force the function literal () => 42 to have type void Function() just because that's what f expects, we will allow it to have its "natural" type int Function().

It is not an error to pass a value of type int Function() to a parameter of type void Function()
(indeed, int <: void, and there's nothing unsound in ignoring an integer), but this means that the error that we'd normally get when "returning a value to void" disappears:

void h() {
  return 42; // Error, returning an `int` with return type `void`.
}

So this is actually an example of a case where we need "voidness preservation", which is the extension of the checks concerned with the type void to cases where void is just part of the type (e.g., void Function() vs. int Function(), or List<void> vs. List<int>).

Voidness preservation as a lint is requested in #57770, and it has previously been discussed (e.g., #30352, #34455).

So this issue provides an example which is highly motivating, but other than that it is actually a duplicate of #57770. I won't close it because it could be argued that this special case should be treated separately, but I think it's useful to at least be aware of this connection.