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

control_flow_in_finally could be enhanced to consider functions that return Never #4865

Open
matanlurey opened this issue Feb 2, 2024 · 2 comments
Labels
P2 A bug or feature request we're likely to work on set-recommended Affects a rule in the recommended Dart rule set type-enhancement A request for a change that isn't a bug

Comments

@matanlurey
Copy link
Contributor

In flutter/engine#50255 @jonahwilliams and I debugged a tough "this program doesn't catch errors" bug.

It was basically narrowed down to this minimal program:

void main() async {
  try {
    innerProgram();
  } catch (e, s) {
    exit(1);
  }
}

void innerProgram() async {
  try {
    await runAndFail();
  } finally {
    // BUG: Even if a failure happens, the program will terminate.
    exit(0);
  }
}

void runAndFail() async {
  throw 'Fail';
}

https://dart.dev/tools/linter-rules/control_flow_in_finally appears to be able to philosophically catch this bug.

I wonder if we could enhance it to lint functions that return Never, i.e.:

void main() {
  try {} finally {
    // LINT
    throw 'Big Bad';
  }
}

void main() {
  Never panic() => throw 'Big Bad';

  try {} finally {
    // LINT
    panic();
  }  
}

void main() {
  try {} finally {
    // LINT
    exit(0);
  }
}
@github-actions github-actions bot added the set-recommended Affects a rule in the recommended Dart rule set label Feb 2, 2024
@lrhn
Copy link
Member

lrhn commented Feb 2, 2024

The lint description does not say whether throw counts as control flow.
Since any function call can throw, it's hard to disallow completely, and a function returning Never is basically the same as a throw.
If a second error happens, it's perhaps fair that an original error gets clobbered and lost. After all, there is still some error.

Also, it sometimes makes sense to throw during clean-up, like

  var resource = alloc();
 try {
  ...
 } finally {
  respurce.dispose();
  if (!resouce.isDisposed) throw SomeException("Resource not disposed");
 }

or similar. So we probably don't want to warn about just any throw.

So, if anything, it might make sense to warn if a finally block always throws.
Warn if control flow cannot reach the end of the finally block. That should catch any onconditional control flow or Never-function-calls.

Any non-error control flow should still be warned about, because those can also hide an error, and not behind a second error.

@modulovalue
Copy link
Contributor

I've proposed the idea of explicit control flow here: #4054.

Under a policy that enforces "explicit control flow" exit(...) would have been highlighted as implicit control flow. Fixing implicit control flow would have caused throw_in_finally/control_flow_in_finally to kick in which would have prevented this bug.

Any type of implicit control flow can, in my experience, lead to similar bugs like the one that motivated this issue. I claim, that the issue is not with control_flow_in_finally, but with allowing Never-related implicit control flow.

@pq pq added the P2 A bug or feature request we're likely to work on label Feb 14, 2024
@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on set-recommended Affects a rule in the recommended Dart rule set type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants