[Extension type] Add a `@redeclare` annotation #53121

New issue

Closed

[Extension type] Add a @redeclare annotation#53121

Assignees

Labels

P3devexp-pkg-metaimplementationlegacy-area-analyzertype-enhancement

Extension types don't override but rather redeclare so we should probably discourage/disallow the use of the @override annotation on extension type members and provide an alternative annotation such as @redeclare to communicate when an extension type member is intended to be redeclaring. (Along with that we should consider a lint, like unnecessary_redeclare to flag misplaced annotations (e.g., like unnecesssary_overrides).

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

added

mentioned this

[Extension type] linter support #58838

self-assigned this

on Aug 3, 2023

mit-mit

Member

Sorry, what's the difference between overriding and redeclaring?

lrhn

Member

Overriding is virtual, and replaces the existing method on all instances of the subclass, whether they are known to have the subclass interface or not. An override must have a compatible signature.

A redeclaration doesn't need to have a compatible signature. It shadows the original when declared on an extension type which also implements a superior with a same-named member.

The former is providing a specialized version of the overridden member, the latter is just providing a completely new member with the same name.

The idea behind the warning here would be that the name collision might be accidental, and by making you provide an annotation, you can state that it was deliberate. Then we can warn you if you make an accidental shadowing, which is easier to do than an accidental override, since you don't have to match the signature, and we can warn you if you claim to be shadowing something, but isn't.

I'm worried about the latter. Shadowing is unrelated to the thing it shadows, so if that goes away, it really only needs to be an info that the annotation is unnecessary. It doesn't change the meaning of the used-to-be-shadowing member. Making it a warning, which some will then make an error, is more fragile than necessary. Or make it a lint, so it's opt-in, unlike the unnecessary override warning.

For the unannotated redeclaration, that could be a lint to, maybe the same one endless both. I won't put the redeclaration value in the platform libraries, so it would go in package:meta.

Also consider allowing the use of override, which then requires it to have a compatible member signature to at least one member its shadowing.

mit-mit

Member

Got it. @redeclare wasn't a term I'd heard before. Did we consider something like @shadow ?

lrhn

Member

I'm not sure where the "redeclare" name comes from. I'm not even sure we need the annotation, but if there is an audience for it, it's definitely a lint we can provide.

I think "shadow" is a little too specific, and transitive (I'd expect it to say what it shadows).

(If we allowed new on a declaration, to make it not introduce a virtual override, the opposite of override, it would be fitting here.)

bwilkerson

Member

I'm not sure where the "redeclare" name comes from.

I comes from the feature specification, which says:

Hence, we use a different word to describe the relationship between a member named m of a superinterface, and a member named m which is declared by the subinterface: We say that the latter redeclares the former.

eernstg

Member

@pq wrote:

we should probably discourage/disallow the use of the @override annotation on extension type members and provide an alternative annotation such as @redeclare

That would be great!

I do think a case can be made for the ability to announce explicitly that a given redeclaration relationship is intended.

In particular, it seems very plausible to me that some extension type hierarchies will be designed such that redeclarations never occur. Indeed, several of the early proposals about extension types (known as views at the time, I think) enforced this property by making every redeclaration a compile-time error.

The motivation for avoiding redeclarations would be that a hierarchy with no redeclarations will behave in a manner which is more like the behavior of a class hierarchy: Assume that e has static type T and S is a supertype of T, assume that T has a method named m and S also has a member named m. The latter is then also a method, and e.m() behaves exactly the same as (e as S).m(). In other words, behaviors are preserved across an upcast; with regular classes this is ensured because of late binding, and with extension types with no redeclarations it is ensured because late binding would be a no-op (the most specific implementation of any member is always the statically known one), so it doesn't matter that extension types don't have late binding.

Developers and organizations who wish to outlaw redeclarations could then enable the lint and check that @redeclare is never used.

Conversely, other developers/organizations might prefer to allow redeclarations. In this case it would make sense to enable the lint and have the @redeclare annotation on every redeclaration. This would serve as a reminder for anyone who looks up the declaration, because they need to remember that e.m() and (e as S).m() (where e has type T and T <: S as above) may run two completely unrelated pieces of code (so it's not a good idea to assume that "I already know what m will do").

All in all, I think it's fair to say that @redeclare is similar to @override, but sufficiently different to justify a firm distinction (that is, using a different annotation). Also, @redeclare is arguably even more justified than @override because there is no guarantee that a redeclaration and the redeclared declaration (that is, the "overriding" respectively "overridden" declaration) are related in any way, and we can't rely on late binding to ensure that "we always get the right one".

bwilkerson

Member

A minor nit, unless someone cares deeply. :-) The approach we've consistently taken in the analyzer is that any diagnostics related to annotations are considered to be warnings (enabled by default) because the user has explicitly told us what their expectations are. If we add a declaration of redeclare, then users will automatically get diagnostics when a redeclaration occurs without the annotation or when the annotation is used where it shouldn't be, such as on a non-member declaration or a member that doesn't redeclare anything from a supertype.

But if we want a diagnostic that prohibits the use of redeclare, then that would be a lint (disabled by default) because we'd want the user to explicitly opt in to that prohibition.

lrhn

Member

That sounds fine to me: A warning to have @redeclare on something which doesn't actually shadow anything, and a lint to warn if you lack a @redeclare on something which does shadow, or if you shadow at all (whether with or without @redeclare`, it won't save you from "no_redeclaratons").

eernstg

Member

OK, // ignore: no_redeclarations would still be available for the rare exception where someone wants to be saved. ;-)

MemberAuthor

What a great discussion. Thanks for all the added consideration!

I'll take a look at the annotation and diagnostic/lint work.

Adding @MaryaBelanger for context: brace yourself for some doc reviews! 😉

changed the title ~~[-][Extension type] Consider a `@redeclare` annotation[/-]~~ [Extension type] Add a `@redeclare` annotation

on Aug 7, 2023

MemberAuthor

Developers and organizations who wish to outlaw redeclarations could then enable the lint and check that @redeclare is never used.

Do we anticipate a use case where only some redeclarations are meant to be disallowed? That is, should we allow specific declarations to be non-redeclarable? (Something analogous to @nonVirtual?)

bwilkerson

Member

While that's certainly possible, my guess is that we should wait to add anything similar to nonVirtual until we have clear evidence that users want it. I might say the same for the lint, honestly. But I do think that we want redeclare when we release the feature.

eernstg

Member

Do we anticipate a use case where only some redeclarations are meant to be disallowed?

I think this would be more like a type hierarchy thing than a member declaration thing: With no_redeclarations, developers would be able to rely on a simpler model of the software ("there's nothing special about extension type instance member invocation"). If redeclarations exist then the model would have to be more elaborate ("extension type instance members are resolved statically, so you need to be careful about the static type of e when making calls like e.foo()").

In particular, with no_redeclarations it is probably feasible for clients to ignore the fact that a given type hierarchy is expressed using extension types rather than classes. In contrast, developers who are using an extension type hierarchy where some members are redeclared will need to understand that those members are resolved statically (so they can't rely on the language semantics to choose the right implementation for the given receiver, they're going to get exactly the one which is known at compile time at the call site).

So it probably doesn't make much sense to avoid this kind of complexity on a per-member basis, it's more like a package level or organizational level decision. A lint which is enabled/disabled on a per-package basis would probably be fine for that purpose, and @nonRedeclared probably won't be very helpful.

Finally, it should be noted that we've had extension declarations for years, and they are statically resolved, and I've only rarely heard any complaints about surprising "overrides":

// This is the one we know about.
extension on num {
  void foo() => print("Good ole foo won't hurt a fly!");
}

// This one slipped under the radar.
extension on int {
  void foo() => print("Hehe! I'll swat that one!");
}

void main() {
  num n = 1;
  n.foo(); // All flies ok.
  if (n is! int) return;
  n.foo(); // Oops!
}

Of course, what I really want here is IDE support for dispatch visualization: Statically resolved invocations have pale blue background color, OO dispatched invocations have pale green background color, and dynamic invocations have blinking red background color. ;-)