Debugger returns empty string as URI when paused inside a macro-generated script #54824
Comments
|
Unsure if related, but the {
"jsonrpc": "2.0",
"result": {
"type": "Script",
"class": { /* ... */ },
"size": 80,
"fixedId": true,
"id": "libraries\/@19252696\/scripts\/\/18d792687ca",
"uri": "",
"_kind": "kernel",
"_loadTime": 1707134584778,
"library": {
"type": "@Library",
"fixedId": true,
"id": "libraries\/@19252696",
"name": "",
"uri": "file:\/\/\/C:\/Dev\/Google\/dart-language\/working\/macros\/example\/bin\/observable_main.dart"
},
"lineOffset": 0,
"columnOffset": 0
},
"id": "38"
}(Also unsure if there should be two slashes in the ID |
lrhn
added
area-vm
This non-null assertion is an error. If any URIs are returned from the VM that are not `file://` URIs and also return `false` from `isResolvableUri` then this would fail, even though the return values allow `null`s and should be used. I can't currently write a test for this because in theory there aren't any cases where would actually happen, but I triggered it due to another bug (#54824) and think it's better removed. Also added some TODOs about some behaviour that may need to change as macro support progresses (it's not far enough for me to make these changes yet). Change-Id: I4108963b6b88bbbff4bb47f69909391900b5d3ba Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/350300 Reviewed-by: Ben Konyi <[email protected]> Reviewed-by: Helin Shiah <[email protected]> Commit-Queue: Ben Konyi <[email protected]>
bkonyi
added
area-front-end
|
If the
The script ID takes the form of |
|
The CFE doesn't currently generate the merged augmentation uri with the correct |
|
I'm seeing URIs with that change, thanks! "topFrame": {
"type": "Frame",
"kind": "Regular",
"location": {
"type": "SourceLocation",
"script": {
"type": "@Script",
"fixedId": true,
"id": "libraries/@19252696/scripts/dart-macro%2Bfile%3A%2F%2F%2FC%3A%2FDev%2FGoogle%2Fdart-language%2Fworking%2Fmacros%2Fexample%2Fbin%2Fobservable_main.dart/18d843c57a3",
"uri": "dart-macro+file:///C:/Dev/Google/dart-language/working/macros/example/bin/observable_main.dart"
},
"tokenPos": 117,
"line": 5,
"column": 21
}, |
|
Although, Here's what I currently see: @bkonyi is my expectation correct, and if so, is it related to this issue or should I file another? (I'm unfamiliar with where the data comes from that's used for these mappings). |
|
The data for the mappings comes from the kernel's If we expect that |
I don't know if this has been specifically agreed, but I believe that we need this for the behaviour we're targetting. The IDEs are unable to handle the mapping of The Without this, files outside of Should I file a separate issue about this (perhaps it needs some further discussion?). |
|
It does sound to me like This goes both ways, resolving a cc @lrhn |
We definitely should be able to support this with the
I was under the impression that both the analyzer generated code should be identical to the code generated by the CFE. Is that not the case?
Maybe? I do think it's related to this issue and probably only requires a small change somewhere in this CFE CL (probably at this line) to support. I'll leave a comment. |
Oh, I see. I assumed that they'd both be needed for the same reason as the originals, but my knowledge of this area is poor :-) I'm proceeding for now on the assumption that we will have both, and will wait for @johnniwinther's response. If he agrees and thinks it should be included in his fix, no need for another issue. If it's not clear that's the right direction, I'll file an issue for further discussion.
Yep, that's my expectation (although I just discovered it's not currently - I filed #54848 about that). However if we don't make these URIs match, VS Code doesn't know that they're the same file - it just knows it has an analyzer-backed file with one URI, and a VM-downloaded file with another URI, and will show them to the user as if they are distinct files. We need both DAP and LSP to be using the same URIs for this to work correctly (and for LSP we must use unique file URIs, because |
|
I'd definitely prefer if we didn't need to resolve The URI is an identifier (I in URI) of generated code, it's not a locator for it (L in URL). It doesn't say where to find the code. If we need to find "the macro augmentation for file:///.../bar.dart", I'd rather go the other way and convert You can still have macro-augmentations for files outside of I think everyone would be happiest if the code of an augmentation doesn't need to exist on disk at all. Only write it out if some tool is unable to work if it doesn't have physical files. And those files should not be inside the |
|
To be clear, the reason we need this is not because we need to find files on disk, or even that we need to tie this script back to the original source file. We need it because it happens to be the unique URI that the editor is using to identify this content and we need the debugger and the editor to use the same reference. So, it would be fine if we used However it's not fine for us to only have That said, I don't know how (or if) this interacts with package_config/etc.. these URIs from my perspective are only required for mapping in the debugger (eg. via I guess in theory, I could drop the prefix from the scheme, use the existing mappings, and then put the prefix back.. however this feels like an assumption we might not want to bake into lots of places. |
Actually, it occurs to me that they may also be printed in stack traces in stderr.. Ignoring the ambiguity of the IDE wants to linkify this output (if there are multiple versions of a package in the workspace), the |
|
I've implemented the minimum support for I can allow I don't want to start claiming that I would actually prefer if we don't change |
The main thing is that we want all the tools to do this consistently, and if we handle it in this package we get a consistent implementation that handles any URI any tool might encounter. Otherwise should we publish another package that just removes the prefix before calling this package, and then adds it back? That doesn't seem right either. I am still a bit confused about the exact issue that is happening here though, it does feel wrong to me for tools to be resolving the dart-macro+package uris in this way. It seems like more just a quirk of the fact that IDEs really want to deal in file URIs. So maybe it really is only the editor plugins that have to deal with this, and the logic should live there? |
|
I agree with most of the above.. I don't think this really affects package_config. I also think that ideally no tools should should be removing and re-adding these scheme prefixes. I think that you could easily get from
To be clear, the issue here is not about getting to file URIs (or back to the original script that caused this one to be generated), we just need to get to a unique URI for the script that is the same both inside the analyzer and the debug adapter. It doesn't matter whether this looks like a file URI or not. When you click "Go to Definition" in your IDE (when there is no running app) you will get to a copy of the generated macro code. When you add a breakpoint in this file, we need to be able to send that breakpoint to the runtime when you start debugging, and that means we need a unique URI to refer to it by. Today, we are working on the basis that this URI will be |
With one, sadly rather significant, exception. The semantics of // library URI: package:foo/bar.dart
augment 'dart-macro+package:foo/bar.dart'; // Inserted by macro generation
// ...which is the semantics that we want the post-macro-generation program to have, depends on
This is not a package URI, and it's not inside any The hack, which I have implemented, is to allow (I'd rather allow |
I don't personally have any opinion on these URIs, but for the URIs we'll use to communicate with the editor, we couldn't do the same thing (eg. we can't use There's certainly no reason we couldn't map between Whatever the specifics of the URIs, we do need some way to map between them in the debug adapter. Using |
|
I do like the idea of using a query suffix instead of a scheme prefix. Our I think the WhatWG URL spec does accept a query component on a file URL, though, which was my first worry. We do want to move Basically All in all, I think we can choose to say that It would have the added advantage that an import inside a generated file will be resolved relative to the original file, because a base URI of We could also use a fragment, |
|
For the URI that is used to communicate between LSP and the editor, and the editor and the debug adapter, we definitely cannot use So whatever is being used inside the runtime, we must ultimately convert this to a custom-scheme URI in the debug adapter to go to the client (and the LSP server must also use those same custom-scheme URIs to provide content and functionality like navigation/diagnostics).
It occurs to me we probably have another exception here. In the debug adapter, we have settings to allow the user to debug "My code", "My code + external packages" or "My code + external packages + sdk". In order to determine what is "My code", we look at whether the resolved file URI is inside a set of workspace paths. To handle the same for macros, we'll need to strip the |
|
So, generally, we need the
Any time it matters where a Dart file is, a If we actually want to read the file, then that's not where it is (there is a completely different file there). The question is then who should be stripping the The obvious choices are the tools that macro-compilation is integrated into: Compilers and the analyzer. Maybe the LSP. The compilers and analyzer are the ones that need to ask about the package of a To see whether the file is inside a specific workspace, the debug adapter may need to know to strip the prefix, and ask about another file, and again assume the answer to be the same. That's more annoying, but unless the analysis server starts providing a service to check whether a URI is included below another URI, so it can do the prefix-twiddling, the code that actually knows what the the workspace location is, will have to know about macro-URIs too, if it wants them to be included. |
I don't think the LSP server needs to know this directly (it wraps the analyzer which should take care of most of this, minus some custom mapping because the analyzer currently uses paths and not URIs in many APIs). The debug adapter does need to know, only for the "is in workspace" check. One thing that might be helpful in these discussions is defining some terms for these different kinds of URIs. Originally in the debug adapter, we just had two things:
We ended up with some helpers to convert between Paths and URIs and it seemed fairly simple. Then we started mapping SDK sources and started dealing with org-dartlang-sdk URIs. Today, file paths are really going away entirely and instead we need to use URIs everywhere (because the client is no longer restricted to physical files). So now we have at least three "kinds" of URIs in the debug adapter. I don't know if these already have names, but I think it would be useful to define them (so we can name APIs the same across tools and be on the same page in discussions). For example:
(1) is what we see in the VM and will show up in things like call stacks in the debugger view. In the debug adapter, we will convert them to (2) using In the opposite direction, the editor will send us (3) (for example when the user adds a breakpoint), we'll convert it to (2) with the built-in mappings in the debug adapter, and then use The check the debug adapter does for whether a URI is "my code" will take (3) and just do the (unfortunate, but simple and probably necessary) stripping of the URI prefix and then compare the result against the workspace roots it knows about. |
|
I don't thing we need to "resolve" a It's a non- In a way, There is no benefit in converting I think the main reason we have the (If a macro generated file contains a macro application, will it get a |
Yep, when I said "in lib"/"not in lib" above, I just meant things that would/wouldn't resolve to @bkonyi what are your thoughts in putting logic like described above into DDS? (although that makes me wonder if today we already rely on / guarantee DDS is available?) |
|
So the problem is that:
I still think a better uniquification would be to append the location of the That's globally unique and it's defined entirely in terms of the original library's identifying URI, plus, if it's a package URI, an identification of the package configuration that |
Yes, this is an accurate summary :)
I assume in this world, I was not a fan of this idea originally, but now knowing what I do, I feel like this might make more sense. But, I suspect this could also be a big analyzer change. Currently it assumes only a single generated file, and it uses synthetic paths ( (fyi @scheglov @bwilkerson.. this is a long thread, but it's summarised well in @lrhn's comment above this one.. I'd be interested to hear your thoughts) (Maybe we should move this into #53950 or #54323 because we're probably a bit off-topic for the current title of this issue, with the original (described) issue somewhat resolved) |
|
The analyzer does canonicalize solo_test_X() async {
var a = newFile('$testPackageLibPath/a.dart', '');
await assertNoErrorsInCode('''
// ignore:unused_import
import 'file://${a.posixPath}';
''');
var uri = result.libraryElement.importedLibraries.first.source.uri;
expect('$uri', 'package:test/a.dart');
} |
|
My personal preference would be to resolve this by having the common front end canonicalize |
Just to play devils advocate - while this situation is increasingly less likely - such canonicalization does rely on there being a package config file which knows about the package these files belong to. Technically, you could still have such a script loaded as a file URI in a context where it has either no package config or a package config with no entry for its containing package. And then also loaded separately with a package config which does know about its package. In which case, you still end up with two separate libraries pointing at the same underlying "file". This is only valid for the IDE context, where multiple workspaces might exist at once. But that is the primary context we are trying to support here also. |
Can this happen within a single VM though? I don't think this is a problem in the editor itself because the editor is always using the file-like URIs and doesn't care at all about the package versions. The real blocker here is getting to the point where we can give one of these file-like URIs to a VM and have it map it concretely to the loaded script and back (using For non-macros, we can give Based on discussions above, I understand we cannot do this today with If all scripts were canonicalized as Paul suggests, I think this might allow populating The other (sound) alternative I think is to come up with a new format for the unique URIs to go into There are some slightly less-sound options (such as mapping this in DDS using the rules I noted above) but I really worry that this will be difficult to ensure is always accurate because we're using the same URIs on both sides of the debug adapter to mean different things and we'd need to ensure all code operating on a URI correctly handles it for the given context. |
No, not within a single VM afaik (always one package config). But I don't think that matters. The issue of different file contents for the augmentations based on the import URI of the library is still present, and so using the file URIs as the URI in the editor will always have the possibility of this conflict. Very likely, it just doesn't matter. But canonicalizing things when possible doesn't fully solve the issue. A concrete example of this would be if you created a subdir of a package, lets say |
|
That's true. That particular setup feels like an edge case though and would only impacts users doing that (and there's a way to avoid it). The current issue is that we're unable to support even the projects that are doing everything "correctly", because we can't map between these URIs (due to There are some trade-offs here and I don't know who is best placed to decide on a solution (my preferences would be Paul's canonicalization *, followed by Lasse's package-config in URIs *, followed by some mapping function in DAP/DDS). I'd prefer not try to map these in DAP unless it's clear the other two are no-gos because I think it's an overall worse solution. * I am assuming that with either of these, that (I'm available to chat during UK-ish hours if I can help move this decision along, though I think you have a good understanding of the issue) |
|
I am completely OK with just accepting that a niche edge case like this has some broken behavior, FWIW. As long as the core assumptions aren't baked into the language spec, and ideally not the compiler/SDK. It is much easier later on to make changes at the tooling layer than the language/SDK layers. |
|
Does "As long as the core assumptions aren't baked into the language spec, and ideally not the compiler/SDK" mean that Paul's suggestion won't work? (eg., even with that canonicalization, the If so, then I think the next best option may be Lasse's suggestion - first agreeing on a URI format (again 😄) that could go into |
Canonicalizing the URIs (to the package: equivalents) is I think a good, and safe thing to do. We just can't guarantee canonicalization always happens. It is dependent on the package config being set up properly.
This would also still be safe to do, within a single compile. I am less convinced it is the right thing to do, for the compiler. It is outside the spec (but this entire concept of "import URI" versus "file URI" is outside the spec afaik). It might be the easiest pragmatic choice though. But it might be hard to roll back if we regret it later.
I would really love to avoid this if we could, but if we need to then yes including the package config path or something in the URI (via query param etc) would resolve this issue too. I am not sure how this would actually end up looking like in the IDE though? |
Decision reachedIn the language meeting today we reached a decision on this 🚀 , assuming the CFE work is feasible:
cc @johnniwinther @scheglov - I believe this is a no-op for the analyzer because it already does this canonicalization. |
🎉 Thank you for letting me hijack your meeting today, it was great to get to a decision 😄 Should we file specific issues for this work and close this one, because it's very long/noisy and probably not the best way to track what's left. (I'm also keen to start testing once the change above is available and can build the SDK locally, so I'm interested in being CC'd on the CL if it might give me a head-start if landing the change might take a little longer). |
|
We have several options here on how to specify this change. I'd probably go for:
This means that if a Our imports already do a bunch of normalization (fx case-, escape-, and path-normalization), but this is not just one more. The other normalizations depend only on the URI syntax, this one depends on external metadata, a "package configuration", which we will keep completely abstract in the spec. A package configuration is just, in this context, a partial mapping from URIs to |
|
@johnniwinther thank you for https://dart-review.googlesource.com/c/sdk/+/355140 - I just tested and it seems to be working exactly as I hoped, this is great :-) I'll leave it to others to decide whether to close this issue and open a more specific one about the canonicalization (which may need announcing as a breaking change). |
|
@johnniwinther actually, my testing was bad - I was testing in the bin/main.dartimport 'package:macro_proposal/foo.dart' as foo;
void main() {
foo.main();
}lib/foo.dartimport 'package:macro_proposal/with_hello.dart';
void main() {
A().hello();
}
@WithHello()
class A {}lib/with_hello.dart// There is no public API exposed yet, the in-progress API lives here.
import 'package:_fe_analyzer_shared/src/macros/api.dart';
macro class WithHello implements ClassDeclarationsMacro {
const WithHello();
@override
Future<void> buildDeclarationsForClass(
ClassDeclaration clazz,
MemberDeclarationBuilder builder,
) async {
builder.declareInType(DeclarationCode.fromString('''
void hello() {
throw 'a';
}
'''));
}
}If I run this and pause on the exception, I see an empty string URI for the top frame's script. If I run it without the debugger, the printed stack trace from the exception also shows no URI: |
|
@bkonyi Do you know if some plumbing is needed for the new file uri? |
|
@johnniwinther I noticed in your CL Most of the other instances of Edit: Not certain it fixes all the cases though, my tests still fail with empty strings, so maybe that's not the right fix (or not the complete fix). |
|
Ah, gotcha. I'll wait for Ben's input then, thanks! |
|
I'll have to dig around in the VM to understand what's happening here. I wouldn't expect to need any extra plumbing for this, though, as long as we're still populating these URIs in |
|
@bkonyi thanks! Let me know if anything isn't clear or you can't repro it from the above. This comment and using the cli is probably the easiest way to repro the issue of empty strings, but DAP's use is a bit more complex. If you have a proposed fix I'm happy to build it locally and test with my in-progress DAP changes. |
|
Some other stuff came up, so @derekxu16 is going to look into this for me. |
It looks like this is true, when sdk/runtime/vm/kernel_loader.cc Lines 1931 to 1932 in 52f996b Stepping into the sdk/runtime/vm/compiler/frontend/kernel_translation_helper.cc Lines 3083 to 3096 in 52f996b |
|
cc @johnniwinther ? |
|
@johnniwinther I built that locally and my new tests are now passing, so it looks good to me - thank you! 🙂 |
|
Considering this fixed by 0a68ca9, I'll open more specific issues if I hit any other issues. Thanks :) |
If I step into a macro-generated script with the debugger, the
urior the script is an empty string:We use these
uris in the debug adapter to map back to source code to display to the user. It's not clear to me if this is a) a bug, b) just a known issue/incomplete or c) expected and the debug adapter will need to handle it.@jakemac53 @bkonyi
Edit: I think actually we definitely needs these URIs, because it's the only way we can map back to the source code the user already has in the editor from the analyzer (otherwise we'd have to download the source from the VM which we generally don't want for VS Code because it looks like a duplicated file to the user).
The text was updated successfully, but these errors were encountered: