-
Notifications
You must be signed in to change notification settings - Fork 205
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
Update augmentation specification. #4123
base: main
Are you sure you want to change the base?
Conversation
* Says that the entire class scope is in the lexical scope for members. * Update `augemented` specification, include constructors. * Only reserved inside expressions and bodies of augmenting member-like declarations, not type-like declarations. * Try to give grammar. It's really a parameterized grammar per kind of declaration. We don't have good parameterized grammars, so it's said in prose. * Changes specification for non-redirecting generative constructors. * Adds order of initialization of instance variables with initializers. * Says that body cannot use `augmented`. * Therefore no longer needs to reuse variable bindings, so don't. * Each augmenting declaration binds actuals to formals, remember any super parameters or super initializers, executes initializer list entries, and then executes its augmented declaration. When that comes back, it executes its body. * (Layering an augmentation on top of an NRG-constructor auto-calls augmented at the end of its initializer list.) The initializing formals and super parameters need more work. * Some small tweaks to disallow invalid combinations.
Needs more work. No updated version/changelog, pretty sure the rules for using initializing formals and super parameters are inconsistent, aka. not updated everywhere, and super-parameters need more formal specification. Still: WDYT? How far is it from what we want? |
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.
These changes are all great. Thank you for warming up my half-baked proposal into something edible. :)
@@ -237,16 +238,16 @@ which may be used to provide a body for an abstract member._ | |||
|
|||
An augmentation that replaces the body of a function may also want to | |||
preserve and run the code of the augmented declaration (hence the name | |||
"augmentation"). It may want to run its own code before the augmented | |||
code, after it, or both. To support that, we allow a new expression syntax |
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.
Haha, yes. This brings such joy to my dumb proofreader's heart. :)
in the function body refers to the augmented function, and it must | ||
occur only as the function of a function invocation, which means | ||
it is immediately followed by one, or no, type argument list | ||
and then an argument 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.
This is a little difficult to read. Maybe:
"which means it is immediately followed by a type argument list (which may be omitted) then an argument list (which must be present but may be empty)."
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 went through a lot of phrasings here, including "zero or one type parameter lists", but the plural annoyed me.
Maybe just:
it is immediately followed by an argument list, or a type parameter list and then an argument list.
_Tear offs are not possible, neither are member accesses like | ||
`augmented.call()`._ | ||
The expression `augmented` must not occur in an optional parameter | ||
default value expressions _It is a reserved word in such expressions, |
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.
"default value expressions" -> "default value expression."
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.
Going with singular "expression" rather than removing "an".
* **Augmenting operators**: Operators are methods with special | ||
invocation syntax. When augmenting an operator declaration, `augmented` | ||
refers to the augmented operator *method*, and it must occur only | ||
as the function of a function invocation. For example, when augmenting | ||
`operator +` you could use `augmented(1)` to call the augmented |
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.
"could" -> "would"
It's possible to close over `augmented` inside an augmenting function, to, | ||
for example, do `list.forEach((x) => augmented(x));` in a function augmentation, | ||
but doing `List.forEach(augmented)` is not allowed. That would treat the | ||
augmented function as if it had an identity._ |
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 is a good thing to point out. It feels weird at first, but then I realized that super
has the same limitation and it's never bothered me.
super-initializer of either the augmented or augmenting constructor. | ||
|
||
Invoking a non-redirecting generative constructor, with an argument list and a | ||
new object to initialize, two effects. |
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.
"two effects." -> "has two effects:".
declaration has a body, execute that body of this declaration in a | ||
scope that has the parameter scope of the binding-actuals-to-formals | ||
step as parent scope, and with `this` bound to the object being | ||
initialized. |
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 whole section is really good.
| : | | ||
.-----------------. : .-----------------. | ||
| C.isEven() body | : | C._isOdd() body | | ||
'-----------------' : '-----------------' | ||
``` |
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.
My ASCII art diagram lives on! :)
|
||
_It still is a compile time error for both a static and instance member | ||
of the same name to be defined on the same type, even if they live in | ||
declarations. You cannot work around this restriction by moving 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.
"live in declarations" -> "live in different declarations for the same class".
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.
Looking good so far, will continue my review tomorrow.
The exact grammar and meaning of the `augmented` keyword depends on what is | ||
being augmented. | ||
|
||
* **Augmenting variables**: Within an augmenting variable declaration's |
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 sounds repetitive with the new wording, maybe remove all of can only be used in an initializer expression, and it
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 repetetive, and unnecessarily so.
I think one of the parts is a leftover that should have been removed.
An augmenting variable declaration has no other place to put the word than in the initializer expression.
(Well, except in metadata. We can just generally say, once and for all, that augmented
is not a reserved word inside metadata annotations on the augmeting declaration.)
The reserved words parses as a `<primary>` expression. | ||
Evaluating `augmented` invokes the augmented declaration's getter | ||
function, and evaluates to the returned value. | ||
The static type of `augmented` is the declared return type of the augmented |
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.
Should we specify this as the declared return type or something a bit more general, like how we use the "static type" for initializer expressions?
For instance, does "declared return type" cover the override case correctly?
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.
Good point. We want the return type, not the declared return type. Or maybe we want the inferred return type. If there is a difference, it's the type that the declaration is determined to have after (top-level?) type inference has been performed.
We just need to define what the return type of a getter declaration is.
The getter return type of a non-augmenting getter declaration is the type denoted by its declared return type, if any.
If there is no declared return type, the getter is an instance getter of a class
, mixin
or enum
declaration, and any superinterface of the surrounding type has a getter with the same name, then the return type is the return type of the getter with that name in the combined superinterface of the current declaration. It's a compile-time error if the combined superinterface does not have a combined type signature for that getter.
Otherwise the return type of the getter is dynamic
.
(We don't infer the type of bodies in top-level inference, right? It's only for variables.)
The getter return type of a variable declaration is the type denoted by the declared type of the variable, if any. Otherwise, if the variable has no initializer and it's an instance variable, it's the return type of the getter or parameter type of the setter in the combined super-interface, if any. Otherwise it's the static type of its initializer expression.
The getter return type of an augmenting getter declaration is the getter return type of the augmented declaration. (Can't change the type. If the augmenting declaration writes a type, it must be that type.)
It gets complicated, because it's basically repeating all of type inference. I hope there is a way to reuse type inference instead.
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.
Right, but I think all we need to state here is that it uses the inferred return type? We don't need to explain how the inference works.
and then an argument list. | ||
_Tear offs are not possible, neither are member accesses like | ||
`augmented.call()`._ | ||
The expression `augmented` must not occur in an optional 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.
The expression `augmented` must not occur in an optional parameter | |
The expression `augmented` must not occur in optional parameter |
Co-authored-by: Jacob MacDonald <[email protected]>
Co-authored-by: Jacob MacDonald <[email protected]>
no earlier declaration to augment.* | ||
invoked with the same value for `this`. For a non-redirecting factory | ||
constructor, it's invoked with the same type parameter bindings for the | ||
surrounding type. |
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 had a similar comment above... I hadn't considered that the type arguments are on the class and not the function... that does make sense. And in any case it is a very weird thing to return a class with a tighter generic type than was given, it probably shouldn't even be allowed.
@@ -724,11 +793,11 @@ It is a **compile-time error** if: | |||
|
|||
* An `abstract` variable is augmented with a non-abstract variable. | |||
|
|||
* An `external` variable is augmented with an `abstract` variable. | |||
* An `external` variable is augmented with a variable declaration. |
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 am not sure if we need this restriction? It is true that the external
keyword would be unnecessary though in this case, but I feel like we have gone back and forth on this a bit so and I am curious what the motivation for not allowing this would be?
Do we need to make similar changes for external functions? The CFE today actually requires us to mark certain things external (but this is a bug).
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.
Let's see if I remember my reasoning.
I think it was mainly that an external
"variable declaration" doesn't introduce a variable to augment, it only introduces a getter and a setter. The only augmentations you should be allowed to make are to getter and setter.
But I can also see wanting to add metadata and comments.
Allowing an external variable declaration with an initializer expression is definitely wrong.
Allowing an augmenting variable declaration with no external
modofier would be weird, because that also looks like it is variable. So it should be marked external
, but that feels like it's an external augmentation, that is, the augmenting getter/setter body is supplied by the compiler.
Maybe we should allow @metadata augment id;
to apply to any declaration, and not be able to do anything other than add metadata (and comments, which the language has absolutely no opinion on, but then, it has no opinion on metadata either, as long as it compiles).
That is:
external int foo; // valid
@newData
augment external int foo; // Reasonable, but looks like the augmentation itself is external.
@newData
augment int foo; // Possible, looks like it's augmenting a variable.
@newData
augment abstract int foo; // Already not allowed (`abstract` and `external` are usually mutually exclusive).
augment int foo = 1; // Completely unacceptable.
@newData
augment foo; // Nice-to-have, but not currently possible.
Which ones should we allow, and what should they mean?
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 think it was mainly that an external "variable declaration" doesn't introduce a variable to augment, it only introduces a getter and a setter. The only augmentations you should be allowed to make are to getter and setter.
That is a compelling argument for me. I think we can leave things as you have defined here. Possibly expand to allow adding metadata/comments.
@newData augment external int foo; // Reasonable, but looks like the augmentation itself is external.
This seems ok to me, it just adds the annotation.
@newData augment int foo; // Possible, looks like it's augmenting a variable.
I don't like this, because it does seem like it is actually making it non-external. And, we don't allow augmenting getters/setters with variables.
@newData augment abstract int foo; // Already not allowed (`abstract` and `external` are usually mutually exclusive).
lgtm
augment int foo = 1; // Completely unacceptable.
agreed (now)
@newData augment foo; // Nice-to-have, but not currently possible.
I think this looks a bit weird so I wouldn't try to allow it
|
||
* First it initializes any instance variables in the class which have | ||
initializing expressions: | ||
* For each instance variable declared by the class, in source order |
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.
Do we need to define "source order" given that fields might be defined in different parts?
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 think it's defined somewhere as the "before" order from parts-with-imports. If not, it should be.
Says that the entire class scope is in the lexical scope for members.
Update
augemented
specification, include constructors.member-like declarations, not type-like declarations.
per kind of declaration. We don't have good parameterized grammars,
so it's said in prose.
Changes specification for non-redirecting generative constructors.
augmented
.remember any super parameters or super initializers,
executes initializer list entries,
and then executes its augmented declaration.
When that comes back, it executes its body.
auto-calls augmented at the end of its initializer list.)
The initializing formals and super parameters need more work.
Some small tweaks to disallow invalid combinations.