feat: only have a defer() method on events that can be deferred

[tonyandrewmeyer]

There are two goals for this PR:

• Prevent charms from calling defer() on events where that makes no sense
• Alert charmers as early as possible when they are using defer() where it doesn't make sense

The original version of the PR adjusted the various event classes so that the events that shouldn't have defer() called didn't have the method at all. This achieves both goals (most IDEs and checkers will detect that a method is being called that isn't defined). However, that was too significant a change, as we could not maintain the pattern for creating custom events and also checks like isinstance(action_event, ops.EventBase).

Instead, we continue with the same practice as we already use for ActionEvent, SecretExpiredEvent, and SecretRotateEvent:

• Set the return type to typing.NoReturn - this shows any code after the defer() call as unreachable (at least in VS Code) - hopefully there is at least a return after the defer() call.
• Raises a RuntimeError if defer() is actually called, which prevents inappropriate defer() use in production and will hopefully be caught in tests.

The additional events that get this behaviour are:

• StopEvent (I feel that this is the most borderline)
• RemoveEvent
• LifecycleEvents

Other options considered (as well as setting the return type):

• Change the signature of defer() in the classes that shouldn't use it, e.g. to have a "does_not_support_defer" argument. This will raise a TypeError instead of RuntimeError or AttributeError, with a hint (the name of the argument) as to why it's happened, and this will usually show in IDEs and linters. This provides an earlier warning, which is good, but I feel it's too much a nice hack.
• Call warnings.warn("x events should not be deferred", RuntimeWarning) or warnings.warn("x events should not be deferred", DeprecationWarning) in the defer() in the classes that shouldn't use it. This is safer, but I'm not convinced that it will be noticed. I had thought that unittest/pytest defaulted to something like -Werror but that isn't the case. That means that unless someone is running their tests with a similar sort of warnings filter, or is paying close attention to the test output, this is likely to be missed.

I've run the current state of the branch over ~135 Canonical charms' tox -e unit and there are no new failures (and the existing failures don't seem to be masking any new issues), so it seems reasonably safe to make this change.

[benhoyt]

Per discussion, I think this is a good attempt, however, I'm concerned it's fairly complex and may break charms, if for example people are observing stop and config-changed and sending them through a common code path that defers. So given that Pyright can't help us here, let's lean towards keeping it simple and updating docstrings and raising RuntimeError (though even that we'll have to be careful about).

[tonyandrewmeyer]

I played around with a few more possibilities as discussed (I also tried using a metaclass to mess with what gets inherited but couldn't find a clean way to do that).

These examples all start with:

class Base:
    def defer(self) -> None:
        pass

This is what we use now, on ActionEvent and similar:

class A(Base):
    def defer(self) -> typing.NoReturn:
        raise RuntimeError()

class B:
    def f(self):
        a = A()
        a.defer()
        return

VS Code doesn't show an error but it does show that the return is unreachable:

It's subtle, but better than nothing. Tests would raise the RuntimeError.

This would be ideal:

class C:
    pass

class D:
    def f(self):
        c = C()
        c.defer()
        return

Since it shows an actual error (and I think pretty much any of the common linters would pick it up):

However, as discussed, it's doesn't seem like we can get to this without breaking backwards compatibility.

The suggestion to make calling an error by having an unused argument:

class E(Base):
    def defer(self, not_deferrable):
        raise RuntimeError()

class F:
    def f(self):
        e = E()
        e.defer()
        return

This does show an actual error (that again almost any linter should find):

And it does have the name of the argument both in the linting and in the exception: TypeError: E.defer() missing 1 required positional argument: 'not_deferrable'.

I like the out-of-the-box thinking, but I'm not sold on doing this.

Anything particularly dynamic, like:

class G(Base):
    def __getattribute__(self, __name: str) -> typing.Any:
        if __name == "defer":
            raise AttributeError("Can't defer this event")
        return super().__getattribute__(__name)

class H:
    def f(self):
        g = G()
        g.defer()
        return

Fails to get detected by linting, so is worse than simply having the raise RuntimeError().

Adding in a warning of some form would generally be good, e.g.:

class I(Base):
    def defer(self) -> None:
        warnings.warn("not deferrable", RuntimeWarning)


class J:
    def f(self):
        i = I()
        i.defer()
        return

And this will output the warning to stderr (if we use RuntimeWarning, or we could use a different category and make it dependant on the warning level), and running with -Werror converts this to an exception. However, while this would work nicely in tests, the RuntimeError already works there, and in most situations the stderr content will get lost when actually running the charm in production (cf. #1077).

[benhoyt]

I think the runtime errors and docstring changes are good and simple. We should still determine whether we want to introduce the runtime errors for existing event types, but I think our first step for determining that is running the raft of charm tests and see if any of those tests break.

But I guess the main decision point is whether we want to do the "not_deferrable" arg hack? Does that typecheck in the Ops codebase itself? Or does Pyright complain about "overload has different args to method in base class" or something?

[tonyandrewmeyer]

We should still determine whether we want to introduce the runtime errors for existing event types, but I think our first step for determining that is running the raft of charm tests and see if any of those tests break.

Yeah. At the moment I have tooling for installing a branch of ops but not for running from local changes (which would be nice, but is more complicated to implement), so I needed the branch to have the changes to be able to run the tests against them.

But I guess the main decision point is whether we want to do the "not_deferrable" arg hack? Does that typecheck in the Ops codebase itself? Or does Pyright complain about "overload has different args to method in base class" or something?

Neither ruff (with our current config) nor pyright complain as long as I give not_differable a type. I thought one of them would complain about the signature changing, but they haven't in my tests.