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
933 stars 17 forks source link

display a warning when user settings are completely ignored if there are any project settings in `pyproject.toml`/`pyrightconfig.json` #513

Open eldar opened 1 month ago

eldar commented 1 month ago

Hi, thank you for this excellent LSP! I'm on NeoVIM and basedpyright reports most of the issues I care about, except the use of undefined symbols like undeclared variable x on the screenshot:

based_pyright

This is my config where I explicitly set reportUndefinedVariable:

          lspconfig.basedpyright.setup({
            -- settings = settings("basedpyright"),
            settings = {
                basedpyright = {
                  analysis = {
                    useLibraryCodeForTypes = true,
                    diagnosticSeverityOverrides = {
                      -- https://github.com/microsoft/pyright/blob/main/docs/configuration.md
                      reportUndefinedVariable = "error",
                      reportUnusedVariable = "warning", -- or anything
                      reportUnknownParameterType = false,
                      reportUnknownArgumentType = false,
                      reportUnknownLambdaType = false,
                      reportUnknownVariableType = false,
                      reportUnknownMemberType = false,
                      reportMissingParameterType = false,
                    },
                    typeCheckingMode = "off",
                  },
                },
            },
            capabilities = capabilities,
            root_dir = require("lspconfig.util").root_pattern(
              "pyproject.toml",
              ".git"
            ),

I am not 100% sure if this is a bug or a missed config flag. VS Code correctly reports reportUndefinedVariable:

pylance_vscode

KotlinIsland commented 1 month ago

Does the basedpyright plugin for vscode work here?

eldar commented 1 month ago

It did work in VS Code and the reason is that I set the typeCheckingMode="off" - which overrode the value of reportUndefinedVariable. Looks like diagnosticSeverityOverrides is not a correct category for this setting, but anyway this is not a bug.

eldar commented 1 month ago

Closing and apologies for the noise.

DetachHead commented 1 month ago

diagnosticSeverityOverrides should be overriding whatever typeCheckingMode is set to, so i think there might be an issue here, unless there's something i'm missing?

eldar commented 1 month ago

Yes, exactly. I'm trying to set individual setting that should override typeCheckingMode - but that doesn't seem to work. Setting typeCheckingMode to basic, standard, strict, etc, does change the diagnostics.

basedpyright = {
  analysis = {
    useLibraryCodeForTypes = true,
    diagnosticSeverityOverrides = {
      -- https://github.com/detachhead/basedpyright/blob/main/docs/configuration.md
      reportUndefinedVariable = "error",
      reportUnusedVariable = "warning", -- or anything
      reportUnknownParameterType = false,
      reportUnknownArgumentType = false,
      reportUnknownLambdaType = false,
      reportUnknownVariableType = false,
      reportUnknownMemberType = false,
      reportMissingParameterType = false,
    },
    typeCheckingMode = "off",
  },
},

Is this correct structure?

DetachHead commented 1 month ago

so diagnosticSeverityOverrides works on all the other typeCheckingModes but not "off"? if that's the case then it's definitely a bug, in which case we should re-open this issue.

i have a project that's set up this way using pyproject.toml and it seems to work, so maybe this issue only occurs in the language server?

eldar commented 1 month ago

diagnosticSeverityOverrides doesn't work for me either way, regardless of the value of typeCheckingMode - which I also set to basic or standard. Perhaps this is a problem with how nvim-lspconfig is setting up basedpyright, but then again typeCheckingMode does work. I can check whether setting these in pyproject.toml works (I presume it would) - the one I posted here done in Lua's global configuration of NeoVIM.

DetachHead commented 1 month ago

do you have a tool.basedpyright or tool.pyright section in your pyproject.toml? if so, does deleting it make the diagnosticSeverityOverrides in your lsp config work?

eldar commented 1 month ago

I tried pyproject.toml and it works:

[tool.basedpyright]                                                                                                                                                                                                        
typeCheckingMode = "basic"                                                                                                                                                                                               
reportDuplicateImport = "warning"

However setting the same from LSP config in NeoVIM doesn't - and I tried placing reportDuplicateImport both in analysis section or in diagnosticSeverityOverrides - in both cases it is ignored:

basedpyright = function()                                                                                                                                                                          
   lspconfig.basedpyright.setup({                                                                                                                                                                  
     -- settings = settings("basedpyright"),                                                                                                                                                       
     settings = {                                                                                                                                                                                  
       basedpyright = {                                                                                                                                                                            
         analysis = {                                                                                                                                                                              
           useLibraryCodeForTypes = true,                                                                                                                                                          
           typeCheckingMode = "basic", -- off, basic, standard, strict, all                                                                                                                        
           reportDuplicateImport = "warning",                                                                                                                                                      
           diagnosticSeverityOverrides = {                                                                                                                                                         
             reportDuplicateImport = "warning",                                                                                                                                                    
           },                                                                                                                                                                                      
         },                                                                                                                                                                                        
       },                                                                                                                                                                                          
     },                                                                                                                                                                                            
     capabilities = capabilities,                                                                                                                                                                  
     root_dir = require("lspconfig.util").root_pattern(                                                                                                                                            
       "pyproject.toml",                                                                                                                                                                           
       ".git"                                                                                                                                                                                      
     ),                                                                                                                                                                                            
   })  

if so, does deleting it make the diagnosticSeverityOverrides in your lsp config work?

I cannot delete tool.basedpyright section in my pyproject.toml completely because it contains exclude paths - without it LSP won't work at all. However if I comment out all of the type checking related settings in pyproject.toml - LSP config still doesn't work.

DetachHead commented 1 month ago

i can't seem to reproduce this unfortunately. do you think you could create a minimal reproduction of the issue, similar to the one in this comment?

eldar commented 1 month ago

Hey! I managed to narrow down the problem and prepared a test case. Here is the neovim config and corresponding python test case and pyproject.toml. I used Mason to automatically install basedPyright but there should be no problem in altering the config and using the LSP in the $PATH.

# download the NeoVIM config with Pyright
export NVIM_APPNAME=basedpyright-bugreport
mkdir ~/.config/$NVIM_APPNAME
wget https://gist.githubusercontent.com/eldar/8a79dd4d84b63ed277667401f6cd3e81/raw/397deeb5f8f4a1730ad0d1358cbce99cd5998e5b/init.lua -P ~/.config/$NVIM_APPNAME

# download the python test case
git clone https://github.com/eldar/basedPyright-issue-513.git
cd basedPyright-issue-513
nvim test.py

This reproduces the issue as on the attached screenshot:

basedPyright-513

Duplicate import is not detected despite that I set reportDuplicateImport to warning in init.lua. The reason for that is the included pyproject.toml with a single line:

[tool.basedpyright]

Enabling basedPyright config in pyproject.toml makes the settings provided in init.lua irrelevant, even though there is no typeCheckingMode or diagnosticSeverityOverrides in the TOML file. You can verify that commenting out [tool.basedpyright] line in pyproject.toml enables detection of duplicate imports as specified in init.lua.

The expected behavior here is that individual settings provided in pyproject.toml should take priority over global settings in the editor, but merely including a section in the project specific file should not invalidate the entire config. Thanks for looking into this!

DetachHead commented 1 month ago

i think this is the same issue as https://github.com/microsoft/pyright/issues/836

tbh this is one of the few times where i agree with eric, specifically this comment https://github.com/microsoft/pyright/issues/836#issuecomment-659808974 where he says if this was supported, it would result in users seeing different errors to what they'd see in their CI.

this has been raised before in the basedtyping discord, and we came to an agreement that the current behavior is acceptable, but there should be some sort of warning visible to the user that this occurs (#67) instead of just silently ignoring the settings.

eldar commented 1 month ago

Hey, thanks for tracking it down to the upstream issue!

I would still argue that the original behavior is not particularly thought through. More specifically, it basically renders the default settings in the editor/IDE useless. I added basedpyright section to pyproject.toml because I wanted to add workspace-specific exclude paths which seems like a common thing to do. Also, the same usecase (using project specific configuration for exclude paths) was reported by the author of the issue in the upstream Pyright repository. At the same time, I feel that configuring LSP diagnostics is something that most people do quire rarely and it's easier if it is a part of the editor configuration.

The workaround is simple enough though which is just to copy around corresponding pyright options between different projects, so this is not really a blocker.

DetachHead commented 1 month ago

fair enough, several people have raised this both here and upstream so i think it's reasonable to support it

DetachHead commented 1 month ago

i do think this functionality should be opt-in though, since i personally would never want any global config to override project-specific config, i feel like that's something the user should have to explicitly enable themselves.

come to think of it, that kinda sounds like the "extends" option which already exists. does that cover this use case?

// path/to/global/config.json
{
    // global settings go here
}
// project/pyrightconfig.json
{
    "extends": "/path/to/global/config.json"
    // project-specific settings go here
}