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

Explicit Never-related control flow #4054

Open
modulovalue opened this issue Feb 7, 2023 · 6 comments
Open

Explicit Never-related control flow #4054

modulovalue opened this issue Feb 7, 2023 · 6 comments
Labels
lint-proposal P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@modulovalue
Copy link
Contributor

Null safety added the ability to specify that e.g. a function will never return successfully by making it possible to set its return type to the bottom type Never.

void foo() {
  bar();
  // Anything below bar here is known to never be executed because bar returns a value of type Never.
  print("baz");
}

Never bar() {
  throw Exception();
}

This is great because now we can more precisely specify our intent to the type system, and e.g. have the analyzer notify us that no statement that comes after a function returning Never will be executed.

However, this has made a source of implicit control flow explicit and therefore introduced implicit control flow into Dart.

implicit/explicit control flow

By explicit control flow I'm referring to a "style of programming" where, under the assumption that no statement throws, all statements will either be:

  • followed by the next statement that follows them in the source code or
  • will be followed by a different statement that does not necessarily follow it in the source code AND this behavior is explicit because the statement contains some modifier such as return, throw, break or continue.

Example:

int? foo(
  int i
) {
  if (i == 2) {
    return 0;
  } else {
    for (;;) {
      switch(i) {
        case 0:
          break;
        case 2:
          continue;
      }
      break;
    }
    return null;
  }
}

By implicit control flow I'm referring to a "style of programming" where neither the first, nor the second case above can be observed.


In my experience, being able to infer the control flow just from the syntax alone helps with code readability because we don't have to examine any types to see how code flows. Also, types might not always be available such as on github or in a presentation.

I think it would be great if e.g. the linter could help Dart developers maintain the explicit control flow style.

One idea that I had for restoring explicit control flow in the presence of Never within Dart is to require all expression that are known to be Never to be required to be thrown.

Consider the following example that applies this idea tho the first example above:

void foo() {
  throw bar();
  print("baz");
}

Never bar() {
  throw Exception();
}

In this example, all function invocations that are known to return Never are thrown. This has the effect that it restores explicit control flow.

A lint that warns about such implicit control flow (i.e. expressions of type Never that are not thrown) would be needed.

only_throw_errors would not support this idea because it expects only Exceptions and Errors to be thrown. See: #4053

@srawlins
Copy link
Member

srawlins commented Feb 8, 2023

I like this idea. That being said, we're more-or-less only taking on new rules that have the blessing of the style- and language-oriented folks on the Dart team. It could be a while.

@srawlins srawlins added style P3 A lower priority bug or feature request lint-proposal labels Feb 8, 2023
@modulovalue
Copy link
Contributor Author

It could be a while.

@srawlins No worries, thank you for taking a look into this.

@bwilkerson
Copy link
Member

... introduced implicit control flow into Dart.

If I understand you definitions, Dart has always had implicit control flow because of exceptions. Looking at a normal method invocation, it isn't possible to know whether or not it will throw an exception. I don't think it's possible to eliminate implicit control flow without eliminating exceptions: given that it's valid for a method to either return normally or to throw an exception it wouldn't be possible to add an explicit throw before every potentially non-returning method.

@modulovalue
Copy link
Contributor Author

Dart has always had implicit control flow because of exceptions. [...] it wouldn't be possible to add an explicit throw before every potentially non-returning method.

@bwilkerson thank you for pointing this out. I agree with your observation.

Please allow me to differentiate between known implicit control flow and possible implicit control flow.

Functions might throw, so there's always the possibility of encountering possible implicit control flow. However, with the introduction of Never, it became possible to be explicit about implicit control flow by turning it into known implicit control flow. This can be done by, for example, changing the return type of a function to Never.

void foo() {
  throw Exception("...");
}

Never bar() {
  throw Exception("...");
}

void main() {
  // A source of "possible implicit control flow".
  foo();
  // A source of "known implicit control flow".
  bar();
  // "known implicit control flow" made explicit by throwing.
  throw bar();
  // "known implicit control flow" made explicit by returning.
  return bar();
}

To summarize, an expression might introduce:

  • possible implicit control flow: if its static type can't be guaranteed to be Never
  • known implicit control flow: if its static type can be guaranteed to be Never
  • explicit control flow: if it is the expression of a control flow related statement
  • implicit control flow: if it is not the expression of a control flow related statement

I think we should:

Possible Known
Explicit N/A Prefer
Implicit N/A Avoid

Even if it is not possible to eliminate all possible implicit control flow, I think it would be helpful to help prevent known implicit control flow by encouraging expressions that are known to evaluate to a value of type Never to be thrown or returned.

@lrhn
Copy link
Member

lrhn commented Feb 5, 2024

While I can see why explicit control flow can be nice, it's also unnecessarily verbose.

We have "dead code" warnings for code after an unconditional control flow. The throw and return before bar() in the example above are dead code. They'll never execute. They are, by the very definition, unnecessary code. And the return is actively misleading, since it looks like the function can return a value here, when in fact it doesn't.

If a function returns Never, I expect it to be named appropriately, and a call like throwStateError("banana"); or Expect.fail("Test failed!"); is good enough for me to recognize that this thing probably doesn't continue.

With dead-code warnings, there won't be code immediately after a call returning Never, including no return after it, so the only places where one might not immediately guess that a function always throws, from context, is at the end of a void function or at the end of a branch, before a join-point that the other branch can still reach.

It's not a lint that I'd ever use. Not if it makes me write dead code to make up for other people not naming their functions properly. 😉

@modulovalue
Copy link
Contributor Author

I think that, rather than calling it "dead" (as in dead code) or "unnecessary" (as in useless), it would be more useful to call it "redundant".

Redundancy can be bad, but it is not always bad (e.g., important information should perhaps be shared multiple times in different ways to prevent misunderstandings), but I agree that redundancy comes with overhead, and that it is good to justify that overhead.

So the question is: is the upside worth the overhead?

To me, it seems like the answer is yes.

One concrete datapoint that supports my opinion is this issue: #4865. It would have been prevented by a policy that makes known implicit control flow explicit.

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

No branches or pull requests

5 participants