[RFC] <<Deprecated>> Attribute

On Thu, May 7, 2020 at 00:22 Benjamin Eberlei kontakt@beberlei.de
wrote:

Hi everyone,

The attributes RFC specifically looked into the use of attributes at the
engine/compiler level.

To have a showcase of this with the 8.0 release I want to propose this
RFC
for a <<Deprecated>> attribute:

https://wiki.php.net/rfc/deprecated_attribute

<<Deprecated>> is the most obvious engine attribute to add at the
beginning
in my opinion, as

• all other languages with Attributes/Annotations support have it as well
• its effects are quite obvious to understand, as such offer a good
  example
  and intro to attributes
• with potential property/class constant deprecation it adds something
  that
  was not possible in userland before.

Let me know what you think!

greetings
Benjamin

Hi Benjamin,

Two things:

1. as a matter of course, I prefer things that can be done in userland be
   done in userland unless there is a clear performance or other win. This
   doesn't qualify for me, except that it would be possible to use the same
   mechanism in core, but that really only gives us the ability to reason
   around depredations with reflection and I don't see much value in that. I'm
   not entirely convinced that a PSR and a defecto implementation isn't a
   better solution here. This isn't a show-stopper though.

Can you clarify a bit your argument "ability to reason around deprecations
only with reflection"?

At the moment we can only reason about deprecations by using both
token_get_all and finding that a method calls trigger_error
E_USER_DEPRECATED and by looking at the doc comments (via reflection) for
"@deprecated". My argument for this attribute is that it leads to both a
documentation based deprecation, and a runtime declaration at the same
time. It is a better result than what we can do in userland in my opinion.

I don't see any valid reason to be checking for deprecations at runtime
with reflection. This attribute has more value when reading the code or
doing static analysis. Neither of which requires that it be implemented in
core. Plus, userland gives us the reflection ability anyway.

2. AFAICT you cannot add an attribute to a function argument, and the
   only way I can see to resolve that is to add a "DeprecatedArgument"
   attribute applied to the function that takes the name as the first arg (and
   the message as the second). Now you're looking at:

<<DeprecatedArgument('foo')>>
<<DeprecatedArgument('bar')>>
function bat($foo = null, $bar = null) { }

It's not great IMO. Having said that, I don't think this bit is really
necessary — unlike the other suggested uses, an argument isn't an isolated
thing, so much as part of the function signature as a whole and removing
arguments is better done by deprecating the entire function and introducing
a new version with a different name. So maybe we just don't need this?

It is possible to add attributes to parameters. This was added as an
improvement during the Attributes RFC (it was not possible in the first
draft).

I went back and looked and still missed that, oops! I still stand by my
statement about it being bad practice ;)

• Davey

https://wiki.php.net/rfc/deprecated_attribute

Thanks for the RFC.
Why didn't you mention support for deprecating classes/interfaces/traits
themselves?
If there is a reason, it should be in the RFC at least to me.

It feels complicated to me, because there are so many entry points to a
class: 0. declaring/loading it 1. extending it, but not always because the
replacement could extend it 2. calling it statically 3. instantiating it 4.
i probably forget a bunch. I think this requires spreading the code to
handle this around into many places and is not easy to do without overhead
for classes that are not themselves deprecated. So I decided to keep this
out for now.

It is.
But the other cases are equally complex. That's the point I tried to make
in my previous answer and that I will continue to make by sharing my
experience on the topic.

That's why I'm pretty sure that runtime side-effect will severely limit the
usefulness of this attribute.
I very much wish we would adopt the tag, but without any runtime side
effect (which means in practice that php-internals doesn't need to define
it, this could be left to userland.)

• all other languages with Attributes/Annotations support have it as well

Did you research what runtime side effects the other languages provide?

They mostly warn or fail on compile and don't have runtime effects. The
same goes for any other deprecation that PHP triggers though, they are all
at runtime. So I don't think this can be compared, similar to how the error
stack of PHP cannot be compared to any other language.

OK thanks.

I'm concerned about runtime side-effects. Attributes should lead to none

of them to me. The moment an attribute leads to runtime side effects, it
should be turned into code instead, because it looses its "declarative"
property.

To me Symfony using Annotations at the moment is also all about side
effects based on declared annotations that are then automatically turned
into code. This is the same happening here.

That's somewhat more subtle.

Most annotations don't lead to any direct notice. We also double these
annotations with calls to trigger_error() to notify end users about them.
Both ways are required to fit several kinds of reporting tools (runtime
collectors and static analyzers). This works well when calling a method or
when instantiating a class.

In other situations, annotations are not helpful, because the deprecations
map to e.g. a specific value of an argument. That's something that no
static analyzer can spot and only inline trigger_error() can report.

When extending a class, in debug mode only, we have a special autoloader
that uses reflection to check whether the parent class or one of its method
are deprecated. It triggers a deprecation notice when it detects this
situation. Before triggering the notice, it also checks the namespace of
the parent class: if it matches the namespace of the child class, no
deprecations are reported. This allows the very common pattern of ("new
API" extends "legacy API"), which is required to make the "new API" pass
the old type hints that we have to keep in place for BC. This aspect is
unaddressed in your proposal (and doesn't need to be actually, pure
declarations are fine, others can build or reuse the same logic in
userland).

We also have another strategy to report deprecations related to
inheritance, which is purely local (no need for the autoloader I just
described). By using reflection, we sometimes check inline whether the
currently called method is being overridden, and we trigger a deprecation
if yes (or e.g. if the signature of the child method misses an argument
that we want to add in the future).

From my experience, e.g. a method can be declared as deprecated but can

still be called internally without any issue. This should not be reported
to anyone as its internal. Preserving BC often requires this, and only
runtime logic can decide when a deprecation should be reported to the
end-user. Note that this is different from configuring the global error
reporting level: deciding locally that some declaration should not be
reported is the important part of this.

Can I ask how you are handling this use-case in Symfony at the moment? If
you catch that in the error handler by looking at the backtrace, then this
is exactly possible with this declarative approach as well, in fact the
generated code is just calling trigger_error() and then triggering user
error handlers.

There is no cooperation ever between the error handler and the decision to
locally report a deprecation. All deprecation triggering sites in the code
decide on their own. Also, the backtrace is never ever used to make such a
decision.

In order to not trigger a deprecation when calling a deprecated method,
we call the method with an extra argument that we read via func_get_arg().
There is no other more accurate way.

The variety of strategies that we use is quite high because the variety of
situations that exists in real apps is also quite high, so we had to find a
way for each of them. I invite you to check the source code of Symfony,
branch 3.4 or 4.4, to have an example of each of them. I would be happy to
guide you (or anyone reading) through this if you want to take the journey,
that's quite useful for the discussion if we want to get deeper into it.

That's also why I very much favor @trigger_error() and why static analysis

works only for the simpler cases.

If one wants to save the duplicate "declaration" (attribute + explicit
call inline), then this should be opt-in to me, e.g. <<Deprecated(true)>>.

If you only want the static analysis effect, developers are still free to
use /** @deprecated */ instead, which I don't expect either static analysis
tools nor IDEs will drop support for.

That'd be a quite unsatisfactory outcome...

As an additional step, I very much wish that the attribute could accept

two arguments: the package and the version of it. From my experience,
making deprecations actionable is critical to help ppl to fix them. And
making them actionable starts with allowing ppl to quickly identify which
package cannot be upgraded to the next major yet.

This is what make us work on symfony/deprecation-contracts
https://github.com/symfony/deprecation-contracts, which provide this
single trigger_deprecation(string $package, string $version, string $
message, ...$args); function. From the example in the readme:

trigger_deprecation('symfony/blockchain', '8.9', 'Using "%s" is deprecated, use "%s" instead.', 'bitcoin', 'fabcoin');

Generates:
Since symfony/blockchain 8.9: Using "bitcoin" is deprecated, use "fabcoin" instead.

Joke aside, this is very useful and I wish the attribute could borrow from the experience we gathered on the topic.

This would be nice:
<<Deprecated('symfony/blockchain', '8.9', 'Using "%s" is deprecated, use "%s" instead.', 'bitcoin', 'fabcoin')>>

The Deprecated attribute accepts a message that gets appended to the
default message, so you could do <<Deprecated("since symfony/blockchain
8.9, use fabcoin() instead")>>, which would turn the message into:

Deprecated: Function bitcoin() is deprecated, since symfony/blockchain
8.9, use fabcoin() instead in /path/to/file:123

A free-form text is more flexible for use-cases where there are no package
and versions, we can't expect to assume every deprecation will be triggered
from a versioned composer package only.

A free-form text is what we're using since years yes. It works, but it
makes it hard if not impossible to report structured info about which
package block and cannot be updated to their next major. Providing this
info in a structured way is useful and that'd what I'm trying to convey
here. I think we can assume that every deprecation will be able to report
some package+version info (it doesn't have to be a composer package, what
matters is having the fields easily identifiable)

Nicolas