-
Notifications
You must be signed in to change notification settings - Fork 217
Add proposal for private named parameters. #4410
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: main
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! Just commented on a couple of locations that look like a typo.
Experiment flag: private-named-parameters | ||
|
||
This proposal makes it easier to initialize and declare private instance fields | ||
using named constructors parameters. It addresses [#2509][] and turns code like |
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.
Typo:
using named constructors parameters. It addresses [#2509][] and turns code like | |
using named constructor parameters. It addresses [#2509][] and turns code like |
|
||
* They can make the parameter positional instead. | ||
|
||
* They can commit to the API they want by making the field public and the |
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.
Typo:
* They can commit to the API they want by making the field public and the | |
* They can commit to the API they want by making the field private and the |
We don't want to put users in a position where the code that's most pleasant to | ||
write requires them to sacrifice semantic properties they want like | ||
encapsulation. | ||
|
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.
At this point it would make sense to add a short paragraph to mention that this proposal contains a couple of rules that are concerned with primary constructors (just so the reader doesn't get confused when this proposal talks about a 'field parameter').
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.
OK, reading it again I can see that line 267 says '(for a primary constructor)', but I didn't notice that on the first reading.
class Id { | ||
late final int _region = 0; | ||
|
||
this({this._region, final int _value}) : assert(_region > 0 && _value > 0); |
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.
Even here, I think it would be helpful to add a comment saying something like // If primary constructors are supported.
class House { | ||
int? bedrooms; // 3. The corresponding field. | ||
House({this.bedrooms}) { | ||
print(bedrooms); // 1. The parameter variable. |
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.
Actually references the instance variable. The this.bedrooms
parameter only introduce a variable in the initializer list.
So, an example would be:
class House {
int? bedrooms; // 3. The corresponding field.
House({this.bedrooms})
: assert(bedrooms == null || bedrooms >= 0); // 1. The parameter variable
}
Given an initializing formal or field parameter (for a primary constructor) with | ||
private name *p* in constructor C: | ||
|
||
* If *p* has no corresponding public name *n*, then compile-time error. *You |
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.
Only if it's a named parameter?
Or do we not allow privately named positional parameters. (It's consistent, but more breaking, you can no longer do foo(int _id, {String? id})
which you technically could before.
named, this then avoids the compile-time error that would otherwise be | ||
reported for a private named parameter.* | ||
|
||
* The local variable in the initializer list scope of C is *p*. *In the |
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.
The name of the ...
*Note that the proposal only applies to initializing formals and field | ||
parameters. A named parameter can only have a private name in a context where it | ||
is _useful_ to do so because it corresponds to a private instance field. For all | ||
other named parameters it is still a compile-time error to have a private 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 feels like it conflates the orthogonal concerns of being initializing/formal and being positional/named.
The first sentence is about the former, without referring to the latter.
The second sentence introduces named parameters as the reason for the restriction to initializing formals and field parameters, and sounds like it talks only about named parameters.
That leaves me wondering what the reasoning is for positional parameters, and if they're even allowed.
## Runtime semantics | ||
|
||
There are no runtime semantics for this feature. It's purely a compile-time | ||
renaming. |
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.
Nit-pick: The semantics in the "binding actuals to formals" algorithm needs to know about this, and know to bind the argument named n to the variable named p.
I know that names are (or should be) compiled away, but the language semantics doesn't do that, so there are runtime semantics, even if there might be no runtime implementation.
|
||
This proposal takes code that it is currently a compile-time error (a private | ||
named parameter) and makes it valid in some circumstances (when the named | ||
parameter is an initializing formal or field parameter). Since it simply expands |
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.
So it doesn't work for positional parameters?
should document any private named parameters using their public name. The fact | ||
that the parameter initializes or declares a private field is an implementation | ||
detail of the class. What a user of the class cares about is the corresponding | ||
public name for the constructor parameter. |
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.
More importantly: Authors should document the public name. You write // The [foo] is ...
to document this._foo
.
The generated documentation should follow suit.
The tools may support [_foo]
as well, and display it as foo
, but the important thing is what authors should write in the doc source.
To divide the labor up some on primary constructors, I figured it makes sense to handle private named parameters as a separate feature with its own independent proposal.
I do hope we ship them both as a batch, but this way it's easier for us and the implementation teams to work on them in parallel or independently.