Skip to content
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

Implement "Constructor Promotion" #5291

Closed
wants to merge 8 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Mar 24, 2020

RFC: https://wiki.php.net/rfc/constructor_promotion

As recently discussed on list, this implements the following short hand syntax:

class Point {
    public function __construct(
        public float $x = 0.0,
        public float $y = 0.0,
        public float $z = 0.0,
    ) {}
}

This desugars to:

class Point {
    public float $x;
    public float $y;
    public float $z;

    public function __construct(
        float $x = 0.0,
        float $y = 0.0,
        float $z = 0.0
    ) {
        $this->x = $x;
        $this->y = $y;
        $this->z = $z;
    }
}

zend_class_entry *scope = op_array->scope;
zend_bool is_ctor =
scope && zend_string_equals_literal_ci(op_array->function_name, "__construct");
if (!is_ctor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could add a test of global functions/closures - I'd guess that the following snippet probably has a misleading error message (not sure what scope->properties_info is)

<?php
function __construct(public $x) {}

LGTM otherwise

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test in 717134e, error message seems fine. Or did you expect something different there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry, I missed that you had function __construct() in particular in mind here. Changed the name in 7139cdc.

Still works fine though, because is_ctor above includes a check that scope is not null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, didn't see the scope part of the check

@enumag
Copy link

enumag commented Mar 26, 2020

Somehow I'm more interested in the TODO: Allow comma here. 😛

@okdewit
Copy link

okdewit commented Mar 26, 2020

@nikic Is there a reason to not go a step further, and use a "Kotlin style" syntax?

class Point (
    public float $x = 0.0,
    public float $y = 0.0,
    public float $z = 0.0,
) {
    //
}

@TysonAndre
Copy link
Contributor

inline_function:
...
	|	fn returns_ref '(' parameter_list ')' return_type backup_doc_comment T_DOUBLE_ARROW backup_fn_flags backup_lex_pos expr backup_fn_flags
			{ $$ = zend_ast_create_decl(ZEND_AST_ARROW_FUNC, $2 | $12, $1, $7,
				  zend_string_init("{closure}", sizeof("{closure}") - 1, 0), $4, NULL,
				  zend_ast_create(ZEND_AST_RETURN, $11), $6);
				  ((zend_ast_decl *) $$)->lex_pos = $10;
				  CG(extra_fn_flags) = $9; }

After this change, static arrow functions with parameters don't get the doc comments parsed properly in Reflection.

It should be easy to fix by changing fn returns_ref '(' parameter_list ')' return_type backup_doc_comment to fn returns_ref backup_doc_comment '(' parameter_list ')' return_type and renumbering $n, unless there's a reason not to.

An example of affected code: $c = /** doc comment */ static fn(?int... $args): array => $args;

@carusogabriel carusogabriel added this to the PHP 8.0 milestone May 30, 2020
@nikic
Copy link
Member Author

nikic commented Jun 5, 2020

@TysonAndre Nice catch! It should be fixed now.

public function __construct(
public float $x = 0.0,
public float $y = 1.0,
public float $z = 2.0

Choose a reason for hiding this comment

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

In the RFC code example the trailing comma is allowed, but not here.

Which is correct?

Choose a reason for hiding this comment

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

This is the subject of different RFC

Copy link

Choose a reason for hiding this comment

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

@FlorianSteenbuck
Copy link

Is there any reflection possibility for visibility ?

@nikic
Copy link
Member Author

nikic commented Jan 5, 2021

@FlorianSteenbuck The visibility should be accessible through ReflectionProperty as usual.

@FlorianSteenbuck
Copy link

FlorianSteenbuck commented Jan 5, 2021

@nikic ok: https://3v4l.org/JsbXT

<?php

class Point {
    public function __construct(
        public float $x,
        public float $y = 0.0,
        public float $z = 0.0,
    ) {}
}

foreach ((new ReflectionClass(Point::class))->getProperties() as $property) {
    var_dump($property->name);
}
string(1) "x"
string(1) "y"
string(1) "z"

@zlianon
Copy link

zlianon commented May 5, 2021

@nikic what is the reason for which it is not allowed to declare promoted property in an abstract constructor?

interface InstructionInterface
{
    /**
     * InstructionInterface constructor.
     *
     * @param mixed $operand
     */
    public function __construct(
        private mixed $operand,
    );

    /**
     * @param SplStack $stack
     */
    public function __invoke(SplStack $stack): void;
}

@nikic
Copy link
Member Author

nikic commented May 5, 2021

Promoted properties combine a property declaration with initialization of that property in the constructor. There is no way to initialize the property in an abstract constructor.

Your example is doubly illegal because it would also require declaring a property in an interface, which is not allowed.

franzholz added a commit to franzholz/typo3db_legacy that referenced this pull request Jan 15, 2022
 * RemoveUnusedVariableInCatchRector (https://wiki.php.net/rfc/non-capturing_catches)
 * ClassPropertyAssignToConstructorPromotionRector (https://wiki.php.net/rfc/constructor_promotion php/php-src#5291)
 * StrContainsRector (https://externals.io/message/108562 php/php-src#5179)
Davidmattei added a commit to Davidmattei/elasticms that referenced this pull request Dec 1, 2022
Applied rules:
 * ClassPropertyAssignToConstructorPromotionRector (https://wiki.php.net/rfc/constructor_promotion php/php-src#5291)
 * ReadOnlyPropertyRector (https://wiki.php.net/rfc/readonly_properties_v2)
Davidmattei added a commit to Davidmattei/elasticms that referenced this pull request Dec 1, 2022
Applied rules:
 * ClassPropertyAssignToConstructorPromotionRector (https://wiki.php.net/rfc/constructor_promotion php/php-src#5291)
 * ReadOnlyPropertyRector (https://wiki.php.net/rfc/readonly_properties_v2)
Davidmattei added a commit to Davidmattei/elasticms that referenced this pull request Dec 1, 2022
Applied rules:
 * ClassPropertyAssignToConstructorPromotionRector (https://wiki.php.net/rfc/constructor_promotion php/php-src#5291)
 * MixedTypeRector
 * ReadOnlyPropertyRector (https://wiki.php.net/rfc/readonly_properties_v2)
Davidmattei added a commit to Davidmattei/elasticms that referenced this pull request Dec 1, 2022
Davidmattei added a commit to Davidmattei/elasticms that referenced this pull request Dec 1, 2022
Applied rules:
 * ClassPropertyAssignToConstructorPromotionRector (https://wiki.php.net/rfc/constructor_promotion php/php-src#5291)
 * StrStartsWithRector (https://wiki.php.net/rfc/add_str_starts_with_and_ends_with_functions)
 * NullToStrictStringFuncCallArgRector
 * ReadOnlyPropertyRector (https://wiki.php.net/rfc/readonly_properties_v2)
georgfranz added a commit to franz-agency/zetacomponents-Database that referenced this pull request Dec 11, 2023
Applied rules:
 * LongArrayToShortArrayRector
 * TernaryToNullCoalescingRector
 * ClassPropertyAssignToConstructorPromotionRector (https://wiki.php.net/rfc/constructor_promotion php/php-src#5291)
 * MixedTypeRector
 * ChangeSwitchToMatchRector (https://wiki.php.net/rfc/match_expression_v2)
 * NullToStrictStringFuncCallArgRector
 * TypedPropertyFromAssignsRector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants