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

Default property with @NamedArgumentConstructor #402

Merged
merged 4 commits into from
Feb 14, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Values() will return an array of position
  • Loading branch information
Vincz committed Feb 6, 2021
commit f1f293ade13b63c6820d2d37bcdb84bbd2e00ce9
57 changes: 36 additions & 21 deletions lib/Doctrine/Common/Annotations/DocParser.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@

use function array_keys;
use function array_map;
use function array_values;
use function class_exists;
use function constant;
use function count;
use function defined;
use function explode;
use function gettype;
Expand Down Expand Up @@ -607,7 +609,7 @@ class_exists(NamedArgumentConstructor::class);

// choose the first property as default property
$metadata['default_property'] = reset($metadata['properties']);
} elseif (PHP_VERSION_ID < 80000 && $metadata['has_named_argument_constructor']) {
} elseif ($metadata['has_named_argument_constructor']) {
foreach ($constructor->getParameters() as $parameter) {
$metadata['constructor_args'][$parameter->getName()] = [
'position' => $parameter->getPosition(),
Expand Down Expand Up @@ -863,17 +865,36 @@ private function Annotation()
);
}

$defaultProperty = 'value';
// Change the default property only if the @NamedArgumentConstructor
// tag and the default_property are set
$arguments = $this->MethodCall();

$positionalArguments = $arguments['positional_arguments'] ?? [];
$namedArguments = $arguments['named_arguments'] ?? [];

if (
Copy link
Member

Choose a reason for hiding this comment

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

Could you extract this block into a private helper please?

$values = $this->assembleValues($arguments, $name);

Method name could be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

self::$annotationMetadata[$name]['has_named_argument_constructor']
&& self::$annotationMetadata[$name]['default_property'] !== null
) {
$defaultProperty = self::$annotationMetadata[$name]['default_property'];
}
$values = $namedArguments;
foreach (self::$annotationMetadata[$name]['constructor_args'] as $property => $parameter) {
$position = $parameter['position'];
if (isset($values[$property]) || ! isset($positionalArguments[$position])) {
continue;
}

$values[$property] = $positionalArguments[$position];
}
} else {
$values = $namedArguments;
if (! empty($positionalArguments) && ! isset($values['value'])) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use count instead, empty does not communicate intent well

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't count() > 0 much less effective than !empty() ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but the output of this code is usually cached and even if not, this is not a tight loop

Copy link
Member

Choose a reason for hiding this comment

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

I wanted to toy with phpbench so I build a little benchmark from this raw benchmark:

<?php

class ArrayCheckerBench
{
    public function provideArrays(): \Generator
    {
        yield 'empty array' => ['array' => []];
        yield 'huge array' => ['array' => range(0, 100000)];
    }

    public function checkArrayWithEmpty(array $x): bool
    {
        return empty($x);
    }

    public function checkArrayWithCount(array $x): bool
    {
        return count($x) === 0;
    }

    /**
     * @Revs(1000000)
     * @Iterations(5)
     * @ParamProviders({"provideArrays"})
     */
    public function benchCheckIfArrayIsEmptyWithEmpty(array $parameters): void
    {
        $this->checkArrayWithEmpty($parameters['array']);
    }

    /**
     * @Revs(1000000)
     * @Iterations(5)
     * @ParamProviders({"provideArrays"})
     */
    public function benchCheckIfArrayIsEmptyWithCount(array $parameters): void
    {
        $this->checkArrayWithCount($parameters['array']);
    }
}

Here are the results:

vendor/bin/phpbench run ArrayCheckerBench.php --report=aggregate --retry-threshold=0.5
PhpBench @git_tag@. Running benchmarks.

\ArrayCheckerBench

    benchCheckIfArrayIsEmptyWithEmpty # emp.I4 [μ Mo]/r: 0.285 0.285 (μs) [μSD μRSD]/r: 0.000μs 0.06%
    benchCheckIfArrayIsEmptyWithEmpty # hug.R1 I1 [μ Mo]/r: 0.287 0.287 (μs) [μSD μRSD]/r: 0.000μs 0.12%
    benchCheckIfArrayIsEmptyWithCount # emp.R1 I1 [μ Mo]/r: 0.309 0.309 (μs) [μSD μRSD]/r: 0.001μs 0.28%
    benchCheckIfArrayIsEmptyWithCount # hug.R3 I3 [μ Mo]/r: 0.311 0.311 (μs) [μSD μRSD]/r: 0.001μs 0.34%

2 subjects, 20 iterations, 4,000,000 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 0.285 [0.298 0.298] 0.285 (μs)
⅀T: 5.958μs μSD/r 0.001μs μRSD/r: 0.201%
suite: 134621f1b80d1bb3f7ed3eaf6bc32c7bb639d7d1, date: 2021-02-07, stime: 15:08:37
+-------------------+-----------------------------------+-------------+---------+-----+-------------+---------+---------+---------+---------+---------+--------+-------+
| benchmark         | subject                           | set         | revs    | its | mem_peak    | best    | mean    | mode    | worst   | stdev   | rstdev | diff  |
+-------------------+-----------------------------------+-------------+---------+-----+-------------+---------+---------+---------+---------+---------+--------+-------+
| ArrayCheckerBench | benchCheckIfArrayIsEmptyWithEmpty | empty array | 1000000 | 5   | 526,184b    | 0.285μs | 0.285μs | 0.285μs | 0.285μs | 0.000μs | 0.06%  | 1.00x |
| ArrayCheckerBench | benchCheckIfArrayIsEmptyWithEmpty | huge array  | 1000000 | 5   | 64,194,552b | 0.287μs | 0.287μs | 0.287μs | 0.288μs | 0.000μs | 0.12%  | 1.01x |
| ArrayCheckerBench | benchCheckIfArrayIsEmptyWithCount | empty array | 1000000 | 5   | 526,184b    | 0.308μs | 0.309μs | 0.309μs | 0.310μs | 0.001μs | 0.28%  | 1.08x |
| ArrayCheckerBench | benchCheckIfArrayIsEmptyWithCount | huge array  | 1000000 | 5   | 64,194,552b | 0.310μs | 0.311μs | 0.311μs | 0.313μs | 0.001μs | 0.34%  | 1.09x |
+-------------------+-----------------------------------+-------------+---------+-----+-------------+---------+---------+---------+---------+---------+--------+-------+

Looks like count takes almost 10% more time, so in relative terms, it's far better indeed. In absolute terms though, it seems to be 30 ns slower… I don't think we should worry about such small differences.

if (count($positionalArguments) === 1) {
$value = $positionalArguments[0];
} else {
$value = array_values($positionalArguments);
}

$values = $this->MethodCall($defaultProperty);
$values['value'] = $value;
}
}

if (isset(self::$annotationMetadata[$name]['enum'])) {
// checks all declared attributes
Expand Down Expand Up @@ -1027,7 +1048,7 @@ private function Annotation()
* @throws AnnotationException
* @throws ReflectionException
*/
private function MethodCall(string $defaultProperty): array
private function MethodCall(): array
{
$values = [];

Expand All @@ -1038,7 +1059,7 @@ private function MethodCall(string $defaultProperty): array
$this->match(DocLexer::T_OPEN_PARENTHESIS);

if (! $this->lexer->isNextToken(DocLexer::T_CLOSE_PARENTHESIS)) {
$values = $this->Values($defaultProperty);
$values = $this->Values();
}

$this->match(DocLexer::T_CLOSE_PARENTHESIS);
Expand All @@ -1054,7 +1075,7 @@ private function MethodCall(string $defaultProperty): array
* @throws AnnotationException
* @throws ReflectionException
*/
private function Values(string $defaultProperty): array
private function Values(): array
{
$values = [$this->Value()];

Expand All @@ -1075,23 +1096,17 @@ private function Values(string $defaultProperty): array
$values[] = $value;
}

$namedArguments = [];
$positionalArguments = [];
foreach ($values as $k => $value) {
if (is_object($value) && $value instanceof stdClass) {
$values[$value->name] = $value->value;
} elseif (! isset($values[$defaultProperty])) {
$values[$defaultProperty] = $value;
$namedArguments[$value->name] = $value->value;
} else {
if (! is_array($values[$defaultProperty])) {
$values[$defaultProperty] = [$values[$defaultProperty]];
}

$values[$defaultProperty][] = $value;
$positionalArguments[$k] = $value;
}

unset($values[$k]);
}

return $values;
return ['named_arguments' => $namedArguments, 'positional_arguments' => $positionalArguments];
Copy link
Member

Choose a reason for hiding this comment

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

In resolvePositionalValues, you validate $positionalArguments. Can't we do this here already? I don't like that we pass around potentially invalid metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Values() method is part of the parsing process. If we move the validation inside, it will have to know what kind of annotation we are talking about. The validation is different is you use @NamedArgumentConstructor or not.
This is valid without @Name("1", foo="bar", "2") but should not be valid with @NamedArgumentConstructor as it's supposed to mimic a regular constructor call with named arguments.

Copy link
Member

Choose a reason for hiding this comment

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

All right, thank you for clarifying. 👍🏻

}

/**
Expand Down