Warn on "parent" without parent at compile-time by morrisonlevi · Pull Request #3404 · php/php-src

morrisonlevi · 2018-07-25T01:41:13Z

This adds a compile-time warning of level E_STRICT to usages of parent in class-scope where the class does not have a parent, e.g.

class InvalidClass {
  function method() {
    echo parent::class;
  }
}

This currently has two test failures where the E_STRICT is emitted twice. I was not sure if the check was always redundant or if in some cases it isn't. I am hoping a reviewer will know better than me.

If a method definition like this is executed it generates a runtime error, so this should not be a controversial change. In PHP 8.0 we should change this from E_STRICT to an E_COMPILE_ERROR.

Majkl578 · 2018-07-25T01:56:12Z

Zend/tests/compile_time_parent_error_01.phpt

+
+?>
+--EXPECTF--
+Strict Standards: Cannot use "parent" when no class scope is active in %s on line %d


This message is wrong, isn't it? Class scope is active, there is just no parent.

Oops, I copied the wrong line. Will fix.

nikic · 2018-07-25T08:44:44Z

Zend/zend_compile.c

+		           && !CG(active_class_entry_parent_name))
+		{
+			zend_error_noreturn(
+				E_STRICT,


We no longer introduce E_STRICT warnings, please use E_WARNING instead.

E_WARNING does not mean this will get changed in a future release. E_DEPRECATED seems odd. I think E_STRICT is the best choice, and unless my memory is failing me we never voted on not adding new E_STRICT; only to remove existing ones.

We decided to remove the E_STRICT category in https://wiki.php.net/rfc/reclassify_e_strict. If you'd like to go against this prior decision, this change is going to need an RFC.

Imho either E_DEPRECATED or E_WARNING are sensible choices.

This does not match my memory. I was pretty certain we only voted to reclassify existing ones, not remove the category entirely. I will choose E_DEPRECATED.

nikic · 2018-07-25T08:49:04Z

Zend/zend_compile.c

 	CG(active_class_entry) = ce;
+	if (extends_ast) {
+		zend_string *extends_name = zend_ast_get_str(extends_ast);
+		CG(active_class_entry_parent_name) = zend_string_copy(extends_name);


This assigns the non-resolved class name. Either this should assign the correct class name, or should just be a boolean for whether it has a parent, to prevent confusion when other code wants to use this member in the future.

I think other code will need the name of the parent; for instance we reject string $parent = parent::class in parameters but there's no need to actually do this now. Will investigate resolving the class name.

Just to check, what do you mean by "non-resolved"? I assume Bar in use Foo as Bar; class Baz extends Bar {} or something?

Yes, that's right. You can either use zend_resolve_class_name, or try to get the already resolved name from above, though extracting it from literals might be a bit inconvenient.

Dmitry has changed this code recently; it should be easy to use now.

nikic · 2018-07-25T08:50:47Z

Zend/zend_compile.c

+	if (extends_ast) {
+		zend_string *extends_name = zend_ast_get_str(extends_ast);
+		CG(active_class_entry_parent_name) = zend_string_copy(extends_name);
+	}


Shouldn't it be NULLed out in the else case? I think it will currently not behave correctly if you have an outer class with parent and an inner (anonymous) class without.

That sounds right. Will investigate.

morrisonlevi · 2018-07-26T00:43:36Z

I've fixed a number of issues and rebased on latest master. I have supposedly enabled support for parent::class in compile-time expressions when a parent exists but need tests for this.

There is still an annoyance: for compile-time expressions that use parent::class but are invalid because there isn't a parent then there will be both an E_DEPRECATED and a compile time error. Still not sure how to fix this one.

carusogabriel · 2018-07-30T00:57:12Z

Just a question: "Warn on "parent" without parent at compile-time"

compile-time: Do we have it here in the interpreter?

Majkl578 · 2018-07-30T11:09:52Z

Yes.

morrisonlevi · 2018-08-11T23:24:43Z

The debug build failed on Travis, but it works for me locally. I adjusted a few more tests, including some for parent::class in constant expressions.

This still needs entries in NEWS/UPGRADING but aside from that can anyone spot issues, or would like more tests in certain areas?

php-pulls · 2018-09-16T23:39:43Z

Comment on behalf of carusogabriel at php.net:

Labelling

> - Core > Using "parent" inside a class without parent is deprecated, and will throw > a compile-time error in the future. Currently an error will only be > generated if/when the parent is accessed at run-time. Refs: * https://github.com/php/php-src/blob/42cc58ff7b2fee1c17a00dc77a4873552ffb577f/UPGRADING#L303 * php/php-src#3404 * php/php-src#3564 * php/php-src@a9e6667 (originally committed as hard compile time error) * php/php-src@deb44d4 (reverted back to deprecation error)

Majkl578 reviewed Jul 25, 2018

View reviewed changes

morrisonlevi force-pushed the parent branch 2 times, most recently from 2180d20 to 4443e98 Compare July 25, 2018 02:13

nikic reviewed Jul 25, 2018

View reviewed changes

morrisonlevi force-pushed the parent branch from 4443e98 to 13d0d0c Compare July 26, 2018 00:38

Warn on "parent" without parent at compile-time

eb2fc79

morrisonlevi force-pushed the parent branch from 13d0d0c to eb2fc79 Compare August 11, 2018 22:17

php-pulls added the Feature label Sep 16, 2018

morrisonlevi closed this Sep 28, 2018

morrisonlevi deleted the parent branch September 28, 2018 21:12

morrisonlevi restored the parent branch September 28, 2018 21:12

morrisonlevi deleted the parent branch September 28, 2018 21:14

morrisonlevi mentioned this pull request Sep 28, 2018

Add compile-time warning on invalid parent use #3564

Closed

jrfnl mentioned this pull request Jun 14, 2019

PHP 7.4: new RemovedOrphanedParent sniff PHPCompatibility/PHPCompatibility#818

Merged

Conversation

morrisonlevi commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morrisonlevi commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carusogabriel commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Majkl578 commented Jul 30, 2018

Uh oh!

morrisonlevi commented Aug 11, 2018

Uh oh!

php-pulls commented Sep 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

morrisonlevi commented Jul 26, 2018 •

edited

Loading

carusogabriel commented Jul 30, 2018 •

edited

Loading