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

Completing C# await keyword in non-async methods results in the wrong cursor location being used due to formatting. #6127

Closed
NTaylorMullen opened this issue Feb 25, 2022 · 8 comments
Assignees
Labels
bug Something isn't working feature-formatting
Milestone

Comments

@NTaylorMullen
Copy link
Contributor

Describe the bug:
Completing the await keyword in non-async methods results in Razor generating an incompatible edit that moves the cursor location.

Version used:
17.2-Preview1+

To reproduce:

  1. Open a Razor file with the following text and cursor at the pipe (|):
@code {
    private void Foo()
    {
        aw|
    }
}
  1. Manually invoke completion (ctrl + space) and select await

Expected Behavior:
Cursor is after await

Actual Behavior:

devenv_6M80d9rOOT

Additional Notes:

I dug into why this was happening and C# is depending on completionItem/resolve to fill out the text edit for that completion item. Only problem is the TextEdit C# returns is valid but Razor transforms it into something invalid.

Original C# completion item text edit (from C# server):

"textEdit": {
    "range": {
        "start": {
        "line": 97,
        "character": 12
        },
        "end": {
        "line": 99,
        "character": 10
        }
    },
    "newText": "async void Foo()\r\n    {\r\n        await"
},

After Razor formats the edit (from our HTMLC# server):

"textEdit": {
    "range": {
        "start": {
        "line": 0,
        "character": 7
        },
        "end": {
        "line": 4,
        "character": 4
        }
    },
    "newText": "\r\n    private async void Foo()\r\n    {\r\n        await\r\n    "
},

Note the extra newline and spaces after await.

One extra piece I found interesting is that C#'s completion item says it's a snippet but doesn't actually provide any tab-stops. Technically this is valid in the LSP specification but felt weird (fyi @dibarbet).

@NTaylorMullen NTaylorMullen added bug Something isn't working feature-formatting labels Feb 25, 2022
@NTaylorMullen NTaylorMullen added this to the 17.2-Preview2 CC:~2/28 milestone Feb 25, 2022
@DoctorKrolic
Copy link
Contributor

Just interesting: how did you get what LSP sends to the client (and vice versa if it is possible)? I've tried once, but it was buried somewhere too deeply, so I didn't find out how to get the raw text of requests and responses in the end...

@NTaylorMullen
Copy link
Contributor Author

NTaylorMullen commented Feb 28, 2022

Just interesting: how did you get what LSP sends to the client (and vice versa if it is possible)? I've tried once, but it was buried somewhere too deeply, so I didn't find out how to get the raw text of requests and responses in the end...

If you boot devenv.exe with the /log parameter you can see all of the requests / responses & their content in %localappdata%\Temp\VisualStudio\LSP

@davidwengier
Copy link
Contributor

Are we using GetMinimalChanges in this formatting path? If so, are we setting lineDiffOnly to false? Could try that?

@allisonchou
Copy link
Contributor

@dibarbet would you mind validating whether this is fixed by your recent changes?

@dibarbet
Copy link
Member

dibarbet commented Apr 7, 2022

it is not - it needs this fix - dotnet/roslyn#60499 (+ razor side fix for formatting which I have locally) as well as a fix for dotnet/roslyn#60598

@NTaylorMullen
Copy link
Contributor Author

Fixed via: #6626

@DoctorKrolic
Copy link
Contributor

Will this fix work in 17.4 p1 by default, or some settings will be requred to be changed? I've seen some tweaks around new single-server completion (whatever this means) and as far as I know it won't be enabled for all clients by default. If I'll need to change this setting, can you please tell where to find it, thanks!

@NTaylorMullen
Copy link
Contributor Author

Will work by default. I pushed single server completion on-by-default in 17.4. I still have all the feature flags around in case we see something catastrophic happen I can remotely disable it for the world 😄

@ghost ghost locked as resolved and limited conversation to collaborators Aug 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working feature-formatting
Projects
None yet
Development

No branches or pull requests

5 participants