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

Can not override methods from parent class #72

Open
hoanbui29 opened this issue Nov 2, 2024 · 16 comments
Open

Can not override methods from parent class #72

hoanbui29 opened this issue Nov 2, 2024 · 16 comments
Labels
upstream bug Server/neovim issue

Comments

@hoanbui29
Copy link

When I try to override the methods from parent class, nothing happened.
The methods are still showed on cmp, but I can't generate code for that. Pls help!
CleanShot 2024-11-02 at 18 45 55@2x
CleanShot 2024-11-02 at 18 47 04@2x

@seblyng
Copy link
Owner

seblyng commented Nov 2, 2024

I don't think there is much for me to do for this: #7

I can leave this open a little bit to see if I get some time to investigate later. I cannot guarantee anything though as I am pretty sure this is just the server not following the spec (as it unfortunately doesn't do for several things)

@hoanbui29
Copy link
Author

I don't think there is much for me to do for this: #7

I can leave this open a little bit to see if I get some time to investigate later. I cannot guarantee anything though as I am pretty sure this is just the server not following the spec (as it unfortunately doesn't do for several things)

Thank you very much!

@drzbida
Copy link

drzbida commented Feb 10, 2025

I don't think there is much for me to do for this: #7

I can leave this open a little bit to see if I get some time to investigate later. I cannot guarantee anything though as I am pretty sure this is just the server not following the spec (as it unfortunately doesn't do for several things)

Could you check dotnet/roslyn#76141 (comment) when you have time? I've tried implementing the command in lsp_commands similar to how it's done for "roslyn.client.nestedCodeAction", following the linked vscode implementation, but I still get the exception mentioned in the issue so it doesn't get to execute. Not sure if something else is required so that the command doesn't get sent to the server (if that's why the exception is happening)

@seblyng
Copy link
Owner

seblyng commented Feb 10, 2025

Yeah, from initial testing I am also getting the same thing, and the handler is not even being called🤷‍♂️

@drzbida
Copy link

drzbida commented Feb 11, 2025

Seems like a neovim issue.

After looking through neovim 0.10.X code it seems that it's impossible for this custom command to trigger as workspace/executeCommand is only manually handled when triggered via code actions or code lens.

In latest neovim nightly it seems that some custom handling for commands was done in lsp -> completion.lua, but I'm still trying to figure why it still doesn't work.

@seblyng
Copy link
Owner

seblyng commented Feb 14, 2025

Hmm, I tested the PR that should close the linked issue you created in neovim. I got curious since it seemed to be for the completion part of neovim itself, therefore having nothing to do with nvim-cmp and blink.cmp for example.

The PR seems to trigger the command (I didn't implement it yet), but it (of course) still doesn't trigger when using nvim-cmp or blink.cmp. I haven't looked too much into why this might be, or what capabilities they might be missing/have not implemented correctly. Do you have any insights into this? @drzbida

EDIT:

I think I found out. Especially for nvim-cmp which I currently used. There is a PR that seems to try and resolve this issue, but there haven't been any new progress there for two months: hrsh7th/cmp-nvim-lsp#73

@drzbida
Copy link

drzbida commented Feb 14, 2025

It seems the issue was closed only with the mentioned problem for the native completion implementation, but the core issue was not adressed. I was thinking that LSP requests for executeCommand should first check if there is a client command no matter what, but they didn't adress this part :) So currently all completion plugins need to do it on their own.

I'm also using nvim-cmp, but I'm thinking of migrating to blink as nvim-cmp seems to be kinda "unmaintained" for quite a while now.

For blink.cmp I see that this was added recently

Saghen/blink.cmp#1130

But it's missing the part where it checks for the custom commands. I will most likely open a PR there after I migrate my config if nobody does it by then.

@seblyng
Copy link
Owner

seblyng commented Feb 14, 2025

Okay yeah thanks! I have also been thinking about moving to blink.cmp lately, but have wanted for it to get more mature first.

Anyways, thanks a lot! If you wouldn't mind, could you link this issue or just send a message here or something if you some day open a PR about that in blink? If I switch, I might also look into this, but don't know yet if/when I might do that

@drzbida
Copy link

drzbida commented Feb 18, 2025

Saghen/blink.cmp#1255 Got merged. According to Saghen/blink.cmp#1203 v0.12 should be the last release with breaking changes until 1.0

Caveats:

  • The LSP server is sometimes very slow on the very first completionItem resolve that contains a command. It's possible and I hope it's just me, but the result is that sometimes the first override completion will not work. Video attachment here:
    Problems with executing LSP commands Saghen/blink.cmp#1219 (comment). What esentially happens is that when the completion is accepted, it has a timeout in which it can respond to the resolve, otherwise the edit is applied with the currently known information (so command is not available)

I've found that increasing blink's resolve_timeout_ms to 1000 ms in the newest version always works for me.

  • If you are using blink's autobrackets feature or possibly any other autobrackets functionality, you will get extra brackets after the override completion with the current implementation. Modifying the document with a command is actually a hack in LSP specs 3.17, so we must fix it here.
public override void _Dro // accept _DropData(Vector2 atPosition, Variant data)
public override void _DropData(Vector2 atPosition, Variant data)
{
    base._DropData(atPosition, data);
}() // extra brackets here
  • If you happen to be typing & accepting fast, sometimes the LSP returns broken line endings. I don't understand why this happens but I don't really think it's worth investigating
Notification(int what)\r\n    {\r\n        base._Notification(what);\r\n    }\r\n\r\n\r

Which would result in neovim in something like

public override void Notification(int what)
{
    base._Notification(what);
}

^M

I'll open a draft PR with fixes for the last two issues and it would be good if someone tests it more extensively (especially on something else than Windows)

@seblyng
Copy link
Owner

seblyng commented Feb 18, 2025

Thanks a lot🙌 I will see if I can setup blink.cmp again and test this out a bit.

If anyone else sees this, I hope they can also test it if interested😅

@molostovvs
Copy link

It works for me on linux.
There is indeed a problem with brackets, as described above.
Also the indexers from the autocomplete list started working:

List<int> list = [];
x. // select this[] completion item from completion list
x[] // selection result

@drzbida
Copy link

drzbida commented Feb 18, 2025

There is indeed a problem with brackets, as described above.

There are problems with the brackets using the PR above? If so, can you please provide an example that doesn't work for you?

@molostovvs
Copy link

Initially I wrote about the problem with parentheses without using the code from PR.
Now I have tested the PR - parentheses are still inserted.

override -> public override string ToString() => base.ToStri<cursor is here>ng();()

@drzbida
Copy link

drzbida commented Feb 19, 2025

Thanks, I didn't have expression bodied methods enabled and forgot about this case. I've updated the PR.

The cursor will be here though

public override string ToString() => base.ToString()<C>;

I guess it makes the most sense for the cursor the be here

public override string ToString() => <C>base.ToString();

But depending on what this complex edit is used for, it might not be feasible to do without treesitter. If it's used only for override completions it should be easy to do without.

@drzbida
Copy link

drzbida commented Feb 19, 2025

I've found another problem with the autobrackets that somehow i can only trigger with ToString

public override str -> accept ToString

tr is for some reason replaced with () before the command executes

The current line text before the command executes it

public override s()

and the result becomes

public override s()ing ToString()
{
    return base.ToString();
}

this is not solvable, it's most likely a bug in blink

EDIT:

Actually this is the completion item

{
  client_id = 1,
  client_name = "roslyn",
  command = {
    arguments = { {
        uri = "file:///D:/Dev/csharp/mtlq/src/Mtlq/Commands/BaseCommand.cs"
      }, {
        newText = "ing ToString()\r\n    {\r\n        return base.ToString();\r\n    }",
        range = {
          ["end"] = {
            character = 23,
            line = 16
          },
          start = {
            character = 23,
            line = 16
          }
        }
      }, false, 515 },
    command = "roslyn.client.completionComplexEdit",
    title = "CompleteComplexEditCommand"
  },
  commitCharacters = { "(" },
  cursor_column = 21,
  data = {
    ResultId = 3,
    TextDocument = {
      uri = "file:///D:/Dev/csharp/mtlq/src/Mtlq/Commands/BaseCommand.cs"
    }
  },
  documentation = {
    kind = "markdown",
    value = "```csharp\r\nstring Symbol.ToString()\r\n```\r\n  \r\nReturns a string that represents the current object\\."
  },
  exact = false,
  filterText = "ToString",
  insertTextFormat = 1,
  insertTextMode = 1,
  kind = 2,
  label = "ToString()",
  score = 27,
  score_offset = 0,
  sortText = "ToString",
  source_id = "lsp",
  source_name = "LSP",
  textEdit = {
    _index = 1,
    newText = "s()",
    range = {
      ["end"] = {
        character = 23,
        line = 16
      },
      start = {
        character = 20,
        line = 16
      }
    }
  },
  textEditText = "s"
}

The server response is incorrect, so Roslyn is at fault. I've also tested in VSCode and it's not working there either (obivously somehow here it works like half the time)

20250219-0925-42.6827148.mp4

@seblyng
Copy link
Owner

seblyng commented Feb 19, 2025

Interesting🤔 will try to look at the PR today or tomorrow.

I guess it makes the most sense for the cursor the be here

I actually think it makes sense to have it as the first example you had🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream bug Server/neovim issue
Projects
None yet
Development

No branches or pull requests

4 participants