Skip to content

Analyzer exhaustiveness-checking inconsistent for switch on record #52908

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

Closed
nex3 opened this issue Jul 11, 2023 · 7 comments
Closed

Analyzer exhaustiveness-checking inconsistent for switch on record #52908

nex3 opened this issue Jul 11, 2023 · 7 comments
Labels
legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on

Comments

@nex3
Copy link
Member

nex3 commented Jul 11, 2023

This code:

bool doIt(Object arg1, [Object? arg2]) {
  switch ((arg1, arg2)) {
    case (_, null):
      return true;

    case (_, _?):
    // case (_, _):
      return true;
  }
}

produces this error with dart analyze on Dart 3.0.5:

  error - test.dart:2:6 - The body might complete normally, causing 'null' to be returned, but the return type, 'bool', is a potentially non-nullable type. Try adding either a return or a throw statement
          at the end. - body_might_complete_normally

However, if I uncomment the case (_, _): I get this info instead:

   info - test.dart:8:5 - This case is covered by the previous cases. Try removing the case clause, or restructuring the preceding patterns. - unreachable_switch_case

My expectation is that the first form should work, because it does exhaustively cover all possible cases of the input record. Regardless, it seems like the analyzer should at least be consistent about whether it considers this switch to be exhaustive or not.

@bwilkerson
Copy link
Member

@johnniwinther I don't know who should look at this, but I'm hoping you know.

@johnniwinther johnniwinther self-assigned this Jul 14, 2023
@johnniwinther
Copy link
Member

I'll take a look at it.

@pq pq added the P2 A bug or feature request we're likely to work on label Jul 17, 2023
@johnniwinther
Copy link
Member

The problem is that the flow analysis uses the static type of the switch statement scrutinee to determine whether the switch is exhaustive. Because (Object, Object?) is not an always exhaustive type, it expects the switch to complete normally.

If the switch was written as a switch expression (which is always exhaustive) there is no problem in writing:

bool doIt(Object arg1, [Object? arg2]) => switch ((arg1, arg2)) {
      (_, null) => true,
      (_, _?) => true,
    };

@pq @bwilkerson In this case we probably shouldn't report a lint about the unreachable case, but instead suggest adding a default: case (or rewritting as a switch expression).

@johnniwinther johnniwinther removed their assignment Jul 18, 2023
@nex3
Copy link
Member Author

nex3 commented Jul 18, 2023

I'm surprised to learn that a switch statement's exhaustiveness status is determined only by the type of its argument, and not also by whether a return is required at its position.

@johnniwinther
Copy link
Member

With "a return is required at its position" do you mean that it is the last statement of a non-void method?

cc @munificent

@nex3
Copy link
Member Author

nex3 commented Jul 19, 2023

Well, that or it's in a conditional block that is itself the last statement of a non-void method, and so on.

@munificent
Copy link
Member

Yeah, this is a known annoying pain point. It's a consequence of the fact that we do flow analysis separate from exhaustiveness checking. In a perfect world, we'd do those as one holistic process and you wouldn't run into this situation. I'm going to close this as a dupe of dart-lang/language#2977 and we can resume the discussion there.

The suggestion to treat a switch statement as always-exhaustive if it occurs in a position where a value must be returned is really interesting and one I honestly never considered. Thanks for bringing that up!

@munificent munificent closed this as not planned Won't fix, can't repro, duplicate, stale Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

6 participants