language icon indicating copy to clipboard operation
language copied to clipboard

More flexibility for the `base` keyword

Open nate-thegrate opened this issue 1 year ago • 3 comments

Proposal

Define base as follows:

if A is a base class, then all subtypes of A must inherit from A.



All of the guarantees from the Class modifiers documentation would still hold:

  • The base class constructor is called whenever an instance of a subtype of the class is created.
  • All implemented private members exist in subtypes.
  • A new implemented member in a base class does not break subtypes, since all subtypes inherit the new member.
    • This is true unless the subtype already declares a member with the same name and an incompatible signature.

There would only be 1 difference:

You must mark any class which implements ~~or extends~~ a base class as base, final, or sealed.

Adding the base modifier to a subtype would only be required when the class is implemented within the same library. (Alternatively, I would be in favor of disallowing implementation within the same library altogether, though that would be a breaking syntax change.)


Making this change to the keyword would enable the following:

a.dart

base class A {}

b.dart

abstract interface class B extends A {
  int get value;
}

c.dart

class C extends A implements B {
  @override
  int get value => 42;
}



The analyzer may need to traverse several steps up the class heirarchy to check whether a class declaration is valid, but it already does this with the current base modifier:

base class A extends Foo {}

base class B extends A {}

base class C extends B {}

base class D implements C {}

A base class can be implemented within its own library, so whether base class D implements C is valid depends on whether Foo is a base class.

base class

nate-thegrate avatar Jun 13 '24 21:06 nate-thegrate

The analyzer may need to traverse several steps up the class heirarchy to check whether a class declaration is valid, but it already does this with the current base modifier:

This is true, but it never needs to traverse across multiple libraries. Traversing the hierarchy within a library is fairly reasonable, I think, because it's all one unit of code. If you can edit any of it, you can edit all of it. If you need to understand any part of it, you may need to understand the whole thing.

Extending that across libraries is something we did consider at some point during the design process, but I felt then and now that it's too brittle and too confusing.

In your example, with what you propose, imagine if at the point in time that class C was authored, A did not have base on it yet. The author of C believes they are writing a class that can be freely implemented, which it can at that point. Later, the author of A adds base, which is a breaking API change. But the author of C might not see the breakage because class C itself wouldn't be broken. It's just that every downstream class implementing C is now an error.

To avoid those kinds of situations, we felt it was better for every class that extends a base class to have to itself be explicitly marked base too. That makes it easier for someone looking at the class declaration to know what they can do with it. With the current design, when A is changed, the author of C discovers immediately that their own class's affordances have changed too and they are required to put base on it and advertise that fact to their downstream users.

You are definitely right that there's a weird edge case where you may need to traverse a chain of classes within a single library to determine if an implements is valid or not. I hope that users rarely run into this.

Figuring out the right set of modifiers, what guarantees they offer, and what constraints they require in order to meet those guarantees, was surprisingly hard given Dart's very permissive history. What we ended up with isn't as elegant as I'd hoped, but it's hard to get a good sense for whether there are tweaks we should make that would be an improvement or not.

munificent avatar Jul 11 '24 22:07 munificent

Thanks for responding here.

What we ended up with isn't as elegant as I'd hoped, but it's hard to get a good sense for whether there are tweaks we should make that would be an improvement or not.

I completely identify with that conclusion.

At this point, I don't feel strongly about how the base keyword should behave—as of today, it isn't used at all in flutter/flutter or in my personal projects—but I'd still like to engage with the ideas shared here.



imagine if at the point in time that class C was authored, A did not have base on it yet. The author of C believes they are writing a class that can be freely implemented, which it can at that point. Later, the author of A adds base, which is a breaking API change. But the author of C might not see the breakage because class C itself wouldn't be broken.

You bring up a good point; I should amend my previous statement:

All of the guarantees from the Class modifiers documentation would still hold, as long as the base class isn't implemented within its own library.

(collapsing this section, since I no longer believe it's a good idea)

In my mind, you'd be able do the following (though I don't know why you would want to):

base class A {}
class B extends A {}
class C extends B {}
class D implements C {}

and then another library could implement D:

class E implements D {} // compiles without issues

Adding the base modifier to A would break any class that implements A, B, or C without inheriting from A, but the error message would provide a solution:

class F implements B {} // error: the class 'A' can't be implemented outside of
                        // its own library because it's a base class.
class F extends A implements B {} // works!



It's kind of amusing to read the benefits of the base keyword and then watch the Flutter framework completely disregard them.

A new implemented member in a base class does not break subtypes, since all subtypes inherit the new member.

A new implemented member was added to ColorScheme and then cherry-picked into a stable release without any mention of potentially breaking subtypes.


All implemented private members exist in subtypes.

I wasted about half an hour trying to set up the following enum:

enum AppKey implements GlobalKey {
  screen1,
  screen2,
  screen3;

  GlobalKey get _key => GlobalObjectKey(this);

  @override
  BuildContext? get currentContext => _key.currentContext;
}

only to realize that GlobalKey relies on a _currentElement getter to function, and even if I define that getter in my enum, it doesn't work.


I could make a PR that adds the base keyword, but it wouldn't be worth it, since it'd involve adding the keyword to every GlobalKey subclass, and it could invalidate a class that would otherwise work fine:

class MyKey extends GlobalKey implements MyKeyInterface {
 // ...
}



I feel like the right way to go is to remove the keyword requirement for subclasses:

  • It enables class C extends A implements B
  • In the case of GlobalKey (and other similar classes), it could greatly mitigate debugging frustration without any downsides
  • one less word to type!

At the same time, it just doesn't feel like the benefit is worth the hassle.

Though perhaps in the future there will be another language feature proposal that synergizes with this one :)

nate-thegrate avatar Jul 12 '24 01:07 nate-thegrate

#4041 was the proposal I had in mind to synergize with this one.

Since a base class with a base method guarantees that the method won't be overridden outside the declaring library, I imagine that the base keyword would start to see more use.

The additional flexibility proposed here would allow class C extends A implements B when A is a base class and B inherits from A, which I believe to be highly valuable.

Since the base class modifier's guarantees fail to hold if it were able to be implemented by a non-base class in the same library, I'll go ahead and modify the original proposal to prevent that problem.

nate-thegrate avatar Aug 16 '24 19:08 nate-thegrate

I can see the argument for "a supertype of subtypes of A" that isn't intended to be concrete. It's not something you can implement from scratch, but you can attach it to something that already implements A.

You can get the effect using a mixin instead.

base mixin B on A {
  int get value;
}

and

class C extends A with B {
  @override
  int get value => 42;
}

but it is a little awkward when it really is an interface.

There is no good way to express the constraint "Cannot be implemented unless also extending something else". It's going to be ad-hoc, recognizing that B is marked interface and subclasses a base class. That combination has no marker itself.

But let's go with it.

  • A concrete class which has a base class/mixin as superinterface must have that class/application-of-mixin as a (direct or transitive) superclass, and must be marked base, final or sealed.
  • An abstract class can do whatever, including things that'll never allow a concrete subclass. So don't. (It's not like the compiler can't track whether there is a base-marked superinterface for each abstract class.)

That gives all the same guarantees for objects, and allows you to write abstract unmarked or interface classes that has a base class as superinterface.

The main reason this wasn't the design is that it's hard to read. Looking at B, you can't see that you can't just do class D implements B { ... }. In fact, it says that you can by being declared as abstract interface class B .... It's a transitive requirement that isn't visible in the declaration.

We could allow you to write abstract interface base class B extends A { .... }, which means that this type is an abstract interface, but it's an interface that inherits a base requirement. (So read the docs, or check the error message, to see which class you need to extend.) It's a class that you can implement, but that you can't just implement. It's a tag, not a class. But what if you could extend B too, so you'd have to remove the interface, then it would just be base.

Or maybe that requires a new modifier, because interface base is neither interface nor base. You can't just implement it, and maybe you can't extend it either (if it only did implements A). It's a new thing. Let's call it base* (transitive base).

  • A class that has a base class as superinterface must be marked as one of base, final, sealed or base*.
  • If it's base*, the class must be abstract.
  • (If it's not abstract, it must still extend all base classes it has as superinterfaces.)
  • A class can be marked interface base* which means it can be implemented, but not extended, or it can be base* (aka <none> base*) and it can be both implemented and extended. (Outside of the library, as usual. Inside the library, you can ignore the modifiers, but not transitive requirements from other libraries.)

Then the question is whether the extra complexity to readers, from having a fifth or sixth class modifier with a fifth meaning, is worth it. (Especially if some of the cases can be covered by using a mixin.)

I do get it. I never use base for anything myself. The use-case of having a class that someone must extend, but also can extend, just doesn't crop up for me. It's either an interface or a final class. The few cases where I don't want someone to implement a class, it's usually because it's not intended as an interface to begin with, it's just a skeleton for another interface, but I don't want to make subclasses be marked base too. Maybe if I wrote more frameworks, it would happen more.

lrhn avatar Nov 15 '24 12:11 lrhn

I don't want to make subclasses be marked base too.

Amen to that.

As far as solving https://github.com/flutter/flutter/issues/158876, I think ideally we would have

base mixin class GlobalKey {
  // ...
}

In some sense, this wouldn't even be a breaking change, since any attempt to implement GlobalKey would crash anyway.

The only issue is how all GlobalKey subtypes would need to be marked with base. One possible solution would be a transitive base* and a dart fix:

class MyKey extends GlobalObjectKey {}

// would automatically be changed to:
base class MyKey extends GlobalObjectKey {}

This would work, though I can imagine a future where a majority of Flutter framework classes are marked as base. It's just 1 extra word, but given that there's a potential dot syntax on the horizon to make things a bit more concise & convenient, it does feel weird to take a step in the opposite direction here.



Say I had a project with the following:

class RenderBoxKey extends LabeledGlobalKey {
  RenderBoxKey(super.label);

  RenderBox? get renderBox => currentContext?.findRenderObject() as RenderBox?;
}

class InkKey extends LabeledGlobalKey implements RenderBoxKey {
  InkKey(super.label);

  @override
  RenderBox? get renderBox => currentContext?.findAncestorRenderObjectOfType<InkController>();
}

If adding base to subtypes wasn't a requirement, this would be fine.

But unfortunately, adding the keyword via dart fix isn't possible here; there would need to be an additional class declaration.

abstract base* class RenderBoxKey implements LabeledGlobalKey {
  factory RenderBoxKey(String label) = _RenderBoxKey;

  RenderBox? get renderBox;
}



This issue is proposing "only require base when implementing within the same library." I strongly feel that this change would have no significant downsides, but reading back over my previous comment, I don't think I did a great job of conveying that.

If we take that example and replace A with GlobalKey:

imagine if at the point in time that class C was authored, GlobalKey did not have base on it yet. The author of C believes they are writing a class that can be freely implemented, which it can at that point. Later, the author of GlobalKey adds base, which is a breaking API change. But the author of C might not see the breakage because class C itself wouldn't be broken. It's just that every downstream class implementing C is now an error.

There are 2 possibilites:

1. class C implements GlobalKey

The class would no longer compile, i.e. the author would see the breakage.

2. class C extends GlobalKey

The author might not see the breakage, but I don't think it's true that every downstream class implementing C is now an error.

class MyClass extends GlobalKey implements C {} // not an error

The author of C believes they are writing a class that can be freely implemented, and it can: if a subtype of C doesn't inherit from GlobalKey, it would crash anyway, since it would be missing the private _currentElement field.

If we follow this issue's proposal, making GlobalKey a base mixin class would be a non-breaking change, aside from how C would need to use extends or with instead of implements. The only significant difference would be catching invalid subtypes with static analysis instead of them going unnoticed until runtime.


This could also make base class subtypes self-documenting without using the keyword:

abstract interface class C implements B {} // generic interface

abstract interface class C extends B {} // conveys that B is a `base class`

nate-thegrate avatar Nov 15 '24 20:11 nate-thegrate