Lint the situation where a noSuchMethod thrower is introduced because of privacy

[eernstg]

[Edit 2024: Note that the implicitly induced member as of today will throw rather than invoke noSuchMethod. This just makes it even more important to introduce the lint as proposed, because it is even less likely that the implicitly induced member can be useful.]

Cf. #47148.

It seems to contradict the overall static safety of Dart that we silently allow a concrete class B to implement/extend a class A in a different library with an unimplemented private member _m:

In this situation, an implicitly induced member known as a noSuchMethod forwarder is added to B; this forwarder will invoke noSuchMethod on this, and pass some data to describe the invocation.

However, it is almost impossible to make that forwarder do anything other than throwing a NoSuchMethodError. In particular, we can't add a noSuchMethod implementation that does if (invocation.memberName == #_m) .. to B, because #_m, being a private symbol, isn't equal to the given member name, because that's the #_m of a different library.

For example:

// Library 'lib.dart'.
class A { void _m() {}}
void f(A a) => a._m();

// Library 'main.dart'.
import 'lib.dart';
class B implements A {} // No compile-time errors.
void main() => f(B()); // `NoSuchMethodError`.

This issue is a request for a lint that flags any class (like B in the example) where a noSuchMethod forwarder is induced because of the privacy rule.

[eernstg]

Recent changes to the language include a change to the semantics of these implicitly induced private members: They will throw, unconditionally, rather than forwarding to noSuchMethod.

The main reason for this is that it would otherwise be unsafe to promote certain private instance variables, cf. dart-lang/language#2020 and https://github.com/dart-lang/language/labels/field-promotion, and the ability to perform these promotions is given a high priority by the language team.

With the introduction of class modifiers, we can now use base on a class C in a library L to ensure that subtypes of C are subclasses of C or of some other declaration in L which is a subtype of C. In particular, as long as said superclass in C implements all its private methods it will be safe to call private members on any expression of type C.

This lint will then be useful in several ways, albeit indirectly:

• Any lint on a class D will indicate that there is a superclass C whose interface includes one or more private members which are not implemented. The next steps could then be (1) to use extends rather than implements on the superinterface path to C, and (2) notify the maintainers of C that they might want to make C a base class.
• Any lint on a class D that has a base superclass C whose interface includes one or more unimplemented private members could be taken as a hint that the library L of C is buggy: Every class C2 declared in L which can be used as a superclass from other libraries and which is a subtype of C should not have any unimplemented private members.

Note that it is not difficult to create this situation:

// Library 'lib.dart'.

base class C {
  void _m() {}
}

@reopen
abstract base class C2 implements C {
  void _m();
}

void foo(C c) => c._m();

// Some other library.
import 'lib.dart';

base class D extends C2 {}

void main() => foo(D()); // Throws.

@dart-lang/language-team, WDYT? Are you willing to give this lint request some support?

[munificent]

As I understand it, there's two ways to get a no such method exception from a call to a private member:

Implementing a class's interface in another library

// lib_a.dart
class C {
  void _m() {}
}

callM(C c) => c._m();

// lib_b.dart
class D implements C {}

main() {
  callM(D()); // Ka-boom.
}

Note that the only way this can happen is if lib_a.dart calls _m() on an instance of C that is not this. If you call _m() from within some method C, then it will always succeed because if you've inherited the method containing the call, then you've inherited _m() too.

Extending a class with an abstract method in another library

// lib_a.dart
abstract class C {
  void _m();

  callM() => _m();
}

// lib_b.dart
class D extends C {}

main() {
  D().callM(); // Ka-boom.
}

Note that this can happen even with calls to private methods on this.

In both cases, you have an instance invocation of a member that isn't actually there but the type checker doesn't catch it so it fails at runtime.

I do like the idea of a lint to guard against these. But I worry it will be hard to come up with a precise set of semantics for the lint that don't lead to a lot of false positives. In practice, this seems to not be a large pain point for users, so tolerance for false positives is probably very low.

Bad lint semantics

The obvious semantics for the first example would be to report a lint error if:

• A class allows being implemented externally.
• And declares any private members.

If both of those are true, it's possible for a non-this call to the member to fail.

But I am sure that rule would have a ton of false positives. In my own code, I often define a class that I use as a public interface but that also has its own private implementation. It only accesses private members on this, so it's safe and harmless to do so.

Better lint semantics

Since it is the private member access that fails, that suggests that we should lint on those instead. So maybe report a lint error on a private member access where:

• The class allows being implemented externally and the access is not on this (the first example).

• The class allows being extended externally and the declaration of the private member is abstract (the second example).

Note that with this, the lint would only ever fire in the library where the private members are declared, not in the other libraries that might be extending or implementing these classes. I think that's probably the right place for it.

To fix the lint, the library author would have to mark the class base, final, interface, or sealed (depending on whether access is on this or not) and/or make the abstract member non-abstract.

I still worry that this lint would have a lot of annoying false positives, but it would be interesting to look into. I'm not sure if it's worth putting any significant effort into it, though.

[eernstg]

That's a nice analysis, @munificent! I agree that we're addressing the root cause by linting the class that declares a private member and allows external implements relations, and the abstract class that has unimplemented private members and allows any kind of external subtypes.

However, I'm suggesting that we also lint the other end of the relationship: We should lint any class that has a throwing noSuchMethod stub due to privacy. That is in a very real sense the root cause of the throwing behavior! The associated static analysis is very well-established (the compilers will generate those throwing stubs in the first place, so of course we know exactly on which classes they will exist).

Moreover, this lint is also actionable in many cases. With implements, the maintainer of D may very well be able to change implements to extends:

// lib_a.dart
class C {
  void _m() {}
}

callM(C c) => c._m();

// lib_b.dart
class D extends C {} // Lint is now gone.

main() {
  callM(D()); // OK!
}

With the other scenario, the external class D might be able to extend some other class than C where no private members are unimplemented. In any case, the maintainers of D might very well want to know that they're asking for a run-time error as long as they declare D in a way that causes a throwing stub to be generated.

I think it would be nice to have lints on all three: Consider a class C in a library L with a member _m which is private to L:

• Lint C if _m is unimplemented, and C can have any non-bottom subtypes outside L.
• Lint C if _m is called on any receiver other than this, and C can be implemented outside L.
• Lint each class D declared outside L that has a throwing noSuchMethod stub for _m due to privacy, unless _m is implemented by every subtype of C in L that can have external subtypes, and every invocation of _m has the receiver this.

I made the lint on the throwing stub a bit more complex in order to avoid false positives. We can of course cut down on the complexity by approximating this rule in the safe direction (e.g., it's simpler to just require that _m is not abstract on any type in L).

To address the first lint, we can implement _m or prevent the subtypes (make C private, check typedefs and indirect subtypes). With the second lint, make C a base class. With the third lint, change the superinterface relationship with C.

[lrhn]

I'm with @munificent here: The problem is the private member invocation. If all such invocations are on this, there is no problem.
Making it someone else's problem that you have a library private member is putting blame where it cannot be addressed, and exposing details that the user should never need to worry about.
One of the two primary purposes of library private declarations is to be hidden and safe, to not be able to cause name conflicts and make sure other libraries don't need to care about the name at all. (The other purpose is to prevent access, but often it's not as much "prevent" as "pretend it's not even there").

I'd be very annoyed if a private member in another library causes any warnings in my library. I'd ask the other library author to fix that, but that's all I can do.
The warning really should be in the original library, telling the author that it would give warnings in other libraries if we had the other approach. So they can fix it immediately, not wait until someone else gets the problem and comes back and asks for a fix.

I'm suggesting that we also lint the other end of the relationship: We should lint any class that has a throwing noSuchMethod stub due to privacy.

Why?
If it won't be called, then it doesn't matter.
If it might be called, we already told the original author of that invocation that it might hit a throwing implementation.

Who benefits from getting a warning in the subclass?
(Rule of thumb: Don't give a warning that isn't actionable.)

It's true that in some situations, you can fix the "problem" by using extends instead of implements, but we don't know that in general.

I'd go with just:

• Lint the invocation, if the target can have a throwing stub as implementation of that method.

"Can have a throwing stub" is the case if:

• The target is not this or super, and:
  • The target type, or any subtype in the current library, can be implemented outside of the library.
  • The target type, or any subtype in the current library, has an abstract implementation, and can be
    inherited (extended or mixed in) outside of the library.
  • The "can be implemented"/"can be inherited" can be based on interface/base/final modifiers,
    but it can also be based on privacy. A private subclass cannot be implemented or inherited.
  • A lint can also be even more clever, and distinguish same-package and other-package uses.
    - If the target type, or any subtype, is not exposed in the package API, then it's considered package private.
    - Then we don't give any warnings at the invocation point.
    - Instead we warn only if there is a subclass in the same package which does get a throwing implementation. Because we assume that we can see all subclasses.
    - If a type is exposed in the package API, then we give the warning at the invocation as normal, if that type (or any subtype) can be implemented or inherited-and-is-abstract, because we don't assume we can see all subclasses.
• The target is this, and the method is abstract in any subclass which can be extended outside of the library.
• The target is super in a mixin declaration, and the super-interface type, or any subtype in the current library, can be implemented in another library, or can be inherited in another library, and has an abstract member.

(That is, generally I'd let the lint consider which cases are safe and which are not. It's allowed to use information that we won't have in the language specification, so I don't want to be too precise in defining when there is a problem, if some other available information proves that the problem isn't real.)

[eernstg]

OK, we all agree on the nature of the lints in the library that declares the private member. That's great! I'm sure we can sort out the details.

The remaining part is that we don't agree on giving developers a heads-up at the point where they actually cause the throwing stub to be created.

If it might be called, we already told the original author of that invocation that it might hit a throwing implementation.

I think that's a bit too optimistic.

It is not a given that every organization/developer is able and willing to change every public class C with a private member to be a base (or stronger) class. It is also not a given that they are enabling these lints, so maybe they don't notice the issue in the first place. That is, the fact that we might hit a throwing implementation could be a known issue with no solution, not at this time anyway.

Still, we refuse to tell the developer who has a class D implements C {...} with a throwing noSuchMethod stub that they set themselves up for a run-time error (or they set up somebody else if D is imported by others). (Yes, we can have extra checks that it can actually be called, and that's good because it eliminates some false positives).

The developer of D might, for example, be able to make it class D extends C {...} (or even class D with M implements C {...}, if the maintainers of C decided that they could provide a mixin M that implements those private members in some benign manner). Or perhaps the maintainers of D don't absolutely need to have that class in the first place. So there are several reasons why this heads-up could be actionable, and I don't see how it could be helpful to deliberately hide the issue.

I'd be very annoyed if a private member in another library causes any warnings in my library.

You can just choose to say that it's a warning about using implements, or a warning against choosing that particular class as your superclass. There's probably no need to spell out which private members are causing this danger because the mitigations do not depend on that, they are just based on the fact that there is a non-empty set of private members causing this potential run-time error.

[bwilkerson]

It sounds like this lint might have a lot of false positives, and that makes me wonder whether a lint is the right way to address this problem.

Not all lints that discover ways that a runtime exception might occur are good to implement. If the probability of the exception is high enough, then finding it while writing the code can be very helpful. But if it's rare enough, then we might consider just improving the exception message to make it easier to debug. For example, could the message explain to the user that the problem is that a method was invoked on a subclass that caused a private, but unimplementable, method to be invoked, possibly with a link to documentation that describes the issue in more detail and suggests possible ways to fix the bug? Given that we're producing the stub at compile time we could presumably gather any information that the lint could while composing the message.

It has the disadvantage that it might not be found if that code path isn't tested, but the advantage that there are no false positives.

Also, the two options are not mutually exclusive. Given that users can disable lints, improvements to the exception message seem advisable anyway.

[munificent]

Still, we refuse to tell the developer who has a class D implements C {...} with a throwing noSuchMethod stub that they set themselves up for a run-time error (or they set up somebody else if D is imported by others). (Yes, we can have extra checks that it can actually be called, and that's good because it eliminates some false positives).

I think the problem with this approach is how code evolves over time. Imagine:

1. Author Cameron creates class C { ... } which has no private members.
2. Author Dana creates class D implements C { ... }. No error since no throwing noSuchMethod is created.
3. Later, Cameron adds a private member to C. Now class D has a compile error.

I think it's an important invariant that adding a private member to a library shouldn't cause compile errors to appear in downstream code.

[eernstg]

@munificent wrote:

I think it's an important invariant that adding a private member to a library shouldn't cause compile errors to appear in downstream code.

I think a compile-time error is preferable to a run-time error, and it isn't a safe bet that "maybe nobody will call that method, at least for a while". ;-)