DetachHead / basedpyright

pyright fork with various type checking improvements, improved vscode support and pylance features built into the language server
https://docs.basedpyright.com
Other
1.21k stars 24 forks source link

document how to configure language server settings in eglot #894

Open VictorCMiraldo opened 3 days ago

VictorCMiraldo commented 3 days ago

I've been trying to use basedpyright (version 1.21.1), but I cannot configure any of the analysis settings. I'm using eglot and this is the result of eglot-show-workspace-configuration:

{
  "basedpyright": {
    "typeCheckingMode": "recommended",
    "analysis": {
      "diagnosticSeverityOverrides": {
        "reportUnusedCallResult": false,
        "reportInvalidCast": false
      },
      "inlayHints": {
        "callArgumentNames": false,
        "functionReturnTypes": false,
        "variableTypes": false,
        "genericTypes": false
      }
    }
  },
  "python": {
    "pythonPath": "/nix/store/sc8hbm8i8n22f5f7wrf2dla333bdwpri-env-default/bin/python",
    "venvPath": null
  }
}

Still, I keep on seeing reportUnusedCallResult pop up all over the code. I'm reasonably confident that eglot is sending the configuration because if I change typeCheckingMode to "basic" or "off" I see a difference. I also keep on seeing inlay hints, even though I much rather not see them. This leads me to believe my basedpyright-langserver is not really respecting the settings under analysis.

I did try a number of variations, adding a "settings" section; moving the "analysis" section to the top-level as if it was in a pyrightconfig.json file; etc... but I had no success so far. Am I doing something obviously wrong here?

I'm not really sure wheter to ask for help in the eglot repo or here. Decided to start here because I can get basedpyright to obey the typeCheckingMode setting, which suggests eglot is not at fault.

Thank you!

DetachHead commented 3 days ago

do you have a pyrightconfig.json file or a pyproject.toml file with a [tool.basedpyright] or [tool.pyright] section in it? if so, it takes priority over language server config. see #513 for more info (specifically this comment)

VictorCMiraldo commented 3 days ago

No, I have no pyrightconfig.json and the pyproject.toml doesn't contain [tool.pyright] nor [tool.basedpyright] sections.

DetachHead commented 1 day ago

unfortunately i'm not familiar with emacs and i was unable to reproduce it in any other editor, so it might be an issue with eglot?

are you able to try lsp-bridge or lsp-mode and see if they have the same issue?

VictorCMiraldo commented 1 day ago

Thank you for trying and looking into this! To my surprise, I successfully used lsp-mode to configure basedpyright. I made an issue on the eglot repo: https://github.com/joaotavora/eglot/issues/1464 It is very weird that typeCheckingMode works but the other settings do not... In anyway, the issue is clearly not basedpyright, so I'll close this this! Pardon the noise and thank you!

VictorCMiraldo commented 21 hours ago

The author of eglot has found the problem and has provided a careful analysis of what went wrong. While we solved the problem there, basedpyright asks for the configuration three times, which maybe could be improved. If for nothing else, maybe we could add the correct eglot configuration in the "Where do I configure these settings" section at the bottom of the LSP Configuration Page.

This is an example config that helps eglot to play ball with basedpyright:

(use-package eglot
  :ensure t
  :config
    (add-to-list 'eglot-server-programs '(
      (python-mode python-ts-mode)
         "basedpyright-langserver" "--stdio"
    ))
    (setq-default
       eglot-workspace-configuration
       '(:basedpyright (
           :typeCheckingMode "recommended"
         )
         :basedpyright.analysis (
           :diagnosticSeverityOverrides (
             :reportUnusedCallResult "none"
           )
           :inlayHints (
             :callArgumentNames :json-false
           )
         )))
)

So at startup, eglot sends the entire config in a workspace/didChangeConfiguration step:

[jsonrpc] e[14:40:58.813] --> workspace/didChangeConfiguration {"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"basedpyright":{"typeCheckingMode":"recommended","analysis":{"diagnosticSeverityOverrides":{"reportUnusedCallResult":"none"},"inlayHints":{"callArgumentNames":false,"functionReturnTypes":false,"variableTypes":false,"genericTypes":false}}}}}}

Then the server proceeds to ask for for the python, basedpyright.analysis and basedpyright sections of the configuration, even though it already received the config above:

(First)

[jsonrpc] e[14:40:58.932] <-- workspace/configuration[1] {"jsonrpc":"2.0","id":1,"method":"workspace/configuration","params":{"items":[{"scopeUri":"file:///home/victor/test-proj","section":"python"}]}}
[jsonrpc] e[14:40:58.932] --> workspace/configuration[1] {"jsonrpc":"2.0","id":1,"result":[null]}

Eglot answers with a null since it doesn't have a "python" section. Then basedpyright proceeds to ask for the "basedpyright.analysis" section:

(Second)

[jsonrpc] e[14:40:58.950] <-- workspace/configuration[2] {"jsonrpc":"2.0","id":2,"method":"workspace/configuration","params":{"items":[{"scopeUri":"file:///home/victor/test-proj","section":"basedpyright.analysis"}]}}
[jsonrpc] e[14:40:58.951] --> workspace/configuration[2] {"jsonrpc":"2.0","id":2,"result":[null]}

Finally, it asks for the basedpyright section: (Third)

[jsonrpc] e[14:40:58.958] <-- workspace/configuration[3] {"jsonrpc":"2.0","id":3,"method":"workspace/configuration","params":{"items":[{"scopeUri":"file:///home/victor/test-proj","section":"basedpyright"}]}}
[jsonrpc] e[14:40:58.958] --> workspace/configuration[3] {"jsonrpc":"2.0","id":3,"result":[{"typeCheckingMode":"recommended","analysis":{"diagnosticSeverityOverrides":{"reportUnusedCallResult":"none"},"inlayHints":{"callArgumentNames":false,"functionReturnTypes":false,"variableTypes":false,"genericTypes":false}}}]}

Now eglot doesn't have a top-level label "basedpyright.analysis" present. So it sends null back on the second request. This is where "the issue" happens: eglot could have looked into the "analysis" sub-section of the "basedpright" section, but that behavior is not specified in the spec; so it does the easy thing and looks for the string "basedpyright.analysis".

I know nothing about LSP to judge what could or should happen with all these calls. That being said, this is a configuration nightmare for the uninitiated (like myself :)), so I'd suggest to place an example working config for eglot on the basedpyright example section, as I wrote above.

DetachHead commented 20 hours ago

would you mind submitting a PR adding the relevant information to the docs? it's located here https://github.com/DetachHead/basedpyright/blob/6d94c2aca2683aaa1f36e5d1687c6240e239b315/docs/configuration/language-server-settings.md?plain=1#L82

thanks

VictorCMiraldo commented 18 hours ago

Sure, I'll make a PR!

I think that the issue lives here, though:

First you fetch the basedpyright.analysis section. Now, because the dot character ('.', ascii 46) doesn't have any specified meaning, some clients look into their workspace config hierarchically (as in, find the basedpyright section, then look for the analysis subsection); others see the entire string as the section name and find nothing (that's eglot's case). Both are right from the point of view that the LSP spec should be more rigid... all it says is that section should be a string (link):

export interface [ConfigurationItem](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#configurationItem) {
    /**
     * The scope to get the configuration section for.
     */
    scopeUri?: [URI](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#uri);

    /**
     * The configuration section asked for.
     */
    section?: string;
}

For basedpyright concretely, you could work around that. Here:

https://github.com/DetachHead/basedpyright/blob/6d94c2aca2683aaa1f36e5d1687c6240e239b315/packages/pyright-internal/src/realLanguageServer.ts#L207-L224

You fetch the entire basedpyright configuration from the client, but you don't read the analysis subsection off of that; instead, you only look at the analysis subsection a little earlier in the code, here:

https://github.com/DetachHead/basedpyright/blob/6d94c2aca2683aaa1f36e5d1687c6240e239b315/packages/pyright-internal/src/realLanguageServer.ts#L129

A more reliable option would be:

            const pyrightSection = await this.getConfiguration(workspace.rootUri, 'basedpyright');
            if (pyrightSection) {
                if (pyrightSection.openFilesOnly !== undefined) {
                    serverSettings.openFilesOnly = !!pyrightSection.openFilesOnly;
                }

                if (pyrightSection.useLibraryCodeForTypes !== undefined) {
                    serverSettings.useLibraryCodeForTypes = !!pyrightSection.useLibraryCodeForTypes;
                }

                serverSettings.disableLanguageServices = !!pyrightSection.disableLanguageServices;
                serverSettings.disableTaggedHints = !!pyrightSection.disableTaggedHints;
                serverSettings.disableOrganizeImports = !!pyrightSection.disableOrganizeImports;

                const typeCheckingMode = pyrightSection.typeCheckingMode;
                if (typeCheckingMode && isString(typeCheckingMode)) {
                    serverSettings.typeCheckingMode = typeCheckingMode;
                }

                // Check whether the client already gave us a `analysis` section here; deliberately ask for it
                // if we didn't receive it.
                if (not pyrightSection.analysis) {
                   pyrightSection.analysis = const pythonAnalysisSection = await this.getConfiguration(workspace.rootUri, 'basedpyright.analysis');
                }

                loadTheAnalysisSettings;   
            }

I can try to whip up a PR in that direction if you think that's reasonable.

Why does lsp-mode works, then? Well, lsp-mode keeps the same local configuration object as eglot:

{ "basedpyright" : { ... , "analysis" : { "lalala" } } }

But when the server asks lsp-mode for the basedpyright.analysis section, lsp-mode makes the executive decision interpret the dot ('.') as a hierarchical operator, so it looks for the basedpyright top-level section of its config, then looks for the analysis sub-section of that. Eglot, on the other hand, just looks for the basedpyright.analysis top-level section, finds nothing, and moves on. So basedpyrights config resolution mechanism assumes the '.' to be a hierarchical operator; but that's not in the spec, so any client that doesn't share the same assumptions needs this very obscure insight to be able to configure basedpyright correctly.