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

Don't require env_path to be passed to LanguageServerInstance #748

Closed
kdheepak opened this issue Jun 10, 2020 · 9 comments · Fixed by #772
Closed

Don't require env_path to be passed to LanguageServerInstance #748

kdheepak opened this issue Jun 10, 2020 · 9 comments · Fixed by #772

Comments

@kdheepak
Copy link

When using LanguageServerInstance in neovim, the default configuration currently starts a process for LanguageServer.jl in the current working directory:

https://github.com/neovim/nvim-lsp/blob/94cce7448f34f172492d98ff6efe10986f73eb9e/lua/nvim_lsp/julials.lua#L14

        project_path = pwd()
		...
        server = LanguageServer.LanguageServerInstance(stdin, stdout, project_path, depot_path);

When using vim / neovim, users are going to possibly going to do this:

cd Package.jl/src/module1/
nvim filename.jl

This means that LanguageServer will be started in the Package.jl/src/module1 folder instead of at Package.jl where the Project.toml file is located.

Additionally, one can imagine a Package.jl that has some tests that depend on external packages. In this case, Package.jl/tests/extras may contain a Project.toml file that specifies some dependencies for running test1.jl.

cd Package.jl/tests/extras/
nvim test1.jl

LanguageServer.jl/SymbolServer.jl will fail to find references / lint correctly in this case.

Would it be possible to have project_path recursively search up from the path provided till it finds a Project.toml?

@davidanthoff davidanthoff added this to the Backlog milestone Jun 10, 2020
@davidanthoff
Copy link
Member

We'll have to rethink the whole env handling story very soon in any case, for example to support situations where there are multiple env in a given workspace folder.

Having said that, I'm really not keen to support anything where the LS starts to look at files outside of the workspace that it is given by the client to work on. We do this with include statements (i.e. we follow include statements that lead outside of the workspace), and I would really like to not do that either. But @ZacLN likes it, and he is the boss here ;) But in all seriousness, I'd be hesitant to add another thing like that where we also start to look for Project.toml files in folders that are not part of the workspace...

We have the same situation in VS Code: if you start code in the folder Package/src, then things won't properly work for you. I think I'm ok with just saying "If you want to edit a package, make sure you open the root folder of the package in the editor, otherwise things will not work properly".

@jwortmann
Copy link
Contributor

jwortmann commented Jun 11, 2020

if you start code in the folder Package/src, then things won't properly work for you. I think I'm ok with just saying "If you want to edit a package, make sure you open the root folder of the package in the editor, otherwise things will not work properly".

I thought the purpose of env_path was to allow opening arbitrary folders, but to be able to give another path to the project environment via env_path. The main workspace folder is given as rootUri from the client already. If rootUri and env_path must be the same in all cases, then why does env_path exist at all? That would be duplicated information, so env_path could be omitted. And if rootUri is null (single file mode) you can still fall back to the default Julia environment, i.e. dirname(Pkg.Types.Context().env.project_file).

At least I also think it is a bad choice to specify the env_path (and maybe depot_path too) in the server's starting options. If still neccessary, they should be turned into workspace/configuration settings instead, because that's where per-project settings belong. The starting command for the server should not contain variables (besides arguments which never change like e.g. stdin/stdout), so that it can be hardcoded into the client (plugin).

@kdheepak
Copy link
Author

kdheepak commented Jun 11, 2020

Thanks for the quick response on this! I think the current interface where LanguageServer.jl is passed in the environment is simple and clean, and not implementing something that recursively searches for Project.toml files would be nice. I mainly wanted to open an issue to track this, since now that Neovim mostly works with LanguageServer.jl I expect more people will report similar issues :)

In eglot-jl the following is used:

# Get the project environment from the source path
project_path = something(Base.current_project(src_path), Base.load_path_expand(LOAD_PATH[2])) |> dirname

I'll look into get something similar working for neovim as well.

@kdheepak
Copy link
Author

It turns out src_path doesn't need to be a path to a file. It can be the path to a folder as well. So the change in this PR works for me when I start neovim in any folder within a project.

@kdheepak
Copy link
Author

The spec also has the following that is sent as part of the first request from the client to the server:

	/**
	 * The rootPath of the workspace. Is null
	 * if no folder is open.
	 *
	 * @deprecated in favour of rootUri.
	 */
	rootPath?: string | null;

	/**
	 * The rootUri of the workspace. Is null if no
	 * folder is open. If both `rootPath` and `rootUri` are set
	 * `rootUri` wins.
	 */
	rootUri: DocumentUri | null;

https://microsoft.github.io/language-server-protocol/specifications/specification-current/#initialize

Is there a reason this can't be used to get the project path?

@davidanthoff
Copy link
Member

@jwortmann I think I agree with everything you wrote, that makes much more sense than how I was thinking about this so far.

The depot is a bit of a different case: that option is really just there so that the VS Code extension can do some of the insulating of the LS process from anything else, I think it falls under the category that "arguments which never change" you mentioned.

But I fully agree on the env: we should probably just try to migrate that a normal configuration setting. We do have some errors related to the configuration story in our crash reporting, and I would want to fix those before we "task" the configuration stuff with more duties.

If we think that the julia env will become a standard configuration setting, then I think we will have to insist that it will be a valid path to a valid julia env. If the config setting is empty, I could see the LS looking for environments in the workspace folders.

@ExpandingMan
Copy link

ExpandingMan commented Jun 12, 2020

Hey guys, I might be a little late to the game here, but I'm trying to figure this out and I think I'm missing a major point: Does LS need a Manifest.toml to work properly? I had thought yes. If yes, it seems we'd need to completely rethink what directory is passed as the project directory, since most projects will not have a Manifest.toml when you're working on them.

My suggestion would be that, by default, LanguageServer do the following to check for the environment with the following priorities:

  1. Check the current directory.
  2. Check the immediate parent directory (this way users don't get screwed if they e.g. start editing code from src).
  3. Fall back to the default environment in the Julia depot.
  4. Give up, warn user.

If any of the above locations contains multiple environments in the same directory when checked, give up, warn user (I don't see why anyone would do that anyway).

If the Manifest.toml is not needed, in the vast majority of cases it would hit either options 1 or 2, however even in that case option 3 would be useful for editing independent scripts.

Of course, this is all complicated by the fact that, as far as I can tell, the language server protocol doesn't seem to specify anything outside of pure language analysis, so there don't seem to be any standards whatsoever regarding how a client should communicate this sort of metadata to a server, how the server should report issues to the client etc. One of the infuriating consequences of this has been that none of the clients seem to have a way of reporting back to the user any warnings or information thrown by the langauge server, so things like which environment is being used is effectively hidden from the user, and we are dependent on the good graces of the maintainer of the particular client to provide that information. I believe this lack of standardization of anything outside of the core language analysis tasks is the main reason why using LSP has proved so very painful anywhere outside of the VScode context this package was designed for.

However, because of this same problem, I think we are going to need LanguageServer.jl to do at least a little bit of work to try to figure out what environment it should be looking at, hopefully just the simple list above. Otherwise it will very easily break because it will be too reliant on a million separate language client implementations to always get it right.

@davidanthoff
Copy link
Member

We'll rework the whole env handling thing after Juliacon, but that will be a pretty long project.

In general, the LS will work better than today if it doesn't have information from the client on what env should be used. We will support situations where you open a package that has a Project.toml but no Manifest.toml in a much better way. The plan is to add support for "virtual environments", i.e. if the LS comes across such a stray Project.toml, and the user didn't specify a specific env to use in the client, we will create something like a temporary env that has only this package deved into it and use that for the name resolution. Only, we won't create a temporary env for real because we never want to instantiate packages in the background, instead we will try to use just the resolver logic, figure out for which package/versions we need symbol info and then download that from the (not yet existent) symbol cache in the cloud. We'll also use this concept of a virtual env to properly provide symbols for tests, for example.

I do think that @jwortmann's suggestion from above to just use the configuration system to communicate environment choices in the client to the LS is exactly right, so right now I very much plan to move in that direction as well, going forward.

Having said that: we are still very much in the design phase of much of this, and will not focus on this until after Juliacon.

non-Jedi added a commit to non-Jedi/LanguageServer.jl that referenced this issue Jun 19, 2020
runserver will attempt to pick a reasonable environment to run the
language server against.

Closes julia-vscode#748.

We still need to address running the language server against a project
with no Manifest.
@Seelengrab
Copy link
Contributor

I've run into similar issues trying to seperate the language server environment from my project environment in kakoune.

Just to add my 2 cents:

  • I think letting the editor steer the environment via workspace configuration of the LSP through RPC is a good idea.
  • Having a single editor/connection open doesn't necessarily mean that there is only one julia environment - in kakoune, I can have two different .jl files open that don't share an environment. These two should have different completions and symbol caches - ideally they're internally seperated by the workspace environment the editor is giving to the LSP. That way, standalone files would be able to fall back to the default environment and those contained in a project would have their project environment available.

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

Successfully merging a pull request may close this issue.

5 participants