Should having a concrete private member which does not implement the super interface signature(s) be a static error?

[leafpetersen]

This issue points out a discrepancy in the static errors emitted by the CFE and the analyzer. The question raised is relevant to the question of how to deal with field promotion as well.

I filed an issue to explore the question of whether to require the existence of a combined member signature for inaccessible members, which is relevant to the resolution of this issue as well.

Consider the following program:

//lib.dart
class A {
  void _m(int i) {}
}

abstract class B implements A {
  void _m(num n);
}

// example.dart
import "lib.dart";

class C extends A implements B {}

void main() => C();

This code is currently accepted by the analyzer, and rejected by the CFE. Per @eernstg , section 10.2.2 (Forced by Privacy) applies here, and a noSuchMethod forwarder should be generated, and per the same section, it is not an error for the noSuchMethod forwarder to override a concrete method (as it otherwise would be).

Notably, it is not specified (as best I can tell) what the signature of that noSuchMethod forwarder should be. Depending on the resolution of #2263 that may be obvious (the combined member signature) or not at all obvious. Per current spec, I believe there is not necessarily a well-defined choice, since we do not require a combined member signature to exist.

TODO: @eernstg somewhere in the spec we must specify the check which enforces that in a concrete class, the inherited members satisfy their interfaces also doesn't apply for inaccessible members, right? Since we don't require that for abstract classes, e.g. we could could B extend A above and nothing would change here. Presumably this also filters by accessibility?

The question for this issue is whether we should continue to make an exception to the rule against overriding concrete methods with noSuchMethod forwarders for the case of inaccessible members.

For the purposes of #2020 we would of course have to insist that any such forward always throw.

It seems surprising to me, and not especially helpful, that an instance of A can exist for which calling _m does not reach the implementation specified in A. This seems contrary to the idea of library privacy.

cc @eernstg @lrhn @munificent @jakemac53 @natebosch @chloestefantsova @johnniwinther @scheglov

[leafpetersen]

cc @stereotype441

[eernstg]

somewhere in the spec we must specify the check which enforces that
in a concrete class, the inherited members satisfy their interfaces also
doesn't apply for inaccessible members

I believe this is covered by the fact that the language specification section 'Fully Implementing an Interface' only requires that a concrete class C must implement each member which is accessible to the library where C is declared.

It leaves the inaccessible ones up to implicitly induced noSuchMethod forwarders (some of which will soon be throwers). I commented on the treatment of incorrect overrides in #2263.

[we could let] B extend A above and nothing would change

That's my understanding, too.

Presumably this also filters by accessibility?

The only difficulty here is that C has _m-in-lib.dart in its interface with signature void _m(num), and it does not have an implementation of that signature. So this would 'force' a noSuchMethod forwarder/thrower 'by privacy', and I don't think the specification is particularly vague on that. Would that be the relevant meaning of 'filter by accessibility'?

However, I certainly think it could be useful to lint every class that has an implicitly induced noSuchMethod forwarder/thrower, especially if it does not have a non-trivial noSuchMethod.

[lrhn]

I'm vary about touching this area without doing a large overhaul of the entire idea.
We pretend private members of other libraries do not exist. We then do whatever it takes to ensure that you don't get errors related to those members, because that breaks the abstraction that they aren't there.

I think we should do our darndest to keep the abstraction until such time as we are ready to change to a different model entirely. I don't know what that model would be.
Maybe it'll just be disallowing you from making classes with private members implementable or public-and-extensible (if we have class capabilities). Maybe allow overloading.

That said, we are hitting the edges of the current approach, and need to do something in the cases where we can't just add a NSM forwarder/thrower.

The way we do things now, we do need to make the _m member a member of the interface of C, which means we need to choose a signature. We may not have specified what the signature should be, but that's fixable.
If nothing else, we can choose void _m(Object? _) => throw NoSuchMethodError(...).

The real problem comes when we have:

// library a
abstract class A {
  void _f([int x]);
}
abstract class B {
 void _f({int x});
}
/// library b
class C implements A, B {} // No unified interface.

We can't combine the interfaces and have both optional positional and named parameters.

Even worse for

abstract class A {
  void _f();
}
abstract class B {
  abstract int _f;
}

In that case, we just can't create a valid interface containing both. (Not without overloading, at least, they need to be different V-table entries.)

So, if we can't prevent errors totally, where should we draw the line? salvage what can be salvaged, or push the line back a little? (And which criterions should be base that choice on?)

That calling someA._m() doesn't reach A._m may seem surprising, but there are other compatible _m declarations in the same library. One in B, and likely in at least one concrete class implementing B. The problem is that the author of A thought that _m in A was different from _m in B. It's not, it's the precise same name, and B._,m even has a compatible signature, and both A and B are public classes.

Should we warn authors about situations like that? Distinct private declarations in the same library has the same private name (not one introduced in a shared supertype) would give a warning. The workaround is to rename one of them, or give them a shared supertype. If people do that, their users won't get conflicts, and we won't have to worry as much about making such conflicts into errors.

That still won't help with:

class A {
  void _m(int x) {}
}
abstract class B extends A {
  void _m([int x]);
}
/// other library
class C extends B {}

That's a shared supertype, and a public abstract extensible class which cannot be validly extended from outside the library.
I think we should warn about that too.

So, maybe I'd recommend:

• Warn if the same private name is used in two separate public class or mixin declarations, and they don't both inherit it from the same superinterface. (Don't let unrelated publicly exposed private members conflict.)
• Warn if the same private name is used in two separate public class or mixin declarations, and they both inherit it from the same superinterface, and their types are not mutual subtypes. (Don't specialized publicly exposed private members.)
• Warn if a public mixin or abstract class is publicly extensible (public generative constructor) and has a private interface member with not valid implementation.
• Then make it an error for a class to inherit incompatible private members from another library.

If the interface/class/mixin supplying library is warning free, you can still implement a single interface containing public methods (you get a NSM-forwarder/thrower), you can always extend a single class or mixin without issue, and if you try to combine two completely unrelated types, they won't conflict.

[leafpetersen]

Ok, I think we're coming down on the side of answering the question from this issue with a "no": that is, when a concrete private member does not correctly implement its signature in the interface of the class, we introduce a noSuchMethod thrower, rather than a static error. The separate question of what the "its signature in the interface of the class" is (and what to do when it doesn't exist) is covered in #2263 .