-
Notifications
You must be signed in to change notification settings - Fork 28
#3203. Add VM - CFE interaction tests for primary initializer scope #3359
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, awaiting further input from @nshahan.
| /// `<declaringConstructorSignature>`. | ||
| /// | ||
| /// @description Check that members initialized in primary initializer scope can | ||
| /// be debugged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be overlooking something but is this testing what is in scope when paused at the primary constructor (LINE_A)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm afraid not. Seems that you are expecting the test like the below (example is written without primary constructors):
class C {
String x;
C(this.x); // LINE_A
}
void testeeMain() {
debugger(); // LINE_B
var c = C("xxx"); // LINE_C
}
final tests = <IsolateTest>[
hasStoppedAtBreakpoint,
stoppedAtLine(LINE_B),
stepInto,
stoppedAtLine(LINE_C),
stepInto,
stoppedAtLine(LINE_C),
stepInto,
stoppedAtLine(LINE_A),
(VmService service, IsolateRef isolateRef) async {
final isolateId = isolateRef.id!;
final isolate = await service.getIsolate(isolateId);
final lib =
(await service.getObject(isolateId, isolate.rootLib!.id!)) as Library;
// Check the value of 'x' here
},
];Please confirm that it's what you'd expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! In LINE_A we have the constructor declaration, with no initializer list and no constructor body (but we could have those, too), and x in the initializer list would denote the formal parameter whereas x in the constructor body would denote the instance variable. I think this means that LINE_A is simply too ambiguous to answer questions like "do we have access to x?". The parameter and the instance variable may have different values (for instance, with C(String x): assert(x != ""), x = x { x = ""; }, the instance variable does not have a value until the second element in the initializer list has been executed, and it gets a new value in the body.
It would be possible to make the location in code more fine grained by inserting line breaks, e.g.:
class C {
String x;
C(
this.x // LINE_A
);
}At the comment, the formal parameter x is not in scope, and the instance variable x may or may not have been initialized (depends on whether we're before or after this.x in that line).
However, it certainly makes sense to show the value of a formal parameter at the time where it is used (in any way, including the case where it's used to initialize an instance variable as it is here).
I think this implies that "what's in scope?" is a (slightly) wrong question to ask when it is decided what to show in a debugger, which is probably also reflected by the implemented behavior.
@nshahan, WDYT? Is there a concept which is similar to the language-level notion of a scope which is used for this purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please confirm that it's what you'd expect.
@sgrekhov
That looks like what I thought the description comment at the top of the file was saying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT? Is there a concept which is similar to the language-level notion of a scope which is used for this purpose?
I'm trying to ask what values we think would be useful to provide to users when stopped at a breakpoint and what names would be used to access those values.
I don't have a proper name for this concept but "scope" comes close because it largely overlaps when stopped at breakpoints at most locations.
I realize now that the language does define scope but not breakable points so there are times when the defined scope isn't always enough to define what values should be available in the debugger when stopped at a breakpoint.
My original questions about breakpoints at primary constructor lines are all really asking the following:
- If the user sets a breakpoint on that class header line, will the program pause when the constructor gets called?
- If so, what values can be evaluated/inspected and by which names?
- Is pausing in the class header equivalent to pausing at the beginning of the initializers list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user sets a breakpoint on that class header line, will the program pause when the constructor gets called?
I think that would be meaningful and useful. Also, a breakpoint on the body part of a header constructor should probably give rise to the same behavior, and there may other reasons to stop on that line even in the case where there's no code to run in order to "execute" the parameters.
bool get foo {
print('Running foo');
return true;
}
class A() {
this: assert(foo) { print('Executing the constructor `A`'); }
}Here, I'd expect a breakpoint on the this line to allow me to step into the evaluation of foo as well as the invocation of print. Presumably a breakpoint on A() would do the same (because it allows me to single step through the code associated with this constructor invocation even if there's no code associated with A() itself).
If so, what values can be evaluated/inspected and by which names?
You could provide support for evaluating variables in scope using their declared name. An expression like this.x could be used to evaluate an instance variable which is declared in the enclosing class or inherited, even in the case where the instance variable is shadowed by some other declaration (such as a formal parameter).
This would allow the developer to see the value of some variables whose value is not available to the code in the program at the current breakpoint, but it would be useful and meaningful to be able to access them even in the case where there is no value (because the variable hasn't been initialized yet), and also in the case where a declaration exists but is shadowed.
Is pausing in the class header equivalent to pausing at the beginning of the initializers list?
Is there code that implements the side effect associated with passing an actual argument to a formal parameter which is initializing (e.g., this.x) or declaring (e.g., final int x)? The initialization of variables based on these syntactic forms might be considered to be part of the initializer list, in which case it's fine. Otherwise I'd expect to stop before the parameter-based initialization code as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nshahan, I think this thread is clarifying a broader set of topics that aren't blocking for this test to land. Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, except it is unclear to me what properties of the stepping and debugging experience this test attempts to assert. I'm probably just getting lost in the details so don't consider it blocking.
|
The test rewritten. Expected behaviour is based on behavior of the debugger when debugging class C {
String x;
C(this.x);
}Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, awaiting further input.
| /// `<declaringConstructorSignature>`. | ||
| /// | ||
| /// @description Check that members initialized in primary initializer scope can | ||
| /// be debugged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nshahan, I think this thread is clarifying a broader set of topics that aren't blocking for this test to land. Do you agree?
No description provided.