Analysis server not respecting options in textDocument/formatting

[jonas-jonas]

• Dart VM version: 2.4.0 (Unknown timestamp) on "linux_x64"
• Linux (Ubuntu 19.04)

When sending a textDocument/formatting request to the analysis server it doesn't seem to respect the options parameter at all. Is this intended?

More info

Communications between client and server:

[t=1562353069066] LSP4E to org.eclipse.dartboard.server:
{"jsonrpc":"2.0","id":"15","method":"textDocument/formatting","params":{"textDocument":{"uri":"file:///home/jonas/Workspace/runtime-EclipseApplication/test/main.dart"},"options":{"tabSize":4,"insertSpaces":false}}}

{"id":"15","jsonrpc":"2.0","result":[{"range":{"start":{"line":0,"character":0},"end":{"line":5,"character":0}},"newText":"import 'test.dart' as test;\n\nvoid main() {\n  print(\"asd ${test.x}\");\n}\n"}]}

As you can see, before print(\"asd .. in the response there are only two spaces.

Original content (file:///home/jonas/Workspace/runtime-EclipseApplication/test/main.dart):

import 'test.dart' as test;

void main() {
    print("asd ${test.x}");
}

[bwilkerson]

@DanTup

[DanTup]

This is expected, though not ideal.

The server only supports formatting using the dart_style library which means it always uses 2-space indenting (and the only customisation is line length, which we don't currently expose in LSP). This is somewhat the same issue as https://github.com/Dart-Code/Dart-Code/issues/914 though now it's exposed in LSP it's not VS Code-specific.

There are some other limitations you might bump into too, like the response is a single edit that replaces the entire document (rather than the minimal diffs to apply the change). Depending on how Eclipse works, this might cause you some problems (for ex. with preserving your cursor locations, or locations of breakpoints, etc.). VS Code computes the minimal diff for us so we don't hit it there - I'd be interested to know how other editors work.

We also don't support formatting a range (though LSP's capabilities should take care of that, but be aware that on-type formatting will also get a full-file edit).

I think addressing these limitations would either require dart_style to support them, or the server to support an alternative formatter. I don't know how likely either of those are (it may depend somewhat on the demand).

[jonas-jonas]

The server only supports formatting using the dart_style library which means it always uses 2-space indenting (and the only customisation is line length, which we don't currently expose in LSP). This is somewhat the same issue as Dart-Code/Dart-Code#914 though now it's exposed in LSP it's not VS Code-specific.

Ah, I figured as much, after reading up a bit on how dart_style works. Thanks for the clarification.

Eclipse (LSP4E) seems to replace everything in the editor, which is not ideal...

We also don't support formatting a range (though LSP's capabilities should take care of that, but be aware that on-type formatting will also get a full-file edit).

This would be very helpful as I'm not sure if it's feasible for the Eclipse plugin API to compute differences. Do you know the progress in dart_style on this one? I see that they have to support it in order for the server to support is as well.

I guess as for the tabSize problem, I will just set the defaults in Eclipse so that typing code in would result in the same indentation (two spaces).

[DanTup]

Eclipse (LSP4E) seems to replace everything in the editor

That's what VS Code did originally (which messed with breakpoints and cursors). We do still have some issues with VS Code (it'll only compute minimal diffs for us if the file size is below 100kb.. it used to be 10k which Flutter users started hitting.. hopefully it happens much less now).

It might make sense to compute a better diff in the server (@bwilkerson we discussed this in the past, but the VS Code changes meant it was less pressing). I'm not sure how complicated it would be (I've never had to write any diff code), but if it's only changing whitespace then maybe that simplifies things a little.

Do you know the progress in dart_style on this one? I see that they have to support it in order for the server to support is as well.

I don't think dart_style plans to support things like tab indenting (it's very opinionated by design). Format-range I think there's an open issue for, but I don't know what the plans are. It's possible if we computed smaller diffs of the full edit in the server that we could discard those outside of the range to get range-formatting more easily.

I guess as for the tabSize problem, I will just set the defaults in Eclipse so that typing code in would result in the same indentation (two spaces).

That's what we do in VS Code - we set the Dart language default to 2-spaces. Users can override it and will then find that it doesn't work, but that's just an accepted limitation for now.

[bwilkerson]

It might make sense to compute a better diff in the server ... I'm not sure how complicated it would be ... but if it's only changing whitespace then maybe that simplifies things a little.

I've never written a diff algorithm before either, but I was considering it recently and it isn't hard to find a good the algorithm to start with, so it probably wouldn't be too hard to implement. Thinking about it briefly, though, I suspect you'd be better off just writing a normal line-oriented diff algorithm and not trying to use the fact that only whitespace has changed in order to optimize it. The diffs will likely be small enough for these purposes and you wouldn't need to prove that your algorithm was correct.

It's possible if we computed smaller diffs of the full edit in the server that we could discard those outside of the range to get range-formatting more easily.

I think it would be even easier than that. Given that only whitespace changes have been made by the formatter, then the AST for the before an after would differ only by offsets of tokens. If we assume that all of the ranges would be from the start of one token to the end of another it should be easy to map the before offsets to the after offsets to compute the range of characters to replace. (We could also fairly easily extend ranges to include the white space before or after, such as to the beginning or end of a line.)

[DanTup]

I suspect you'd be better off just writing a normal line-oriented diff algorithm and not trying to use the fact that only whitespace has changed in order to optimize it.

I was thinking about the case where dartfmt makes changes to lots of lines (like wrapping/unwrapping, which it does a lot on unformatted code) that might mess up things like breakpoint locations. By only providing edits that touch whitespace, it seems like the editors could track them better (than if we just "deleted 4 lines, inserted 3 lines" for ex). I don't know enough about any of the line-based diff algorithms to know if they could handle better than that though.

I also thought it might be pretty simple to just split the old/new text on whitespace boundaries and step through them - I figured the non-whitespace segments would be identical, so it's just a case of comparing the whitespace ones to figure out if there's an edit there. However, now I think again - it's possible that whitespace is being added where there was none, or removed where there was (eg. foo.bar() being wrapped onto two lines) so I think that thought was rather flawed!

I think it would be even easier than that. Given that only whitespace changes have been made by the formatter, then the AST for the before an after would differ only by offsets of tokens.

Ah, interesting! Though right now I think our format call only deals with strings. Do you mean to just re-parse it into an AST and then compare? I wonder if doing the actual diff as I described above might also be simpler on the AST than the bare string?

[bwilkerson]

Do you mean to just re-parse it into an AST and then compare?

Yes. If we parse both the before and after then we could use a structured comparison to find the region of formatted text that we want to apply.

I wonder if doing the actual diff as I described above might also be simpler on the AST than the bare string?

Sorry, I didn't follow that question.

[DanTup]

Sorry, I didn't follow that question.

I meant, could/should we use this to build the full set of edits (not only for discarding changes that are outside of the range for range formatting), rather than implementing a full text/line-based diff algorithm?

[bwilkerson]

Ah. Yes, we could do that. It would be a bit harder because the fact that the offsets of two corresponding tokens is different doesn't mean that there was a change near them; differences earlier in the file impact everything that comes after that. We'd basically have to keep a running delta to know where real differences exist.