Quick-fixes that insert text with completion/choices show backslashes in front of closing curly braces

[FMorschel]

Describe the issue

When using the quick-fix for missing override, with an Iterable<Record> (tested with Set and List as well), the last item inside the record always ends with \ at the end.

To Reproduce
Steps to reproduce the behavior:

1. Add the following code to your project in vscode:

abstract class A {
  void fn(Iterable<({int a, int b})> r);
}

class B extends A {
}

abstract class C {
  void fn(({int a, int b}) r);
}

class D extends C {
}

2. Quick fix for missing override.

Expected behavior

Expected the quick fix to not add \ at the end.

Screenshots

20231215_104425.mp4

[FMorschel]

Also, maybe this should be a separate issue but I'll ask here anyway just to be sure.

If you do:

abstract class C {
  void fn(({int b, int a}) r);
}

class D extends C {
}

Using the quick fix reorders the named values to be in alphabetical order:

abstract class C {
  void fn(({int b, int a}) r);
}

class D extends C {
  @override
  void fn(({int a, int b}) r) {
    // TODO: implement fn
  }
}

That could make some users a little confused when searching by text for the same record that was originally written (which happened to me). Is this intended and if so, why?

[bwilkerson]

Thanks for reporting the problem with the slash!

As for the re-ordering, that's an unfortunate consequence of the internal implementation and/or specification.

Two records types are considered to be the same if the number and types of the positional fields match, and if the names and types of the named fields match. In other words, the order of the named fields isn't considered when comparing record types.

In order to implement this efficiently, the analyzer normalizes the named fields in it's internal representation. Unfortunately, that means that we've lost the order in which the fields were declared by the time we get to the fix, so there's no way we can duplicate the original order.

[FMorschel]

I see, thanks for clarifying!

Just a new update. tested with a Map as well, the problem seems to be when the Record is inside a <> pair and it only happens when there is a named value inside the Record

[bwilkerson]

I'm not sure where the problem might be. I was able to reproduce the issue in VS Code using a build from HEAD, with

abstract class A {
  void m(Iterable<({int a, int b})> r);
}

class B extends A {}

I thought I had an idea of where the bug might be, so I wrote a test of DartEditBuilder.writeType, but that passed. So then I wrote a test of DartEditBuilder.writeOverride and that test passed too. Then I wrote a test of CreateMissingOverrides and that passed. So I added a protocol-level test and that passed (as in, we're passing the right result back to VS Code).

@DanTup It's possible that my test of LSP isn't right, but it looks like the right edit is getting returned. Is there any chance the bug is on the DartCode or VS Code side?

[FMorschel]

Tested on IntelliJ CE, no issues. Seems to me that the problem is only with VSCode

[DanTup]

Here's what the edit looks like in the JSON from the server:

{
	"insertTextFormat": 2,
	"newText": "$0\n  @override\n  void m(${1|Iterable<({int a\\, int b\\})>,Object|} ${2:r}) {\n    // TODO: implement m\n  }\n",
	"range": {
		"end": {
			"character": 19,
			"line": 4
		},
		"start": {
			"character": 19,
			"line": 4
		}
	}
}

This looks correct to me - there are two backslashes - one to escape the other backslash in the string, meaning the actual content looks like:

$0
  @override
  void m(${1|Iterable<({int a\, int b\})>,Object|} ${2:r}) {
    // TODO: implement m
  }

The remaining backslashes are to escape the comma (so it's not a separator between the two choices) and the closing bracket (so it doesn't end the choice block) - this seems to match what's documented here.

So assuming I'm interpreting the escaping rules correctly, I think it's either a bug in Dart-Code (if we're doing any massaging of this value), or VS Code. Will take a look.

[DanTup]

IMO this is a VS Code bug, because we're escaping a character that the docs say can be escaped. However, it could be argued that the escaping is unnecessary in a choice, because they're surrounded by {| and |}. I've opened microsoft/vscode#201059 for confirmation - if it's working as intended then hopefully the docs can be made more specific, and we'll have to change the escaping in the server to match. That change would likely be here:

sdk/pkg/analysis_server/lib/src/lsp/snippets.dart

Lines 152 to 153 in 2261250

	static final _escapeSnippetChoiceTextRegex =
	RegExp(r'[$}\\\|,]'); // Replace any of $ } \ | ,

[DanTup]

Apparently this isn't a bug and we should not escape any characters for the specific cases that don't require it (eg. in choices, we cannot escape } even though we escape it on the others).

I've opened some PRs to VS Code + LSP docs to try and make this concrete before I start changing things:

microsoft/language-server-protocol#1868
microsoft/vscode-docs#6928

[DanTup]

The VS Code docs + LSP changes landed. I've opened a server fix at https://dart-review.googlesource.com/c/sdk/+/345661