Skip to content
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

Switch expressions assist conversion with OR removes comments #56597

Open
FMorschel opened this issue Aug 29, 2024 · 7 comments
Open

Switch expressions assist conversion with OR removes comments #56597

FMorschel opened this issue Aug 29, 2024 · 7 comments
Labels
analyzer-assist Issues with analysis server assists analyzer-quick-fix analyzer-server area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@FMorschel
Copy link
Contributor

FMorschel commented Aug 29, 2024

From an example posted by @pq in #58862:

switch (character) {
  /// 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 something;

  default:
    return somethingElse;
}

Turns into the following losing all comments:

return switch (character) {
  /// See [General Punctuation]
  /// (http://www.unicode.org/charts/PDF/U2000.pdf).
  0x00A0 || 0x1680 || 0x180E || 0x2000 || 0x2001 || 0x2002 || 0x2003 || 0x2004 || 0x2005 || 0x2006 || 0x2007 || 0x2008 || 0x2009 || 0x200A || 0x200B || 0x2028 || 0x2029 || 0x202F || 0x205F || 0x3000 || 0xFEFF => // Zero width no-break space.
    1,

  _ => 2
};

After formatted:

return switch (character) {
  /// See [General Punctuation]
  /// (http://www.unicode.org/charts/PDF/U2000.pdf).
  0x00A0 ||
  0x1680 ||
  0x180E ||
  0x2000 ||
  0x2001 ||
  0x2002 ||
  0x2003 ||
  0x2004 ||
  0x2005 ||
  0x2006 ||
  0x2007 ||
  0x2008 ||
  0x2009 ||
  0x200A ||
  0x200B ||
  0x2028 ||
  0x2029 ||
  0x202F ||
  0x205F ||
  0x3000 ||
  0xFEFF => // Zero width no-break space.
    1,
  _ => 2
};

A smaller example:

switch (character) {
  /// See [General Punctuation]
  /// (http://www.unicode.org/charts/PDF/U2000.pdf).
  case 0x00A0: // No-break space.
  case 0xFEFF: // Zero width no-break space.
    return 1;
  default:
    return 2;
}

Turns into:

return switch (character) {
  /// See [General Punctuation]
  /// (http://www.unicode.org/charts/PDF/U2000.pdf).
  0x00A0 || 0xFEFF => // Zero width no-break space.
    1,
  _ => 2
};

I believe it should become something along the lines of:

return switch (character) {
  /// See [General Punctuation]
  /// (http://www.unicode.org/charts/PDF/U2000.pdf).
  0x00A0 || // No-break space.
  0xFEFF => // Zero width no-break space.
    1,
  _ => 2,
};

But I think the result being alone is weird in all of the above so I suggest doing:

return switch (character) {
  /// See [General Punctuation]
  /// (http://www.unicode.org/charts/PDF/U2000.pdf).
  0x00A0 || // No-break space.
  0xFEFF // Zero width no-break space.
    => 1,
  _ => 2,
};

Currently, in the formatter, if you have the above, it formats the => in one line and the result in a new one. Will file an issue there and link here.

@dart-github-bot
Copy link
Collaborator

Summary: The Dart switch expression conversion with OR operator removes comments associated with individual cases, leading to code that is less readable and harder to understand. The user suggests preserving comments by placing them after the OR operator, but this results in inconsistent formatting.

@FMorschel
Copy link
Contributor Author

Issue created dart-lang/dart_style#1551

@FMorschel
Copy link
Contributor Author

This also inspired me to create #59529.

@devoncarew devoncarew added area-front-end Use area-front-end for front end / CFE / kernel format related issues. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Aug 29, 2024
@lrhn lrhn added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-assist Issues with analysis server assists and removed area-front-end Use area-front-end for front end / CFE / kernel format related issues. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. labels Aug 29, 2024
@FMorschel
Copy link
Contributor Author

FMorschel commented Aug 29, 2024

Also:

return switch (value) {
  0 || //case 0
  1 || //case 1
  2 //case 2
    =>
    2,
  _ => 3,
};

Becomes:

switch (value) {
  case 0 || //case 0
  1 || //case 1
  2:
    return 2;
  default:
    return 3;
}

So we are still losing the last comment here.

So if you pick the first example and convert it twice you lose every comment there. Really bad if #58862 ever happens.

@scheglov scheglov added analyzer-server analyzer-quick-fix P2 A bug or feature request we're likely to work on labels Aug 29, 2024
@FMorschel
Copy link
Contributor Author

Converting the following does keep the comments, but if converting back it loses the last comment as mentioned above:

switch (value) {
  case 0 || //case 0
        1 || //case 1
        2: // case 2
    return 2;
  default:
    return 3;
}

Just point it out so tests can also be added for this case.

@pq
Copy link
Member

pq commented Aug 30, 2024

Thanks for all of these examples @FMorschel!

/fyi @scheglov (who fixed the associated #54567)

@FMorschel
Copy link
Contributor Author

NP!

He had already seen this issue since he added the priority.

Also, something to keep in mind would be #56602 because as you can see in the first example, it doesn't format the output. I completely ignored it first since it is so easy to format in the IDE but my colleague pointed that when fixing via CLI with the possible future lint this would not be expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-assist Issues with analysis server assists analyzer-quick-fix analyzer-server area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

6 participants