Skip to content

[Class Modifiers] Why do we propagate base through implements edges. #2909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
leafpetersen opened this issue Mar 14, 2023 · 6 comments
Closed
Labels
class-modifiers Issues related to "base", "final", "interface", and "mixin" modifiers on classes and mixins.

Comments

@leafpetersen
Copy link
Member

In the section on transitive restrictions, the class modifiers proposal says the following:

This proposal takes the last option where types have exactly the restrictions they declare but a lint can be turned on for users who want to be reminded if they re-add a capability in a subtype.

The subsequent section on "Inherited restrictions" motivates preventing re-adding capabilities to classes from other libraries, but is silent on the question of re-adding capabilities to classes in the same library.

Nonetheless, the section on disallowing implementation chooses to disallow some (but not all) capabilities to be re-added to classes in the same library. For example:

final class Vehicle {}

class LandVehicle implements Vehicle {}
class AquaticVehicle implements Vehicle {}
class FlyingVehicle implements Vehicle {}

The motivation I've heard for the core restriction is that we wish to allow a user to write a class for which all subtypes are guaranteed to inherit from (and consequently call the constructor of) the original super type. But of course, this is not true - the example above is legal if I add base to the three subtypes, and yet there are still instances of Vehicle that neither inherit from Vehicle nor call its constructor.

Why do we choose to propagate the base restriction over implements edges? This seems under motivated to me, and would seem to disallow useful patterns.

cc @dart-lang/language-team

@leafpetersen leafpetersen added the class-modifiers Issues related to "base", "final", "interface", and "mixin" modifiers on classes and mixins. label Mar 14, 2023
@lrhn
Copy link
Member

lrhn commented Mar 14, 2023

The current transitive property of base ensures that of something is declared base and has private members, then all subclasses also have implementations of those private members.
It does so by only allowing implements in the same library, which can also see and be forced to implement those private members.

A class outside of the library cannot implement, and cannot allow others to implement, that interface without providing implementation for the private members, and since they cannot provide that implementation, they're not allowed to implement.

There is nothing preventing us from removing the base modifier on a subclass in the same library as where it was introduced, and explicitly reopening.
We decided (after some discussion) to not allow that, because it could cause a library to accidentally re-open a closed class without noticing. The argument was that you should be able to look at a base class Foo declaration and correctly deduce that there are no implementations that doesn't subclass the implementation of Foo (or at least some implementation in the same library).

I'd be fine with allowing you to reopen/ignore base and final inside the same library. It's on the author to ensure that all (public) subclasses are also marked base or final, or we can make the lint warn you if they're not and you haven't added @reopen.

We'd then have to decide whether a subclass in another library prevents implementation or not. It's not going to be as simple as "has superinterface marked base or final" any more, if subclasses can reopen. And even the except "in the same library" rules get more complicated. Still doable.

The most likely specification would be something like:

Given an interface I and a superinterface S of I, we say that S prevents implementation of I iff:

  • S is introduced by a declaration marked base or final, and
  • there does not exist any interface T with declaration D s.t.
    • S is a superinterface of T and T is a superinterface of I,
    • D is marked interface or with no access modifier, and
    • D is in the same library as the declaration of S.

(It doesn't say "proper superinterface", so if I is S, it prevents implementation of itself,
and if T is I, I is the reopening declaration declaration itself, necessarily in the same library as S.)

An interface I prevents implementation in a library L if there exists a superinterface S of I,
s.t., S prevents implementation of I and S is not declared in L.

It's a compile-time error if the interface of a declaration D in library L prevents implementation in L,
and D is not marked base, final or sealed.

It's a compile-time error for a declaration in a library L to directly implement an interface I (have
as type in the implements clause) if I prevents implementation in L.

(Aka, if an interface has a base or final superinterface, with no path to it in the superinterface graph that goes through a reopened interface — necessarily in the same library since others cannot reopen — then the interface must not be implementable. Reopening is allowed only in the same library as all base or final superinterfaces. You can't implement or reopen something which is not implementable.)

I think this works, but I'll have to go over it a few times to make sure. (The current rules are easy in comparison: Superinterface marked base or final: Cannot implement outside of same library, must be base, final or sealed.)

@Levi-Lesches
Copy link

Levi-Lesches commented Mar 16, 2023

The current transitive property of base ensures that of something is declared base and has private members, then all subclasses also have implementations of those private members.

A lot of the modifiers spec tries to work around this issue -- restricting implements where the language currently doesn't. But maybe we should take a bigger look at that. I pointed out in #1006 and #2918 that it leads to very non-intuitive behavior:

// a.dart
class A { int _a = 1; }
void test(A a) => print(a._a);
// b.dart
import "a.dart";
class B extends A { }
class C implements A { }

void main() {
  test(A());  // OK
  test(B());  // OK
  test(C());  // Compiles, but crashes at runtime
}

If this wouldn't crash -- somehow, C could provide an implementation for A._a -- then we'd avoid the issue entirely, and there wouldn't be much of a reason to disallow implementing anymore.

@eernstg
Copy link
Member

eernstg commented Mar 16, 2023

The failure ('Compiles, but crashes at runtime') is one of the major reasons why the upcoming base modifier works the way it does. If you want to have a class which is public and has private members then you should probably make it a base class. This prevents implements relations in other libraries, so the dynamic error you mention becomes a compile-time error. If we do not propagate base then this is not true any more, but we would then presumably have a 'implicit_reopen' lint such that it is at least possible to maintain that discipline by choice.

@Levi-Lesches
Copy link

I agree that base would solve the problem, but I cover in #1719 (comment) why it may be needlessly complicated to

ensures that of something is declared base and has private members, then all subclasses also have implementations of those private members

and why #2918 would probably be better served to address that issue at its root.

@munificent
Copy link
Member

In the section on transitive restrictions, the class modifiers proposal says the following:

This proposal takes the last option where types have exactly the restrictions they declare but a lint can be turned on for users who want to be reminded if they re-add a capability in a subtype.

The subsequent section on "Inherited restrictions" motivates preventing re-adding capabilities to classes from other libraries, but is silent on the question of re-adding capabilities to classes in the same library.

Nonetheless, the section on disallowing implementation chooses to disallow some (but not all) capabilities to be re-added to classes in the same library. For example:

final class Vehicle {}

class LandVehicle implements Vehicle {}
class AquaticVehicle implements Vehicle {}
class FlyingVehicle implements Vehicle {}

The motivation I've heard for the core restriction is that we wish to allow a user to write a class for which all subtypes are guaranteed to inherit from (and consequently call the constructor of) the original super type.

Yes, there is a larger motivation that a library author should be able to author a class and guarantee that all subtypes of it inherit from it. And nothing another library can do can break that invariant.

But, as you say, that doesn't explain why we propagate base/final along implements restrictions.

The proposal is generally OK with letting the library author break an invariant themselves in other areas. And, indeed, the proposal used to allow breaking it in this case too. If I remember right, after a lot of discussion (which is probably buried in an issue or PR somewhere) Lasse and I felt that it was the right trade-off because otherwise the transitive rule for "cannot be implemented locally" gets more complex.

Consider:

// lib.dart
base class A {}
class B implements A {} // Let's say we allow this.

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

base class C extends A {}
class D implements C {} // ERROR: "D" must have "base" since it inherits from A.

class E extends B {}
class F implements E {} // OK: "B" has already broken the chain.

// Where it gets weird:
class G implements C, E {}

Is G allowed? Along one supertype path, it reaches A without going through a subtype in "lib.dart" that explicitly re-adds implementation, so that's an error. Along another path, it goes through a type that does break the chain and re-add implements, so it's OK?

As Lasse says, we could probably come up with a specification that sorts this out. Basically saying that if there is at least one path through the supertype DAG that reaches a type that re-adds implementability before reaching a type that prohibits it, then you're good.

But that felt really complex and subtle to me. My time working on games and package managers has endeared me to pathfinding algorithms, but this felt like a dubious use of them. :)

Instead, I suggested that it's simpler, safer, and easier for users to understand if you simply can't re-add the implements capability at all, even in your own library. You're correct that it's more restrictive than the proposal is. And it's not necessary to be this restrictive.

But I wasn't confident that users (or even we) could fully understand the more permissive algorithm where we look for a valid path in the superinterface graph. If this turns out to be too onerous of a restriction (unlikely, I think) we can always loosen it in a future release. But, especially now given the current schedule, I thought it was good to have something simpler even if it's more restrictive.

@leafpetersen
Copy link
Member Author

Closing this, I think no further discussion is needed at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
class-modifiers Issues related to "base", "final", "interface", and "mixin" modifiers on classes and mixins.
Projects
None yet
Development

No branches or pull requests

5 participants