-
Notifications
You must be signed in to change notification settings - Fork 131
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
Standardise the ability for client/DA to use URIs in place of file paths (enabling debugging of non-file:/ sources) #444
Comments
In VS Code, we do try to parse the string field as a URI. That happens here https://github.com/microsoft/vscode/blob/aae2b0d4c589c367437411b5e6eb8ddf0386476f/src/vs/workbench/contrib/debug/common/debugSource.ts#L133 |
What should a client do with a random URI? We have this problem in LSP where server spams out 'jdt://' and client has no clue what to do with that. The source request is an elegant solution. |
Ah, that's interesting - I'll do some testing of that. It should really be part of the spec before debug adapters rely on it though. I can add a flag to only send URIs for VS Code in the meantime, but I think I'd only want to do it if there's a path to it being standard.
VS Code has an API ( Eg., if I call
It has the issues I mentioned above - the client can't relate these sources to sources that the user discovered when there was no debug session (via language server), or across debug sessions (because they have no stable identifiers, just random |
@roblourens I just tested it, and it works as you described - this is great (but I think still needs to become standard). Thank you for highlighting it! :-) |
This is all very well, but would require a client VS code extension, and a custom extension for any other editor per debug adapter that uses that functionality. That goes against the main selling point of the protocol:
There would need to be a mechanism in the protocol to retrieve contents for custom scheme's, but the source already kinda is that. |
All it requires is that the client knows how to get resources from a scheme. VS Code already has this support, and it seems likely LSP will get it.
I'm not sure I agree. The protocol supports many opt-in capabilities that add functionality to clients that support it. The protocol is not intended to restrict all debuggers and clients to only functionality that all other debuggers and clients can support.
Source does it in a way that is debug-session specific and cannot interact with sources provided by any other source in the editor. This has many drawbacks that I have mentioned above. Why should debugging only work properly if your source files are on the local physical disk of a machine? Why shouldn't debugging be able to work with custom remote filesystems or when running in sandboxes or browsers with only virtual resources? All of VS Code and LSP has been built using |
So far no client capabilities require any language-awareness and a client can implement them reasonably in a generic way that's usable with any debug adapter. That's not possible for custom scheme's without some standard retrieval mechanism.
Because as @puremourning pointed out, that language servers use URIs with custom scheme's without providing a standardized way to look up the document is already causing friction in other clients. See also microsoft/language-server-protocol#336 |
I'm not sure that support URIs with custom schemes introduces any editor-specific restrictions. Considering:
If we support this, I would be happy to adopt it in the JavaScript debugger. For example, a euser can debug JavaScript on remote URIs (e.g. https://example.com) and sourcemaps can generate URIs of virtual files ( |
There's no language-awareness required here. Just that a client is able to tell a DA what URI schemes (besides
Sure it is. An editor would not say that it knows how to get
This doesn't make any sense to me. This is an implementation issue. Servers should not return URIs that it they don't know a client can handle. The fix is to have a mechanism for servers to know what a client supports (and in the meantime, for servers not to return non-file URIs), not to prevent the ability to have these custom schemes. LSP servers doing bad things is not a reason not to support new features that provide better debugging experiences. The LSP request above is exactly what I want to implement (an LSP equiv of VS Code's API). But there's no reason that supporting arbitrary URIs here in DAP requires any specific implementation (in LSP or otherwise). That's an implementation detail the debugger should not need to know about. If a debug adapter and the corresponding LSP server (or VS Code extension, or some other functionality of an editor) both agree they know how to use a custom URI scheme (which has been made up by a language), why shouldn't they be able to use it? This already all exists and works in VS Code today, I really don't understand what drawbacks there are to making it standard. "Debug adapters might return garbage URIs the client cannot handle" does not seem like an argument against this functionality - they can also return invalid file paths and invalid DAP responses. The spec can't protect you from misbehaving implementations. |
This is already the case for the most part. They're still problematic because users wonder why something doesn't work in
That's not the concern or the argument I made. My concern is that there is no way for a client to implement this generically. It would need scheme specific implementations for each debug adapter that starts using this functionality to reach feature parity with vscode. That's getting back to dedicated implementations per editor per debug adapter - the problem DAP and LSP claimed to solve. This has already been a major pain point with LSP and its undefined client commands and it was nice that DAP didn't suffer from this problem. It would be really disappointing to see this change |
In my case, the launch program is not on the custom scheme, it's for additional sources that don't exist on disk. I think it would be better as part of capabilities (since the editor already knows which schemes it can handle). I actually just found this in /**
* Determines in what format paths are specified. The default is `path`, which
* is the native format.
* Values: 'path', 'uri', etc.
*/
pathFormat?: 'path' | 'uri' | string; This is odd, because it's a single value (not an array) so can't indicate support for both paths and URIs, and it allows for an arbitrary string with no explanation of what it is. My suggestion would be that specific URI schemes are provided instead (and perhaps /**
* Determines the schemes the client supports in URIs used for paths.
*
* If defined, it means the client accepts URI strings in any `path` fields such as `Source.path`.
*/
pathUriSchemes?: string[]; // ["file", "danny"]
As an author of LSP and DAP servers used in other editors, I completely sympathise with this - but nothing in my request here requires that other editors get broken. I want this in the spec specifically to make things standard and not VS Code-only. I care very much about the LSP and DAP servers I work on being usable (and as functional as possible) with other editors.
I don't agree with this. If/when LSP gets support for |
I'm working on some debugger functionality that needs to know whether the client supports URIs in path fields (as VS Code does). There's an existing As a simple way forward, could we add something like @dbaeumer @roblourens any thoughts? |
If this functionality is going to exist, I guess it makes sense for a DA to know whether or not it's available- but the current state of things is a little suspicous and I wonder whether vscode is doing the wrong thing by saying |
The argument that 'if LSP gets a feature that doesn't exist yet' is hard to justify today in isolation. But besides that, the idea that 'DAP clients can just interact with whatever LSP client happens to exist on the client' seems to bake in the assumption that DAP and LSP have any relationship whatsoever which today they don't. Indeed DAP clients and LSP clients (of which there are many) for many editors are completely independent. I would strongly discourage any DAP notion that requires a LSP implementation. |
Honestly I don't know how FWIW as a workaround, I'm sending a new flag (
Using URIs instead of file paths makes no assumptions about, nor has any dependencies on LSP. The way in which I'd like to use this would involve implementations in both of those, but that's an implementation detail. A client can happily only support Not adding this to the spec isn't likely to stop this from happening, it just means that clients and servers will implement it in non-standard ways (as VS Code already has, and Dart is likely to take advantage of). This will make things worse for other clients, not better. I care very much about supporting clients other than VS Code and that's exactly why I'm here asking for a standard way to do something that is currently very VS Code-specific. I would like my DA to work with clients that do not handle URIs, but I also would like clients that are using the Dart LSP server and Dart DA to be able to support these virtual files without needing Dart-specific coding, and that's only possible if this becomes standard. |
@DanTup apologies perhaps I misunderstood what was being proposed. So long as everything is opt in and people who don't opt in aren't second class, I have no objection to that! |
@puremourning np, sorry if I wasn't very clear. My intention is absolutely that everything here should be opt-in and both the client and DA should completely understand the capabilities of the other. Today, VS Code supports URIs but does not tell the DA a) that it supports them or b) which schemes it supports. This means DAs are either making assumptions they probably should not, or VS Code extensions are having to pass non-standard flags in to tell the DA that they support this. IMO the existing My proposal would be something like: Client -> Server capabilitiesinterface InitializeRequestArguments {
/**
* Client supports receiving URIs with the following schemes in place of file paths for (such as in `stackTrace` responses).
* 'file' should be listed explicitly if the client supports file URIs instead of paths.
* If not provided, the DA must not send URIs to the client.
*/
supportsUriSchemes?: string[];
} Server -> Client capabilities (
|
uri
on Source
as an alternative to path
I'm still keen to make the above standard.. VS Code is already supporting it and Dart is going to use it. If nobody has any objections, I'll open a PR with roughly what's in my comment above and we can discuss further there. |
Edit 2024-02-15: It turns out VS Code is already handling (and sending) URIs but without any standard way to negotiate this with the client. Rather than adding
uri
fields, it would probably make more sense to standardise this with capabilities. My current suggestion is detailed below at #444 (comment)In VS Code and LSP,
Uri
s are used everywhere instead of paths. This allows workspaces that contain projects that are not backed by the file system (for example virtual workspaces or virtual documents).While doing some testing of
registerTextDocumentContentProvider
in VS Code to provide virtual documents, I discovered that in the debugger we cannot return sources with URIs that would be handled via theTextDocumentContentProvider
because theSource
interface has only filepath
s and noturi
s.While it's possible to provide a
sourceReference
and download the source from the debugger, this results in a poor user experience because each debug session gets its own versions of these files, and they do not match the one the user may have used Go-to-Definition on (when there was no debug session). This is a very confusing experience (especially as each "copy" of the document has its own breakpoints) and something we already had to solve for file paths (that is, we try to never download sources from the debugger and always map them to disk-based sources that match where the language server would navigate to).My suggestion would be to add a
uri
field toSource
, and have a capability for the client to tell the server that it will read auri
field (if provided) instead ofpath
(and has the capability to read these documents) - either as a boolean, or a list ofscheme
s that it can load content for.The text was updated successfully, but these errors were encountered: