Add #[Deprecated] attribute based on ZEND_ACC_DEPRECATED #6521

beberlei · 2020-12-18T20:03:59Z

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

Todos:

Does not show the deprecation when opcache or jit is enabled (haven't figured it out in detail)
Memory leak for string arugments of internal attributes.
Convert deprecation to exception is not tested yet, and code is not adopted from internal deprecations.
Tests for methods, and failure scenarios of attribute usage

iluuu1994

Tests for methods seem to be missing 🙂

TysonAndre

Seems like useful functionality overall, some comments that are easy to address

TysonAndre · 2020-12-22T18:43:01Z

Zend/zend_vm_def.h

@@ -3945,6 +3945,10 @@ ZEND_VM_HOT_HANDLER(130, ZEND_DO_UCALL, ANY, ANY, SPEC(RETVAL,OBSERVER))
 	zend_function *fbc = call->func;
 	zval *ret;

+	if (UNEXPECTED((fbc->common.fn_flags & ZEND_ACC_DEPRECATED) != 0)) {


Convert deprecation to exception is not tested yet, and code is not adopted from internal deprecations.

Should this go after SAVE_OPLINE? I see that it's done in ZEND_DO_FCALL_BY_NAME,, and if I remember correctly, it may matter if:

The set_exception_handler throws an Exception - Definitely add a unit test of this.

Also, I forget how it should work, but HANDLE_EXCEPTION() may need to be called directly or indirectly to return to the caller

TysonAndre · 2020-12-22T18:45:55Z

Zend/zend_vm_def.h

@@ -4074,6 +4082,10 @@ ZEND_VM_HOT_HANDLER(60, ZEND_DO_FCALL, ANY, ANY, SPEC(RETVAL,OBSERVER))
 			ret = EX_VAR(opline->result.var);
 		}

+		if (UNEXPECTED((fbc->common.fn_flags & ZEND_ACC_DEPRECATED) != 0)) {
+			zend_deprecated_function(fbc);


Elsewhere, I see a much more verbose handling that does include HANDLE_EXCEPTION. I haven't checked if all of the teardown applies, but this should test that subsequent statements aren't executed (e.g. call_deprecated_function_when_error_handler_throws(); echo "Unreachable\n";

if (UNEXPECTED((fbc->common.fn_flags & ZEND_ACC_DEPRECATED) != 0)) { zend_deprecated_function(fbc); if (UNEXPECTED(EG(exception) != NULL)) { UNDEF_RESULT(); if (!RETURN_VALUE_USED(opline)) { ret = &retval; ZVAL_UNDEF(ret); } ZEND_VM_C_GOTO(fcall_by_name_end); } }

Yes, i haven't copied this over because i don't fully understand it yet, but left open a todo. A test that converts deprecation into exception will probably reveal what this is used for.

TysonAndre · 2020-12-22T18:48:08Z

Zend/zend_attributes.c

+
+	ZVAL_UNDEF(&tmp);
+	str = zend_string_init(ZEND_STRL("message"), 1);
+	zend_declare_typed_property(zend_ce_deprecated_attribute, str, &tmp, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_MASK(MAY_BE_STRING));


Should it be MAY_BE_STRING or MAY_BE_STRING|MAY_BE_NULL?

Add a test of $deprecated = new Deprecated('x'); $deprecated->message = null;?

TysonAndre · 2020-12-22T18:50:09Z

Zend/zend_attributes.c

+			return;
+		}
+
+		if (Z_TYPE(message) != IS_STRING) {


The signature in stubs has ?string $message = null - would be more consistent if this allowed explicitly passing #[Deprecated(null)] to work the same way as new Deprecated(null);

TysonAndre · 2020-12-22T18:50:27Z

Zend/zend_attributes.c

+		zval message;
+
+		/* As this is run in the middle of compilation, fetch the attribute value without
+		 * specifying a scope. The class is not fully linked yet, and we may seen an


Suggested change

* specifying a scope. The class is not fully linked yet, and we may seen an

* specifying a scope. The class is not fully linked yet, and we may see an

TysonAndre · 2020-12-22T18:51:11Z

Zend/tests/attributes/deprecated_002.phpt

+--FILE--
+<?php
+
+#[Deprecated()]


Add test of Deprecated(null)?

TysonAndre · 2020-12-22T19:01:11Z

Zend/tests/attributes/deprecated_001.phpt

+--FILE--
+<?php
+
+error_reporting(E_ALL | E_DEPRECATED);


nit: As of PHP 8.0, E_ALL includes E_DEPRECATED, so this is redundant in a test targeting 8.0

TysonAndre · 2020-12-22T19:01:47Z

Zend/zend_attributes.c

+			zval_ptr_dtor(&message);
+
+			zend_error_noreturn(E_COMPILE_ERROR,
+				"Deprecated::__construct: Argument #1 ($message) must be of type string, %s given",


Same for the message not mentioning null

TysonAndre · 2020-12-22T19:07:23Z

Zend/tests/attributes/deprecated_001.phpt

+--EXPECTF--
+Deprecated: Function test() is deprecated in %s
+
+Deprecated: Function test2() is deprecated, use test() instead in %s


Something like is deprecated. Reason: %s would be my preference for what may end up being seen in practice, such as Deprecated('inefficient'), 'will remove', 'Sentence 1. Sentence 2.', etc., but there's probably an argument that people should match deprecation messages to whatever format is expected by PHP and I'm not great at writing error messages.

Should this avoid the extra formatting for the empty string #[Deprecated('')]?

pronskiy · 2020-12-27T14:35:17Z

Zend/zend_attributes.stub.php

@@ -6,3 +6,8 @@ final class Attribute
 {
    public function __construct(int $flags = Attribute::TARGET_ALL) {}
 }
+
+final class Deprecated


Could you please elaborate what are the benefits of making it final?

Doesn't it make sense for similar attributes to inherit from this one? E.g. PhpStorm's deprecated attribute.

Good catch. For now the code uses an API that finds exact matches of Deprecated attribute usage, inheritance would require some changes. It should be possible to add them, i will look into it next time I work on the code.

If it wasn't final, then the php compiler wouldn't be able to do this check by name and cache it in opcache without knowing what other files contained. Opcache works on a single file at a time - knowing what the parent classes of a given class are is impossible when you know only a single file's contents, so even making it emit E_DEPRECATED for a subclass would be impractical to do without hurting performance and require attempting to autoload any user-defined attribute when a file is being compiled.

It also makes it much easier to read code - you don't have to know the parent classes to tell if something would emit a deprecation notice at runtime.

Speaking of which, it may be useful to prevent class_alias where the source is a built-in attribute (which has observable effects) because the aliased classes wouldn't actually have an effect, but that's out of scope.

You are right, i forgot it will require autoloading during compile, which does not really work

@TysonAndre, @beberlei, thanks for the clarification. I now understand the technical limitation.

I'm wondering if there is any other way to make the attributes compatible. Here are the ideas:

Match the signatures: (string $message) --> (string $message, string $replacement)

Add options: (string $message) --> (string $message, ...$options)

Split PhpStorm attribute into two separate: #[Deprecated] and #[DeprecatedReplacement]

While the latter is easier for this RFC, I believe such solution is not future-proof. I mean a similar issue may arise later with other attributes introduced and it might not be always possible to split.

What do you think?

/cc @nikic

iluuu1994 · 2022-06-26T20:56:51Z

@beberlei This has been stale for a while. Are you planning to pursue this RFC at some point?

beberlei marked this pull request as draft December 18, 2020 20:05

cmb69 added the RFC label Dec 19, 2020

iluuu1994 reviewed Dec 19, 2020

View reviewed changes

TysonAndre reviewed Dec 22, 2020

View reviewed changes

pronskiy reviewed Dec 27, 2020

View reviewed changes

iluuu1994 added the Waiting on Author label Jun 26, 2022

beberlei closed this May 2, 2023

beberlei force-pushed the DeprecatedAttribute branch from d1ed400 to 56d1cc7 Compare May 2, 2023 18:20

beberlei mentioned this pull request May 22, 2023

RFC: Add #[\Deprecated] Attribute #11293

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add #[Deprecated] attribute based on ZEND_ACC_DEPRECATED #6521

Add #[Deprecated] attribute based on ZEND_ACC_DEPRECATED #6521

beberlei commented Dec 18, 2020 •

edited

Loading

iluuu1994 left a comment

TysonAndre left a comment

TysonAndre Dec 22, 2020 •

edited

Loading

TysonAndre Dec 22, 2020

beberlei Dec 28, 2020

TysonAndre Dec 22, 2020

TysonAndre Dec 22, 2020

TysonAndre Dec 22, 2020

TysonAndre Dec 22, 2020

TysonAndre Dec 22, 2020

TysonAndre Dec 22, 2020

TysonAndre Dec 22, 2020

pronskiy Dec 27, 2020 •

edited

Loading

beberlei Dec 27, 2020

TysonAndre Dec 27, 2020

beberlei Dec 27, 2020

pronskiy Jan 4, 2021

iluuu1994 commented Jun 26, 2022

	* specifying a scope. The class is not fully linked yet, and we may seen an
	* specifying a scope. The class is not fully linked yet, and we may see an

Add #[Deprecated] attribute based on ZEND_ACC_DEPRECATED #6521

Add #[Deprecated] attribute based on ZEND_ACC_DEPRECATED #6521

Conversation

beberlei commented Dec 18, 2020 • edited Loading

iluuu1994 left a comment

Choose a reason for hiding this comment

TysonAndre left a comment

Choose a reason for hiding this comment

TysonAndre Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pronskiy Dec 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iluuu1994 commented Jun 26, 2022

beberlei commented Dec 18, 2020 •

edited

Loading

TysonAndre Dec 22, 2020 •

edited

Loading

pronskiy Dec 27, 2020 •

edited

Loading