Debugger via VM evaluates constructor argument instead of class member

[mkorbel1]

Refiling Dart-Code/Dart-Code#5363 here after discussion between @DanTup and @bkonyi

Describe the bug
In some scenarios (see reproduction below), the debugger shows the wrong (a stale, previous) value of a class field. This can make debugging super confusing, since the actual behavior of the code doesn't match what the debugger is showing.

To Reproduce

Here's a simple piece of code that reproduces the issue.

class A {
  List list;
  A(this.list) {
    this.list = [3];

    print(list);
  }
}

void main() {
  A([1, 2]);
}

If you put a breakpoint on the print statement, you'll see two values of list depending on whether it is preceded by this..

With mouse-over on list in the print statement:

With mouse-over on this.list, which is the same variable:

You can also see something similar going on in the variables window:

The pattern appears to be related to the constructor containing this. referencing a field of the class, then whether or not further references to that variable include this. or not controls whether you get the latest or a stale value for that variable.

The output of the program is as expected:

[3]

Expected behavior

Variable values shown by the debugger match reality and are consistent, regardless of whether an unnecessary this. is included.

Screenshots

(see above)

Please complete the following information:

You can run the Dart: Collect Diagnostic Information command from the VS Code command palette (F1) to easily capture this information or provide it manually.

• Operating System and version: Windows 11 + WSL
• VS Code version: 1.95.3
• Dart extension version: v3.102.0
• Dart/Flutter SDK version: 3.5.4 (stable) (None) on "linux_x64", no Flutter
• Target device (if the issue relates to Flutter debugging):

See details below:

Workspace Environment

Dart Code extension: 3.102.0

App: Visual Studio Code
App Host: desktop
Remote: wsl
Host Kind: wsl
Version: linux 1.95.3

Workspace type: Dart (LSP)
Workspace name: thislist [WSL: Ubuntu]

Dart (3.5.4): /usr/lib/dart
Flutter (undefined): undefined (No device)

Output from 'dart info'

/usr/lib/dart/bin/dart info

If providing this information as part of reporting a bug, please review the information
below to ensure it only contains things you're comfortable posting publicly.

General info

• Dart 3.5.4 (stable) (None) on "linux_x64"
• on linux / Linux 5.15.167.4-microsoft-standard-WSL2 Process tests sometimes cause timeout on Linux #1 SMP Tue Nov 5 00:21:55 UTC 2024
• locale is C.UTF-8

Project info

• sdk constraint: '^3.5.4'
• dependencies:
• dev_dependencies: lints, test

[bkonyi]

FYI @derekxu16. Also cc @alexmarkov who might have some insights as to scoping.

[alexmarkov]

Parameter (local variable) list should be hidden according to the language specification:

Initializing formals constitute an exception to the rule that every formal parameter
introduces a local variable into the formal parameter scope (9.2). When
the formal parameter list of a non-redirecting generative constructor contains
any initializing formals, a new scope is introduced, the formal parameter ini-
tializer scope, which is the current scope of the initializer list of the constructor,
and which is enclosed in the scope where the constructor is declared. Each
initializing formal in the formal parameter list introduces a final local variable
into the formal parameter initializer scope, but not into the formal parameter
scope; every other formal parameter introduces a local variable into both the
formal parameter scope and the formal parameter initializer scope.

It looks like front-end is not taking this rule into account when deciding if list is a reference to a parameter or a field during expression evaluation.

/cc @johnniwinther

[DanTup]

Is it the case that the same name could be either in the same frame? For example:

class A {
  final String foo;
  final String bar;
  A(String foo)
      : foo = '',       // foo 1
        bar = foo;      // foo 2
}

Something I mentioned in another issue is that it might be useful if we can provide a line/col instead of a string expression when evaluating. It also solves some other issues like:

var aaa = 'one';
f() {
  var aaa = 'two';
  debugger();
}

If you stop at debugger() and hover over aaa on the first line, you'll get two if we only evaluate for the current frame using the name. VS Code/DAP does give us the line/col, so it could be nice if we could instead "evaluate the expression at line 1 column 5"?

[DanTup]

Although, using line/col only works for hovers, and won't fix this issue if you type list into the Debug Console/watch window... so probably even if we did that, there's a fix needed here (although what to do in the initializer case noted above - assuming it was valid - I'm not sure).

[johnniwinther]

@jensjoha The normal encoding of the AST does support the special scoping of variables from initializing formals so the error might be introduced when we compute the scope from the AST.

[jensjoha]

Thank you for your report and for the reproduction.
This is certainly a corner-case that we have not thought about.

What happens is this:

Compiling this (slightly modified) file:

import "dart:developer";

class A {
  List list;
  A(this.list) {
    list = [3];
    debugger();
    print(list);
  }
}

void main() {
  A([1, 2]);
}

results in this kernel file:

library;
import self as self;
import "dart:core" as core;
import "dart:developer" as dev;

import "dart:developer";

class A extends core::Object {
  field core::List<dynamic> list;
  constructor •(core::List<dynamic> list) → self::A
    : self::A::list = list, super core::Object::•() {
    this.{self::A::list} = core::_GrowableList::_literal1<dynamic>(3);
    dev::debugger();
    core::print(this.{self::A::list}{core::List<dynamic>});
  }
}
static method main() → void {
  new self::A::•(core::_GrowableList::_literal2<dynamic>(1, 2));
}

where we see that the constructor A(this.list) is rewritten as A(List<dynamic> list) : this.list = list, super().
Now, when compiling the body of the constructor it knows that list refers to this.list, but then
a) The VM just shows the locals that it has where it is paused (which includes list);
b) the expression compilation doesn't do anything special for constructors, so when
c) the VM says "compile list inside the class A in a situation where we have a local list, then
d) the expression compilation just finds list as the local.

As at least @johnniwinther and I have talked about many times, expression compilation is not really - and shouldn't really be - "Dart" - it is and should be something different and/or more.
Being able to - in a debug context - get one's hands on whatever was given as an argument might be useful. It might also be confusing. I don't know what the best thing to do here is, nor - assuming we can come up with a best thing - if it would be worth the effort.

[mkorbel1]

Thanks for the explanation of the underlying reason for this behavior.

Expanding the capabilities of the debugger expressions to view more than what dart code directly could access seems like an interesting concept. For example, I often wish I had unrestricted access to private variables of objects via debug expressions -- dart code doesn't have access, but the debugger does already via the UI. However, the ability to write (somewhat) complex Dart expressions in the debug console is invaluable (e.g. I often will use .where or .map on collections to help debug in the console). Adding the ability to declare temporary debug-only variables, similar to the Python debugger, could be cool too.

Regardless, the "proper" behavior in this context seems obvious to me: this.list and list should be reference the same thing in the debugger (and the same thing as the actual compilation/execution). There are all sorts of contexts where a prior value of a variable has changed after some lines have executed, and I'm not sure why getting "one's hands on whatever was given as an argument" is worth an exception. If it's so important, people can save the value in another variable or set a breakpoint earlier to see it. The reasoning of why this bug exists doesn't justify the confusing behavior, since to the programmer there is only one variable.

Regarding "if it would be worth the effort," I think fixing this seems extremely valuable and important. Debug tooling when switching to a new language takes some getting used to, and many people will instinctively go to the debugger to understand why their code isn't working, especially if they are new to Dart. Seeing the wrong value in a variable in the debugger leaves developers confused and frustrated and may altogether dissuade them from sticking with Dart if they have alternatives under consideration. There's a huge difference between thinking the debugger is practically perfect vs. thinking the debugger shows the wrong information in some contexts. If people don't trust what the debugger is showing them, then they might start regressing to print statement debugging.