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

Default to parent directory if opening a single Scala file. #67

Merged
merged 1 commit into from
Jan 15, 2021

Conversation

ckipp01
Copy link
Member

@ckipp01 ckipp01 commented Jan 15, 2021

In order to make sure Metals will work in any scenario, this small
change just grabs the parent directory of the opened file if it can't
find any of the root patterns.

Closes #66

Relates to scalameta/metals#2384

In order to make sure Metals will work in any scenario, this small
change just grabs the parent directory of the opened file if it can't
find any of the root patterns.

Closes scalameta#66

Relates to scalameta/metals#2384
@oblitum
Copy link

oblitum commented Aug 29, 2021

Hi.

I'm now the official nil workspace police 👮🏽‍♂️.

Please read this carefully.

You have been warned.

@ckipp01
Copy link
Member Author

ckipp01 commented Aug 29, 2021

Hi.

I'm now the official nil workspace police 👮🏽‍♂️.

Please read this carefully.

You have been warned.

Hi @oblitum, I'm not fully sure I understand your concern. On the Metals side, Metals actually needs a workspace to be set in order for this to work. So in the case of opening a single file, the parent directory actually needs to be set as the root_dir or Metals won't function correctly.

@oblitum
Copy link

oblitum commented Aug 29, 2021

I understand your condition, but your innocent solution may lead to problematic outcomes. The spec defines this about workspaces, emphasis mine:

The workspace folders configured in the client when the server starts. This property is only available if the client supports workspace folders. It can be null if the client supports workspace folders but none are configured.

(1) This implies that a conforming server should handle null workspace initialization, given the spec allow clients to ask for that.

From what I could infer, this is the least problematic contract that the LSP could have reached for having LSP features in a standalone file authoring situation. Meaning, e.g., one to be able to edit a file of a given type standalone and LSP being able to provide code completion for system/globally installed libraries/modules/packages without the involvement of some workspace at all.

In the many possible situations of opening files for editing, say, a scratch file on /tmp, or some system file on some system path, or some file on a remote server, etc, it's possible to notice that "workspaces" are just a "feature", not a "requirement", and hence, it can be said that's why the protocol is the way it is, even though they're basically a requirement in project dev.

When you fix the situation the way you're fixing it, what I'm seeing happening is not only the server not supporting (1), but the client enforcing "project dev" on anything. I see that the spec didn't go that contract because it can be potentially problematic, like, leading servers to scan large and deep directories because the parent directory happens to be /, or ~ or who knows what, which in fact has been a recurrent issue with many LSP clients.

If the server is unable to handle null workspaces, can't deal with standalone files editing, and require a workspace because a project dev should be always assumed, then that's the case, in coc.nvim for example, of setting requireRootPattern and erroring out when none can be inferred, or interactively always prompt the user, so that it becomes configured.

So, if you still go the route of implicitly creating workspaces out of thin air, you should realize all that.

@ckipp01
Copy link
Member Author

ckipp01 commented Aug 30, 2021

I understand your condition, but your innocent solution may lead to problematic outcomes.

I don't think you fully do understand our condition, and I also think you're misinterpreting the spec a bit.

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

Like many things inside of the spec, it's quite vague. There is no wording here or in the workspaceFolders that say you must pass in null if you don't have a full workspace, but simply that you can. From the Metals perspective, you're never in that situation. Even if you are editing a single file, the directory that contains that file is your workspace. So it therefore must be set. One of the main reasons for this is that we need to write a .metals/ dir to that dir where we keep some specific things relevant only to that file that we are editing even if it's a single file.

So, if you still go the route of implicitly creating workspaces out of thin air, you should realize all that.

Again, this isn't an implicit creation, but rather a very explicit one. If we don't find a build file as we traverse down, then we explicitly want the directory that the file was opened in to be the parent dir. It's intentional.

@oblitum
Copy link

oblitum commented Aug 30, 2021

I also think you're misinterpreting the spec a bit.
[deprecated section]
Like many things inside of the spec, it's quite vague. There is no wording here or in the workspaceFolders that say you must pass in null if you don't have a full workspace, but simply that you can.

Any intention in quoting a deprecated section? Also, I don't see what value it can bring, that's also why I refer to the actual one. The spec states "It can be null if the client supports workspace folders but none are configured". What I read here (and have explained already but I may have not been clear enough) is not that it's explicitly telling that it must be null when there's no workspace, no, I'm not reading "can" as "must", that's not the case. To make it very clear imagine this hypothetical but valid situation (that's why protocols exist): some client implementer go about reading the spec, that part in specific, without ever learning how a server handles it, it's a "blind" implementation, what does that part states that the client can do? That in the case it doesn't have a workspace it can send null to indicate that situation, and that may indeed be a valid, real, situation, e.g. let's imagine here the situation of opening up a file in vim on a read-only directory as a counter example to "file's directory as workspaces". I can only read that some client spec implementer by reading that can feel safe about the fact that it can send null if it supports workspaces but none could be configured. And then finally we come to the implied server's responsibility, which is that it must handle null cases, not because it's explicitly said so, but because it has no option if it wishes to cope with such conforming null-sending clients.

From the Metals perspective, you're never in that situation. Even if you are editing a single file, the directory that contains that file is your workspace. So it therefore must be set. One of the main reasons for this is that we need to write a .metals/ dir to that dir where we keep some specific things relevant only to that file that we are editing even if it's a single file.

All I see here is some implementation very-specific detail which seems to be taken so strongly as to overrule even the spec if necessary. Personally, that it must create a directory on the same path I open some file, is invasive. On spec's side, what's happening here is that this server does not support working without workspaces, and is implicitly configuring them, even when, ideally, would be best not to.

Again, this isn't an implicit creation, but rather a very explicit one. If we don't find a build file as we traverse down, then we explicitly want the directory that the file was opened in to be the parent dir. It's intentional.

That kind of behavior can never be inferred by reading the spec, so, how is that explicit? Do you warn users that that is going to happen or something in that sense? Is it culturally assumed in metals-land? Alternate discussion here just seems to land at simply not implicitly creating a workspace at all and warning users of that.

@ckipp01
Copy link
Member Author

ckipp01 commented Aug 30, 2021

@oblitum If you're actually a Metals user and have some concerns with the way we handle this, feel free to open up an issue in the main Metals repo.

@scalameta scalameta locked as resolved and limited conversation to collaborators Aug 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opening a standalone Scala file with no .git/ will blow up.
2 participants