dart-lang / language

Design of the Dart language
Other
2.62k stars 200 forks source link

Allow augmenting member declarations immediately in the same body? #3967

Open eernstg opened 2 weeks ago

eernstg commented 2 weeks ago

Thanks to @lrhn for bringing up this situation. We could consider supporting an augmenting member declaration that follows the corresponding member declaration immediately, that is, in the same body. The feature specification does not mention this situation, which is probably an oversight.

It seems consistent to allow it, given that we've introduced support for having top-level augmentations with a similar structure. For example:

// Proposal.

class A {
  void foo() => print('Get started.');

  augment void foo() {
    augmented();
    print('Add some stuff!');
  }
}

// Already supported.

void fooTop() => print('Get started.');

augment void fooTop() {
  augmented();
  print('Add some stuff!');
}

If we do introduce support for this kind of "dense" member augmentation then we'd need to adjust the rules about the meaning of augmented slightly (https://github.com/dart-lang/sdk/issues/56143#issuecomment-2213758089): An identifier expression of the form augmented would have the augmentation related semantics when any enclosing declaration is augmenting, otherwise it would be a normal identifier which is subject to lexical lookup (which includes a potential transformation to this.augmented). Similarly, any declaration named augmented is an error when it occurs inside an augmenting declaration. In short, augmented always has the special meaning if any enclosing declaration has the modifier augment.

lrhn commented 2 weeks ago

We could consider supporting an augmenting member declaration that follows the corresponding member declaration immediately, that is, in the same body.

That's what I specified in #3800, so it's already assumed to be possible. I might have forgotten to mention it, but it's not disallowed, and the grammar and semantics allow it, so it's allowed, and that was intentional. (I'm not sure it was disallowed in the prior spec either.)

The most common usage I'd expect is:

class Something {
   int value;
   augment set value(int newValue) {
     var oldValue = value;
     augmented = newValue;
     if (oldValue != newValue) notifyChange(); // Or something.
   }
}

For methods, anything you can do with an augment is something you can also do in just one function body (except possibly calling the augmented() twice, but I've yet to find a use-case for that which isn't better solved by a local helper function.)

Depending on what we do for constructors, there might be something worth augmenting, but for now I think augmenting the setters/getters of variable declarations is the only actually useful feature.

An identifier expression of the form augmented would have the augmentation related semantics when any enclosing declaration is augmenting

That's vaguely specified, and possibly meaningless outside of an expression body. Having the "augmentation related semantics", which defines the meaning of fx augmented() inside a function body, and making the word reserved inside that body, doesn't say anything about what augmented means outside of a body. Since a class can be augmenting,

augment class C {
  final bool augmented = false;
}

is a declaration that should be affected by this phrasing, but the spec doesn't say anything about what augmented means in that position, so "having the augmentation related semantics" is not fully defining.

I'd just say:

Inside an augmenting method, setter, getter, operator or constructor body, or inside an augmenting variable declaration's initializer expression, the augmented identifier is a reserved word. Inside a method, operator or constructor body, the reserved identifier may be used syntactically an expression denoting the function of a function invocation expression, like augmented<TypeArgs>(args) (type arguments optional) with the same number of arguments as the augmented function for methods and operators, no arguments for constructors. Inside an augmenting setter body, the reserved word may be used syntactically as an assignable expressions being assigned, augmented = expression. (Not readable, so no augmented += expression.) Inside an augmenting getter body or an augmenting variable declaration's initializing expression, the identifier may be used as a <primary> expression, which is not assignable. Any other occurrence of the reserved identifier augmented is a compile-time error.

We can also say that augmented is a reserved word for code syntactically inside any augmenting declaration, in which case the above augmented class C becomes invalid (a reserved word is not a valid name for a variable), and then we introduce the same exceptions as above, where the reserved word is allowed, and then we just have to assign semantics to those.

It's worth noticing that nothing prevents using augmented inside a nested function of the augmenting function. Or having that function escape the invocation that created it. That is, the following should be able to work, unless we do something to prevent it. I don't think we should.

class C {
  final void Function(int)? _setX;
  int x;
  augment set x(int value) {
    _setX ??= (int value) { augmented = value; };
    if (value < 0) throw "Don't be negative!";
    augmented = value;
  }
  C(this.x) { x = x; }
}
void main() {
  var c = C(42);
  try { 
    c.x = -1;
  } catch (e) {
    print("Nope");
  }
  c._setX(-1); // Gotcha!
  print(c.x); 
}

That's just being weird for being weird, but it also allows:

  augment set value(Object? value) {
     if (value is Future<Object?>) { 
       value.then((var result) { augmented = result; });
     } else {
       augmented = value;
    }
  }

or similar operations that depend on using a callback before doing the real operation. I think it should work.

eernstg commented 2 weeks ago

That's what I specified in https://github.com/dart-lang/language/pull/3800, so it's already assumed to be possible.

3800 was landed on June 28th so the version of the feature specification that I referred to includes those changes. I don't see any support for concluding that an augmenting member declaration inside a non-augmenting declaration is supported, it looks like the opposite is assumed:

An augmentation declaration can add new members to an existing type, or even modify the code of an existing declaration.

Doesn't say "A declaration (augmenting or not) can ... modify the code of an existing declaration".

Instance and static members inside a type may themselves be augmentations. In that case, they augment the corresponding members in augmented type declaration

It would have been natural to mention that the 'augmented type declaration' could be the same declaration as the augmenting one, except that in this case the "augmenting" type declaration isn't augmenting, because that implies the keyword augment up front.

If Ctop is a non-augmenting declaration, the declarations of C is the set of syntactic instance member declarations of Ctop.

Nothing about stacks of syntactic instance member declarations here.

Anyway, we just need to decide whether we want it or not, and then double check that the text says what it should say. I think it's a good idea! :smile:

An identifier expression of the form augmented would have the augmentation related semantics when any enclosing declaration is augmenting

That's vaguely specified, and possibly meaningless outside of an expression body.

We do need to introduce a specific term to talk about all the different variants of semantics for the word augmented when it refers to an augmented declaration, but that's just about the terminology. I do think we have two potential semantics for augmented as an identifier expression: It can be a normal identifier, in scope, or it can be an invocation of an augmented member declaration (there are many cases, with different syntax like augmented(42) or augmented + 42).

I don't see any reason why this would be less precisely defined when such an identifier expression occurs in a block body of a function, as opposed to an expression body.

augment class C {
  final bool augmented = false;
}

is a declaration that should be affected by this phrasing, but the spec doesn't say anything about what augmented means in that position,

It is an error:

A compile-time error occurs if a declaration with the basename augmented occurs in a location where the outermost enclosing declaration is augmenting. This error is applicable to all such declarations, e.g., local functions, local variables, parameters, and type parameters.

If this proposal is accepted then this should be adjusted to say "any" rather than "the outermost".

It's worth noticing that nothing prevents using augmented inside a nested function of the augmenting function.

Except the spec, of course.

   augment set value(Object? value) {
     if (value is Future<Object?>) { 
       value.then((var result) { augmented = result; });
     } else {
       augmented = value;
    }
  }

As I mentioned, it is easy to establish a workaround for access to a top-level declaration named augmented, but it is not easy to create a workaround if the meaning of augmented is that top-level declaration, but you need to invoke an augmented member.

So you're in deep trouble if we do as you suggest and the opposite is needed, but it's not much of a problem if augmented is used to invoke the augmented declaration for an enclosing declaration whenever there is one.

jakemac53 commented 2 weeks ago

I don't have any argument against allowing these augmenting member declarations inside the original (non-augmenting) class-like declaration.

lrhn commented 2 weeks ago

Ack, you're right.

What is allowed is

class C {}
augment class C {
  void foo() {}
  augment void foo() {
    // Do more!
  }
}

That is, augmenting a declaration in the same class body where it's declared. That class body just have to be of an augmenting class declaration. I'll call that an omission, since I thought it was allowed.

Given that that is allowed, I think we should allow the same thing inside a non-augmenting class declaration. The specification of behavior should be consistent, so it's just a matter of removing a restriction.

jakemac53 commented 2 weeks ago

@eernstg could you take on this one? I will be out the next couple of days

eernstg commented 2 weeks ago

Yep, I'll do that.

eernstg commented 1 week ago

https://github.com/dart-lang/language/pull/3980