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

Improve plugin start #238

Closed
wants to merge 2 commits into from
Closed

Improve plugin start #238

wants to merge 2 commits into from

Conversation

tdroxler
Copy link
Member

@tdroxler tdroxler commented Jul 8, 2024

Resolves #237

I searched for a while, but vscode doesn't seems to have builtin stuff for finding a different the root dir.

For testing I tried manually every case I could think, starting from various places, with/without the rootFiles etc.

Please give it a try, the goal is really to have a good UX so users don't need to care about how/where ralph-lsp is launched.

We ensure that the root dir is correctly found.
Otherwise `.ralph-lsp` will be created at the wrong place and
compilation won't work as expected.
@tdroxler tdroxler requested a review from simerplaha July 8, 2024 08:36
@simerplaha
Copy link
Member

simerplaha commented Jul 9, 2024

Test 1

  • Create a project directory /Users/home/VSCodeProjects/my_project.
  • Write a project file code.ral to my_project.
  • Launch VSCode on my_project.

Issue

  • The workspace directory is not set to my_project as intended, instead, It is set to two directories up in /Users/home/ causing ralph.json to be written to /Users/home/.ralph-lsp/ralph.json or ~/.ralph-lsp.
  • After resolving errors in ralph.json, it results in permission error /Users/home/Pictures/Photos Library.photoslibrary: Operation not permitted.
    image
  • If another project /Users/home/VSCodeProjects/my_project_2 is created, both projects share the same ralph.json in /Users/home/.ralph-lsp generated by the above test.

Test 2

  • Following Test 1, create another directory /Users/home/VSCodeProjects/contracts.
  • Launch VSCode on my_project created in Test 1.

Issue

  • This time, the ralph.json generated in Test 1 is ignored, and another one is generated in /Users/home/VSCodeProjects/.ralph-lsp/ralph.json
  • After resolving the artifactPath error, It compiles the sibling directory contracts instead of the launched project my_project.

Safety issue: Writes files outside the intended workspace directory.

For vscode we need the same approach as nvim, to look in current and parent folders where is the "real" root dir.

The "real root dir" is not the actual workspace directory I opened. This is changing the intended workspace directory and writes to parent directory which is unintended. In addition to the above tests, If I open /Users/home/VSCodeProjects/contracts, this writes to VSCodeProjects, if /Users/home/contracts is opened it writes to /Users/home. The access should be restricted to the intended directory opened.

This behaviour is unsafe as it writes files to a directory outside the workspace directory. Changing the workspace directory to a parent directory ("look in current and parent folders") without explicit permission is unsafe.

There's an issue with that: If you launch vscode from contracts folder, it will create there a .ralph-lsp/ralph.json and that later will compains that contracts and artifacts don't exist.

The complaint behaves as intended and provides clear errors which can be resolved easily in the IDE itself. If the goal is to emit a ralph.json with contractsPath and artifactsPath set to directories other than the default contracts and artifacts folders, this can be implemented on the server side. For example, the server can emit the ralph.json with contractsPath = "" and artifactsPath = "", setting source directories to root directory.

I'm not sure how this change improves UX (nvim, VSCode, IntelliJ are different users). But if it must be implemented for all, please consider modifying the the emitted ralph.json on the server-side.

@tdroxler
Copy link
Member Author

Way too much edge cases indeed.

I spent few hours digging lot's of extension for various LSP servers and I think I start to see a common way to handle those config stuff.

I will prepare a full response tomorrow.

@tdroxler
Copy link
Member Author

Let's continue de discussion here
This PR is indeed not at all the right way for vscode

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

Successfully merging this pull request may close these issues.

Server and Clients configuration
2 participants