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

No analyzer warning or any other errors when I use a timer wrong with =>. #31863

Open
apwilson opened this Issue Jan 11, 2018 · 6 comments

Comments

Projects
None yet
6 participants
@apwilson
Contributor

apwilson commented Jan 11, 2018

I wrote the following code and was surprised when _updateTime was never called:

new Timer.periodic(
  const Duration(seconds: 1),
  (_) => _updateTime,
);

What the code should have looked like was the following:

new Timer.periodic(
  const Duration(seconds: 1),
  (_) => _updateTime(),
);

The surprising part about this is in the first code set I was returning a function instead of calling it which is invalid as the Timer callback is a void function but I received no run-time or analysis-time errors.

Why I'd expect an error is because this does generate an analysis error which I think is synonymous to the first code set:

new Timer.periodic(
  const Duration(seconds: 1),
  (_) {
    return _updateTime;
  },
);

_updateTime's signature looks like this:

void _updateTime() {
 // do stuff
}
@zanderso

This comment has been minimized.

Show comment
Hide comment
@zanderso
Member

zanderso commented Jan 11, 2018

@zoechi

This comment has been minimized.

Show comment
Hide comment
@zoechi

zoechi Jan 12, 2018

Contributor

I don't think there is anything the analyzer can do, is there?
Timer.periodic expects a function with one parameter and (_) => _updateTime exactly matches that requirement.

There is also nothing wrong with the function

(_) => _updateTime

A function is allowed to contain arbitrary expressions and _updateTime is a perfectly valid expression, it's just not the expression the developer actually wanted, but how what should that be inferred from?

dart-lang/linter#866 might be related.

Contributor

zoechi commented Jan 12, 2018

I don't think there is anything the analyzer can do, is there?
Timer.periodic expects a function with one parameter and (_) => _updateTime exactly matches that requirement.

There is also nothing wrong with the function

(_) => _updateTime

A function is allowed to contain arbitrary expressions and _updateTime is a perfectly valid expression, it's just not the expression the developer actually wanted, but how what should that be inferred from?

dart-lang/linter#866 might be related.

@leafpetersen

This comment has been minimized.

Show comment
Hide comment
@leafpetersen

leafpetersen Jan 12, 2018

Member

This is an unfortunate side effect of the decision to allow arrow functions with a void return type to return values. If you rewrite that callback as a block bodied function with return _updateTime you'll get an error.

I don't immediately see a language fix for this, given that we did decide to allow returning values from void typed arrow functions.

This feels like a really solid case for a hint or a lint though. To me, it seems clear that:

  • using a function typed variable as a statement (without calling it) is almost certainly an error
  • returning a function typed variable from void typed arrow function is almost certainly an error
    • especially if the function typed variable has return type void itself

cc @munificent @pq @bwilkerson

Member

leafpetersen commented Jan 12, 2018

This is an unfortunate side effect of the decision to allow arrow functions with a void return type to return values. If you rewrite that callback as a block bodied function with return _updateTime you'll get an error.

I don't immediately see a language fix for this, given that we did decide to allow returning values from void typed arrow functions.

This feels like a really solid case for a hint or a lint though. To me, it seems clear that:

  • using a function typed variable as a statement (without calling it) is almost certainly an error
  • returning a function typed variable from void typed arrow function is almost certainly an error
    • especially if the function typed variable has return type void itself

cc @munificent @pq @bwilkerson

@lrhn

This comment has been minimized.

Show comment
Hide comment
@lrhn

lrhn Jan 12, 2018

Member

One helping hand could be that the analyzer recognizes that you have a body of a void function with no side-effects. I don't know if the analyzer catches "useless operations" like print; or {x:y}.toList;, generally closurization of a method (which is one thing we know has no side-effect) as the last operation of a statement - but if it does, then the return position of a void .. => function could be treated the same.

Member

lrhn commented Jan 12, 2018

One helping hand could be that the analyzer recognizes that you have a body of a void function with no side-effects. I don't know if the analyzer catches "useless operations" like print; or {x:y}.toList;, generally closurization of a method (which is one thing we know has no side-effect) as the last operation of a statement - but if it does, then the return position of a void .. => function could be treated the same.

@lrhn

This comment has been minimized.

Show comment
Hide comment
@lrhn

lrhn Jun 22, 2018

Member

This is not something we are planning to change in the language right now.
If we revisit void in the future, we might reassess that part as well, but that's not currently planned.

Member

lrhn commented Jun 22, 2018

This is not something we are planning to change in the language right now.
If we revisit void in the future, we might reassess that part as well, but that's not currently planned.

@zoechi

This comment has been minimized.

Show comment
Hide comment
@zoechi

zoechi Jun 22, 2018

Contributor

Should be covered by the linter rule unnecessary_statement dart-lang/linter#1015

Contributor

zoechi commented Jun 22, 2018

Should be covered by the linter rule unnecessary_statement dart-lang/linter#1015

@bwilkerson bwilkerson added type-enhancement and removed type-bug labels Sep 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment