Linter: Annotation for methods that can be overridden but not called

Created on 28 Sep 2016 · 15Comments · Source: dart-lang/linter

We found one customer who was calling a method that is internal to Flutter's framework. It's supposed to be called by the framework to notify the subclass of what's going on. We had it marked @protected, but that doesn't stop people from calling it from within the class itself!

One option to catch such cases would be an annotation that makes the method visible for the purposes of overriding, but flags calls from outside the original library, as if it was _private for the purposes of calling, but @protected for the purposes of overriding (and calling super within overrided).

flutter enhancement lint request

Source

Hixie

👍4 👎1

Most helpful comment

Yeah, more or less. The specific case is:

abstract class State {
  @protected
  @mustCallSuper
  void deactivate() {
    // Does nothing, called by framework internals when object is
    // being transitioned to the inactive state.
    // Subclasses free resources when this is called.
  }
}

class MyState extends State {
  @override
  void deactivate() {
    // Do my own special thing
    super.deactivate();
  }

  // the following method is called when the user taps on this widget somehow
  void _tapHandler() {
    deactivate(); // wait, what? this is crazy
  }
}

Ideally the analyzer would catch the call to deactivate and say something we could configure, like: "If you want to remove a widget from the widget tree, you must notify the parent widget so that it can get rebuilt and omit the widget from its build. Calling deactivate() directly will have no useful effect and may, depending on what this specific State subclass does, cause the widget to behave incorrectly, e.g. by having it stop animations."

Hixie on 28 Sep 2016

👍2

All 15 comments

That sounds pretty complicated, and I'd worry about it being abused/confused with @protected (or having a bunch of methods that will show up in the public API, but not really be part of the public API).

If I understand correctly, you have something like:

abstract class Widgetzoid {
  void notifyChanges() {
    // Do internal thing.
  }
}

And you want this be OK:

class MyWidget extends Widgetzoid {
  @override
  void notifyChanges() {
    // Do my own special thing
  }
}

But this to be BAD:

class MyWidget extends Widgetzoid {
  MyWidget() {
    notifyChanges();
  }
}

matanlurey on 28 Sep 2016

Yeah, more or less. The specific case is:

abstract class State {
  @protected
  @mustCallSuper
  void deactivate() {
    // Does nothing, called by framework internals when object is
    // being transitioned to the inactive state.
    // Subclasses free resources when this is called.
  }
}

class MyState extends State {
  @override
  void deactivate() {
    // Do my own special thing
    super.deactivate();
  }

  // the following method is called when the user taps on this widget somehow
  void _tapHandler() {
    deactivate(); // wait, what? this is crazy
  }
}

Hixie on 28 Sep 2016

👍2

Ah OK, I understand now, thanks for explaining.

This is a similar thing we'd like to lint for Angular Dart, definitely useful.

/cc @ferhatb

matanlurey on 28 Sep 2016

👍1

/cc @bwilkerson

pq on 7 Oct 2016

If addressed, probably better in the analyzer proper?

cc @bwilkerson

pq on 13 Oct 2016

There are discussions about how much visibility control to make an explicit part of the language, but I don't expect any change in that area before 2.0.

I think what's being proposed is something like this: https://codereview.chromium.org/2420653002/

bwilkerson on 13 Oct 2016

Just to be clear, I have not sent out the above CL for formal review; I'm hoping to hear whether it matches what you're asking for.

bwilkerson on 17 Oct 2016

The relevant part of that patch is this, right?:

 111 /// Used to annotate an instance member that was made public so that it could be
 112 /// overridden but that is not intended to be referenced from outside the
 113 /// defining library.
 114 ///
 115 /// Tools, such as the analyzer, can provide feedback if
 116 ///
 117 /// * the annotation is associated with a declaration other than a public
 118 ///   instance member in a class, or
 119 /// * the member is referenced outside of the defining library.

If so, then yes, I think that comment matches what I've described. Ideally one would be able to control the message that is shown (see the comment above for an example).

By the way, @visibleForTesting is another good one that I would love to see deployed (noticed it in the patch).

Hixie on 18 Oct 2016

@visibleForTesting was added in version 1.0.2 of the meta package.

bwilkerson on 18 Oct 2016

But does the analyzer support it?

Hixie on 18 Oct 2016

No, there's no hint for it yet.

bwilkerson on 18 Oct 2016

The new annotation is now available (https://codereview.chromium.org/2420653002/). Hint support to follow.

bwilkerson on 9 Nov 2016

Was hint support ever added for this annotation? If so, what do I put in my analysis_options.yaml to enable it?