Proposal <switch_on_type>

[FMorschel]

<switch_on_type>

Description

Avoid using switch statements on a Type object. Instead, use type checks with pattern-matching expressions like String _ or int().

Details

Using a switch on a Type can lead to bad code that is prone to breaking when class hierarchies change or new types are added. This approach is not type-safe and doesn't take advantage of Dart’s strong static typing system. Instead, it's better to leverage more robust alternatives such as pattern matching (switch with exhaustiveness over sealed types) to handle multiple types in a type-safe manner.

Kind

Guard against errors: This lint guards against the misuse of a switch on Types, promoting more maintainable and safer code.

Bad Examples

void handleType(dynamic variable) {
  switch (variable.runtimeType) {
    case String:
      print('String detected');
    case int:
      print('Integer detected');
    default:
      print('Unknown type');
  }
}

Good Examples

void handleType(dynamic variable) {
  switch (variable) {
    case String _:
      print('String detected');
    case int():
      print('Integer detected');
    default:
      print('Unknown type');
  }
}

Discussion

The inspiration comes from #56763 and #59087.

This is motivated by cases like the SDK issue above where at the Discord discussion previous to it the user was using runtimeType on the switch creating some confusion as to how to use switch patterns. Since in most cases, the user is not trying to actually switch on the runtimeType anymore now that we have expression matching like the above, it should probably be safe for us to call out on that.

If this lint passes, I believe it should probably be under Effective Dart.

Discussion checklist

• List any existing rules this proposal modifies, complements, overlaps or conflicts with.
• List any relevant issues (reported here, the [SDK Tracker], or elsewhere).
• If there's any prior art (e.g., in other linters), please add references here.
• If this proposal corresponds to [Effective Dart] or [Flutter Style Guide] advice, please call it out. (If there isn't any corresponding advice, should there be?)
• If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

[FMorschel]

Here is a CL for this https://dart-review.googlesource.com/c/sdk/+/413600.

[FMorschel]

@devoncarew I think you are someone to ask about this being added to core/recommended. Any takes on this suggestion? The CL suggests replacing the switch in runtimeType that works on type literals for patterns on the base variable so we can be exhaustive.

[devoncarew]

about this being added to core/recommended. Any takes on this suggestion?

Hi! We have a process; it's mostly just creating an issue against package:lints. We then periodically review the suggestions against our rubric for the two sets.

https://github.com/dart-lang/core/tree/main/pkgs/lints#evolving-the-lint-sets

For this specific lint, I'd wonder how often this might occur in practice, the severity for people w/ the 'bad' (pre-lint) code, how long the lint had existed (re: stability / maturity of the impl.), whether it had been tried against larger codebases, ...

[FMorschel]

Thanks! Note: There is no mention of the rubric at the link.

[devoncarew]

It's mostly here: https://github.com/dart-lang/core/tree/main/pkgs/lints#lint-sets, though that section could certainly be expanded (why would something be included? why not?).

[FMorschel]

About your questions, most of them I'm unsure I can answer but:

how long the lint had existed

This is not yet merged, but it is a lint that would encourage the usage of the "new" pattern feature so I'd say that should have some weight.

stability / maturity of the impl.

This is a really simple impl (314 lines added, 0 removed), if the switch is done over runtimeType it triggers. So I'd say there is not much that can go wrong here.

whether it had been tried against larger codebases

It hasn't up until now.

Here are the tests

Would trigger:

void f1(num i) {
  // Triggers
  (switch (i.runtimeType) {
    const (int) => null,
    const (double) => null,
    _ => null,
  });
  // Triggers
  switch (i.runtimeType) {
    case const (int):
      break;
    case const (double):
      break;
    default:
      break;
  }
}
void f2(MyClass i) {
  // Triggers
  (switch (i.runtimeType) {
    const (MyClass) => null,
    _ => null,
  });
  // Triggers
  switch (i.runtimeType) {
    case const (MyClass):
      break;
    default:
      break;
  }
}
class MyClass {
  @override
  Type get runtimeType => int;
}

Would not trigger:

void f(num i) {
  (switch (i) {
    int _ => null,
    double _ => null,
  });
  switch (i) {
    case int _:
      break;
    case double _:
      break;
  }
}

// @dart = 2.19
void f2(num i) {
  switch (i.runtimeType) {
    case int:
      break;
    case double:
      break;
    default:
      break;
  }
}

---

I'm only asking you because at the CL I got these messages:

@srawlins wrote:

The "developer experience" team will discuss whether we want this lint rule and will maintain it. I'll defer the review until we've landed on that decision.

@pq wrote:

It would be great to bump the related issue and see if you can garner some buy-in specifically from folks who think hard about lints adopted by package:lints. If this is advice they'd want to see adopted in the core or recommended sets, it would be a great incentive to take it on.

So after the last comment, I came over to see if I could find who should I ask for opinions.

[pq]

Thanks for following up, @FMorschel.

Sorry if there's any frustration. This has come up a bunch and we've not done a great job of setting expectations on what it takes for a contribution to get accepted (and there is some nuance).

To help with lints, in particular, we did create a bit of process since we were getting a lot of contributions and that's what the notion of status is meant to help with. Some of that is captured in this doc here.

As for this proposal in particular, it would be great to get some input from folks like @natebosch , @lrhn and @munificent.

Thanks for your patience in any event. We've got a lot of balls in the air!

[lrhn]

We already warn about such switch cases if you're not switching on a Type, so the code should already be there.

I think it's probably a good lint. You shouldn't be using Type objects for much of anything, for all the reasons listed here. It's just not good object oriented programming to care about the exact runtime type of an object, because it breaks subtype substitutability.

So +1 from me.

[natebosch]

I was skeptical that this would come up very often, but I do see a number of occurrences on github. (Also a few .runtimeType.toString() which I think we still won't catch)

I think this is worth landing.

[FMorschel]

I could test this too, I think. Not that hard. I agree it would also help

[FMorschel]

Also a few .runtimeType.toString()

I've added that to the lint. Please take a look at the tests to help me find any cases I missed testing, but I think we are covered.

Once you all agree on landing this we can review the examples to add this as a bad one.

---

Edit: to be very explicit. Only calls to toString on runtimeType will trigger this too. Other cases won't.

[FMorschel]

• ☂️ [tracking] rename lints #58924 was just brought to my attention and I'd like to ask your opinion on:

The current suggestion for naming is more neutral:

[Lints should be given a] short name using Dart package naming conventions. Naming is hard but strive to be concise and consistent. Prefer to use the problem as the name, as in the existing lints control_flow_in_finally and empty_catches. Do not start a lint's name with "always", "avoid", or "prefer". Where possible, use existing rules for inspiration and observe the rules of parallel construction.

Should I remove the avoid for this lint? @srawlins @pq

[pq]

Absolutely. Great catch. I'd be inclined to rename to switch_on_runtimeType.

Thank you!

[FMorschel]

Only because @bwilkerson seems a bit confused about this lint on the CL comments:

• This lint is intended to warn users about using runtimeType to evaluate an instance's type.
  • Either by testing against const (MyType);
  • Or by calling toString() and testing on the types written down as strings.
• It is not concerned with using switches on Types (as you suggested by the new test).
  If this is something the team would like, we can rethink this lint to address this too, no problem.
• The suggestion this lint tries to give is that users should use pattern matching on the instance they are calling runtimeType (I'll update it on the CL when I'm back since the existing one seemed to confuse you).

[bwilkerson]

I don't think I was confused about the lint. I think I was asking whether we had the right semantics for it. I asked in the wrong place, though. I should have asked here rather than on the CL.

Some of the comments in this issue are directly related to the use of runtimeType, but others are related to using an expression of type Type as a switch's expression. I'm just wondering why we're limiting the lint to explicit uses of runtimeType given that the only way to get a Type is either directly or indirectly using runtimeType. Isn't the test below, that I suggested on the CL, equally bad?

void f(Type t) {
  switch (t) {
    case int: break;
  }
}

Why would we want to allow that when we disallow the following?

void f(Object o) {
  switch (0.runtimeType) {
    case int: break;
  }
}

[FMorschel]

To be fully fair, we can do:

class MyCustomType implements Type {}

But I'm aware nobody would do this (no reason for it, I don't think).

Why would we want to allow that when we disallow the following?

I'm completely open to reworking this to make this work if you think this is the best course of action. I've never seen anyone do either my above example or your last, but I think it would be reasonable to trigger on anything with a static type of Type.

If you want to discuss this internally or here and tell me the decision I'll work towards making it meet the definitions you prefer. If you think no discussion is needed, please only say so.

[lrhn]

that the only way to get a Type is either directly or indirectly using runtimeType

... or a type variable. I've seen code like

... Foo<T>(...){
  switch (T) {
    case String: ...
    ...
  }
}

And code that tried to parse "$T" to find a type variable, then switch on that string.

I also believe the reflectable package has classes implementing Type.

[FMorschel]

I would also like to know the team opinion on the following for me to rephrase the current message:

• Reduce the use of "Try..." in messaging #60380

[srawlins]

I would also like to know the team opinion on the following for me to rephrase the current message:

I think we can stick to one consistent messaging for now. And if we change the messaging to be more prescriptive, we can do that wholesale, so that we remain consistent.

[FMorschel]

Sorry, I'm not sure I ever got a definitive answer if should I make this trigger for any Type typed variable? Or should I keep this for only runtimeType?

Sorry if this was answered and I didn't understand.

I am concerned, actually, with @lrhn's example of doing "$T". This is also a bit weird, I'd then prefer for the code to test on the actual type against const (TestedType) and not strings, but of course only when there is no getter available for this or something like this.

[srawlins]

I also find @lrhn 's example motivating. It seems like it should not be too hard to expand to look at any switch expression with type Type. Except the name will have to change, so much code re-organizing in the CL. And maybe change name to "switch_on_type". CC @munificent who specced patterns and considered switches and types.

[natebosch]

Changing to "switch_on_type" sounds good to me. Once we have an implementation it might be worth running against a big corpus like our internal code to see if anything surprising comes up.

[FMorschel]

I've updated the CL to trigger for anything assignable to Type.

And code that tried to parse "$T" to find a type variable, then switch on that string.

About these cases, I added a warning to any string interpolation that has this pattern. I think it is safer to assume the user added white spaces or something manually and tests it everywhere than to have a false-negative for things like "$T. (whitespace before).

One case I didn't know how to handle was something like:

void f(Object? o) {
  final type = o.runtimeType.toString();
  (switch (type) {
    'int' => null,
    _ => null,
  });
}

If anyone knows of a lint that handles cases like this, I'd love to take a look at it to implement this here too.

[srawlins]

If anyone knows of a lint that handles cases like this, I'd love to take a look at it to implement this here too.

Meh, I wouldn't worry about this one.

[bwilkerson]

If anyone knows of a lint that handles cases like this, I'd love to take a look at it to implement this here too.

Even if we thought this case was worth worrying about, handling it would require flow analysis, which needs to be a separate arc of work (assuming that we ever get around to doing it).

[FMorschel]

I need one last help here. There is a pkg/linter/test/integration_test.dart failing test about an all.yaml file. Opening it, the first line says:

# Auto-generated options enabling all lints.
# Add these to your `analysis_options.yaml` file and tailor to fit!

But none of the suggested commands through my edits updated this file. I could obviously just place the lint name at the right position alphabetically, but I'd like to know if someone knows what I should run.

I was expecting dart run pkg/linter/tool/generate_lints.dart (that messages.yaml suggests to run) would update this, but I don't know why it didn't.

Also, as a side-note, maybe this failing test could tell us what to run here?

[bwilkerson]

Yeah, the comment really ought to say what to run to generate the file.

Or the file ought to not say "generated" at all, because I don't think there's a generator for this file. It's possible that we lost the generator when moving the linter package into the sdk repository, or we might have never had one. I'm not sure.

If I were in your place I'd just hand-edit the file and call it good.