A few advantages of the extension type:

• The type cast here is no longer required.
• Instead of only containing abstract fields, SplashController now houses the business logic for controlling splashes.
• An abstract class is often meant to be extended or implemented by multiple subtypes, whereas an extension type's purpose is self-documenting.

If I understand correctly, this is also achievable via composition, i.e. making _RenderInkFeatures a private member of SplashController, and the extension type improves only on performance, since the compiler compiles them to the exact same type.

Absolutely!

And aside from performance, there's one other benefit of being compiled to the same type:

bool doubleCheck(SplashController controller) {
  return controller is RenderBox; // Returns `true`, 100% guaranteed!
}

Since every splash controller is a RenderBox instance, the objects work beautifully with the PaintPattern API, allowing us to clean up all these tests:

final BuildContext context = tester.element(find.byType(DatePickerDialog));
final SplashController controller = Material.of(context);

expect(
  controller,
  paints..circle(color: datePickerTheme.dayOverlayColor?.resolve(const <WidgetState>{})),
);

We could migrate the tests to Material.of(context) without the extension type, but the extension type provides improved type safety, as shown by the fact that InkFeature no longer needs any type casts!

Hmm I don't quite get the type safety part you mentioned. I'm comparing the extension type with an approach that makes the RenderBox a member of SplashController (not with extending RenderBox). This approach also guarantees type safety. Querying the closest splash render box still needs some changes, but that won't be the biggest concern anyway.

We currently have an abstract class MaterialInkController that's implemented by _RenderInkFeatures; this structure requires a type cast in the InkFeature constructor.

an approach that makes the RenderBox a member of SplashController (not with extending RenderBox). This approach also guarantees type safety. Querying the closest splash render box still needs some changes, but that won't be the biggest concern anyway.

Yeah, that sounds right! Making SplashController a class with a private render object member would also ensure type safety:

class SplashController {
  SplashController._(this._renderBox);
  final _RenderInkFeatures _renderBox;

  // ...
}

However, this wouldn't allow us to clean up the tests:

testWidgets('SplashController test', (WidgetTester tester) async {
  final SplashController controller = Material.of(context);

  expect(
    controller,
    paints..circle(color: datePickerTheme.dayOverlayColor?.resolve(const <WidgetState>{})),
  );
});

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: Object or closure painting: 'a circle'
  Actual: <Instance of 'SplashController'>
   Which: was not one of the supported objects for the "paints" matcher.

IMO the extension type is the clear winner here, in terms of its performance, clean interface, type safety, and testability.

Can you explain what kind of cleanup you're looking for here?

So your PR produces:

    final BuildContext context = tester.element(find.byType(DatePickerDialog));
    final SplashController controller = Material.of(context);
    expect(
      controller,
      paints..circle(color: datePickerTheme.dayOverlayColor?.resolve(const <WidgetState>{})),
    );

It replaces the old tester.allRenderObjects.firstWhere with the internal name, which is a win for sure. But this is also achievable with my approach with minimal addition, such as exposing the object as a getter:

class SplashController {
  RenderObject get object => _object;
  RenderObject _object;
}

The test will look like

testWidgets('SplashController test', (WidgetTester tester) async {
  final SplashController controller = Material.of(context);

  expect(
    controller.object,
    paints..circle(color: datePickerTheme.dayOverlayColor?.resolve(const <WidgetState>{})),
  );
});

IMO the bigger problem is not how concise the test can be written, but more of the fact that a class structure we choose implies a relationship between the classes - just like the good old is-a-vs-has-a case.

Making the controller and the object the same object, whether through inheritance or extended class, implies that the controller is a render object, which IMO is not correct. The splash controller controls a group of splash effects, while a render object is a virtual representation of something drawn on the screen. While the splash effects will eventually be presented via an render object, it still is not the object itself. An extension class makes this situation worse, because it implies that "a splash controller is actually a render object in disguise", which is an incorrect way to think about it.

In general, I think class structure should be evaluated based on what makes the code easier and more intuitive to understand and match the mental model instead of more concise to write.

You're absolutely right that exposing the render object as a getter would allow us to clean up the tests. The only downsides would be the class declaration incurring the performance cost of a wrapper, and that an extra public RenderObject field would show up in autofill suggestions.

Overall, I think we mostly share the same understanding, though I don't quite agree with a couple of things from the previous comment's final paragraph.

a render object is a virtual representation of something drawn on the screen.

A RenderObject is "an object in the render tree". In this PR, SplashController wraps the _RenderSplashes representation type, which extends off of RenderProxyBox.

An extension type makes this situation worse, because it implies that "a splash controller is actually a render object in disguise", which is an incorrect way to think about it.

I personally think that's a good way to think about it; maybe the ideal conceptualization would be "a render object with a custom interface".
Conceptually, it's very similar to the current MaterialInkController class but has greater type safety. Rather than showing the RenderProxyBox inner workings, a SplashController exposes exactly what's needed so that descendant widgets can paint splashes.

The only downsides would be the class declaration incurring the performance cost of a wrapper

I'm sure this performance is negligible.

and that an extra public RenderObject field would show up in autofill suggestions.

This is technically true, but this is of lower priority, especially since SplashController is not meant to be used by end developers much.

I personally think that's a good way to think about it; maybe the ideal conceptualization would be "a render object with a custom interface".

If you really want to expose a render object (by itself) with a custom interface, we can do it as well, but we should call it something else instead of a controller. Flutter's other controllers are "simple" classes that are not exactly the tree node that they control (and I think it's a design we should keep). In other words, you can either expose a render object or expose a controller, but not expose a render object as the controller.

I totally agree that we should expose limited interface to developers who want to control splashes, but extension type isn't the only way, so you can't use this point to prove that we need to do extension type. Wrapping with private property achieves everything you want, including limiting interface and improving type safety.

So basically using an extension type might have the following advantage over wrapping as a private property & getter:

• Performance gain (which is likely negligible)
• One fewer autofill suggestion

But has the following disadvantage:

• Incorrect usage of the is-a model
• Require a new/special policy for extension type

Both disadvantages are of higher priority than the advantages, therefore we should not use the extension type.

If you really want to expose a render object (by itself) with a custom interface, we can do it as well, but we should call it something else instead of a controller.

That's a good point. We could call it RenderSplashes instead to match the private class name:

extension type RenderSplashes._(_RenderSplashes _renderObject) { ... }

I would be totally happy with this; the only disadvantage here would be a lack of support for data-driven fixes:
final RenderSplashes controller is a bit more confusing than final SplashController controller, and unfortunately we cannot rename variables with a dart fix.

The majority of Flutter's "controller" types are Listenables; it'd be pretty awesome if we were 100% consistent and every controller was a listenable. But if this is something we really did want to pursue, there'd be quite a bit of work to do:

// listenables                       // non-listenables
AnimationController                  MaterialInkController
CupertinoTabController               DrawerController
CarouselController                   ExpansionTileController
SearchController                     MenuController
TabController                        ScaffoldFeatureController
PlatformViewController               SystemContextMenuController
DraggableScrollableController        PersistentBottomSheetController
TextEditingController                ContextMenuController
TransformationController             HeroController
PageController                       MagnifierController
ScrollController                     OverlayPortalController
SnapshotController                   TreeSliverController
UndoHistoryController
WidgetStatesController

advantages over wrapping as a private property & getter:

• Performance gain (which is likely negligible)
• One fewer autofill suggestion

disadvantages:

• Incorrect usage of the is-a model
• Require a new/special policy for extension type

I think this is a really good summary, thank you.

And thanks for linking to the is-a vs. has-a page; it was well-written and very informative. It's super interesting to apply this distinction to extension types, since they satisfy both: an extension type that wraps a render object has a field pointing to that render object, and the extension type instance itself is a render object instance, according to type checks at runtime.

One could argue that we shouldn't ever use extension types, since they blur the lines of the is-a/has-a model. But I personally don't find that compelling, especially since these lines are already blurred elsewhere in the framework:

/// [AnimationController] is an [Animation<double>] instance…
class AnimationController extends Animation<double> {
  /// …and it has an [Animation<double>] field.
  Animation<double> get view => this;
}

(Though to be fair, I don't think the .view field is very useful and kinda wish we didn't have it.)

I completely agree that in this case, we don't need to use an extension type, but I do feel that it's better to do so.
In the case of SplashController, I see the is-a/has-a flexibility as a benefit, not a drawback; as far as an extension type policy, I think we'd be doing ourselves a disservice by not having one.