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

feat: provide LSP API support #543

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

angelozerr
Copy link
Contributor

@angelozerr angelozerr commented Sep 26, 2024

This PR provide LSP API support for:

  • diagnostic (done)
  • completion (initialized)
  • formatting (in progress)
  • TODO other features....

Please read docs/LSPApi.md for a sample.

@angelozerr
Copy link
Contributor Author

angelozerr commented Sep 26, 2024

@CppCXY @FalsePattern this PR should start working but please give me feedback about the LSP API which should fix #424 and #388

@angelozerr angelozerr force-pushed the lsp-api branch 3 times, most recently from 3312518 to 48e8439 Compare September 26, 2024 17:24
@angelozerr
Copy link
Contributor Author

angelozerr commented Sep 26, 2024

@InSyncWithFoo please follow this PR which provides diagnostic support. Any feedback are welcome about the name, how to the configuration is done, etc

@angelozerr angelozerr force-pushed the lsp-api branch 6 times, most recently from 16e8d33 to 8172067 Compare September 27, 2024 09:01
@FalsePattern
Copy link
Contributor

The API draft seems alright to me so far.
LSPCustomizedFormattingSupport will resolve the proxy workaround i'm doing in zigbrains, I don't really have a use case for the other two at the moment so I cannot comment on those.

@angelozerr
Copy link
Contributor Author

Thanks for your feedback.

Please note the Api will change à lot since we have some discussion this morning with @fbricon

@angelozerr angelozerr force-pushed the lsp-api branch 2 times, most recently from 7a222db to 891e153 Compare September 27, 2024 12:23
@InSyncWithFoo
Copy link
Contributor

InSyncWithFoo commented Sep 27, 2024

I would also like to customize code actions and hover, since these two, along with the original trio of diagnostics, completion and formatting, are the most commonly used.

For reference, the native client uses a boolean to configure hover support, whereas that of code actions allows for separate customizations of quick fixes and intentions, as well as whether the server should be asked to provide such actions.

It would be nice to be able to tweak the textDocument/hover response before displaying it; for example:

class MyServerHoverFeature : LSPHoverFeature {

    // Better name needed (`beforePopupDisplay`? `getPopupContent`?)
    fun modifyContent(content: String): String? {
        if (somethingIsWrong) {
            return null  // No popup should be shown
        }

        return frobnicate(content)
    }

}

@angelozerr
Copy link
Contributor Author

It would be nice to be able to tweak the textDocument/hover response before displaying it; for example:

Can you do that with LSP JetBrains? Have you implemented this customization?

@InSyncWithFoo
Copy link
Contributor

Can you do that with LSP JetBrains? Have you implemented this customization?

No and no. It is sometimes necessary, though. See this issue for an example of how the default might look wrong (not necessarily in that specific manner).

To give another example: currently, if the returned Markdown snippet has a code block at the very start, it will be rendered with an aesthetically unpleasing dark background.

@@ -50,4 +51,7 @@ public interface LanguageServerFactory {
return LanguageServer.class;
}

@NotNull default LSPClientFeatures createClientFeatures() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Features might be enabled or disabled for one project but not another, so this method should be passed the current project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you.

In short answer, it exists LSPClientFeature#getProject()

LSPClientFeatures is set with the LanguageServerWrapper which have the project. We need LanguageServerWrapper to support server capability see LSPFormattingFeature.

The LanguageServerWrapper set her instance when LSPClientFeatures is created (I did that because I don't want to expose LanguageServerWrapper in the API)

The LanguageServerWrapper hosts the Project, so LSPClientFeatures know the Project.

* @param file the file.
* @return true if LSP formatting can be done even if it exists a custom formatter and false otherwise.
*/
protected boolean canFormatEvenIfExistingFormatter(@NotNull PsiFile file) {
Copy link
Contributor

@InSyncWithFoo InSyncWithFoo Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The native LspFormattingSupport has a similar method:

fun shouldFormatThisFileExclusivelyByServer(
    file: VirtualFile,
    ideCanFormatThisFileItself: Boolean,
    serverExplicitlyWantsToFormatThisFile: Boolean
): Boolean

According to the doc comments, ideCanFormatThisFileItself means whether LanguageFormatting.forContext returns a non-null value, whereas serverExplicitlyWantsToFormatThisFile means whether the file matches the registered DocumentSelectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have dicussed with @fbricon and we decide to have an isEnabled method for each LSP*Feature with a custom context instead of having isEnabled with several parameters to be sure that API will not broken if we need new fields.

According to the doc comments, ideCanFormatThisFileItself means whether LanguageFormatting.forContext return a non-null value

Thanks for your feedback. I removed this canFormatEvenIfExistingFormatter and now you have now:

    /**
     * Returns true if existing formatter are overrideable and false (default value) otherwise.
     *
     * @return true if existing formatter are overrideable and false (default value) otherwise.
     */
    protected boolean isExistingFormatterOverrideable(@NotNull PsiFile file) {
        return false;
    }

In this case LanguageFormatting.INSTANCE.forContext(file) != null i sdone.

If you override isExistingFormatterOverrideable to returns true, it will skip the test with LanguageFormatting.INSTANCE.forContext(file) != null`

@InSyncWithFoo @FalsePattern @CppCXY do you like this idea?

Copy link
Contributor Author

@angelozerr angelozerr Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

means whether the file matches the registered DocumentSelectors.

Today we check server capability, but not DocumentSelectors. Once LSP API will be merged,I will work on #517

@angelozerr angelozerr force-pushed the lsp-api branch 4 times, most recently from c40cf43 to 1da540e Compare September 27, 2024 16:36
@InSyncWithFoo
Copy link
Contributor

Note to self: Replace the 4-star review with a new 5-star one when this PR is merged.

@angelozerr angelozerr force-pushed the lsp-api branch 2 times, most recently from 003cbea to 0d570cf Compare September 28, 2024 09:58
*/
public LanguageServer getServer() {
return server;
}

/**
* Returns the LSP4J language server initialized.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returns the LSP4J language server, guaranteed to be initialized at that point.

*
* @return true if server stopping required when files closed and false otherwise.
*/
public boolean isServerStoppingRequiredWhenFilesClosed() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keepServerRunning() or keepServerAlive()

Signed-off-by: azerr <azerr@redhat.com>
@angelozerr angelozerr marked this pull request as ready for review October 3, 2024 15:24
@angelozerr angelozerr merged commit 50c6a56 into redhat-developer:main Oct 3, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants