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

Show errors for whole workspace, not just open files #145

Open
maflAT opened this issue Mar 4, 2023 · 23 comments
Open

Show errors for whole workspace, not just open files #145

maflAT opened this issue Mar 4, 2023 · 23 comments
Labels
enhancement New feature or request

Comments

@maflAT
Copy link

maflAT commented Mar 4, 2023

Hi!
Is there a setting, that would enable this behavior?
Equivalent to Pylance's python.analysis.diagnosticMode setting.

@MartinBernstorff
Copy link

Given the speed of ruff, this sounds awesome!

@charliermarsh
Copy link
Member

I have to look into how this works with Pylance, I'm totally unfamiliar with how it does that 😅

@charliermarsh
Copy link
Member

(But no such setting exists right now AFAIK, to answer the original question.)

@MartinBernstorff
Copy link

Ah yeah, maybe it's Microsoft granting special privileges to Pylance. I know that the Mypy extension by Matan Grover does a similar thing, so should be possible!

Being able to quickly see all the problems and use the "Go to next" shortcut in VSCode makes migrating an old codebase to ruff, or enabling new rules, much easier.

@joshjhans
Copy link

Also looking for this feature! 😄

@jcphlux
Copy link

jcphlux commented Jun 5, 2023

yeah this would be a great feature!

@eric-october
Copy link

upvote

@zanieb
Copy link
Member

zanieb commented Aug 19, 2023

If anyone is willing to make a contribution or do research on how other tools do this, I'm happy to collaborate.

Otherwise, please use the 👍 reaction on the first post instead of bumping the thread.

@IamMaxim
Copy link

I have digged into how mypy extension does this.

Mypy has a "daemon mode". Mypy daemon outputs all of its data to stdout, as it does not have LSP capabilities. It is more like invoking ruff check <directory> --watch and parsing it using extension, but while ruff re-analyzes entire project (and prints ALL of problems for the entire project to stdout as soon as ANY of the files is updated), mypy only prints new problems ONLY for the file updated to stdout. But that should not play a big role, it is only an implementation detail.

Basically, mypy extension launches mypy in daemon mode, passing the entire project directory as argument. Then, as new data arrives to stdout, it continuously parses it and updates problems list. This has its own downsides: you only get problem updates after you save a file, not while typing.

I have not yet got into how LSP works and ruff/ruff-vscode codebase, though.
I will look into how ruff LSP works, but my preliminary understanding is that ruff LSP does not have watch mode (otherwise, I think you should have already came up with an idea). Probably, we would need to combine ruff check --watch to analyze all project files and use LSP to provide updates while typing.

Anyway, @zanieb, I would be happy to collaborate on this.

@charliermarsh
Copy link
Member

I haven't researched this deeply, but based on the above, I think my main questions would be (and these are largely focused on implementation):

  • How do you return diagnostics that correspond to the non-current document, and even to documents that the LSP has never seen before? (E.g., we do LSP_SERVER.publish_diagnostics(document.uri, diagnostics). How would we publish diagnostics for other documents, such that "Go to next" would allow you to page through them?)
  • Is it easy for us to get the workspace path for a given file? (I think so?)
  • What happens if you have multiple workspaces open? How do we know which directory paths to analyze? (I'm not an expert on VS Code workspaces but that kind of thing often requires consideration in the LSP.)
  • We could perhaps just run Ruff over the whole workspace (any files that haven't changed will hit the cache) rather than just the open file (as opposed to trying to use --watch which IMO would be more likely to cause problems since the LSP should be sending us events rather than us trying to detect them).

I do want to mention that I think there's a decent change we rewrite the LSP some time in the next few months. We want to integrate it into Ruff more deeply -- as written, it's basically a wrapper around the Ruff executable CLI. I don't know when that will happen (perhaps not for a long time), we don't have a concrete plan around it, it's just something that has come up a few times. So while I'm happy to support this, if anyone works on it, I don't want them to be caught off guard if that happens (which would probably make some but not all of this work redundant, since we'd be completely redesigning the LSP).

@IamMaxim
Copy link

@charliermarsh for the first point, here's the function in mypy plugin repository which shows how to set diagnostics for any file in a workspace: https://github.com/matangover/mypy-vscode/blob/master/src/extension.ts#L440C2-L440C2

For the second point, there are vscode.workspace.rootPath and vscode.workspace.workspaceFolders properties which I think should fit.

For the third: there is vscode.workspace.getWorkspaceFolder(uri) function that returns workspace for a given file.

For the fourth: yes, I agree, this could be a good starting point. There are already examples of how we can populate diagnostics list for different files in mypy repository, so this shouldn't be difficult.

There should also be a possibility to use VS Code's own file watcher instead of ruff --watch, otherwise, we would just have two watchers for the same files. But I think there should a one big decision: is it LSP server who drives which files are checked and checks them automatically (push approach), or is it LSP client (VS Code plugin) which watches for file changes and requests new checks from LSP server. Both approaches seem equally reasonable for me with respect to VS Code plugin, but other LSP clients (like neovim) could have worse tooling, which requires to implement watching and other workspace-related stuff manually.

Doing push approach would move most of the work on this feature to ruff repository, pull approach would leave it in this repo.

As for LSP rewrite — I will do some research on what LSP provides as an interface and how ruff implements it.

I will research LSP specification and be back :)

@IamMaxim
Copy link

Update: I also noticed that rust-analyzer extension shows errors for non-open files.

@MartinBernstorff
Copy link

vscode-mypy has recently implemented it: microsoft/vscode-mypy#81 (comment)

@charliermarsh
Copy link
Member

Thanks @MartinBernstorff, that's really helpful since we use the same template as that extension so there's some common code.

I think we could mostly follow the patterns implemented in there although it may actually be easier for us since Mypy returns diagnostics by following imports (so they have to filter out irrelevant diagnostics if the scope isn't set to file).

So, something like:

  • Introduce a reportingScope setting, similar to in vscode-mypy.
  • In _run_tool_on_document, run over the current working directory (or the workspace directory? I'm not sure, this needs to be tested) if the reporting scope is set to "workspace".
  • Change calls to LSP_SERVER.publish_diagnostics to publish to the appropriate document (the above PR will be helpful for that).

I'd be happy to review PRs in this direction.

@charliermarsh
Copy link
Member

Actually, there's at least one issue with this approach (I assume it impacts the Mypy extension too?) which is that if we run ruff ${current_directory} in order to surface violations for all files in the workspace, we run the risk of analyzing the files on disk rather than the modified contents in the editor (which we currently avoid by taking the text from the LSP and passing it to Ruff over stdin). I don't know how to avoid that with this approach or what the extent of the impact would be. Like I said, I would expect this to apply to the Mypy extension too, at least based on my cursory read.

@theroggy
Copy link

theroggy commented Sep 1, 2023

Actually, there's at least one issue with this approach (I assume it impacts the Mypy extension too?) which is that if we run ruff ${current_directory} in order to surface violations for all files in the workspace, we run the risk of analyzing the files on disk rather than the modified contents in the editor (which we currently avoid by taking the text from the LSP and passing it to Ruff over stdin). I don't know how to avoid that with this approach or what the extent of the impact would be. Like I said, I would expect this to apply to the Mypy extension too, at least based on my cursory read.

Personally I use autosave, which avoids (amongst others) this...

@abceleung
Copy link

Actually, there's at least one issue with this approach (I assume it impacts the Mypy extension too?) which is that if we run ruff ${current_directory} in order to surface violations for all files in the workspace, we run the risk of analyzing the files on disk rather than the modified contents in the editor (which we currently avoid by taking the text from the LSP and passing it to Ruff over stdin). I don't know how to avoid that with this approach or what the extent of the impact would be. Like I said, I would expect this to apply to the Mypy extension too, at least based on my cursory read.

Maybe lint the whole project once during startup, after that only lint modified content (I think the Sqlfluff VSCode extension does this). This should more or less achieve the same thing?
Screenshot 2023-11-30 103156

@zanieb
Copy link
Member

zanieb commented Nov 30, 2023

Maybe lint the whole project once during startup, after that only lint modified content

Hm what if I like.. switch git branches and don't have the changed files open?

I think we may be better off with the approach Charlie outlined, even if it doesn't solve the "on-disk vs in-buffer" issue.

@carlos-sarmiento
Copy link

I think @charliermarsh approach is pretty good as an explicit command. The command description could indicate that it will analyze files on disk and warn users they have unsaved files open.

@fgimian
Copy link

fgimian commented Dec 23, 2023

In case it helps, the Go VS Code extension has a similar option which is very handy:

image

Personally, I would love it if Ruff always showed all errors for my entire workspace. I think linting the entire workspace on save of any file would probably be optimal as a change in one file may result in different violations or fixes in other files.

golangci-lint (Go's linter) is actually significantly slower than Ruff and appears to do just this.

Personally, I think this is the one change that would make Ruff a lot more useable in VS Code. Presently, I am running it via the CLI a lot to see where my issues reside (especially on larger projects with hundreds of files).

Cheers
Fotis

@y2kbugger
Copy link

Actually, there's at least one issue with this approach (I assume it impacts the Mypy extension too?) which is that if we run ruff ${current_directory} in order to surface violations for all files in the workspace, we run the risk of analyzing the files on disk rather than the modified contents in the editor (which we currently avoid by taking the text from the LSP and passing it to Ruff over stdin). I don't know how to avoid that with this approach or what the extent of the impact would be. Like I said, I would expect this to apply to the Mypy extension too, at least based on my cursory read.

Personally I use autosave, which avoids (amongst others) this...

but causes other issues, such as making hotreload workflows really annoying. Typing almost immediately restarts your flask server and then the page reloads to a crash.

@iorlas
Copy link

iorlas commented Sep 6, 2024

One more upvote there. An absence of such feature confuses some engineers migrating to VS Code, and would certainly help a looot for deeper code review sessions.

We consider to run ruff separately and just create simple plugin to review the reports, but it feels ugly.

@dhruvmanila
Copy link
Member

This is a feature that's on the roadmap for the red knot project that's currently a WIP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests