Skip to content

"::class resolution as scalar" Feature #187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

ralphschindler
Copy link
Contributor

FOR RFC: https://wiki.php.net/rfc/class_name_scalars

Patch addresses:

  • Allows for Name::class, self::class, static::class, parent::class resolution to a scalar based on current use rules and current namespace.
  • Reuses existing keyword "class"
  • Alters zend_compile.c\zend_resolve_class_name() to facilitate compile time class name resolution (and runtime by way of FCALL)

* Allows for Name::class, self::class, static::class, parent::class resolution to a scalar based on current use rules and current namespace.
* Reuses existing keyword "class"
* Alters zend_compile.c\zend_resolve_class_name() to facilitate compile time class name resolution (and runtime by way of FCALL)
* Added class scope checking for self,parent+static
* Fixed tab/spaces in phpt file
@laruence
Copy link
Member

laruence commented Sep 9, 2012

just a quick test with your patch, got a memleak:

<?php

class b {
  public function dummy() {
      echo self::class;
    }
}

lead to a memleak:

[Sun Sep  9 22:19:52 2012]  Script:  '/tmp/1.php'
Zend/zend_language_scanner.l(1907) :  Freeing 0x2B1498A80288 (5 bytes), script=/tmp/1.php
=== Total 1 memory leaks detected ===

@ralphschindler
Copy link
Contributor Author

I've located the leak and will work the fix out, thanks- (I forgot to compile with --enable-debug).

* Fixed memory leak by adding zval_dtor() on class_name in zend_compile.c's zend_resolve_class_name()
@@ -0,0 +1,56 @@
--TEST--
class name as scaler from ::class keyword
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicking: scaler -> scalar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Thanks for pointing this out.

I am aware that this implementation needs more work (there are additional edge cases). But I wanted to bring it to a vote to ensure that additional work on this feature would not ultimately be all-for-naught, as outside of core/master inclusion, this feature is effectively useless.

@lstrojny
Copy link
Contributor

I am working on a few more tests and some remaining issues (parent::class in type hints and constants e.g.)

@lstrojny
Copy link
Contributor

@ralphschindler would love to get this merged, as the RFC ended positively. Could you drop me a message to discuss the remaining work?

@ralphschindler
Copy link
Contributor Author

I need to rework the patch as per the conversations that were had on the internals list. My plan is to do it during the holidays this week (American Thanksgiving), so I should be able to get this updated by end of week.

In its current state, it's not good for merge.

@lstrojny
Copy link
Contributor

@ralphschindler, cool, thanks! If you need help with anything, drop me a note.

@lstrojny
Copy link
Contributor

lstrojny commented Dec 2, 2012

@ralphschindler any news?

@lstrojny
Copy link
Contributor

@ralphschindler ping

@lstrojny
Copy link
Contributor

lstrojny commented Jan 6, 2013

@ralphschindler ping again.

@nikic
Copy link
Member

nikic commented Jan 6, 2013

@ralphschindler By the way, if you want this in 5.5 you need to hurry. We're close to feature freeze ;)

@ralphschindler
Copy link
Contributor Author

@nikic I have it mostly working, in two different forms, any chance you could help me bring it to completion?

@nikic
Copy link
Member

nikic commented Jan 7, 2013

@ralphschindler Depends on how much help you need, but in principle yes :) What are you still missing / what issues are you having with the implementation?

@ralphschindler
Copy link
Contributor Author

@nikic I will push what I have to my github account, and give you a summary of the decisions I need to make.

Mostly, I need to decide how to handle situations like this:

class Foo {
    public function bar(static::class $foo, parent::class $bar, self::class $baz) {}
}

I believe I had self::class working in that scenario, but the other two were not with the current implementation of the FETCH_CLASS_CONST.

@nikic
Copy link
Member

nikic commented Jan 8, 2013

@ralphschindler Assuming you mean $foo = static::class etc.

Parameter initializers (and all other initializers) in PHP only accept compile-time scalars. As static::class and parent::class are not resolvable at compile time they should just throw an error there. parent::class is theoretically resolvable at compile-time (because it only needs the name and not the actual CE) and you can obtain it by checking the op array for the FETCH_CLASS op of the class declaration. But I don't think that it's worth adding such a hack for something nobody will do anyway (what's supposed to be the use for initializing a parameter to the name of the parent class?)

@lstrojny
Copy link
Contributor

lstrojny commented Jan 8, 2013

I agree with @nikic here. If parent and static throw an error, that’s more than fine.

@ralphschindler
Copy link
Contributor Author

Since I decided to go in a different direction with the implementation and wanted to keep a clean commit history, I re-branched from master and applied my fixes there and will open up a new PR. see #256

@Majkl578
Copy link
Contributor

Majkl578 commented Jan 9, 2013

@ralphschindler: You don't need a new pull request, just force-push to this branch, the history will update here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants