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

fix(uri): don't use full path for custom schema #19773

Closed
wants to merge 3 commits into from

Conversation

sigmaSd
Copy link
Contributor

@sigmaSd sigmaSd commented Aug 14, 2022

deno lsp uses a custom schema deno:

uri_from_bufnr returns the full path of the buf file but this doesn't work with custom schema

This fixes the last remaining bug here neovim/nvim-lspconfig#2005 relative dependencies inside virtual text document is not resolved correctly

Notes:

  • maybe if relative_name:match("deno:") should become if is_custom_schema(realtive_name) where is_custom_schema returns true for any schema that matches URI_SCHEME_PATTERN but is not equal to file:///
  • this changes fixes all my issues with denols (with some minor modifications that I can pr later)

This is more to start a discussion I thought showing code is easier to discuss then raising an issue

deno lsp uses a custom schema `deno:`

`uri_from_bufnr` returns the full path of the buf file but this doesn't work with custom schema

This fixes the last remaining  bug here <neovim/nvim-lspconfig#2005> `relative dependencies inside virtual text document is not resolved correctly`

Notes:
- maybe `if relative_name:match("deno:")` should become `if is_custom_schema(realtive_name)` where is_custom_schema matches `URI_SCHEME_PATTERN` but is not equal to `file:///`
- this changes fixes all my issues with denols (with some minor modifications that I can pr later)

This is more to start a discussion I thought showing code is easier to discuss then raising an issue
@sigmaSd
Copy link
Contributor Author

sigmaSd commented Aug 14, 2022

For example here is how coc-deno is currently working-around the issue https://github.com/fannheyward/coc-deno/blob/1b42c38a9d78baf14ac90de2347a4e240f020de5/src/extension.ts#L126

@clason clason added the lsp label Aug 14, 2022
@github-actions github-actions bot requested a review from mfussenegger August 14, 2022 14:50
@sigmaSd
Copy link
Contributor Author

sigmaSd commented Aug 14, 2022

Btw the full path in the buffer name is coming from vim.fn.bufadd it seems to add the full path to most things except the one that start with atleast double slash, as in:

  • deno:// it will keep it the same
  • deno:/ will become ${currrentPath}/deno:/

for example in deno lsp case:

deno:/https/deno.land/x/simple_shell%400.13.0/mod.ts becomes /home/user/dev/deno:/https/deno.land/x/simple_shell%400.13.0/mod.ts

runtime/lua/vim/uri.lua Outdated Show resolved Hide resolved
@ii14
Copy link
Member

ii14 commented Aug 14, 2022

I don't think handling every custom URI scheme globally is a good solution. In my opinion a better alternative would be to make URI translation configurable per LSP client. I'm not sure what are all the contexts where vim.uri is used from, so solving this issue could require some more fundamental changes.

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Aug 14, 2022

My personal opinion is

  • The best solution is to fix vim recognition of url scheme which is not conformant to spec, and fixing it will make this just work
  • I wasn't planning to keep it hard-coded like this if relative_name:match("deno:") now that I read some spec I think what I would replace it with is is_not_special_scheme https://url.spec.whatwg.org/#special-scheme

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Aug 14, 2022

  • For the first option vim seems to 'recognize' scheme here
    int path_is_url(const char *p)
    :// seems to be also used in other places in the code-base changing it maybe a breaking change
  • I updated the pr with the second option

In my opinion a better alternative would be to make URI translation configurable per LSP client

I'm fine with any option that works but I'm not sure how to implement this

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the proper solution is to fix the URI recognition.
Where is the conversion happening? Is

return vim.fn.bufadd(uri_to_fname(uri))

The problem, or bufadd itself?

I think bufname returns the path relative to the current directory, but with LSP the workspace root might be different than the current directory, so this approach here could break with that.

And there's also #19534 which kinda contradicts with this PR

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Aug 15, 2022

Yes the problem is in bufadd

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Aug 15, 2022

bufadd calls eventually path_is_url which has the problem

int path_is_url(const char *p)

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Aug 15, 2022

Personally I want to just change if (strncmp(p, "://", 3) == 0) { to if (strncmp(p, ":/", 2) == 0) { (the spec only requires : but I think this is good enough) but no idea what other implications can this have.

It could be debated that any thing that relied on this bug is incorrect though.

@ii14
Copy link
Member

ii14 commented Aug 15, 2022

The problem is that unlike Windows, Linux only reserves the / character in file paths. Changing it from :// to : would most likely break editing some files with : in the name, for example :edit foo:bar.txt. It's not something that should be that common, but still something you might occasionally encounter.

Maybe this could be disambiguated by :e ./foo:bar.txt


--- Get a URI from a bufnr
---@param bufnr number
---@return string URI
local function uri_from_bufnr(bufnr)
local relative_name = vim.fn.bufname(bufnr)
local maybe_scheme = relative_name:match(URI_SCHEME_PATTERN)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't followed the thread, so sorry if this has been covered, but why can't relative_name be fname here?

Copy link
Member

@ii14 ii14 Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvim_buf_get_name always returns the full path. When you create a buffer with a name deno:some/file, the current directory will be prepended to it (/home/user/deno:some/file). With bufname() it prefers returning the short name (deno:some/file), but it's generally not 100% reliable.

Copy link
Member

@lewis6991 lewis6991 Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this is all a nasty hack. nvim_buf_get_name is doing exactly the right thing. We shouldn't be relying on the cwd the parse the correct uri.

deno should be using something like deno://some/file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deno:some/file is technically compliant with the spec, vim just treats it like a normal path.

Two potential solutions we have right now is to start complying with the URI spec, or translate paths to and from deno://

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How feasible do we think it is to comply with the URI spec? I think that would be preferable if it doesn't break too much.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be straightforward, all checks for :// have to be replaced with :, I think it's just in few places. But yeah, the question is how much of the user stuff would it break.

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Aug 16, 2022

I opened #19797 to propose changing path_is_url

@justinmk
Copy link
Member

Closing for #19797

@justinmk justinmk closed this Aug 23, 2022
justinmk pushed a commit that referenced this pull request Aug 24, 2022
Problem:
path_to_url() returns false for single-slash URIs ("foo:/" vs "foo://").
This is not compliant with the URI spec. https://url.spec.whatwg.org/#url-representation
LSP in particular allows single-slash URIs.

Solution:
Relax path_to_url() to accept single-slash URIs. This is not fully
compliant (only ":" is required by the spec), but it is hopefully good
enough without causing false-positives in typical text files.

ref https://url.spec.whatwg.org/#windows-drive-letter
ref #19773
ref #19773 (comment)
smjonas pushed a commit to smjonas/neovim that referenced this pull request Dec 31, 2022
Problem:
path_to_url() returns false for single-slash URIs ("foo:/" vs "foo://").
This is not compliant with the URI spec. https://url.spec.whatwg.org/#url-representation
LSP in particular allows single-slash URIs.

Solution:
Relax path_to_url() to accept single-slash URIs. This is not fully
compliant (only ":" is required by the spec), but it is hopefully good
enough without causing false-positives in typical text files.

ref https://url.spec.whatwg.org/#windows-drive-letter
ref neovim#19773
ref neovim#19773 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants