dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
634 stars 170 forks source link

Explicit Never-related control flow #4054

Open modulovalue opened 1 year ago

modulovalue commented 1 year ago

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:

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 commented 1 year ago

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.

modulovalue commented 1 year ago

It could be a while.

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

bwilkerson commented 9 months ago

... 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 commented 9 months ago

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:

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 commented 9 months ago

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. :wink:

modulovalue commented 9 months ago

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: https://github.com/dart-lang/linter/issues/4865. It would have been prevented by a policy that makes known implicit control flow explicit.