dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.06k stars 1.56k forks source link

Review breakpoint type resolving scope #56301

Open FMorschel opened 1 month ago

FMorschel commented 1 month ago

Hello. Please consider https://github.com/dart-lang/sdk/issues/56300.

Summary: The is operator in Dart evaluates to true when checking against an unresolved identifier, instead of throwing an error. This behavior is observed in Flutter, but not in standalone Dart, leading to unexpected results in conditional breakpoints and expression evaluation.

Now, if this ends up being fixed as non-Flutter Dart, for every type that is not in scope:

For non-Flutter dart, you get a reasonable error:

image

I'd like to question here if there could be a way for breakpoint expressions to "ignore" that scope.

Use case:

I have an extension of FocusNode, wich is a Flutter (third party/package) class. But as an extended class, some of the logic is done inside Flutter files and they have no way of knowing about my subclass. I'd like to place a breakpoint there and only stop there when my class stops there so I can see if I made anything wrong or I am missing anything. But in my case, all FocusNode/FocusScopes in my screen stop there and that makes me have to skip a lot of breaks and hope for me to not missclick on the right one.

In this specific case, I can of course do a simple empty wrapper on the exact method I'd like to see and break point there and go inward to see whats going on. Well, two more things here:

dart-github-bot commented 1 month ago

Summary: The is operator in Dart incorrectly evaluates to true when checking against an unresolved identifier in Flutter, leading to unexpected behavior in conditional breakpoints and expression evaluation. This issue is not observed in standalone Dart. The user requests a way for breakpoint expressions to ignore scope, allowing them to debug code within extended classes without being interrupted by unrelated breakpoints.

DanTup commented 1 month ago

What makes this complicated is that if the type is not in-scope, it could be ambiguous (for example you could have two class Foos in your project, neither of which are in-scope.. which one should be used?).

C# has something where you can use global::Namespace.Foo to reference Foo in Namespace even if it's not "imported" (although I don't know if you can use it in debugger evaluation expressions), but AFAIK there's nothing similar in Dart.

I think the VM Service's evaluate methods may also allow us to pass additional names to have in-scope, but I don't know a) if it would work here or b) how the debugger/editor would be able to build scopes to satisfy this (we can't parse the expression so we don't really know that it has type names in, even if we had a way to search for them).

FMorschel commented 1 month ago

Maybe if we could indicate what it should import in some way?

johnniwinther commented 1 month ago

cc @jensjoha

a-siva commented 1 month ago

Not sure if this is a VM issue, as indicated in comment-2243272766 the client tool would have to build scopes appropriately.

bkonyi commented 1 month ago

@DanTup, if you make use of the evaluteInFrame RPC, the type should be in scope without having to explicitly provide it.

DanTup commented 1 month ago

@bkonyi we do use evaluateInFrame, but the issue here is that the class used in the expression is not in-scope.

The issue here is not a bug, but a request to try and improve the situation where you have a variable of some type and want to do an is check (for example in a conditional breakpoint), but there's nothing you can write for the type that will work, because it isn't imported and in-scope.

I've hit this myself before, and often end up rewriting the expression to be .runtimeType.name == 'Foo', but it's not equivalent so isn't always possible. As far as I know, there's no expression we can write to reference a type that's not in scope (for example like .NET's global::Namespace.Type).

I suspect for the huge majority of cases, "the first class found anywhere with the name Foo" would do just what the user wanted, but ofc that's not very sound (or understandable to the user).

I'm not sure (without having some way to reference a type that's not in scope) there's a lot we can do here, but maybe others have ideas? 🤔

bkonyi commented 1 month ago

@DanTup can you share some sample code to help me better understand the context?

DanTup commented 1 month ago

@bkonyi this is a bit contrived, but imagine this code:

void main() {
  foo(<String>{});
}

void foo(Set s) {
  print(s); // Conditional breakpoint here "s is LinkedHashSet"
}

If I add a breakpoint to the marked line with the condition s is LinkedHashSet, it doesn't work because LinkedHashSet isn't in scope (even though it is the type of the variable):

image

While the error is clear (at least for Dart - https://github.com/dart-lang/sdk/issues/56300 makes this really confusing for Flutter, because it just silently doesn't trigger), besides adding an unused import to your code and reloading, there's not a clear way to make the breakpoint trigger when you want.

I don't know what a good fix it, but it would be nice if there was a way to fix this, by being able to write an expression like s is LinkedHashSet (and have some way to specify where LinkedHashSet comes from) without having to add an unnecessary import and reload the app.

bkonyi commented 1 month ago

Is this a problem if the declaration or import is present in the file? Or is this only a problem with types defined in dart:core?

FMorschel commented 1 month ago

Or is this only a problem with types defined in dart:core?

It happens with any type that the declaration is unknown.

Is this a problem if the declaration or import is present in the file?

No. When this happens, it does work.

The following example works for both B and C because their type declarations are in the scope of the breakpoint (present in file like B or imported like C).

File bug.dart:

import 'c.dart';

void main() {
  foo(<String>{});
  bar(B());
  bar(C());
}

void foo(Set s) {
  print(s); // Conditional breakpoint here "s is LinkedHashSet"
}

void bar(A a) {
  print(a); // Conditional breakpoint here "a is B" or "a is C"
}

class A {}

class B extends A {}

File c.dart:

import 'bug.dart';

class C extends A {}
FMorschel commented 1 month ago

Just to be clear, about DanTup comment above:

While the error is clear (at least for Dart - https://github.com/dart-lang/sdk/issues/56300 makes this really confusing for Flutter, because it just silently doesn't trigger), [...]

It should be:

For Flutter, currently (until #56300 is resolved) it silently (does) triggers.

Which is really weird, it means that for the current state, it behaves strangely.


I'll explain this with a Flutter case but any third party package would fit here.

Lets say Flutter has a type B and a type D that extends/implements B.

If I'm debugging my Flutter project and I've created some class C for example, extending B because some Flutter widget accepts any B.

Now I go inside that widget declaration to see its current behaviour with my new class instance (that is C) and do this test (foo is C), it sounds to me like every instance of the type the widget accepts (B) is an instance of my new class C. Even when the framework itself passes a D instance somewhere else.

DanTup commented 1 month ago

@bkonyi

Is this a problem if the declaration or import is present in the file? Or is this only a problem with types defined in dart:core?

My example may have been slightly misleading - LinkedHashSet is not in dart:core but in dart:collection, so that name is not in-scope for this file/frame because there's no import for it. In any case where you have a variable of type X but X is not imported and in-scope causes the issue (and any time the type is in scope, it's fine).

@FMorschel

For Flutter, currently (until https://github.com/dart-lang/sdk/issues/56300 is resolved) it silently (does) triggers.

Sorry yes, I got it the wrong way around. That bug results in the condition returning true even when it's not, and that is more confusing than the opposite, because it pauses the debugger and you assume the variable is of the type you wrote, but it might not be. Hopefully that's an easy fix though, whereas this request seems a little tricker 😄

FMorschel commented 1 month ago

Maybe if we could indicate what it should import in some way?

Like the following:

image

Currently, it gives an error:

Debugger failed to evaluate breakpoint condition "import 'dart:collection'; s is LinkedHashSet": evaluateInFrame: (113) Expression compilation error
org-dartlang-debug:synthetic_debug_expression:1:1: Error: Undefined name 'import'.
import 'dart:collection'; s is LinkedHashSet
^^^^^^
org-dartlang-debug:synthetic_debug_expression:1:8: Error: Expected one expression, but found additional input.
import 'dart:collection'; s is LinkedHashSet
       ^^^^^^^^^^^^^^^^^

It is not the best possible option, but it would at least make sure that we could run these evaluation tests in breakpoints and such.