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

LSP enablePaths option not working in non-VSCode editor #19802

Closed
sgwilym opened this issue Jul 11, 2023 · 16 comments · Fixed by #20358
Closed

LSP enablePaths option not working in non-VSCode editor #19802

sgwilym opened this issue Jul 11, 2023 · 16 comments · Fixed by #20358
Assignees
Labels
bug Something isn't working correctly lsp related to the language server

Comments

@sgwilym
Copy link
Contributor

sgwilym commented Jul 11, 2023

I'm trying to add support for the enablePaths option to my Deno extension for Nova.

But it doesn't seem to matter how I pass the paths to the LSP upon initialisation — src, ./src, /Users/me/project/src, file://Users/me/project/src — the LSP keeps working on files outside of these paths.

Here's a gist with the initialisation RPC call. As far as I can tell, this should work. Could some other option be messing with it?

Could this be something to do with Nova? I can see that its built-in LSP client is sending RPC calls like this for files outside of the enabled paths:

{
  "jsonrpc" : "2.0",
  "id" : 10,
  "method" : "textDocument\/hover",
  "params" : {
    "textDocument" : {
      "uri" : "file:\/\/\/Users\/gwil\/Projects\/nova-deno\/build.ts"
    },
    "position" : {
      "line" : 7,
      "character" : 12
    }
  }
}

And even though this file is outside the enabledPaths, the Deno LSP server will send back a result:

{
  "id" : 11,
  "jsonrpc" : "2.0",
  "result" : {
    "range" : {
      "start" : {
        "line" : 8,
        "character" : 16
      },
      "end" : {
        "line" : 8,
        "character" : 20
      }
    },
    "contents" : [
      {
        "value" : "const code: string",
        "language" : "typescript"
      },
      "The bundles code as a single JavaScript module."
    ]
  }
}

Should it do that? Should it send back nothing?

@dsherret
Copy link
Member

There is a test for this exact scenario here:

let res = client.write_request(
"textDocument/hover",
json!({
"textDocument": {
"uri": root_specifier.join("./file.ts").unwrap(),
},
"position": { "line": 0, "character": 19 }
}),
);
assert_eq!(res, json!(null));

Why are all the forward slashes escaped in these values? Maybe it has something to do with that?

https://gist.github.com/sgwilym/02f8787b39d4b6d0e580482ef0a14802#file-initialisation_rcp-json-L195

@dsherret dsherret added question a question about the use of Deno lsp related to the language server labels Jul 11, 2023
@sgwilym
Copy link
Contributor Author

sgwilym commented Jul 11, 2023

Yeah, I noticed that too, but as there was escaping in other parts of the initialisation options I assumed it was okay. Maybe it's not?! Which would be a pain, as the IDE's built-in client is escaping the values I pass through it whether I want that or not.

@dsherret
Copy link
Member

It might not be an issue. I'm unsure and still looking into this.

@dsherret
Copy link
Member

It seems like enablePaths doesn't work with absolute paths. Try providing just ./src

@dsherret
Copy link
Member

dsherret commented Jul 11, 2023

Oh, actually that looks to be a vscode_deno behaviour with not working with absolute paths: https://github.com/denoland/vscode_deno/blob/db25171806abe8d8807ca41f8d8237b68dd708a8/client/src/extension.ts#L105 (that's for supressing built-in ts diagnostics, which is specific to vscode_deno).

I'm not sure what the issue here is with nova, but there is a passing test in the deno repo. Does Deno output any logs to stderr like "Unable to resolve a file path for deno.enablePath"?

@sgwilym
Copy link
Contributor Author

sgwilym commented Jul 11, 2023

Not that I can see. I get back a response indicating a successful initialisation.

The form of the path doesn't seem to affect what happens. src, ./src, /User/project/src all seem to have no effect.

@sgwilym
Copy link
Contributor Author

sgwilym commented Aug 4, 2023

Interestingly, after the introduction of 1.36 and #19791 I can get this behaviour using the exclude option in deno.json.

But this is exclusive, rather than the inclusive enablePaths option. But on the other hand, this can be more easily checked into a repo. Still, I would like to be able to support enablePaths in my extension, maybe exclude working provides a clue?

@sigmaSd
Copy link
Contributor

sigmaSd commented Sep 1, 2023

Same issue with helix, only exclude worked for me as well

@nayeemrmn
Copy link
Collaborator

Here's a gist with the initialisation RPC call.

deno/cli/lsp/config.rs

Lines 702 to 724 in a745549

pub fn update_enabled_paths(&mut self) -> bool {
if let Some(workspace_folders) = self.workspace_folders.clone() {
let mut touched = false;
for (workspace, _) in workspace_folders {
if let Some(settings) = self.settings.specifiers.get(&workspace) {
if self.update_enabled_paths_entry(
workspace,
settings.enable_paths.clone(),
) {
touched = true;
}
}
}
touched
} else if let Some(root_uri) = self.root_uri.clone() {
self.update_enabled_paths_entry(
root_uri,
self.settings.workspace.enable_paths.clone(),
)
} else {
false
}
}

@sgwilym It might work if you don't specify workspaceFolders. If that's specified we appear to acquire enablePaths() for each folder by using the workspace/configuration request, and you probably haven't implemented that client capability.

But I don't think we should ignore the root workspace settings if workspaceFolders is there. So the above else if is a problem. I'll see if I can reproduce and that fixes it.

@nayeemrmn nayeemrmn self-assigned this Sep 1, 2023
@nayeemrmn nayeemrmn added bug Something isn't working correctly and removed question a question about the use of Deno labels Sep 1, 2023
nayeemrmn added a commit that referenced this issue Sep 2, 2023
Fixes #19802.

Properly respect when clients do not have the `workspace/configuration`
capability, a.k.a. when an editor cannot provide scoped settings on
request from the LSP.

- Fix one spot where we weren't checking for the capability before
sending this request.
- For `enablePaths`, fall back to the settings passed in the
initialization options in more cases.
- Respect the `workspace/configuration` capability in the test harness
client.

See:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration.
@sgwilym
Copy link
Contributor Author

sgwilym commented Sep 2, 2023

Thank you @nayeemrmn, looking forward to using this feature at last!

@nayeemrmn
Copy link
Collaborator

@sgwilym Can you confirm on the --canary version that this fix works for you?

@sigmaSd
Copy link
Contributor

sigmaSd commented Sep 3, 2023

I can confirm this works in helix now.

@sgwilym
Copy link
Contributor Author

sgwilym commented Sep 4, 2023

@nayeemrmn Unfortunately I still seem to have the same problem. Sorry!

Here is the initialisation request, with many variants of src included in enablePaths. Unfortunately files outside of src still are processed by the LSP, and show tooltips, code actions, etc.

How the LSP is initialised is a bit out of my control, as my Deno extension uses Nova's built-in LSP client. So I may have to communicate with the devs for that editor to change how the LSP server is initialised.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 4, 2023

@sgwilym When testing, do you also have a nova config file which has enablePaths in it? If not then nova is responding to workspace/configuration requests with an empty config file which is overriding your initialization options.

@sgwilym
Copy link
Contributor Author

sgwilym commented Sep 5, 2023

@nayeemrmn My extension is responsible for providing the values of enablePaths, yes.

I went looking for workspace/configuration requests, and see these logged:

{
  "jsonrpc" : "2.0",
  "id" : 8,
  "method" : "workspace\/configuration",
  "params" : {
    "items" : [
      {
        "section" : "deno",
        "scopeUri" : "file:\/\/\/Users\/gwil\/Projects\/nova-deno\/build.ts"
      }
    ]
  }
}

So it seems like Nova is sending workspace/configuration requests for each file that's open, and this is overriding the enablePaths options I'm providing to Deno's LSP server?

@nayeemrmn
Copy link
Collaborator

So it seems like Nova is sending workspace/configuration requests for each file that's open, and this is overriding the enablePaths options I'm providing to Deno's LSP server?

Correct. You can disable that by turning off this capability, i.e. switching https://gist.github.com/sgwilym/02f8787b39d4b6d0e580482ef0a14802#file-initialisation_rcp-json-L20 to false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly lsp related to the language server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants