Convert to switch expression/statement assists don't trigger formatting

[FMorschel]

Convert to switch expression/statement assists don't trigger formatting.

Inspired by https://github.com/dart-lang/sdk/issues/56597 I started testing a bit and saw the current behaviour.

Usually, this is not a problem but in big fall-through case lists like the following it is:

switch (v) {
  case 0:
  case 1:
  case 2:
  case 3:
  case 4:
  case 5:
  case 6:
  case 7:
  case 8:
  case 9:
  case 10:
  case 11:
  case 12:
  case 13:
  case 14:
  case 15:
    return v;
  default:
    return 0;
}

return switch (v) {
  0 || 1 || 2 || 3 || 4 || 5 || 6 || 7 || 8 || 9 || 10 || 11 || 12 || 13 || 14 || 15 => v,
  _ => 0
};

Also, if converting it back as is, it still doesn't trigger formatting. But if the cases are already formatted like:

return switch (v) {
  0 ||
  1 ||
  2 ||
  3 ||
  4 ||
  5 ||
  6 ||
  7 ||
  8 ||
  9 ||
  10 ||
  11 ||
  12 ||
  13 ||
  14 ||
  15 =>
    v,
  _ => 0
};

It does keep them in the current line.

[dart-github-bot]

Summary: The "Convert to switch expression/statement" code assist doesn't trigger formatting when converting a switch statement with multiple fall-through cases to a switch expression. This results in a single long line of code, making it difficult to read.

[srawlins]

@bwilkerson @DanTup none of our assists trigger formatting, right?

[DanTup]

That is how I understand it too. I think they're generally written to produce code that is formatted approximately right (and indention is made correct for where the code is inserted), but I don't think there's any actual format step (nor do I think we're likely to produce code that is 100% formatted the same as the formatter will do, because of things like line lengths and wrapping).

(that said, it could be nice if we did :) )

[FMorschel]

Here is one issue about this question https://github.com/dart-lang/sdk/issues/54021.

Here is @lrhn's last comment:

Yep, I think the fix tools should assume that input is formatted, and make sure its own changes are formatted before being written back.

Then dart format & dart fix would be the recommended order. (Really, "dart format all the time", like on save, is what we would be assuming.)

I believe this should be true for assists as well, although there is currently an open issue about making a lint that could use this assist as its quick-fix.

[srawlins]

If we always formatted the whole file after changing one line (for a fix or an assist), what is the workflow for codebases that don't use the formatter? Or who use the formatter but with different options (like line length)? I think it would be a bad workflow.

We can decide to marginalize such codebases. Can we make it an IDE-setting? Always format after applying an assist or a fix? An IDE setting would not apply to dart fix, but we could add a flag to dart fix.

[bwilkerson]

Formatting code before we make edits is an interesting idea, and one we've discussed multiple times.

I don't think we should make the assumption that every user runs the formatter over their code, but even if we don't make that assumption, running the formatter over the code to be inserted wouldn't be likely to cause additional work for users that don't use the formatter and would be an improvement for those that do.

But before we could choose to do that we'd need to first ensure that formatting the code could be done in a performant way. The edits for all assists and fixes are computed before we send them back to the client. If formatting the code weren't efficient, this could introduce an undesirable delay in presenting these options to users. For example, if we needed to format the entire file in order to get a small region formatted, then editing large files might be adversely impacted.

Also, it isn't clear to me that this is a change that we want to try to make incrementally, so we might need to schedule some dedicated time to make it happen. (Though it's possible that most of the work could be done in a generic way, such as in ChangeBuilder, so that it wouldn't require as much of an effort as it initially sounds like it would.)

I believe this should be true for assists as well, although there is currently an open issue about making a lint that could use this assist as its quick-fix.

Fixes and assists differ primarily in terms of whether a diagnostic is required in order for the change to be offered.

Can we make it an IDE-setting?

I have, and would continue to, argue for an option in the analysis options file that tells the language server that the code base is expected to be formatted. I don't think we want it in the IDE because then it's an individual (rather than team) setting.

[DanTup]

I don't think we should format an entire file after applying an assist.

At most, I think we should format the code that will be inserted or a slightly expanded version. We do support "Format Selection" in VS Code where we run the formatter for the whole document (because it's all that it supports), then compute the minimal edits to get from the original source to the resulting source, then discard the edits that are completely outside of the range.

But, there would be some overhead in doing this - we'd need to apply the edits we built to a copy of the file (or at least, enough of a subset of it to be valid to format), format it, then compute the minimal diffs. Right now, we inline the edits for fixes, so we'd end up doing this for all assists/fixes as you moved your cursor around even before you've selected them. I don't think that would be a good idea, so we we were to do this I think we would want to look at making the edits computed lazily (like we do for refactors), when the action is selected.

I do have some other questions if we did this though:

• For fixes/assists that edit other files, do we expect to also format them?
• Is this specific to assists/fixes, or do we expect that all edits server produces should be formatted correctly?
  • For example, if you rename a file or class such that places that reference its name would wrap differently when next formatted, should we be reformatting them as part of those edits?

[DanTup]

btw, it's not a replacement for this idea, but since things aren't always discoverable in VS Code, it may be worth calling out that in VS Code there are a couple of different formatting options/actions:

• format-on-save
• format-on-type (this will trigger a document format when you press certain characters like { or ;)
• format a selection
• format only regions that have changed according to Git so you can format your new code without formatting unchanged code and getting a huge diff

[bwilkerson]

I don't think we should format an entire file after applying an assist.

I agree.

For fixes/assists that edit other files, do we expect to also format them?

If you mean non-Dart files, I think the answer is 'no', if for no other reason then because we don't a formatter for those formats available to us.

Is this specific to assists/fixes, or do we expect that all edits server produces should be formatted correctly?

I'm sure there are many opinions on this one, but I would like to see all edits, including refactorings, handled the same way. I'm a strong believer in having a consistent and predictable UI, and I think that formatting only sometimes might be worse than not formatting at all.

[DanTup]

For fixes/assists that edit other files, do we expect to also format them?

If you mean non-Dart files, I think the answer is 'no', if for no other reason then because we don't a formatter for those formats available to us.

I meant for Dart files. I was thinking it might be weird to apply formatting to unopened files (because the fix modifies a different file) - although as I write this it occurs to me that we're already modifying those files so they'll already be opened by VS Code and the buffer updated with unsaved changes, so additionally formatting those edits isn't actualy that weird.

I'm sure there are many opinions on this one, but I would like to see all edits, including refactorings, handled the same way.

I also think they should probably be the same, but I think I would be surprised if I renamed a class and other files that referenced it had the affected lines wrapped/unwrapped because it took them over/under 80 characters. I don't know if I think that just because I'm not used to it though, because ultimately they'd be formatted when I hit Save All anyway (and since I already use format-on-type, my code is often formatted without me saving anyway).

[FMorschel]

I think I would be surprised if I renamed a class and other files that referenced it had the affected lines wrapped/unwrapped because it took them over/under 80 characters.

I would expect this. Even more so if my file was formatted before the change and now it is out of line length. I would not like to manually open each and every one of them to format them one by one. As it is today.

[bwilkerson]

I also think they should probably be the same, but I think I would be surprised if I renamed a class and other files that referenced it had the affected lines wrapped/unwrapped because it took them over/under 80 characters.

Of course, the described behavior is dependent on just how much of the surrounding code is included in the 'reformat region'. If the region of code subject to being formatted is only the identifier being renamed, then that would cause such files to not be reformatted. Figuring out the right definition of a reformat region might be one of the most difficult parts of an arc of work like this.

[DanTup]

I would expect this. Even more so if my file was formatted before the change and now it is out of line length. I would not like to manually open each and every one of them to format them one by one. As it is today.

To be clear, that's not what happens today if you're using format-on-save. When you rename something, VS Code will open all modified files and apply the rename to their edit buffers (without any format changes). When you hit Save All to save those files (since they're not saved as part of the refactor), the format-on-save would format them all.

Brian makes a good point about the range. Above, I thought we could just format the part of code being edited (as if you'd highlighted the edited range afterwards and done Format Selection), but I don't think that works. Consider this code:

enum A {
  one,
  two_two_two_two_two_two_two_two_two_two_two_two_two_two_two,
  three
}

This enum is across multiple lines because it doesn't fit on one. But If you rename the middle option to not be so long, the formatter would put it in a single line. Reformatting only the edited part of code would not produce that result, we'd likely need to reformat the whole body of the enum.

Similarly, if I remove just the middle part of a name, the edited range would be only a delete in the middle of the word, so the remaining range from the edit would be empty, and a reformat of an empty range would not change anything.

Should we rename this issue to be more general? I think it's more of a request that any code-editing actions should preserve/apply formatting than anything specific to converting to/from switch expressions.

[lrhn]

We could try to define the nearest "unit of formatting" that a change is part of, so that reformatting ony that unit should be enough to ensure a formatted file is also formatted after the change. A statement, a declaration, the list of enum balues (always any list that an edited element is part of, and then maybe the surrouding unit for that.)

The way we format code, formatting one statement shouldn't affect earlier or later statements, or the block it's part of. Formatting a parameter may affect an entire function. Maybe not its body content (unless it's a => body).