-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
x/tools/gopls: need a theory of URls #64147
Comments
I think there are the following three categories:
These categorize do make sense to me. However, I'm not sure how well we honor them. |
Funny, I was thinking along those lines too! You could imagine a simple reflective traversal over the params JSON object that mutated the DocumentURI fields in place to normalize Then we could use DocumentURI everywhere. Let's try it out. (How do we test it?) A few little helpers (like filepath.Dir and Base and Join) for URLs would also go a long way to avoiding the need for conversions. |
Yes, this has come up in the past (I thought we had an issue for a 'uripath' library). (Aside: I suppose it's not fair to say "this has come up in the past", because it feels like everything has come up in the past!). It's a good idea. I just never had the patience to convince myself of the algorithmic correctness. |
The implementations can convert to and from strings and call filepath, just like we do today. At least it'll centralize any existing bugs in one place. |
Change https://go.dev/cl/542616 mentions this issue: |
Recently we finally got rid of almost everything in the golang.org/x/tools/gopls/internal/span package. Only span.URI remains, and I would like to get rid of it too, either by deleting it, moving it to a sensible location, or fusing it with something else. The question is: what does the span.URI abstraction represent? The disappointing current answer is: "just a URI"; it needn't be a "file:" URI, nor need it be non-empty. Calling URI.Filename on an invalid or non-file-scheme span.URI panics.
The protocol.DocumentURI type represents a URI of an editor document. In practice it is always a "file:" scheme URI (though this isn't required by the LSP docs). However, according to those docs, it may be URL-encoded, so colons in Windows drive letters do not appear literally, and there is evidence in the code that it may contain
"file://"
URIs with only 2 slashes (not 3, and no hostname). So, there is a logical need to convert DocumentURIs as they arrive in requests into some kind of processed internal form, such as span.URI, and this operation is fallible, so its errors must be handled.This suggests that we should retain the distinction between URI types and audit the codebase (possibly with the help of a static analysis in the style of the safetoken tests) for conversions of DocumentURI to span.URI without validation. Should span.URI, renamed to something suitable like protocol.FileURI, be made stricter about the invariant that it represents a valid "file:" URL? It's a large change, and the status quo doesn't currently cause any obvious problems, but currently there is little rhyme or reason to when we validate DocumentURIs.
@findleyr
The text was updated successfully, but these errors were encountered: