`convert to constant pattern` assist in the wrong place

[AbdeMohlbi]

consider this code :

void foo(dynamic value)  => switch (value) {
      String => 'the value is String',
      int => 'The value is an int',
      bool => 'The value is a bool',
      List => 'The value is a List ',
      _ => throw Exception("other types")
    };

this code will trigger convert to constant pattern assist , i guess this is a wrong suggestion in this case beacause the code would became :

void foo(dynamic value)  => switch (value) {
      const (String) => 'the value is String',
      const (int) => 'The value is an int',
      const (bool) => 'The value is a bool',
      const (List) => 'The value is a List ',
      _ => throw Exception("other types")
    };

@FMorschel could u confirm this ?

[dart-github-bot]

Summary: The "convert to constant pattern" assist is incorrectly suggesting to add const to type checks in a switch statement, which would result in invalid code. The assist should not be triggered in this scenario.

[lrhn]

That looks correct. It preserves the behavior of the existing code, while making it more readable what it's actually doing. That's the point of the conversion.

The reason the result looks wrong is likely that the code is wrong, but now you can see it. (The code never worked as intended. It only triggered if the value was a Type object for one of the type String, int, bool or List<dynamic>. The String => code is easily misread as meaning String _ => or String() =>, but it means const (String) => because that's the backwards compatible meaning of case String: in existing pre-pattern switches.)

[AbdeMohlbi]

Hmm ,what's the meaning of String() in this context then ?

[lrhn]

The object pattern String() matches a value of type String. It could check properties of the value too, like String(length: > 4), but it doesn't have to.

[AbdeMohlbi]

Also String _ is triggered by using convert to wild pattern assist is this behavior also correct?

[lrhn]

The String _ is a declaration pattern with a "wildcard" binding, so "convert to wildcard pattern" sounds reasonable.

It changes the meaning of the pattern, but likely to what you would have wanted.

The biggest issue here is how one can figuring out which assist to use. The analyzer gives a warning of

Use 'TypeName _' instead of a type literal.

but it doesn't link that to the "convert to wildcard pattern". Maybe if the message had been

Use the wildcard pattern `String _` instead of a type literal.

it would be easier to know to choose "convert to wildcard pattern".

[AbdeMohlbi]

The String _ is a declaration pattern with a "wildcard" binding, so "convert to wildcard pattern" sounds reasonable.

It changes the meaning of the pattern, but likely to what you would have wanted.

The biggest issue here is how one can figuring out which assist to use. The analyzer gives a warning of

Use 'TypeName _' instead of a type literal.

but it doesn't link that to the "convert to wildcard pattern". Maybe if the message had been

Use the wildcard pattern `String _` instead of a type literal.

it would be easier to know to choose "convert to wildcard pattern".

I guess so , should i close this now then?

[FMorschel]

Use the wildcard pattern `String _` instead of a type literal.

This would be better, yes. And also as mentioned:

I see i searched docs for any simular exemples but found none ,i may add one

I believe this would help a lot of users.

[rrousselGit]

@lrhn I was chatting with @AbdeMohlbi before this issue was made. I think there's a misunderstanding about why this issue was made.

Specifically with regards to:

It preserves the behavior of the existing code, while making it more readable what it's actually doing. That's the point of the conversion.

What @AbdeMohlbi wanted here was not to preserve the original behavior. Because the original behavior was incorrect. They misused patterns and forgot to specify () after types.

They didn't truly want to do Type comparison. They wanted to use pattern matching.

In short, what was expected here was an assist that converts:

switch (any) {
  case Type:
}

into:

switch (any) {
  case Type():
}

I think when we use Type comparisons on anything that's not strictly type, we should suggest converting the Type into patterns instead.

[AbdeMohlbi]

@lrhn I was chatting with @AbdeMohlbi before this issue was made. I think there's a misunderstanding about why this issue was made.

Specifically with regards to:

It preserves the behavior of the existing code, while making it more readable what it's actually doing. That's the point of the conversion.

What @AbdeMohlbi wanted here was not to preserve the original behavior. Because the original behavior was incorrect. They misused patterns and forgot to specify () after types.

They didn't truly want to do Type comparison. They wanted to use pattern matching.

In short, what was expected here was an assist that converts:

switch (any) {
  case Type:
}

into:

switch (any) {
  case Type():
}

I think when we use Type comparisons on anything that's not strictly type, we should suggest converting the Type into patterns instead.

That's what i meant but i guess idon't have enough dart-related-knowledge to say that

[lrhn]

When I check the example program in DartPad, I get two suggestions: "convert to wildcard pattern" and "convert to constant pattern", in that order. (And then some "ignore " here or in the entire file options.)

The first one does what you want. It converts a String type literal pattern to something matching a String. It uses String _ rather than String(), but that shouldn't matter in most cases.

The second one converts the String pattern to const (String), which preserves the existing meaning but gets rid of the warning. That's what you should do if you intended to switch on Type objects, which can happen. (You really shouldn't IMO, but people still do.)

I think having both fixes is a good compromise. By writing just String, it's hard to read and very likely a mistake. There are two ways to fix that: Fix it if it was a mistake, or enforce the existing meaning in a more explicit way if it was intended.

It's not a problem to have the second fix. My only worry is that it's not easy to understand which fix does what, and why.

[FMorschel]

Yes, also in the original example on Discord, the switch was on runtimeType and I'm not sure if maybe we could add a lint to make people avoid that since it's most of the time (if not always) something that the person wouldn't want and that type checking can be achieved the same way by using either the wildcard pattern or by using ().

[FMorschel]

It's not a problem to have the second fix. My only worry is that it's not easy to understand which fix does what, and why.

That's why I agree about adding more explicit cases to the docs for both switch expressions and expressions.

Explaining why this should be done over type checking in most cases.

---

About the fixes, I'm not sure how we can solve this right now. Maybe adding a link to the docs in cases like this similar to what lints do at the end on their names. There is an issue to add a link to the docs on keywords that would help as well: https://github.com/dart-lang/sdk/issues/50085.

[FMorschel]

Here is one linter issue related to this discussion but I don't see any mentions of calling out that the user is switching on runtimeType as I proposed above, so I created https://github.com/dart-lang/linter/issues/5097.