Skip to content

Possible bug in the debugger #51916

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

Closed
bwilkerson opened this issue Mar 31, 2023 · 4 comments
Closed

Possible bug in the debugger #51916

bwilkerson opened this issue Mar 31, 2023 · 4 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. dds-dap DDS issues related to the Debug Adapter Protocol (DAP) implementation vm-service The VM Service Protocol, both the specification and its implementation

Comments

@bwilkerson
Copy link
Member

Using VS Code, in the file https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/test/src/services/correction/fix/make_final_test.dart, insert the following code just before line 266:

  @soloTest
  Future<void> test_listPattern_ifCase_untyped() async {
    await resolveTestCode(r'''
void f(Object o) {
  if (o case <int>[var x]) print(x);
}
''');
    await assertHasFix(r'''
void f(Object o) {
  if (o case [final int x]) print(x);
}
''');
  }

Probably by the time you see this issue the test will have been committed and you can skip the steps above, other than adding the annotation.

Add a breakpoint on the line containing await assertHasFix and run the just edited file under the debugger.

When the debugger stops, expand this in the 'VARIABLES' pane. For me it displayed one field named node whose value was type 'Sentinel' is not a subtype of type 'InstanceRef'.

@bkonyi @DanTup

@jacob314 jacob314 added vm-service The VM Service Protocol, both the specification and its implementation dds-dap DDS issues related to the Debug Adapter Protocol (DAP) implementation labels Mar 31, 2023
@DanTup
Copy link
Collaborator

DanTup commented Apr 1, 2023

This seems to be specific to the new SDK DAP.

Legacy DAP:

Screenshot 2023-04-01 at 10 46 19

Log: dap.legacy.txt

New SDK DAP:

Screenshot 2023-04-01 at 10 55 32

Log:
dap.new.log

The log includes this error. My guess is that while fetching the fields, one of them is a Sentinel and we're not catching it, so the entire request is failing. We should catch this and only show the error against the field we couldn't evaluate.

#0      ProtocolConverter.convertVmInstanceToVariablesList.<anonymous closure> (package:dds/src/dap/protocol_converter.dart:243:46)
#1      ListExtensions.mapIndexed (package:collection/src/list_extensions.dart:150:20)
#2      _SyncStarIterator.moveNext (dart:async-patch/async_patch.dart:554:14)
#3      Future.wait (dart:async/future.dart:523:26)
#4      ProtocolConverter.convertVmInstanceToVariablesList (package:dds/src/dap/protocol_converter.dart:229:38)
#5      DartDebugAdapter.variablesRequest (package:dds/src/dap/adapters/dart.dart:1844:43)
<asynchronous suspension>
#6      BaseDebugAdapter.handle (package:dds/src/dap/base_debug_adapter.dart:136:7)
<asynchronous suspension>

"command":"variables","message":"type 'Sentinel' is not a subtype of type 'InstanceRef'

@DanTup
Copy link
Collaborator

DanTup commented Apr 1, 2023

The issue was the change field being uninitialised. The field values are typed as dynamic in the VM package so they weren't being checked when passed into setEvaluateName.

Got a fix for this (and another instance of the same thing for locals/globals) here:

https://dart-review.googlesource.com/c/sdk/+/292540

@bkonyi I've bumped the version and updated the changelog. If it's possible you could push a release of DDS after this lands, it'd be great to get this fix into Flutter before the branch (I'll manually roll the package if the test_api issue is not resolved so the bot can do it). Thanks :-)

@bkonyi as an aside, could dynamic be eliminated here to avoid this kind of issue? The dartdoc says BoundVariable.value is always InstanceRef, TypeArgumentsRef or Sentinel. If that's true, I think this would be Response (or Response?) and this kind of issue wouldn't be possible (we couldn't pass a Response to the method expecting InstanceRef, but we can pass dynamic).

@lrhn lrhn added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Apr 1, 2023
@bkonyi
Copy link
Contributor

bkonyi commented Apr 3, 2023

@DanTup we can probably do that, but we'll need to update the code generator to do this which may be a bit tricky. We've got other fields in other types that have union types that don't consist of only subclasses of Response, so changing all of our instances of dynamic to Object would be a lot simpler.

Either way, this would be a breaking change and we'd need to do some work to clean up code that is using these dynamic fields without a cast. Can you file a separate issue for this and cc me?

@DanTup
Copy link
Collaborator

DanTup commented Apr 3, 2023

but we'll need to update the code generator to do this which may be a bit tricky. We've got other fields in other types that have union types that don't consist of only subclasses of Response

Ah, I didn't realise it was generated. I have some similar pain in analysis_server (I added an Either3<X, Y, Z> type, but in some cases Y and Z are subtypes of X and we could eliminate the Either3 entirely!) 😄

I've filed #51930, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. dds-dap DDS issues related to the Debug Adapter Protocol (DAP) implementation vm-service The VM Service Protocol, both the specification and its implementation
Projects
None yet
Development

No branches or pull requests

5 participants