dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.19k stars 1.57k forks source link

VM Service api to convert between Dart URIs and local file system URIs #48435

Open jacob314 opened 2 years ago

jacob314 commented 2 years ago

Background:

An implementation of the DAP protocol for Dart needs to be able to get and set breakpoints and resolve stack traces specified in terms of true file system paths not custom Dart URI schemes such as google3:///, package: or org-dartlang-sdk:///. The lookupPackageUris API in the VMService solves part but not all of this problem as google3:/// and org-dartlang-sdk:/// can be returned.

The lookupPackageUris provides file system uris on some platforms but returns google:/// uris for bazel builds. This causes friction for IDEs as they need to guess the local file system path for google3:// URIs and need to know how to convert local file system paths to google3:/// uris.

There is a similar challenge for uris under the dart:sdk where the mapping is

Challenging cases for IDEs: dart:io -> org-dartlang-sdk:///sdk/lib/io/io.dart package:/foo/bar.dart -> google3:///dart/foo/lib/bar.dart easy cases for IDEs: package://foo/bar.dart -> /my/dir/packages/foo/bar.dart

Proposal

Have the embedder register service extensions that define how to resolve these uri schemes. This is similar to how the embedder registers apis to help with hot reload.

Define an api resolveLocalPaths that takes a list of uris that are google3:/// or org-dartlang-sdk:/// return true file system paths.

Define an api resolveDartUris that takes a list of uris and converts them to google3:/// or org-dartlang-sdk:/// if they are under the sdk directory or under the google3 project root.

These APIs should be implemented by flutter tool or the dart tool and not by the core VM Service as the VM lacks the knowledge to know how to resolve the custom google3:/// scheme and may not know what location for the SDK to use.

For google3:/// this api should be fairly easy to implement. As long as you know the root directory for the dart sdk, you know what to substitute for org-dartlang-sdk and as long as you know the google3 root directory you know what to substitute for google3:///. The one challenge is that if the file is not found under google3:/// we need to look in the generated files directory and know what the correct generated file directory is given the bazel config. Details of how to specify this robustly and keep it aligned with the analyzer are TBD.

What will be required that in google3, we will need to make sure flutter tools knows the root directory of the project and knows the directory of the dart sdk used. Potentially some of this information overlaps with the CFE that already knows how to convert from file system uris to Dart uris.

Open question: why isn't the Dart Analyzer the right tool to provide this resolution? Answer: a DAP implementation for a language must not be coupled to the LSP implementation. We see issues in IntelliJ right now where breakpoints fail to be work due to the analyzer being in a broken state and we would like to avoid that and decouple questions about why debugging is working from questions about whether static analysis tooling is working.

jacob314 commented 2 years ago

FYI @helin24 @DanTup. I expect most of the implementation work will be in flutter tools not the VM.

Hard edge cases: generated files might be in multiple directories. We'd like setting breakpoints in any one to have the same effect if possible. Need to be fault tollerant in cases where the LSP has some different notion of where the SDK is. This parts makes me question whether we're right to have this be separate from the LSP.

DanTup commented 2 years ago

I expect most of the implementation work will be in flutter tools not the VM.

Does this also cover dart, so it won't be specific to Flutter projects?

Hard edge cases: generated files might be in multiple directories. We'd like setting breakpoints in any one to have the same effect if possible.

I'm not sure I understand. The breakpoints come from the client, which has the files open. Under what circumstances would a DAP client be trying to set breakpoints in a file using a directory that is different to the one than where that file actually resides?

This parts makes me question whether we're right to have this be separate from the LSP.

I don't think we can reasonably link this to the language server (at least not one the editor is using). The DAP server runs in a completely different process and has no access to the language server (without custom messages and a "proxy" inside the VS Code extension, but that would need doing for all of the DAP clients making it more difficult to use the Dart/Flutter DAP servers).

jacob314 commented 2 years ago

I expect most of the implementation work will be in flutter tools not the VM.

Does this also cover dart, so it won't be specific to Flutter projects?

Yes. Work would be done for both the flutter tool and dart tool.

Hard edge cases: generated files might be in multiple directories. We'd like setting breakpoints in any one to have the same effect if possible.

I'm not sure I understand. The breakpoints come from the client, which has the files open. Under what circumstances would a DAP client be trying to set breakpoints in a file using a directory that is different to the one than where that file actually resides?

Let me make it concrete: The exact same generated file will show up under multiple directories and I don't know which the analyzer will point users to and there is nothing stopping users from opening generated files from the wrong directory manually in their editor. Given this code is not coupled to the analyzer, our default for whether genfiles or blaze-bin is preferred may be different. So what we need is for breakpoints and stack traces to function well enough that the IDE experience is not extremely confusing. Examples of things that would be confusing: Set breakpoint in /my/workspace/google3/blaze-bin/foo/bar/baz.dart but get navigated to /my/workspace/google3/blaze-genfiles/foo/bar/baz.dart when stopped at the breakpoint. An ok mitigation would be if at least the breakpoints show up in the gutter of /my/workspace/google3/blaze-genfiles/foo/bar/baz.dart

For example: /my/workspace/google3/blaze-bin/foo/bar/baz.dart /my/workspace/google3/blaze-genfiles/foo/bar/baz.dart /my/workspace/google3/blaze-bin-my-flags/foo/bar/baz.dart are all the same file just surfaced in different directories.

This parts makes me question whether we're right to have this be separate from the LSP.

I don't think we can reasonably link this to the language server (at least not one the editor is using). The DAP server runs in a completely different process and has no access to the language server (without custom messages and a "proxy" inside the VS Code extension, but that would need doing for all of the DAP clients making it more difficult to use the Dart/Flutter DAP servers).

Agreed. This definitely couldn't be the same language server as the editor is using. Long term it would be nice to bundle a stripped down language server with DDS with just functionality needed for Dart DevTools and IDE debugging but I don't expect we will do that any time soon. Fyi @bkonyi @kenzieschmoll, having some limited analysis server functionality surfaced from DDS would be great for DevTools features like using the analyzer to trigger code changes as part of the Layout Explorer, for syntax highlighting, and for autocomplete in the debugger that leverages the analyzer.

DanTup commented 2 years ago

The exact same generated file will show up under multiple directories

Ah, gotcha!

Examples of things that would be confusing: Set breakpoint in /my/workspace/google3/blaze-bin/foo/bar/baz.dart but get navigated to /my/workspace/google3/blaze-genfiles/foo/bar/baz.dart when stopped at the breakpoint. An ok mitigation would be if at least the breakpoints show up in the gutter of /my/workspace/google3/blaze-genfiles/foo/bar/baz.dart

DAP doesn't give us any way to show extra breakpoints like that. We can "resolve" them to a different location than was requested (not sure they're supposed to change file though), but we can't create additional ones. Some thoughts:

DanTup commented 2 years ago

Slightly related to this is #49863. I had started mapping org-dartlang-sdk URIs in the DAP for now (to make progress on fixing SDK breakpoints). However, it turns out that they are not the same between Dart and Flutter:

Dart: "org-dartlang-sdk:///sdk/lib/core/uri.dart" Flutter: "org-dartlang-sdk:///third_party/dart/sdk/lib/core/uri.dart"

It sounds like it's probably not a good idea for the IDEs/DAP to make assumptions about these paths in case they change, so I guess the real solution is probably this issue - an API that handles this all for us, and the editor/DAP doesn't need to do any special mapping at all.