-
Notifications
You must be signed in to change notification settings - Fork 28
#3203. Add tests for private named parameters #3377
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.
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.
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.
One question for you but otherwise LGTM.
| Expect.equals("null", xRef1.valueAsString); | ||
| final xRef2 = | ||
| await service.evaluateInFrame(isolateId, 0, 'this._x') as InstanceRef; | ||
| Expect.equals("null", xRef2.valueAsString); |
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.
It is confusing to me that we expect _x and this._x to both be null at this time. Why?
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.
Based on the current behaviour of
class C {
String x;
C(this.x);
}x is null at this step. Therefore, I believe, that in this test _x also should be null.
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 current location in code is the initializer list (we could add an initializer list, or we might have stopped at a breakpoint "in the initializer list" even though it's empty, which could be the way the VM sees it) then x would denote the parameter. In contrast, at any location with access to this, including the body of the constructor (if any), x would denote the instance variable, and so would this.x.
The same would be true with _x.
If the current position in code is the initializer list it might still be possible to access the instance variables of the enclosing object using terms like this.x or this._x. The language doesn't allow us to do that, but the debugger might.
So I've argued that we should at least know how it's supposed to work, and preferably look at the treatment of both the parameter and the instance variable in situations where it is the intention that both can be viewed and possibly modified.
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.
(Send an unsent comment.)
|
|
||
| /// @assertion We let users use a private name in a named parameter when the | ||
| /// parameter also initializes or declares a field. The compiler removes the `_` | ||
| /// from the argument name but keeps it for the corresponding field. |
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.
(Oops, it looks like this comment was never sent. Submitting an extra review just for that.)
This is not 100% precise: An invocation of a constructor can pass an actual argument of the form name: value and this actual value can be bound to a parameter whose name is _name (it could be declared as {... this._name ...} or, if the constructor is declaring, {... final T _name ...}).
This means that the initializer list of the constructor can access the parameter as _name and not as name when it's declared as this._name. In the body, the declaring or initializing formal parameter is not in scope, and _name resolves to the instance variable (unless there's a local declaration whose name is also _name).
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.
This statement is a copy from the spec. https://github.com/dart-lang/language/blob/ee29f2eb0388345dbb91494ccc33df73dc5ee0a7/accepted/future-releases/2509-private-named-parameters/feature-specification.md?plain=1#L229-L232
Maybe we should change it there?
No description provided.