Deprecate hook parameters

[nicoddemus]

We already have support for deprecating hooks, where a warning is issued if any plugin implements a deprecated hook.

Recently in pytest 3.8 we introduced a new hook, pytest_warning_captured(warning_message, when, item).

Problem is that only later I realized that item is incompatible with xdist (pytest-dev/pytest-xdist#341) because we can't transfer pytest.Item objects between workers and master. 😓

I would like then to deprecate the item parameter by issuing a warning if a hook implementation uses it.

Given our current deprecation support of hook impls:

@hookspec(historic=True, warn_on_impl=DeprecationWarning("pytest_namespace() is deprecated"))
def pytest_namespace():
    ...

I propose:

@hookspec(historic=True, warn_on_param_impl={'item': DeprecationWarning('item deprecated because of reasons'})
def pytest_warning_captured(warning_message, item, when):
    ...

What do you guys think?

[RonnyPfannschmidt]

#15 might be relevant as well, as there is not just hook implementations, but also hook callers that may suffer from various effects

my particular stance these days is - use a new hook name, the old one is burned

[nicoddemus]

my particular stance these days is - use a new hook name, the old one is burned

In general I agree, but in this case in particular I think it makes sense, as we just released the new hook and it is very likely that nobody besides pytest makes use of the hook yet.

[RonnyPfannschmidt]

@nicoddemus the more we deliberately break the "rules" of visioning, the less they are worth

[nicoddemus]

the more we deliberately break the "rules" of visioning

Which rules are we be breaking?

[RonnyPfannschmidt]

we didnt release it as experimental api - so we have no "right" to change it

[nicoddemus]

We would not be changing it, we would be deprecating it, as we did with a bunch of features for a while now. ^^

[goodboy]

@nicoddemus I suggested something similar in #170.

The jist is we can't do this since pluggy allows external plugins to also invoke a host's hooks and we currently have no way to guarantee impls remove arguments before callers do (or vice-versa actually hence #15).

The dirty details are lengthy and a lot of the ongoing debate on this was due to my lack of understanding of this key point: new argument changes must propagate spec -> call -> impl but argument removal is the opposite.. impl -> call -> spec.

@RonnyPfannschmidt and I have discussed this topic at length in gitter and the conclusion was that you can't have symmetric backwards compatibility for both callers and implementations without allowing impls to define default arguments as in #15 (however we both agree this is not a sane approach).

pluggy's argument declaration and invocation rules

I'll summarize once more the current rules (if not just for my own memory):

1. a call that invokes an impl which declares an arg not provided by the caller is a HookCallError

2. callers introducing a new arg can support impls which do not declare that arg (this is the inverse case of 1 above) - also known as the opt-in arguments

• an impl can declare less args then provided by the caller and spec - intended to be used for introducing new args to hooks but no warning is reported if there is no spec
• if there is a spec, a caller calling a hook with less arguments then defined in the spec results in a warning emitted by the _HookCaller

3. impls declaring args not found in the spec is a PluginValidationError at register time

Again this should clarify:

• arg additions must happen in order spec -> call -> impl
• arg removal must happen in the order impl -> call -> spec

Failure modes

Breaking down the cases in point 1 above in detail, there's 2 ways backwards compat can break:

• the plugin does not invoke the hook; it is invoked by the host or some other plugin:
  If you're a plugin and you start supporting a newly introduced hook arg in your impl and then that arg is deprecated you are stuck being broken on the (range of) host version(s) that first introduced it since the caller won't pass it after removed (violates rule 1). This is un-intuitive since if you start getting warnings on a deprecated argument and then removed it all of a sudden you're broken for only this sub-set of host and plugins-which-call-the-hook versions that first introduced it. The question is, as a plugin maintainer how do you handle all versions sanely without pinning or introspecting the host/caller-plugin version and somehow providing the correct impl at register time?

• the plugin does invoke the hook itself:
  If a new argument was introduced, adopted by the downstream impls it must have been added to the call otherwise you break rule 1 above. In the deprecation case if it's removed from the call before impls do it also breaks. This is also why in gitter @RonnyPfannschmidt and a comment in #15 says:

  its invocation is a public api - adding a parameter is a breaking change

Meaning once you introduce it all the way down the line (i.e. impls added it) there's no going back to older versions of the caller without violating rule 1.

Why deprecating arguments isn't currently possible

We're trying to accomplish the following:

• the spec defines an arg provided by the caller and the impl
• the spec is marked to deprecate one of its arguments (as proposed here) and callers and impls providing the argument will begin to receive warnings
• as long as impls remove the argument before the caller (otherwise we violate rule 1) nothing should break right? Well, this is impossible to guarantee because a plugin which invokes a hook can't guarantee that all plugins which provide impls for that hook will remove the argument beforehand...meaning a plugin which calls the hook and removes the deprecated argument now breaks on all plugins which provide impls for that call but which haven't yet removed the argument.

So, yes, once all impls have removed the argument the spec can remove the argument (so as not to violate rule 3) but how do you get this happen before the callers remove the deprecated argument (so as to avoid violating rule 1). We'd need some way to guarantee that plugins which call hooks remove the argument after all the plugins providing impls have already done so. This is possible since rule 2 permits it but we currently have no way to orchestrate the impls doing it before callers.

@RonnyPfannschmidt I really hope I got it right this time 🤔