dart-lang / language

Design of the Dart language
Other
2.64k stars 198 forks source link

Augmentations allowing static and instance members with the same name. #3936

Closed lrhn closed 2 weeks ago

lrhn commented 1 month ago

Dart disallows two declarations with the same name in the same scope. With augmentations, as soon specified (#3800), the lexical scope of an augment class C is not the same as the scope of its parent declartion class C.

That may or may not allow (depending on how things happen to be written for a language where this distinction didn't exist) something like:

class C {
  int get foo => 42;
}
augment class C {
  static int get foo => 37;
}
void main() {
  print(C.foo + C().foo);
}

It might not be allowed if the rules are written as "a class may not have a static member and an instance member with the same base name". It may be allowed if the rules rely on the catch-all rule of "no two declarations with the same name in the same scope".

We shouldn't rely on accidents of specification, so we should figure out whether we want to allow it, and then make sure the specification matches.

That is: Should different augmenting declarations of the same name-space declaration be allowed to have instance and static members with the same name, as long as they are not in the same lexical scope?

(I say yes, see #1711. If we allow static extensions, #723, then it's also likely that we'll indirectly allow people to effectively have statics and instance member with the same name. Or even today, they can declare a static member and an extension instance member with the same name. Using extensions risks not having them imported, absent a "sticky extension" feature. So we should allow a good way to get the desired effect, rather than only a slightly worse way that people will use then.)

jakemac53 commented 1 month ago

Should we just allow this generally, even within a regular class declaration?

The main reason I would push for "no" as the answer here is it seems very weird if augmentations allow you to do something that can't otherwise be done.

Hopefully, nobody ever does this though because it is highly confusing. But, it isn't any worse than having extension methods which collide with instance methods (which I have seen before, fwiw, and it cost me several hours of debugging a null safety migration which changed the static type of a declaration which altered which method was called).

lrhn commented 1 month ago

Should we just allow this generally, even within a regular class declaration?

IMO, yes. If someone wants to do it, they can use extensions for the instance member today, static extensions for the static member soon enough, and maybe augmentations too, unless we make sure to disallow it. Might as well allow them to write it directly, without the overhead and risk of not having the extension imported.

(My elevator pitch summary of #1711: Generally allowed, applies to all static-allowing namespaces. Constructors and statics still conflict. If both static and instance member declared in the same scope, the name is considered "conflicted" in that lexical scope, just like conflicting imports - only error if referenced by unqualified name. All other accesses are unambiguous about being static or instance.)

What it would allow is something like:

class Color {
  static const black = Color(0, 0, 0);
  static const red = Color(255, 0, 0);
  static const green = Color(0, 255, 0);
  static const blue = Color(0, 0, 255);
  // ...
  static const white = Color(255, 255, 255);

  final int red, green, blue;
  const Color(this.red, this.green, this.blue);

  String toString() => "#${_toHex2(this.red)}${_toHex2(this.green)}${_toHex2(this.blue)}"; // `this.` needed
}
jakemac53 commented 1 month ago

Good point that true static methods don't have the same issues as extension methods since you have to go explicitly through the type anyways as soon as you are outside the lexical scope. SGTM

jakemac53 commented 1 month ago

For posterity, I remembered the exact situation with extension methods which caused me a problem previously.

There was a variable who's type was originally dynamic, so it was just doing an instance member call. As a part of the migration I changed something so that it had a better type, which was a super type of the actual (runtime) type. There was an extension member on that type, lets call the member x. But, the actual (runtime) type of the object also had a member x. Before the better type, it called the instance member, and afterwords it called the extension member. Quite the edge cases really, and doesn't apply to this situation.

jakemac53 commented 1 month ago

I do worry that specifying this is allowed only for augmentations might break some backends which assume this isn't possible. I am not opposed to it in principle though.

lrhn commented 1 month ago

If we don't allow it in general, we should also not allow it with augmentations. That means explicitly saying that it's an error for a fully augmented class/etc. declaration to contain two declarations with the same base name unless:

(Base or augementing declarations must also not contain a member or type parameter with the same base name as the class, other than the unnamed constructor, or a member with the same base name as a type parameter. That can be checked locally since the declaration must repeat the class name and type parameter names. Or we can check that post-augmentation too. It's probably easier to do everything at once.)

jakemac53 commented 1 month ago

That sounds fine to me (and we can later loosen it if we generally allow it)