eclipse / lsp4mp

Technology lsp4mp
Eclipse Public License 2.0
22 stars 27 forks source link

Invalid snippet suggested in completion before the Class statement [context] #402

Open turkeylurkey opened 1 year ago

turkeylurkey commented 1 year ago

This issue revolves around the field and method snippets and the context in which the language server suggests them when the user activates completion (e.g. Ctrl-Space).

Consider this sample class:

package a.b.c;
// Position A
class d{ 
    // Position B
    class Inner {}
    // Position C
}

When you use completion in Position A the language server provides rest_get as one of the choices. This snippet represents the insertion of a method but this is not valid in this context. When you use completion in Position C the snippet rest_get is valid. It looks like method snippets should not be allowed before a class. This requires fix #1.

This brings us to Position B. Currently the language server considers this to be "before a class." We must allow a method snippet here. This exposes an ambiguity in the definitions. Position B is both 'In A Class' and 'Before A Class.' The former should dominate the latter and this would allow the language server to suggest a method snippet here. This requires fix #2.

angelozerr commented 1 year ago

@datho7561 do you think we could improve java cursor context to cover this Issue?

angelozerr commented 1 year ago

@turkeylurkey we know that our java cursor context is not perfect, but it should filter the most common usecases, any contribution are welcome!

datho7561 commented 1 year ago

I thought I handled this case correctly. I'll take a quick look at what is happening.

datho7561 commented 1 year ago

From my testing, cases B and C seem to work properly, but A doesn't work (either with class or public class). For all of these cases, I get Exceptions in the output when I select the completion item.

(I am testing using VS Code, this might be different from Eclipse).

turkeylurkey commented 1 year ago

The reason A doesn't work is that SnippetContextForJava explicitly allows the wrong snippets to be available: https://github.com/eclipse/lsp4mp/blob/e82966dc8c66e4e8d775ce3488d2ae7a2b091462/microprofile.ls/org.eclipse.lsp4mp.ls/src/main/java/org/eclipse/lsp4mp/snippets/SnippetContextForJava.java#L101 If you just remove BEFORE_CLASS then you will break B. That fix requires reworking the code that examines the syntax tree.

angelozerr commented 1 year ago

@turkeylurkey is there any chance that you provide a PR with your idea?

turkeylurkey commented 1 year ago

Unfortunately I'm having trouble building and running in Eclipse to update FindWhatsBeingAnnotatedASTVisitor and making a PR.