FormatAsync removes whitespace outside the requested bounds

[333fred]

Version Used: 3.8.0

Steps to Reproduce:

Consider the following code:

class C
{
    public void M()
    {
        // There is a space after the ; here, and 8 spaces on the next line.
        var i = 1; 
        
    }
}

If I make a FormatAsync call, with the bounds [109..130), I get back a text change removing both the space on the end of the var line and the spaces on the line after, with a changed span of [127..138). This span is outside the bounds of what I asked for. In practice, what this means that if I have on-type formatting turned on in vscode and I have a trailing space, FormatAsync will remove the auto-indentation added in to put my cursor, when all that was asked to be formatted was the previous line.

[CyrusNajmabadi]

This span is outside the bounds of what I asked for. In practice

IN general, we walk a token forward/backward from the span requested as that's how we thought the intended behavior of this API should be. i.e. you're giving the span of nodes/tokens to format, so of course you'd need to touch the whitespace that node/tokens are touching.

It's highly unlikely we could change this as too much is built with this assumption. Note: if you do want to filter this stuff out, i would imagine you could write your own method that just takes the text-changes produced and clamps that as appropriate to whatever span you must limit things to.

[CyrusNajmabadi]

what this means that if I have on-type formatting turned on in vscode and I have a trailing space, FormatAsync will remove the auto-indentation added in to put my cursor

I'm not certain what tihs means. what is 'on type formatting'?

[CyrusNajmabadi]

Put another way, the idea of formatting was that particular nodes/tokens would be annotated. We'd then just grab the span of the node/token and pass that into the formatter (so everythign is consistent). Since the node is what is annotated (and what span is being asked for) the actual region to be formatted extends before/after that node so that node is properly formatted in the context of everything else.

[333fred]

I'm not certain what tihs means. what is 'on type formatting'?

VSCode has a setting that will format as you type. We use it to format the line you just typed, but you don't want it to format the next line at all.

Note: if you do want to filter this stuff out, i would imagine you could write your own method that just takes the text-changes produced and clamps that as appropriate to whatever span you must limit things to.

It is very much not that simple. The new changes include newlines that were outside the requested boundaries, and I can't easily map them back to the original location.

[333fred]

If the existing behavior can't be changed (it's certainly non-obvious), then I'd like to see a new mode added for it.

[CyrusNajmabadi]

We use it to format the line you just typed, but you don't want it to format the next line at all.

Then i would filter out the result to only the region you care about. The text changes are just data that are passed out. I don't see a need for a mode for this. It's very specifically written around a need of a particular vscode feature, and seems like it would be a 1-liner to support. So i would just do it on that end.

[333fred]

As another example of where this had odd consequences, consider the scenario where, in VS, I select the var x = 1; line and hit ctrl+k, ctrl+f (format selection). The spaces on the next line are removed as well, even though they were not selected.

[CyrusNajmabadi]

The spaces on the next line are removed as well, even though they were not selected.

Definitely still by design here. I get that it's not desirable from your perspective, and you have a rational and cohesive alternate design. However, that design can always be layered on what we have here pretty simpler afaict :) i.e. the existing api is effectively "please smartly format the node this span encompasses" and you can easily write the "and definitely clamp to a particular region" on top of that if you want :)

[333fred]

To summarize the conversation @CyrusNajmabadi and I had on discord:

1. It's very hard to map the changes here because formattedDocument.GetTextChangesAsync(oldDocument) gives back one text change, which is partly in a section we want, and partly in a section we do not want, and we can't easily break that up.
2. The roslyn APIs weren't designed for this scenario at all, so support isn't really a goal.
3. If we moved IEditorFormattingService down to the Features layer, O# could take advantage of it and get better behavior. That API does not appear to need any other EditorFeatures layer access, but it is being implemented by 3rd party teams so moving it would require a bit of work.

Any other points I missed Cyrus?

[CyrusNajmabadi]

that seems like a reasonable place to start. :)

[333fred]

@CyrusNajmabadi I took a look at moving IEditorFormattingService down. The only thing that I'm not sure about is how to factor out a call to IIndentationManagerSerivce.GetIndentation, from VS. Presumably I'd make an interface in the features layer, and implement in the editor features layer? And provide some version for non-VS locations? I'm not sure what the standard practice for this type of scenario is.

[sharwell]

I agree that the formatter should filter changes to only affect the requested span. However, given the potentially large amount of work involved with making any progress on this edge case, I'm not sure we'll ever see a resolution on it.

With that said, the elimination of 8 spaces is not a bug. On that blank line, the caret is placed in virtual space 8 positions to the right of the last character on the line. There are no spaces to its left. If the IDE using Roslyn does not understand virtual spaces, you'll need to simulate it somehow by reinserting unwanted spaces after every formatting operation when the indentation service requests positioning in virtual space.

[333fred]

Moved the public API request to #50221. Leaving this open to track the unexpected formatting changes.