[Extension types] diagnostic when implementing a class with a conflicting member

[pq]

Consider the following:

extension type E(C c) implements C {}

class C {
  void c() {}
}

Can you spot the problem?

The issue becomes apparent when you try and invoke c():

void main() {
  E e = E(C());
  e.c();
}

and you see an invocation_of_non_function_expression diagnostic.

Potentially I see two possible improvements:

1. a warning on E, nudging you to rename c to not collide with void c(), and
2. an improved message on the invocation of .c()

(For 2, I'm thinking something more like how we report field and method conflicts CONFLICTING_FIELD_AND_METHOD.)

/cc @bwilkerson @scheglov @srawlins @dart-lang/language-team

[bwilkerson]

Assuming that the spec doesn't already dictate a diagnostic like CONFLICTING_FIELD_AND_METHOD, I think it ought to. (This might be an incomplete part of the support in analyzer.)

Though I think it wants to be reported on either the declaration of the field c or on the C in the implements clause, depending on the scoping rules, which matches option 1 rather than 2.

I don't like option 2 because e.c() could be valid (such as when e.c returns a callable object), and hence it wouldn't always report the potential error.

[eernstg]

In general, an extension type can support implicit "forwarding" of member accesses using implements T where T is a non-extension type which is a supertype of the representation type. However, the extension type can also redeclare any of those "otherwise forwarded" members by declaring something that has the same name. (No requirements about being a correct override, because it's always known at compile time which declaration is used.) That's what we have here, because C has a member named c, and E also has a member named c (the getter that returns the representation object, also known as the representation variable), and the latter redeclares the former.

This situation could be considered to be particularly confusing because we don't have a normal declaration of an instance variable (like the ones we had in several earlier versions of the extension types feature), and hence the representation variable looks like an exception.

However, if we have a lint that flags redeclarations with no @redeclare metadata then it could also flag this representation variable, and then we probably don't need to do more in order to point out the surprising properties of E.c.

[eernstg]

@bwilkerson wrote:

[reporting] ... CONFLICTING_FIELD_AND_METHOD, I think it ought to.

Extension types have all the normal kinds of conflict diagnostics (regular name clashes in the same body scope plus 'class member conflicts' like having an (inherited) instance member with basename n and declaring a static member with basename n, etc.).

The difference between classes/mixins and extension types is that the latter don't require an override relationship when an instance member declaration named n is a redeclaration (that is, some superinterface has an instance member named n).

We could lint situations where a redeclaration is particularly different from what is allowed in a class/mixin, e.g., when a method is redeclared by a getter or vice versa. We could even lint the situation where a redeclaration wouldn't be a correct override if it had occurred in a class/mixin (for instance, when int foo(int) redeclares String foo(num, num), or even when it redeclares int foo(num)).

[lrhn]

Assuming that the spec doesn't already dictate a diagnostic like CONFLICTING_FIELD_AND_METHOD, I think it ought to. (This might be an incomplete part of the support in analyzer.)

As Erik says, there is no conflict here, it's just a shadowing declaration in the extension type, unrelated to the inherited member it shadows. The representation object getter is just a normal extension type member, could be implemented as RepType get repValue => this as RepType;, so it shouldn't need any special casing or rules.

So no error, and no intent to make it an error.

The "annotate-redeclares" lint should cover this just like any other extension member introduced by the extension type declaration..

[bwilkerson]

Thanks. I know that members of the extension type don't override members from inherited types; I regularly remind other people of that fact. But it seems that knowing this doesn't keep me from being tripped up occasionally. I have to wonder how often users will be confused by the semantics.

But I do like the idea of making this part of the annotate_redeclares lint rather than adding a new lint for this one case.

[jacob314]

Putting the @redeclare annotation on the primary constructor looks a little cryptic but I suppose is something we just need to get used to it if Dart supports primary constructors on regular Dart classes in the future. Similarly, @override would need to apply to fields defined by primary constructors for regular classes.

extension type E(@redeclare C c) implements C {}

class C {
  void c() {}
}

[eernstg]

Putting the @redeclare annotation on the primary constructor looks a little cryptic

I think we could get used to that (if it is used at all in practice). Hopefully it will be used very rarely in practice. As @lrhn mentioned, a developer who really wants to access the representation object using a redeclared name could just use another getter:

extension type E(C it) implements C {
  /// ... explaining why this naming is a good idea ...
  @redeclare
  C get c => it;
}

class C {
  void c() {}
}

[srawlins]

Hi @pq, as noted above, it looks like this is not a conflict, but we could enforce the lint rule about @redeclare in such code. Do you know if this is enforced?

[pq]

Thanks for following up on this! I don't believe it is enforced. Seems reasonable though.

[bwilkerson]

We do have the annotation defined and we warn if it's used where it shouldn't be, but I don't see an error code for flagging redeclaring members that aren't annotated.

I think we want to finish this arc of work before the feature ships.

[lrhn]

I believe the warning should be enabled by a lint, probably something like annotate_redeclares, the annotation is there to silence the lint warning.

[bwilkerson]

You're right, that's what we agreed on and what was implemented. In which case this issue is complete.