Documentation for redeclared member is wrong

[Cat-sushi]

import 'package:meta/meta.dart';

extension type I64(int value) implements int {
  @redeclare
  bool get isEven => value.isOdd; // <--
}

bool get isEven
Returns true if and only if this integer is even.

Copied from int.

DartPad Based on Flutter 3.19.0-0.3.pre Dart SDK 3.3.0-279.2.beta

[bwilkerson]

@eernstg @srawlins

While it's true that I64.isEven doesn't override the method from int (hence the desire for redeclare), it does seem like most of the time it will be appropriate to copy the documentation. This example seems analogous to

class IntList implements List<int> {
  @override
  bool get isEmpty => length > 0;
}

Even though the semantics of the getter has been changed, we'd still copy down the documentation in this case. We never have any guarantee that the doc comment for an overridden member will be appropriate for an overriding member, it is appropriate often enough to make "inheriting" it a useful default. Is there a reason why the same reasoning shouldn't be applied for extension types?

[srawlins]

it does seem like most of the time it will be appropriate to copy the documentation

I don't understand this. My understanding is that redeclarations have almost nothing to do with the method they are "redeclaring." The only thing being redeclared is the name. The signature can be completely different; different parameters, different return type. A method can be "re-declared" as a getter; a getter can be "re-declared" as a method. All of this works:

import 'package:meta/meta.dart';

extension type I64(int value) implements int {
  @redeclare
  bool get isEven => value.isOdd; // <--

  @redeclare
  String isOdd(int y) => 'yes';

  int get gcd => 7;
}

void main() {
  var i = I64(7);
  print(i.isEven);
  print(i.isOdd(6));
  print(i.gcd);
}

So I think the documentation should not be "inherited" on a redeclared member.

I may be forgetting the intended purpose of redeclarations; if the idea is to allow not just a redeclaration of a member with a shared name, but to do so for the purpose of overriding, or re-implementing a member, then I guess we should keep the previously-declared documentation. Pretty grungy though when redeclaring a method as getter, etc.

[bwilkerson]

Oh, right. I hate that we chose to use implements; the difference in semantics between extension types and any other types always trips me up.

[eernstg]

@bwilkerson wrote:

most of the time it will be appropriate to copy the documentation

I agree on this. It is true that there are no technical constraints on a redeclaration similar to the 'correct override' relation which is required for an overriding declaration, but I would definitely expect redeclarations to preserve the purpose and obligations of the redeclared member to a high degree and in almost every case. It would surely give rise to a lot of confusion if redeclarations would use an existing ("redeclared") member name for a completely unrelated purpose.

The ability to redeclare a member with a member signature that isn't a correct override can be used to adjust the signature for a given purpose. For example:

import 'dart:async' as async show Future;
import 'dart:async' hide Future;

extension type Future<T>(async.Future<T> _it) implements async.Future<T> {
  Future<R> then<R>(
    FutureOr<R> Function(T value) onValue, {
    T Function(Object, StackTrace)? onError,
  }) =>
      Future(_it.then(onValue, onError: onError));
}

In this case we've adjusted the member signature of then such that the onError parameter is strictly typed. This is not a correct override, but we can do it with an extension type because it isn't technically necessary to have a correct override relation. All other members of async.Future are available as usual, due to implements async.Future<T>.

We can't enforce this kind of "conceptually correct override" relation, but I would expect actual usage to strive for that kind of relationship.

I don't know if that's enough to justify documentation "inheritance", but it seems likely to me that it would work OK in many cases.

[srawlins]

Sounds good. Closing as working-as-intended.

[Cat-sushi]

Anyway, it isn't copied from int.

[eernstg]

We have the following in int.dart:

abstract final class int extends num {
  ...
  /// Returns true if and only if this integer is even.
  bool get isEven;
  ..
}

So how do you know that the text that you mentioned (which is character-by-character identical) wasn't copied from int? I don't see any reason to claim that it wasn't...

[Cat-sushi]

The behavior isn't copied from int.

[Cat-sushi]

So, the documentation is simply wrong.

[eernstg]

The behavior isn't copied from int. So, the documentation is simply wrong.

Ah, but that's your fault!

The default behavior (copying the DartDoc text from the overridden declaration) makes sense because it is usually correct. Like any other default, it's your responsibility as a developer who writes a declaration like I64.isEven to provide a non-default value when the default is wrong.

Also, please don't write a declaration like I64.isEven in the first place. :smile:

[Cat-sushi]

import 'package:meta/meta.dart';

extension type I64(int value) implements int {
  /// lying
  @redeclare
  bool get isEven => value.isOdd;
}

bool get isEven
lying

Is it also my and datdoc's fault?

[Cat-sushi]

If so, the SDK shouldn't allow any redeclaration.

[eernstg]

There's no way to stop a dedicated developer who wants to make any given member declaration behave in a way that violates the expectations associated with the same member name in supertypes. That's purely a matter of good judgment and honest intentions (or the lack thereof). The Liskov substitution principle basically states that it is always a bad idea to violate such supertype-based expectations, and I believe this is something that we're depending on basically many times per line in each and every OO software artifact, including every Dart library. Still, it's a matter of human judgment and not something which can be determined in a formal mathematical sense.

We do maintain strict rules when it comes to the member signature of an overriding declaration, but that's because it affects soundness, and soundness must be a strictly satisfied property of Dart and its implementations (nobody in the language team wants to deviate from this). Hence, Dart tools will never be so permissive that it allows soundness to be violated.

Other than that, it is up to developers to write code that satisfies reasonable expectations, including the Liskov principle, to some high but realistic degree, and one of the consequences is that we should never have a declaration like I64.isEven that returns isOdd. It's misleading, it's not going to help anybody, just don't do it. :smile:

If so, the SDK shouldn't allow any redeclaration.

I don't think there is anything special about the SDK, any particular library could contain business logic which would obviously never be in the SDK, but it could still be absolutely crucial for your software. In other words "is in the SDK" is not the same thing as "is important".

So if we think redeclarations are too dangerous in general then we shouldn't have them.

However, it is consistent with many other parts of Dart as a language that redeclarations are supported, and they are quite permissive, and it is very important that developers use this plethora of freedom responsibly.

That's what I meant when I said that it's "your fault". We trust each other to do the right thing in a large number of ways. One of them is to maintain some notion of semantic consistency between overridden and overriding declarations as well as redeclared and redeclaring declarations.

Based on the assumption that this is already true, it makes sense to say that DartDoc comments are inherited by default.