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

VM Service's lookupResolvedPackageUris returns different org-dartlang-sdk structure for Flutter apps from Dart #49863

Open
DanTup opened this issue Aug 30, 2022 · 14 comments
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@DanTup
Copy link
Collaborator

DanTup commented Aug 30, 2022

I'm not sure whether this is expected - if so, I think it needs documenting (it may already be, but I've not been able to find anything).

The DAP implementations use the VM Service's lookupResolvedPackageUris call to convert URIs like dart:code/uri.dart (which are provided in stack traces) to things that can be mapped to local file paths on the users machine (so that when debug stepping, the same files can be opened that the user gets if they use Go-to-Definition via the analyzer).

If I write code like this, and step into Uri.parse:

var a = Uri.parse('');

Then in the Dart SDK, lookupResolvedPackageUris converts this URI to org-dartlang-sdk:///sdk/lib/core/uri.dart

==> {
	"jsonrpc": "2.0",
	"id": "41",
	"method": "lookupResolvedPackageUris",
	"params": {
		"isolateId": "isolates/1481761977225271",
		"uris": [
			"dart:core/uri.dart"
		],
		"local": true
	}
}

<== {
	"jsonrpc": "2.0",
	"result": {
		"type": "UriList",
		"uris": [
			"org-dartlang-sdk:///sdk/lib/core/uri.dart"
		]
	},
	"id": "41"
}

Currently, the debug adapters assumed that org-dartlang-sdk:///sdk points to the root of the Dart SDK and correctly map this to the lib/code/uri.dart file in the Dart SDK (/sdk is one example given in the docs).

However, when running a Flutter app (using latest master) the result is org-dartlang-sdk:///third_party/dart/sdk/lib/core/uri.dart:

==> {
	"jsonrpc": "2.0",
	"id": "639",
	"method": "lookupResolvedPackageUris",
	"params": {
		"isolateId": "isolates/3750929181587719",
		"uris": [
			"dart:core/uri.dart"
		]
	}
}

<== {
	"jsonrpc": "2.0",
	"result": {
		"type": "UriList",
		"uris": [
			"org-dartlang-sdk:///third_party/dart/sdk/lib/core/uri.dart"
		]
	},
	"id": "639"
}

I could update the code to also assume org-dartlang-sdk:///third_party/dart/sdk should map to the root of the Dart SDK, however it makes me wonder whether there may be other differences that need to be accounted for. I don't know how the mapping for org-dartlang-sdk is built, but it would make sense to ensure any assumptions in the DAP match the actual rules (and include a note in the code where those rules can be found).

@bkonyi do you know if this is intended, and/or where the rules for org-dartlang-sdk are?

@bkonyi
Copy link
Contributor

bkonyi commented Aug 30, 2022

Hm, it looks like the org-dartlang-sdk:/// URI is rooted at the root of the Flutter engine project, so it probably is determined at compilation time based on the current working directory. I don't know if there's any hard rule when it comes to generating absolute URIs for the SDK libraries, so we either need to talk with the CFE team to make sure these URIs are consistent or just assume that they're self-consistent for a given embedder and work around it by assuming that the true path starts at the sdk/lib/ section of the path.

@vsmenon vsmenon added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Aug 30, 2022
@DanTup
Copy link
Collaborator Author

DanTup commented Aug 30, 2022

The problem with looking for sdk/lib and assuming that's the Dart SDK root is that it only works when going from org-dartlang-sdk to a file path, but we also sometimes go from a file path to a org-dartlang-sdk URI (and wouldn't know to insert the third_part/dart part).

We could override this in the Flutter DAP if we can assume that it would always be org-dartlang-sdk:///third_party/dart/sdk/lib for Flutter, but it might be good to confirm that this is intended (and not just accidental) first, and if so ensure it's noted in the VM Service docs (or if there are better docs about org-dartlang-sdk somewhere, linked the VM Service docs to them).

@johnniwinther do you know if the difference described above is intended, or do you know who I could ask?

@a-siva a-siva added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Aug 30, 2022
@alexmarkov
Copy link
Contributor

This could be related to the difference in build rules used to compile platform dill files:

For standalone Dart SDK:

sdk/runtime/vm/BUILD.gn

Lines 182 to 183 in 56df821

single_root_scheme = "org-dartlang-sdk"
single_root_base = rebase_path("../../")

For Flutter:

https://github.com/flutter/engine/blob/9f306a6ca740ea8f9538587c74d3e39d6e3fd684/lib/snapshot/BUILD.gn#L265-L266

In the first case root directory for org-dartlang-sdk scheme points to the Dart SDK directory, which contains sdk/lib. In the Flutter case it points to the engine src directory, which contains third_party/dart/sdk/lib. This difference is also reflected in corresponding libraries.json files.

Note that there are also separate build rules for building platform dill files for dart2js, DDC, dart2wasm as well as Fuchsia-specific Flutter runner and Dart runner. We might also have a separate copy of those build rules in g3 (as it doesn't use gn).

@alexmarkov
Copy link
Contributor

Also note that in case of Flutter some libraries come from Dart SDK and have third_party/dart/sdk/lib/... path related to org-dartlang-sdk:///, but others (such as dart:ui) come from Flutter engine and have flutter/lib/... path related to org-dartlang-sdk:///. So we might not be able to redefine location of org-dartlang-sdk for Flutter just to point to Dart SDK.

@DanTup
Copy link
Collaborator Author

DanTup commented Aug 30, 2022

@alexmarkov do you know whether this is deliberate or a side-effect of the paths being set for another reason?

If deliberate, is it reasonable/feasible to have them documented somewhere (can they be relied on to never change)?

The debug adapter needs a reliable way to map between paths that exist on the users file system and Dart's URIs to correctly handle breakpoints and stepping with the debugger. My original understanding was that org-dartlang-sdk was a consistent way to do this (because the runtime doesn't necessarily know the local SDK paths), but it's not clear to me if the differences are deliberate or accidental.

@DanTup
Copy link
Collaborator Author

DanTup commented Aug 30, 2022

but others (such as dart:ui) come from Flutter engine and have flutter/lib/...

Ah, that's good to know - I had assumed everything from Flutter was going to be package:flutterxxx. I guess we definitely need to handle flutter/lib then (in both directions). I'm still unclear whether Dart could/should be consistent though (and if not, whether it's safe to assume it's always third_party/dart for Flutter).

@DanTup
Copy link
Collaborator Author

DanTup commented Oct 3, 2022

@alexmarkov @bkonyi can anyone confirm whether this difference is deliberate or accidental? I'd really like to fix this (breakpoints in SDKs have been a long-standing issue and we seem very close to fixing it), but I don't want to add more hard-coded assumptions into DAP without some confirmation that this is expected (and ideally, a full set of these mappings so the client can handle them correctly).

@alexmarkov
Copy link
Contributor

@DanTup These different paths just reflect different directory layouts we have while building Dart SDK and Flutter engine, so they are deliberate. As far as I know internal google repo uses a yet another directory layout. I think it's not safe to assume particular directory layout relative to org-dartlang-sdk:/// - it may change as needed.

Maybe we can avoid these assumptions by cutting leading directory parts from org-dartlang-sdk:/// URIs and testing if corresponding source file exists in Dart SDK and Flutter? This way we would only assume that relative paths should eventually match.

@DanTup
Copy link
Collaborator Author

DanTup commented Oct 3, 2022

I think it's not safe to assume particular directory layout relative to org-dartlang-sdk:/// - it may change as needed.

Yeah, that was my concern. I'd like to get to something stable so we can have this issue resolved once and for all.

Maybe we can avoid these assumptions by cutting leading directory parts from org-dartlang-sdk:/// URIs and testing if corresponding source file exists in Dart SDK and Flutter?

This doesn't feel like a particularly great solution. When the debugger breaks and we (the IDE/debug adapter) get a call stack, we may need to translate a large number of URIs/paths in one go. We're already having to send them to the VM to be converted to org-dartlang-sdk:/// and then it was expected to be a simple mapping to the local SDK on disk.

I'm also not sure whether this covers all cases. I don't have any visibility of what schemes and paths can be returned here (whether there may be other paths we're not handling). It would be ideal if there was some documentation about exactly what URIs can be returned from lookupResolvedPackageUris and how a tool that's consuming it can convert any non-file:// URIs to source file paths (if they exist) on the users machine.

I guess this is slightly related to #48435 which I think is asking for an abstraction over this API that handles this (eg., it always just provides file paths/URIs for all inputs, whether they're package:, dart:, etc.).

@DanTup
Copy link
Collaborator Author

DanTup commented Oct 24, 2022

In the interest of providing a fix for this in the near-term, I'm going to look at handling the specific values above in the Flutter DAP. I'll make sure it doesn't fail if this ever changes (it will just fail to map - the same thing it already does today with those paths) and we can revisit it if it does (or if we get something better).

I chatted with @bkonyi briefly and he suggested that one option could be for the compilation tooling to accept an option to specify where the Dart SDK root is relative to the embedding projects root, which could allow this to be consistent (at which point perhaps consistency could become a "promise"). I don't know how involved that is (or whether it could impact other things) - any thoughts @alexmarkov?

@alexmarkov
Copy link
Contributor

Currently, when compiling platform, we specify real directory of org-dartlang-sdk:/// root and the location of libraries.json file using org-dartlang-sdk:///... URI. Locations of particular platform libraries are specified in libraries.json file relative to the location of libraries.json itself, so CFE derives their URIs from the URI of libraries.json file. (@johnniwinther Please correct me if I'm wrong.)

Note that different embeddings of Dart have their own libraries.json files (because they have different sets of libraries) and different definitions of org-dartlang-sdk:/// root (because they have different directory structures).

@DanTup Could you clarify how the proposed option would be taken into account and how it would help?

My understanding is that if we would like to rely on a particular value of URIs of platform libraries, then we would need to unify directory layout when building platform in all projects embedding Dart (so that sdk/lib/core/core.dart is always placed into the same directory relative to org-dartlang-sdk:/// root). This would probably involve a bit of copying things around during build or extending front-end to understand multiple different roots when compiling a platform.

@DanTup
Copy link
Collaborator Author

DanTup commented Oct 24, 2022

Sorry for the wall of text, but some context because it might not be completely clear what all of this relates to from the above :-)

Since forever, if a user VS Code clicked Go-to-Definition on a symbol that led to the SDK, the analysis server would return a file path to the local SDK sources on their disk. If they added a breakpoint in that file, VS Code would send that path to the Dart VM during debug sessions, but the breakpoint would not work (because the running VM doesn't know that path corresponds to the SDK).

(the same was true for package files, although that was fixed some time ago, and the running VM was able to them).

To fix this (or so I thought) some new VM Service APIs were added to map URIs to/from file paths. However, it turned out that this API couldn't map the SDK sources, it could only convert them from dart:foo/bar.dart into org-dartlang-sdk:///sdk/....

So, in the debug adapter (which sits between VS Code's debugger and the VM Service) we have mapping that can convert a URI in the form org-dartlang-sdk:///sdk/... to a file path in the users local SDK folder (and back again). It currently only supports things in org-dartlang-sdk:///sdk and does not handler /runtime or any others (I don't know if any others exist). This largely fixed the original bug, and you could now Go-to-Definition in VS Code to SDK sources, and the debug adapter could translate that path into something the VM understands, and when the debugger breaks, it can translate it back so that VS Code can navigate to the correct SDK source on the users disk.

It turned out this translation of org-dartlang-sdk:///sdk/ didn't work for Flutter for the reasons above. So my proposed fix is to allow the Flutter debug adapter to override the base Dart SDK path from org-dartlang-sdk:///sdk/ to org-dartlang-sdk:///third_party/dart/sdk/ which will be used for mapping Dart SDK sources in both directions.

This may not be a complete fix (we don't handle anything outside of /sdk) and it may not be future proof (if this structure changes, it will stop mapping), however it's no worse than what we have today (which is that none of them work anyway) and in the worst case will just go back to "not working" if things change.

Of course, it would be nice to implement a more complete/more static version (which I guess is what #48435 is about.. removing the need from the "client" - in this case the debug adapter - from needing to handle the org-dartlang-sdk URIs itself and pushing that into the VM/VM Service/DDS or somewhere similar).

@DanTup
Copy link
Collaborator Author

DanTup commented Oct 24, 2022

Re-reading the above, I noticed you gave another example:

but others (such as dart:ui) come from Flutter engine and have flutter/lib/...

So my fix above is probably incomplete and instead of just allowing a single Dart SDK URI to be overridden, it should probably be a collection of mappings. How much it's worth trying to cover all cases probably depends on how long this code is likely to live until there may be something better though.

@DanTup
Copy link
Collaborator Author

DanTup commented Oct 25, 2022

@alexmarkov

@DanTup Could you clarify how the proposed option would be taken into account and how it would help?

Sorry, I think I misunderstood and I explained how my short-term fix would work and not what I wrote after discussing with @bkonyi. Re-reading what you wrote, I'm less certain about how this would work, but now I'm not sure whether Ben meant for these URIs to be consistent, or that the DAP would have a way to access that relative path to the Dart SDK.

Perhaps focusing on the Dart SDK is not the right thing (because of things like dart:ui falling outside of it). Perhaps what would be better is a mapping between the relative org-dartlang-sdk:/// paths and the locations of source files in the shipping SDK that the DAP (or another internal API as described in #48435) could use to perform the mapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

5 participants