Closed
Description
From here:
When I sent breakpoints with addBreakpointWithScriptUri
like this:
[1:46:10 PM] [Observatory] [Info] ==> {"id":"608","method":"addBreakpointWithScriptUri","params":{"isolateId":"1","scriptUri":"file:///Users/dantup/Dev/Google/flutter/examples/flutter_gallery/lib/gallery/app.dart","line":64}}
[1:46:10 PM] [Observatory] [Info] ==> {"id":"609","method":"addBreakpointWithScriptUri","params":{"isolateId":"1","scriptUri":"file:///Users/dantup/Dev/Google/flutter/examples/flutter_gallery/lib/demo/animation/home.dart","line":495}}
[1:46:10 PM] [Observatory] [Info] ==> {"id":"610","method":"addBreakpointWithScriptUri","params":{"isolateId":"1","scriptUri":"file:///Users/dantup/Dev/Google/flutter/examples/flutter_gallery/lib/demo/animation/home.dart","line":496}}
I get this error:
[1:46:18 PM] [Observatory] [Error] NoSuchMethodError: The getter 'id' was called on null.
Receiver: null
Tried calling: id
#0 Object.noSuchMethod (dart:core-patch/object_patch.dart:51:5)
#1 ChromeProxyService.addBreakpointWithScriptUri (package:dwds/src/services/chrome_proxy_service.dart:182:51)
<asynchronous suspension>
#2 VmServerConnection._delegateRequest (package:vm_service/vm_service.dart:911:51)
<asynchronous suspension>
#3 _rootRunUnary (dart:async/zone.dart:1132:38)
#4 _CustomZone.runUnary (dart:async/zone.dart:1029:19)
#5 _CustomZone.runUnaryGuarded (dart:async/zone.dart:931:7)
#6 _BufferingStreamSubscription._sendData (dart:asy
It may be related to the paths (we're sending paths, not package URIs), and there's still an open discussion about the format of these.
Metadata
Metadata
Assignees
Labels
No labels
Type
Projects
Milestone
Relationships
Development
No branches or pull requests
Activity
[-]"getter 'id' was called on null" when calling "addBreakpointWithScriptUri"[/-][+]"getter 'id' was called on null" when calling addBreakpointWithScriptUri[/+]jakemac53 commentedon Sep 11, 2019
It looks like we don't support
file:
uris, @alan-knight can you look at this?jakemac53 commentedon Sep 11, 2019
Fwiw @DanTup we basically only support
package:
uris or relative paths afaik right now - this is related to previous discussions we have had around uris.I think we probably could support this though, but we will have to convert the absolute uri into a
package:
uri (or relative path).alan-knight commentedon Sep 11, 2019
Gary is confused how this ever worked for him if VS Code is always sending file: URLs and we don't have any support for those.
grouma commentedon Sep 11, 2019
In particular, using the custom version of the VS Code plugin and running with Flutter Web (not through the CLI, but with
webdev daemon
) worked. I was able to set breakpoints, step through files and jump to to the right location when paused.alan-knight commentedon Sep 12, 2019
So just to be clear on this, what do the file URLs look like for other circumstances. I think it boils down to three cases
a) as above, they're references into application files in the lib directory. These should be dealt with like package: URLs internally
b) application files but in the web or test directories. These should be dealt with like org-dartlang-app: URLs.
c) libraries in other packages. These should also be treated like package: URLs.
But it seems to me that if we can rely on dwds always having a current directory that is the root of the current package, then we can handle this without ever dealing with proper package resolution.
For (a) we can see that they're $PWD/lib/..., for (b) they're $PWD/notlib/...
So the question is what they look like for case (c) and if we can deal with that simply. Worst case we can go through the .packages file, but I'm not sure we have to.
jakemac53 commentedon Sep 12, 2019
How else would we deal with (c) without going through the
.packages
file?Also note that
$PWD/notlib/
could be another package (theexample
dir often contains a subpackage or multiple subpackages for instance).DanTup commentedon Sep 16, 2019
I never actually had this work myself, but I now see the always-file-uris change was only merged into the previous release (even though it was done a while ago), so you may have been using a build from before then while I was always testing with latest code.
There are several reasons I had preferences for file URIs:
lib/
folder anyway, so why have two different formats.packages
file isn't formally spec'd (at as far as I know, and publicly) and I've had bugs in this code before (for ex. around windows path formats, case sensitivity, comments, trailing slashes - the SDK has hand-maintained .packages files for example)I don't think there's an obvious answer, but I do think any decisions made should documented explicitly somewhere (for ex. in the VM Service spec - FYI @bkonyi). A lot of VS Code's debugger decisions are based on "see what the IntelliJ source does" or "try both and see which work" and then when bugs crop up it's always difficult to know "is VS Code doing something wrong, or is this a bug in the VM?" which complicates getting things fixed.
jakemac53 commentedon Sep 16, 2019
In Dart libraries are identified by their URI. Historically at least the same file imported by absolute
file:
uri andpackage:
uri would actually be two different libraries. So from that sense there is a real concrete difference here.Recently I think this may have changed (at least for the application entrypoint file) where an absolute
file:
uri will actually be canonicalized back to apackage:
uri but I don't know the details. I just know the vm will spit out some sort of message if you do likedart lib/main.dart
.This is the original DEP https://github.com/lrhn/dep-pkgspec/blob/master/DEP-pkgspec.md#proposal.
It isn't in the language spec because afaik the language itself isn't opinionated about uris at all - its completely up to the implementations how uris are resolved.
It makes a lot of sense from an IDE perspective for sure - but it is trivial to map from a
package:
uri to afile:
uri. The reverse is a lot more work (and also actually very error prone due to nested packages).Correct - and those should always be
file:
uris.jakemac53 commentedon Sep 16, 2019
Also note that the
.packages
file format is actually changing slightly due to NNBD. I pushed back on this because its a very core piece of the SDK and imo it should not change in a non-breaking release of the language, but I seem to have lost that battle.dart-lang/language#365
DanTup commentedon Sep 16, 2019
Yeah, I recall some changes around that (possibly dart-lang/sdk#33076). It was another step in the direction of "file paths working everywhere" which seemed great at the time :)
Heh, apparently my nervousness is justified :D Some of those changes definitely sound like they'll break VS Code - I'll add a note on there - if they go ahead, I'll definitely need to update my code.
But here that reverse work isn't going away, it's just being pushed to the IDE to do it instead (the editor is giving us file paths and you want package URIs). My concern is that complicated error-prone work seems better done closer to the source/SDK (in "official" code) since it's less likely to not be updated if things change (for example the change discussed above I was completely unaware of) and it's backwards/forwards compatibility demands are lessened (you can add SDK version constraints to packages and SDK tools are versioned with the SDK, but the IDE is expected to work with older SDKs as well as newer ones).
jakemac53 commentedon Sep 16, 2019
I confirmed that it is still the case that for imports at least absolute file uris and package uris are not the same thing, the following program:
Gives this CFE error from the dart vm and also an analyzer error.
Even if you add the cast you get a runtime failure, so the VM internally also treats these as different libraries:
So, I don't see how supporting absolute file uris can be correct at all, and really shouldn't even be supported in the VM. There is no way to correctly identify which of these two libraries you want to operate on by absolute file uri.
2 remaining items
alan-knight commentedon Sep 16, 2019
So on the web, does that mean that a package: URL and an http: URL would be two different things? A file: URL isn't really possible there, so we'd either have to reject it outright or turn it into one of the others.
DanTup commentedon Sep 16, 2019
I think it's more complicated than this. It is possible for the user to load a file using a file URI (as the above shows), so blindly converting them to
package:
URIs also seems fraught with danger. What if the loaded one is afile://
URI and in VS Code we convert it topackage:
URI? No breakpoints will work :-/jakemac53 commentedon Sep 16, 2019
Try and set it both ways? lol.
jakemac53 commentedon Sep 16, 2019
On a serious not that is a good point though, it is unreasonable for the IDE to know all the transitive ways a library was imported in an app.
In general, by convention at least, nothing under
lib
of a package should be resolved to afile:
uri, they should always bepackage:
uris to avoid theA is not an A
issue.The examples I brought up really do indicate a user error imo, and would not even be allowed in a web context anyways.
That being said most defensive thing the IDE could do is try it both ways. In theory that would be the correct thing to do in the manufactured case above to set the breakpoint in each version of the library as well.
That obviously kind of sucks though.
jakemac53 commentedon Sep 16, 2019
I think in general given all the complexities here I think I have come around to just adding in support for
file:
uris.When we are constructing the
LibraryRef
instances we can create a reverse mapping of absolute file uri to library and do O(1) lookups. This will be significantly less error prone than other strategies because at that time we know exactly what the absolute file uri is for a package: uri.This also brings our behavior in line with the VM, for better or worse, which has generally been our overarching strategy.
It isn't technically correct, but it makes things easier on the ide's and should work out of the box for more tools that work today.
DanTup commentedon Sep 16, 2019
That's actually what we used to do - but there were other complexities (because we have to map breakpoints between VS Code and the VM, and if this mapping isn't one-to-one, you get interesting behaviour like having to delete a breakpoint twice for it to disappear... it's stuff we can work around, but the easier fix was to just not do it :-))
That's true, but you could also argue the same for the VM. If the IDE is just going to send both, the VM could just interpret one of them as both. It seems weird to teach the IDE that "we're just going to always treat these things as the same" if the VM could do it and shield the IDE from the complexity.
Yeah, it'd be nice if this was enforced and there was never any ambiguity.. I think that ship has sailed though. That said, if we had a lint that enforced it or something, it would give more weight for IDEs only working with one way.
Sounds good to me 😄
If it doesn't work out, I'm not against changing things - as long as we can write them down explicitly. There's a lot of ambiguity in these things (because there aren't really many clients) and it makes it hard to ensure they work the same (I've spent a lot of time chasing bugs around breakpoints not being hit and the like). Of course, I'd prefer to keep that complexity closer to the source though :-)
alan-knight commentedon Sep 16, 2019
Interesting side point. In the VM it appears these are actually two separate libraries. If I make a program that imports both package: and file: and debug in observatory they are two separate libraries with two separate URIs. Observatory won't let me set a breakpoint on either one.
DanTup commentedon Sep 17, 2019
That matches what I found from VS Code above. I don't know it if's always been that way, or has happened as fixes have been made to handle paths/package URIs better. It doesn't feel quite right, but then I'm not sure that loading the same file under both URIs is right either. I can't imagine that situation is ever done deliberately (besides us testing!), if it happens it's likely a mistake on the users part (and often causes weird issues - like ending up library-level state duplicated).
alan-knight commentedon Sep 25, 2019
@DanTup
I've landed file URL support in #676
Hopefully this works for you.
DanTup commentedon Oct 1, 2019
Great, thanks! Do I need to wait for a release to try this out? I tried running
flutter update-packages --force-upgrade
but I still see the same error (I don't think the latest version on pub has this change).alan-knight commentedon Oct 1, 2019
Unfortunately it's a bit awkward right now. There's another change that's breaking for Flutter, so we need to fix that. I'll see about getting that done.
alan-knight commentedon Oct 2, 2019
I've created a Flutter pull request. flutter/flutter#41805 that you'll need in order to test this.
alan-knight commentedon Oct 2, 2019
Closing this as it's published. If there are still problems re-open or file another bug.
DanTup commentedon Oct 3, 2019
Awesome, thanks! I'll try it out once that PR lands.