Skip to content
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

The base keyword is problematic #3844

Closed
nate-thegrate opened this issue May 28, 2024 · 4 comments
Closed

The base keyword is problematic #3844

nate-thegrate opened this issue May 28, 2024 · 4 comments

Comments

@nate-thegrate
Copy link

interface is great

A developer can use interface in a class declaration to show that it's meant to be implemented, not extended.

interface class A {}
interface class B {}

class C extends A implements B {}

If C isn't declared in the same library as A and B, it's a super easy refactor:

class C implements A, B {}

Switching from class A to interface class A provides no functional benefit, but it makes the class more descriptive and easier to understand with virtually no downside.

base is too restrictive

Much like interface, the base keyword provides no functional benefit. And unlike interface, it creates significant problems.

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

Unless all 3 classes are declared in the same library, this will not compile. A single-line fix isn't possible: the C class declaration would need to be restructured or removed entirely.


Solution?

A few possibilities:

  1. Remove the keyword
  2. Make it a decorator instead
  3. Allow extending multiple classes
  4. Change what the base keyword does

Remove the keyword

From the dart.dev documentation:

A base class disallows implementation outside of its own library. This guarantees:

  • 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.

I don't think any of this is useful; let me know if you disagree.

Make it a decorator instead

@base
class A {}

no_logic_in_create_state has an "info"-level severity, so it's not a huge deal to break the rule if the situation calls for it. I believe the same severity is appropriate for an "extend this class, don't implement it" rule.

Allow extending multiple classes

base class Foo {}
base class Bar {}

base class A extends Foo as super1, Bar as super2 {
  const A()
    : super1(),
      super2();
}

// Or maybe we still "extend" just 1 other class,
// but we allow classes with constructors to be used as mixins
base class B extends Foo with Bar {
  const B()
    : super(),
      Bar();
}

This would allow for enums extending other types, but adding this language feature might be significantly more trouble than it's worth.

Change what base does

Rather than "the opposite of interface", perhaps the base keyword should be "the opposite of abstract".

Currently:

  • interface should be implemented, not extended
  • base should be extended, not implemented

Proposal:

  • abstract "must be overridden"
  • base "must not be overridden"
abstract class A {
  void init(); // must be overridden in subclasses
}
base class A {
  void init() {
    importantLogic(); // cannot override in subclasses
  }
}

base class B extends A {}

base class C extends A {
  @override
  void init() {} // compile-time error
}

class D implements A {
  @override
  void init() {} // no error, "implements" works fine
}

This would be a breaking change that provides no functional benefit just kidding, it allows for type promotion!

base class A {
  const A(this.value);

  final Object value;

  void init() {
    if (value is int) print(value.isEven);
  }
}

I recently learned that abstract can apply to a class member, not just the class itself:

abstract class A {
  abstract final Object value; // must be overridden in subclasses
}

Maybe the same could be true for base!

class B {
  const B(this.value);

  base final Object value; // must not be overridden in subclasses

  void init() {
    if (value is int) print(value.isEven); // value can be type-promoted
  }
}
@jakemac53
Copy link
Contributor

The properties of base are useful, maybe not for your specific use cases, but they are useful:

The base class constructor is called whenever an instance of a subtype of the class is created.

One example is this allows you to ensure specific validation or setup logic is ran on object creation.

All implemented private members exist in subtypes.

This prevents runtime errors as it allows you to safely assume private members are available. Otherwise any call to a private member could result in a runtime error.

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.

This allows you to evolve your API safely without breaking changes (or at least, far fewer breaking changes), and is very useful for package authors and their consumers alike.

@nate-thegrate
Copy link
Author

@jakemac53 thanks for helping me understand your perspective!

Unfortunately I don't think I'm able to respond in a concise way, so apologies in advance for the wall of text.


ensure specific validation or setup logic is ran on object creation

A constructor body could have important logic, and in that case it seems understandable that a developer would want to add the base modifier.

But on the flip side, it's also very possible that a developer would want to use an API that requires SomeClass, but they don't want to use any of the SomeClass constructors.

And even if the SomeClass constructor is required, the base modifier still adds unnecessary restrictions:

/* will not compile if SomeClass is a base class */

class A extends SomeClass {}
class B extends SomeClass implements A {}

safely assume private members are available

That's a good point—here's an example that throws (without triggering any static analysis warnings):

/* a.dart */

class A {
  void _evaluate() {
    // This is an important method; it's used outside the class declaration.
  }
}

void functionA(A a) {
  a._evaluate();
}
/* b.dart */

import 'a.dart';

class B implements A {}

void functionB(B b) {
  functionA(b);
}

functionB is guaranteed to crash, and the issue can be avoided entirely by adding the base modifier to A.

We could add a linter rule that recommends adding a base modifier anytime a private class member is referenced outside the declaration, but I don't think that would be ideal—if it's useful to access a class member outside the class declaration, then it probably should just be a public member.


evolve your API safely without breaking changes (or at least, far fewer breaking changes)

If an API is being actively worked on, there are many potentially breaking changes:

  1. Adding a class member with a required constructor parameter
  2. Adding a class member with an optional constructor parameter
  3. Adding a class method
  4. Adding an abstract class method
  5. Removing a class method
  6. Adding/removing class method arguments

The base modifier makes 2 & 3 non-breaking (except in the case of name overlap). Number 2 is pretty common, so I agree that the modifier would be very helpful in these cases.

That being said, it's not often that a developer knows in advance which classes will have new members added in the future and which classes won't. And what if someone wants to implement the class and is fine with adapting to breaking changes? A decorator that triggers a linter rule would make more sense than a keyword that causes a compile-time error.



Back when I was brand new to Dart, I wasted a lot of time debugging dumb mistakes, but implementing a class that should have been extended was never a problem, since any time I started using a new API, I would always copy some example code and then tweak it from there. Since accidentally breaking a program by using implements instead of extends is arguably a non-issue, the most notable effect of the base keyword is the superfluous restrictions it imposes on developers who know what they're doing.

@lrhn
Copy link
Member

lrhn commented May 29, 2024

I'll also disagree that base is not useful, it is just not always what you need, in have out seems to rarely be what I want, and there might be room for a non-inherited version of it too.

The goal of base is to ensure that any subtype is a subclass, meaning that the subtype inherits implementation, including private members and constructors (as @jakemac53 mentioned).
The reasons for wanting to ensure that may differ, but they're usually about knowing something about an object, something that the interface alone cannot ensure, while still allowing subtypes to exist. (So not just final.)
That guarantee is precisely why you're not allowed to implement the type, and why all subclasses must be marked as not implementable (it could be inherited, requiring you to write it is for documentation).

You can't absolutely guarantee that a member won't be overridden (or that it must be overridden and must call its super). Not overridden would be like a final member, which is also an option, but a separate feature. Currently things like that are only checked using annotations.
I'd prefer something like that on individual members, not on the entire class.
It's definitely possible but

just kidding, it allows for type promotion!

... it would not allow for type promotion if the type can be implemented. The type would need to be the current base for that.

About your solutions:

Remove the keyword

Just don't use it then.

Make it an annotation

Basically, make it ignorable.
See above: if the protection it gives, which requires it to not be ignorable, is not what you need, don't use it.

Allow extending multiple classes

That makes every class a mixin. Just make the classes that need it mixins. (Let's discuss composite mixins if the current ones are too restrictive.)

Change what base does

That suggested change should be a per-member modifier.

What is the problem you're really trying to solve?
Today you can just not use base. If someone else uses it, you should assume that they want the protection it gives. (Or is the problem that you want to be able to ignore base that other people have added?)

So what is the behavior that you want to achieve, which no modifier gives you today? And what has that got to do with base?

(I sometimes wish I had a non-inherited "do not implement" modifier, for a class that has no reason to exist other than work as a superclass. It's not that it's a problem if you implement the type, it's just meaningless. Something like MapBase, where if you don't extend it, you should just implement Map instead. Nobody will ever care that you implement MapBase, it has exactly the same members. But on the other side, of you create an OrderedMap subclass, maybe with more members, making that implementable is not a problem. That is a non-interface class only.)

@nate-thegrate
Copy link
Author

@lrhn I really appreciate you sharing your thoughts.


If someone else uses it, you should assume that they want the protection it gives. (Or is the problem that you want to be able to ignore base that other people have added?)

Yes, my concern is that at some point I'd want to use somebody's API, and the base modifier would get in the way of doing what I'd like with it. Fortunately, I haven't come across any base classes as of yet.


At this point, I only have 1 point of disagreement:

… it would not allow for type promotion if the type can be implemented.

That's only partially true, since type promotion could still work within the class declaration.

class A {
  base final Object value;
  const A(this.value);

  void classMethod() {
    if (value is int) print(value.isEven); // promoted
  }
}

// also works in a class declaration that extends A
class B extends A {
  const B(super.value);

  void classMethod2() {
    if (value is Size) print(value.width); // promoted
  }
}

Allowing implementation wouldn't be problematic, since everything that benefitted from type promotion would be overridden:

class C implements A {
  @override
  Object get value {
    // ...
  }

  @override
  void classMethod() {
    // ...
  }
}

But it would prevent type promotion any time the type is used as a parameter:

void foo(A a) {
  if (a.value is int) print(a.value.isEven); // error
}

After reading these comments and thinking it over a bit, I don't think any of the solutions I proposed in this issue would be ideal. I'm good with closing this one; perhaps in the future we could improve or expand on the class modifier in a better way.

@nate-thegrate nate-thegrate closed this as not planned Won't fix, can't repro, duplicate, stale May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants