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

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

Open
DartBot opened this issue Aug 7, 2013 · 10 comments
Open

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

DartBot opened this issue Aug 7, 2013 · 10 comments
Labels
lint-request type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented Aug 7, 2013

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
Copy link

sethladd commented Aug 7, 2013

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
Copy link
Member

Added this to the Later milestone.

@clayberg
Copy link

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

@bwilkerson
Copy link
Member

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
Copy link
Author

DartBot commented Apr 4, 2014

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
Copy link
Author

DartBot commented Apr 5, 2014

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
Copy link

kasperl commented Jul 10, 2014

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

@kasperl
Copy link

kasperl commented Aug 4, 2014

Removed Oldschool-Milestone-Later label.

@srawlins srawlins transferred this issue from dart-lang/sdk Jun 8, 2020
@srawlins
Copy link
Member

srawlins commented Jun 8, 2020

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

@eernstg
Copy link
Member

eernstg commented Jun 8, 2020

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 #1124, and it has previously been discussed (e.g., dart-lang/sdk#30352, dart-lang/sdk#34455).

So this issue provides an example which is highly motivating, but other than that it is actually a duplicate of #1124. 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.

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lint-request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants