consider a new lint: use_switch_expressions

[pq]

We might consider a lint to encourage the use of switch-expressions.

BAD

Color shiftHue(Color color) {
  switch (color) {
    case Color.red:
      return Color.orange;
    case Color.blue:
      return Color.purple;
    case Color.green:
      throw 'Green is bad';
    case Color.yellow:
      return Color.green;
  }
}

GOOD

Color shiftHue(Color color) {
  return switch (color) {
    Color.red => Color.orange,
    Color.orange => Color.yellow,
    Color.yellow => Color.green,
    Color.green => throw 'Green is bad'
    Color.blue => Color.purple,
    Color.purple => Color.red
  };
}

[bwilkerson]

Not sure what the rule here would be. Would it look for switch statements where every case returns a value and there's no code following the case (other, perhaps, than a return statement to return the default when there's no default case)? Are there other patterns you have in mind?

[pq]

Yep. That's the pattern I have in mind. Maybe a bit specific but I do see a bunch of candidates for conversion in the SDK.

Here are a few as food for thought.

Future<MacroExecutor> start(SerializationMode serializationMode,
    CommunicationChannel communicationChannel, String program,
    [List<String> arguments = const []]) {
  switch (communicationChannel) {
    case CommunicationChannel.stdio:
      return _SingleProcessMacroExecutor.startWithStdio(
          serializationMode, program, arguments);
    case CommunicationChannel.socket:
      return _SingleProcessMacroExecutor.startWithSocket(
          serializationMode, program, arguments);
  }
}

int firstQuoteLength(String first, Quote quote) {
  switch (quote) {
    case Quote.Single:
    case Quote.Double:
      return 1;

    case Quote.MultiLineSingle:
    case Quote.MultiLineDouble:
      return lengthOfOptionalWhitespacePrefix(first, /* start = */ 3);

    case Quote.RawSingle:
    case Quote.RawDouble:
      return 2;

    case Quote.RawMultiLineSingle:
    case Quote.RawMultiLineDouble:
      return lengthOfOptionalWhitespacePrefix(first, /* start = */ 4);
  }
}

ErrorToken buildUnexpectedCharacterToken(int character, int charOffset) {
  if (character < 0x1f) {
    return new AsciiControlCharacterToken(character, charOffset);
  }
  switch (character) {
    case unicodeReplacementCharacter:
      return new EncodingErrorToken(charOffset);

    /// See [General Punctuation]
    /// (http://www.unicode.org/charts/PDF/U2000.pdf).
    case 0x00A0: // No-break space.
    case 0x1680: // Ogham space mark.
    case 0x180E: // Mongolian vowel separator.
    case 0x2000: // En quad.
    case 0x2001: // Em quad.
    case 0x2002: // En space.
    case 0x2003: // Em space.
    case 0x2004: // Three-per-em space.
    case 0x2005: // Four-per-em space.
    case 0x2006: // Six-per-em space.
    case 0x2007: // Figure space.
    case 0x2008: // Punctuation space.
    case 0x2009: // Thin space.
    case 0x200A: // Hair space.
    case 0x200B: // Zero width space.
    case 0x2028: // Line separator.
    case 0x2029: // Paragraph separator.
    case 0x202F: // Narrow no-break space.
    case 0x205F: // Medium mathematical space.
    case 0x3000: // Ideographic space.
    case 0xFEFF: // Zero width no-break space.
      return new NonAsciiWhitespaceToken(character, charOffset);

    default:
      return new NonAsciiIdentifierToken(character, charOffset);
  }
}

  static String idToString(Id id, String value) {
    switch (id.kind) {
      case IdKind.member:
        MemberId elementId = id as MemberId;
        return '$memberPrefix${elementId.name}:$value';
      case IdKind.cls:
        ClassId classId = id as ClassId;
        return '$classPrefix${classId.name}:$value';
      case IdKind.library:
        return '$libraryPrefix$value';
      case IdKind.node:
        return value;
      case IdKind.invoke:
        return '$invokePrefix$value';
      case IdKind.update:
        return '$updatePrefix$value';
      case IdKind.iterator:
        return '$iteratorPrefix$value';
      case IdKind.current:
        return '$currentPrefix$value';
      case IdKind.moveNext:
        return '$moveNextPrefix$value';
      case IdKind.implicitAs:
        return '$implicitAsPrefix$value';
      case IdKind.stmt:
        return '$stmtPrefix$value';
      case IdKind.error:
        return '$errorPrefix$value';
    }
  }

protocol.ElementKind protocolElementKind(DeclarationKind kind) {
  switch (kind) {
    case DeclarationKind.CLASS:
      return protocol.ElementKind.CLASS;
    case DeclarationKind.CLASS_TYPE_ALIAS:
      return protocol.ElementKind.CLASS_TYPE_ALIAS;
    case DeclarationKind.CONSTRUCTOR:
      return protocol.ElementKind.CONSTRUCTOR;
    case DeclarationKind.ENUM:
      return protocol.ElementKind.ENUM;
    case DeclarationKind.ENUM_CONSTANT:
      return protocol.ElementKind.ENUM_CONSTANT;
    case DeclarationKind.EXTENSION:
      return protocol.ElementKind.EXTENSION;
    case DeclarationKind.FIELD:
      return protocol.ElementKind.FIELD;
    case DeclarationKind.FUNCTION:
      return protocol.ElementKind.FUNCTION;
    case DeclarationKind.FUNCTION_TYPE_ALIAS:
      return protocol.ElementKind.TYPE_ALIAS;
    case DeclarationKind.GETTER:
      return protocol.ElementKind.GETTER;
    case DeclarationKind.METHOD:
      return protocol.ElementKind.METHOD;
    case DeclarationKind.MIXIN:
      return protocol.ElementKind.MIXIN;
    case DeclarationKind.SETTER:
      return protocol.ElementKind.SETTER;
    case DeclarationKind.TYPE_ALIAS:
      return protocol.ElementKind.TYPE_ALIAS;
    case DeclarationKind.VARIABLE:
      return protocol.ElementKind.TOP_LEVEL_VARIABLE;
  }
}

protocol.ElementKind getElementKind(search.DeclarationKind kind) {
      switch (kind) {
        case search.DeclarationKind.CLASS:
          return protocol.ElementKind.CLASS;
        case search.DeclarationKind.CLASS_TYPE_ALIAS:
          return protocol.ElementKind.CLASS_TYPE_ALIAS;
        case search.DeclarationKind.CONSTRUCTOR:
          return protocol.ElementKind.CONSTRUCTOR;
        case search.DeclarationKind.ENUM:
          return protocol.ElementKind.ENUM;
        case search.DeclarationKind.ENUM_CONSTANT:
          return protocol.ElementKind.ENUM_CONSTANT;
        case search.DeclarationKind.FIELD:
          return protocol.ElementKind.FIELD;
        case search.DeclarationKind.FUNCTION:
          return protocol.ElementKind.FUNCTION;
        case search.DeclarationKind.FUNCTION_TYPE_ALIAS:
          return protocol.ElementKind.FUNCTION_TYPE_ALIAS;
        case search.DeclarationKind.GETTER:
          return protocol.ElementKind.GETTER;
        case search.DeclarationKind.METHOD:
          return protocol.ElementKind.METHOD;
        case search.DeclarationKind.MIXIN:
          return protocol.ElementKind.MIXIN;
        case search.DeclarationKind.SETTER:
          return protocol.ElementKind.SETTER;
        case search.DeclarationKind.TYPE_ALIAS:
          return protocol.ElementKind.TYPE_ALIAS;
        case search.DeclarationKind.VARIABLE:
          return protocol.ElementKind.TOP_LEVEL_VARIABLE;
        default:
          return protocol.ElementKind.CLASS;
      }
    }

  WatchEventType _convertChangeType(watcher.ChangeType type) {
    switch (type) {
      case watcher.ChangeType.ADD:
        return WatchEventType.ADD;
      case watcher.ChangeType.MODIFY:
        return WatchEventType.MODIFY;
      case watcher.ChangeType.REMOVE:
        return WatchEventType.REMOVE;
      default:
        throw StateError('Unknown change type: $type');
    }
  }

  DartType instantiate({
    required NullabilitySuffix nullabilitySuffix,
  }) {
    switch (nullabilitySuffix) {
      case NullabilitySuffix.question:
        return NeverTypeImpl.instanceNullable;
      case NullabilitySuffix.star:
        return NeverTypeImpl.instanceLegacy;
      case NullabilitySuffix.none:
        return NeverTypeImpl.instance;
    }
  }

  @override
  TypeImpl withNullability(NullabilitySuffix nullabilitySuffix) {
    switch (nullabilitySuffix) {
      case NullabilitySuffix.question:
        return instanceNullable;
      case NullabilitySuffix.star:
        return instanceLegacy;
      case NullabilitySuffix.none:
        return instance;
    }
  }

  factory Variance._fromEncoding(int encoding) {
    switch (encoding) {
      case 0:
        return unrelated;
      case 1:
        return covariant;
      case 2:
        return contravariant;
      case 3:
        return invariant;
    }
    throw ArgumentError('Invalid encoding for variance: $encoding');
  }

[pq]

/fyi @munificent

[munificent]

I like it! Not sure if it makes sense to always encourage switch expressions, since those could potentially look weird if they get huge, but it's worth experimenting with.

Another pattern to recognize might be a variable declaration immediately followed by a switch statement where every case initializes it.

[pq]

It'd be good to get a bit more feedback on this proposal.

/fyi @eernstg @lrhn @bwilkerson @stereotype441 @srawlins

[bwilkerson]

Not sure if it makes sense to always encourage switch expressions, ... but it's worth experimenting with.

Sounds like it might be too early for a lint rule, based on the lack of information about whether and when users would want to be using an expression.

But that won't prevent us from having an assist in place. And implementing the assist might provide some insights about what usage patterns the lint should or shouldn't report.

[pq]

As per a conversation w/ @kevmoo and @jacob314, some good motivation in flutter/flutter#125930.

[FMorschel]

[...] implementing the assist might provide some insights about what usage patterns the lint should or shouldn't report.

It's been more than a year since it was implemented, would it be a good time to analyze it? I, for one, found out about the assist only yesterday but it was something I had been looking for about some time.

Also, why not create the lint and don't add it to any default set for some time? I think that will help see if users would like something like that or not. And if they find any "false negatives" they would report here.

[eernstg]

I suppose it hasn't been implemented at this time (nothing in here), but it sounds good!

It would presumably rely on detecting a specific kind of switch statement, and transforming it into a switch expression. For example, "every case body is a single return statement" or "every case body is an assignment to the same variable (or perhaps the same "small" set of local variables where we could make it (v1, v2, ...) = switch (e) { ... => (e1, e2, ...), ...}).

If a particular switch statement is very nearly a match to one of these cases, the lint could give hints like "if you can change line 235-236 to be a single return statement then this switch statement can be transformed into return S; where S is a switch expression".

[FMorschel]

Also, one more thing. The current in-place assist doesn't know how to deal with cascading cases like:

T a<T>(int x) {
  switch (x) {
    case 0:
      return 0 as T;
    case 2:
    case 1:
      return 1 as T;
    default:
      return throw UnimplementedError();
  }
}

Only shows if-else option:

Works just fine (empty lines so you can see the case 2 is missing):

Should I create a new issue or reporting it here is enough?

[FMorschel]

About the discussion proposed by the issue title. In one of my projects, I have more than 30 switch cases with only returns (have not looked into variable assigning yet) scattered through more than a dozen files. So, at least for me, this would be great for reducing file lines and easing readability!

[lrhn]

to encourage the use of switch-expressions.

(... instead of switch statements, where possible.)

With that goal, and considering the examples, there are a number of patterns that can become switch expressions.
Whether they're better or worse than the original code might depend on the user, so I'm not necessarily sold on it being a lint. An "info" saying that this thing can be converted to a switch expression, and a quick-fix to do it (and then undo to revert if it's not better) might be sufficient, if that's possible.

The patterns to convert would be anywhere a shared context can be hoisted out of the switch.

A minimal requirement is that the switch is exhaustive, contains no break or continue (or only a legacy trailing break), and the cases differ only in a single expression, and the context type of the expression is the same in each case. (Or a case throws: A branch with a single expression statement where the expression has type Never, can be ignored in the hoisting. Not so for continue or break statements.)

The simple examples would be:

• All non-throwing branches have the form case ...: return someExpression;. Then the conversion would be return switch (...) { ... => someExpression, ...} . If the return statement becomes the only statement of a function body, it can be converted to => too. (Maybe always do that, for a switch expression, it likely is more readable using =>.)
• All non-throwing branches have the form case ...: theVar = someExpression;. Then the conversion would be theVar = switch (...) { ... => someExpression, ...};.

but any recognizable repeated expression context, with the same context type, can be hoisted.
Example:

List<int> values = []
for (var e in someValues) {
  switch (e) {
    case int _: values.add(e);
    case double d: values. add(d.toInt());
    case String s: values.add(int.parse(s));
    default: throw UnsupportedError("Unexpected: $e");
  }
}

can be converted to:

List<int> values = []
for (var e in someValues) {
 values.add(switch (e) {
    int _ => e,
    double d => d.toInt(),
    String s => int.parse(s),
    _ => throw UnsupportedError("Unexpected: $e"),
  });
}

(If we had switch elements, it could be even prettier.)

And as pointed out, multiple patterns for the same expression, like the statement cases case p1: case p2: ctx(body);, isn't allowed for switch expressions, so it must be converted to p1 || p2 => body,, like default: is converted to _ =>.

Then it's a question of limiting ambition. Because if we're really clever we can do fancy stuff.

This example doesn't work directly, because of the elementId and classId declarations.

 switch (id.kind) {
      case IdKind.member:
        MemberId elementId = id as MemberId;
        return '$memberPrefix${elementId.name}:$value';
      case IdKind.cls:
        ClassId classId = id as ClassId;
        return '$classPrefix${classId.name}:$value';
      case IdKind.library:
        return '$libraryPrefix$value';
      // ...
      case IdKind.error:
        return '$errorPrefix$value';
    }

If we start by inlining those, then it could potentially be rewritten as:

  return '${switch (id.kind) {
    IdKind.member =>  '$memberPrefix${(id as MemberId).name}',
    // ...
    IdKind.error => '$errorPrefix';
  })$value';

That's ambitious in that it recognizes that part of an interpolation differs, and it extracts that part into a separate string. Most likely we shuld just hoist the return and ;, and not try to refactor interpolations into having intermediate values.

For

Future<MacroExecutor> start(SerializationMode serializationMode,
    CommunicationChannel communicationChannel, String program,
    [List<String> arguments = const []]) {
  switch (communicationChannel) {
    case CommunicationChannel.stdio:
      return _SingleProcessMacroExecutor.startWithStdio(
          serializationMode, program, arguments);
    case CommunicationChannel.socket:
      return _SingleProcessMacroExecutor.startWithSocket(
          serializationMode, program, arguments);
  }
}

it could hoist everything but the constructor itself:

Future<MacroExecutor> start(SerializationMode serializationMode,
    CommunicationChannel communicationChannel, String program,
    [List<String> arguments = const []]) =>
  (switch (communicationChannel) {
    CommunicationChannel.stdio => _SingleProcessMacroExecutor.startWithStdio,
    CommunicationChannel.socket => _SingleProcessMacroExecutor.startWithSocket,
 })(serializationMode, program, arguments);

That again requires a refactoring where what remains wasn't an expression in the original statement, converting a constructor invocation into a constructor-tear-off + function invocation.
Probably also a step too far, and we should be happy just hoisting the return here too.

Maybe just aim for a few easy patterns where the expression is easily found, without trying to look too hard for it.
Maybe, paraphrased:

• Statements are E; or return E;.
• Expressions are:
  • E(.selector*)(= expr)?
  • primary(.selector)* = E
  • primary(.selector)*<typeArgs>?(..., (id:)? E, ...)(.selector)*(= expr)?
  • primary(.selector)*[E](.selector)*(= expr)?
  • (new|const)? constructor(..., (id:)? E, ...)
  • <typeArgs>?[..., (for/if/...*) E, ...]
  • <typeArgs>?{..., (for/if/...*) E, ...}
  • <typeArgs>?{..., (for/if)* E: expr, ...}
  • <typeArgs>?{..., (for/if)* expr: E: ...}

That is: Check expressions that are (recursively) collection literals, where with an expression that is an element or spread-iterable/map, or a map entry key/value, a selector chain with an expression that is at the head, as a method argument or [] operand, or the part being assigned, or as an argument constructor invocation. If all the cases agree on such a structure, only differing on one E expression, then recurse on that expression to find more agreement, or not. Hoist everything they agree on, leave the expressions they don't.

[bwilkerson]

I, for one, found out about the assist only yesterday but it was something I had been looking for about some time.

How could we have done a better job of letting you know about the assist?

I think that will help see if users would like something like that or not.

We generally need to see some interest from the community before we spend the time to write and maintain a lint.

It would presumably rely on detecting a specific kind of switch statement, and transforming it into a switch expression.

Yes. And that's precisely what the assist does as well. It's reasonable for the lint to report cases for which there's no way to automate the conversion, and it's reasonable for the assist to work in places that the lint doesn't report, but where they overlap they should agree.

If a particular switch statement is very nearly a match to one of these cases, the lint could give hints like "if you can change line 235-236 to be a single return statement then this switch statement can be transformed into return S; where S is a switch expression".

I suspect that for some users such a hint would be too noisy and might discourage them from enabling the lint, and for others it might be confusing and encourage them to break their code. There's a fairly big difference between a lint that helps the user see where a language construct (like a switch expression) would be a cleaner and semantically equivalent alternative and a lint that tries to get users to modify their code in potentially breaking ways.

The current in-place assist doesn't know how to deal with cascading cases like:

Yes, please open an issue in the sdk issue tracker where we can discuss the code pattern you'd like to see the assist produce in this case.

Whether they're better or worse than the original code might depend on the user, so I'm not necessarily sold on it being a lint.

As I noted above, it's fine for the assist to support cases that the lint doesn't flag.

An "info" saying that this thing can be converted to a switch expression, and a quick-fix to do it (and then undo to revert if it's not better) might be sufficient, if that's possible.

I'm not sure what you mean by "info", so I'm not sure whether it's possible.

Maybe just aim for a few easy patterns where the expression is easily found, without trying to look too hard for it.

Yep. That's what we did for the assist, though not for as many cases as you listed. It's interesting to ask the question of whether the cases we don't yet support occur often enough that it would add sufficient value to the tooling to justify the cost of implementing and maintaining the additional code. Insight and opinions welcome.

@pq

[FMorschel]

How could we have done a better job of letting you know about the assist?

Maybe by telling people about it in some public place like the medium release post (the post for this feature) or some (other/second) minor post about cool small changes made (so that original release post is not so long if that's a thing you worry about).

Maybe by placing it on the site somewhere (tracking here).

I found out about it by asking on the Dart Community Discord server, at the help chat by asking for a feature suggested by this issue (then when answered, that there was no such thing, I found this as well).

---

Yes, please open an issue in the sdk issue tracker where we can discuss the code pattern you'd like to see the assist produce in this case.

Will do. (Edit: just did - #55861)

I've already seen some other related issues with other lints not applying to this structure and filled them here. Could someone take a look at them and move them there (if this is not the right place, although they are here because these are with about already existing lints)?

• https://github.com/dart-lang/linter/issues/4980

Mainly take a look at these comments because I'm not sure if fixing the lints will solve the assist on its own, or if the assist should do that in the first place or not:

• require_trailing_commas is not triggered on switch expressions #57138
• unnecessary_cast quick-fix unexpected result in switch expression #56072

[bwilkerson]

Thanks. I took a look at the referenced issues and I think they're in the right place.

The one possible exception is the fix for trailing commas, but the lint would have to be changed first, so we can wait to see whether the lint changes before worrying about the fix.

[DanTup]

How could we have done a better job of letting you know about the assist?

I don't know if it's appropriate here, but I thought it may be useful to share this. In TS, sometimes there are recommendations that are diagnostics with a "hint" severity that have fixes. Unlike normal code actions, they are visible in the editor. Unlike errors/warnings/info, they do not show up in the Problems view, and instead of a full squiggle, they just have a very short grey marker.

info_diagnostic.mp4

These are easy to ignore, but they're more discoverable than general code actions that you might not see unless you happen to put your cursor in the right place.

I wonder if it would be useful to support "hint" as an option in analysis_options like this:

analyzer:
  errors:
    dead_code: hint

It could be a non-committal way of using a lint.. you're not forced to use it everywhere or put ignores all over your code, but you'll still see some minor prompts to use the fixes in your code.

[bwilkerson]

Definitely something worth considering. I could see it helping in some cases, but I'm less certain about others. Specifically, I wonder about assists that are reversible, like the ones that convert between block and expression bodies for functions/methods. Would it be annoying to have a hint on every expression bodied function that it could be converted to a block? (I think it would be, but others might not feel that way.)

[DanTup]

I think it would be

I agree - I was thinking if there are cases where there is a recommendation in one direction. For example the lint described here, where you might want a preference for switch expressions, but without enabling a lint that may trigger in cases that you deem are not better converted. It would only show up in one direction and not "recommend" switching back from switch expressions.

But even if it's not appropriate here, it may be worth knowing about in case it's a good fit for something else in future :-)

[bwilkerson]

... you might want a preference for switch expressions ...

So an optional 'nudge' rather than a lint, where the nudge is to think about applying an assist.

@anderdobo

[pq]

So an optional 'nudge' rather than a lint, where the nudge is to think about applying an assist.

I would love such a facility. We've talked about this a bunch wrt to nudging folks towards adopting language features in general. It could really help with discovery and encourage experimentation.

[FMorschel]

I just want to point out that if this new lint ever comes into existence, and you want to use the current assist as a quick-fix, #56597 would really need to be fixed first. If not, cases mentioned by pq's above comment would lose most if not all comments.

switch (character) {
  case 0: // Some comment
  case 1: // Last comment
    return 1;
  default:
    return 2;
}

Currently converts to:

return switch (character) {
  0 || 1 => // Last comment
    1,
  _ => 2
};

And when converting back:

switch (character) {
  case 0 || 1:
    return 1;
  default:
    return 2;
}

More details in the issue mentioned above.

[FMorschel]

So an optional 'nudge' rather than a lint, where the nudge is to think about applying an assist.

I would love such a facility. We've talked about this a bunch wrt to nudging folks towards adopting language features in general. It could really help with discovery and encourage experimentation.

I'd like to mention here that there may be other lints that could benefit from this behaviour like #59454 for example (the issue is to warn when a function/method is marked as async and has no await nor returns Future so it could be set to a synchronous function). If this was a nudge, I would expect that the only private methods/functions limit could be avoided since then this would not be considered the same as a "problem".