From 7f0f2e3e1ff48e14dfcde6c78478de09619ea3de Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 21:22:01 +0000 Subject: [PATCH 1/3] Skip class-level attribute groups during class/enum to trait conversion A ClassLike node's startTokenPos includes its attribute groups, so convertClassToTrait()/convertEnumToTrait()/adjustOriginalTrait() started scanning inside '#[...]': the first T_STRING of the attribute was renamed to the __AopProxied trait name and the delete-until-'{' step then removed the attribute remainder together with the real class header, producing invalid output like '#[Foo__AopProxied {'. The scan now starts right after the last attribute group of the node. Class-level attributes are kept untouched on the woven trait, where they are legal. Fixes #598 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WFMYyvE4hYRUoMS8mtKrHP --- .../Transformer/WeavingTransformer.php | 33 +++++++++++++++++-- .../Transformer/WeavingTransformerTest.php | 16 +++++++++ .../_files/php80-class-attribute-woven.php | 29 ++++++++++++++++ .../_files/php80-class-attribute.php | 27 +++++++++++++++ 4 files changed, 102 insertions(+), 3 deletions(-) create mode 100644 tests/Instrument/Transformer/_files/php80-class-attribute-woven.php create mode 100644 tests/Instrument/Transformer/_files/php80-class-attribute.php diff --git a/src/Instrument/Transformer/WeavingTransformer.php b/src/Instrument/Transformer/WeavingTransformer.php index f18a05f7..7ef2f3a3 100644 --- a/src/Instrument/Transformer/WeavingTransformer.php +++ b/src/Instrument/Transformer/WeavingTransformer.php @@ -30,6 +30,7 @@ use Go\Proxy\EnumProxyGenerator; use Go\Proxy\FunctionProxyGenerator; use Go\Proxy\TraitProxyGenerator; +use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\EnumCase; use PhpParser\Node\Stmt\Property; use ReflectionProperty; @@ -205,7 +206,7 @@ private function adjustOriginalTrait( string $newClassName ): void { $classNode = $class->getNode(); - $position = $classNode->getAttribute('startTokenPos'); + $position = $this->getPositionAfterAttributeGroups($classNode); if (!is_int($position)) { return; } @@ -223,6 +224,32 @@ private function adjustOriginalTrait( } while (true); } + /** + * Returns the token position where the class/enum declaration scan should start. + * + * A ClassLike node's startTokenPos includes its attribute groups (`#[...]`), so scanning + * from there would rename the first T_STRING inside the attribute to the trait name and + * then delete the real class header (see https://github.com/goaop/framework/issues/598). + * Class-level attributes are kept as-is on the generated trait — attributes are legal + * on traits — so the scan starts right after the last attribute group. + */ + private function getPositionAfterAttributeGroups(ClassLike $classNode): ?int + { + $position = $classNode->getAttribute('startTokenPos'); + if (!is_int($position)) { + return null; + } + $lastAttrGroup = end($classNode->attrGroups); + if ($lastAttrGroup !== false) { + $attrGroupsEnd = $lastAttrGroup->getAttribute('endTokenPos'); + if (is_int($attrGroupsEnd)) { + $position = $attrGroupsEnd + 1; + } + } + + return $position; + } + /** * Convert a regular class declaration into a trait for the trait-based AOP engine. * @@ -241,7 +268,7 @@ private function convertClassToTrait( string $newClassName ): void { $classNode = $class->getNode(); - $position = $classNode->getAttribute('startTokenPos'); + $position = $this->getPositionAfterAttributeGroups($classNode); if (!is_int($position)) { return; } @@ -327,7 +354,7 @@ private function convertEnumToTrait( string $newClassName ): void { $classNode = $class->getNode(); - $position = $classNode->getAttribute('startTokenPos'); + $position = $this->getPositionAfterAttributeGroups($classNode); if (!is_int($position)) { return; } diff --git a/tests/Instrument/Transformer/WeavingTransformerTest.php b/tests/Instrument/Transformer/WeavingTransformerTest.php index faf812cf..ece9e7fe 100644 --- a/tests/Instrument/Transformer/WeavingTransformerTest.php +++ b/tests/Instrument/Transformer/WeavingTransformerTest.php @@ -377,6 +377,22 @@ public function testWeaverCopiesNonScalarAttributeArgumentsFromAst(): void $this->assertStringNotContainsString((string) PHP_INT_MAX, $actualProxyContent); } + /** + * Class-level attributes (with and without arguments) must survive the class→trait + * conversion untouched (issue #598). Previously the first token inside `#[...]` was + * renamed to the trait name and the rest of the attribute plus the real class header + * was deleted, producing a parse error like `#[Foo__AopProxied {`. + */ + public function testWeaverKeepsClassLevelAttributesOnWovenTrait(): void + { + $metadata = $this->loadTestMetadata('php80-class-attribute'); + $this->transformer->transform($metadata); + + $actual = $this->normalizeWhitespaces($metadata->source); + $expected = $this->normalizeWhitespaces($this->loadTestMetadata('php80-class-attribute-woven')->source); + $this->assertEquals($expected, $actual); + } + public function testWeaverMovesInterceptedPropertiesToProxyHooks(): void { $adviceMatcher = $this->createMock(AdviceMatcherInterface::class); diff --git a/tests/Instrument/Transformer/_files/php80-class-attribute-woven.php b/tests/Instrument/Transformer/_files/php80-class-attribute-woven.php new file mode 100644 index 00000000..097134b1 --- /dev/null +++ b/tests/Instrument/Transformer/_files/php80-class-attribute-woven.php @@ -0,0 +1,29 @@ + Date: Fri, 28 Aug 2026 21:23:48 +0000 Subject: [PATCH 2/3] Do not rewrite new expressions inside constant-expression contexts ConstructorExecutionTransformer rewrote every New_ node in the file, including PHP 8.1 'new in initializers' occurrences (parameter defaults, static variable initializers, attribute arguments, constants). The rewritten form '...getInstance()->{Foo::class}(...)' is not a valid constant expression, so any woven file using new in an initializer failed with a compile-time fatal error. The finding pass is now a dedicated NewExpressionFinderVisitor that tracks entry/exit of constant-expression subtrees (Param, StaticVar, PropertyItem and Const_ initializers, EnumCase values, and whole Attribute nodes) and only collects new expressions outside of them. Runtime code nested inside those containers, such as property hook bodies on promoted parameters, is still rewritten. Fixes #603 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WFMYyvE4hYRUoMS8mtKrHP --- .../ConstructorExecutionTransformer.php | 10 +- .../NewExpressionFinderVisitor.php | 104 ++++++++++++++++++ .../ConstructorExecutionTransformerTest.php | 36 +++++- 3 files changed, 143 insertions(+), 7 deletions(-) create mode 100644 src/Instrument/Transformer/NewExpressionFinderVisitor.php diff --git a/src/Instrument/Transformer/ConstructorExecutionTransformer.php b/src/Instrument/Transformer/ConstructorExecutionTransformer.php index 0c6e506c..4ffa4c72 100644 --- a/src/Instrument/Transformer/ConstructorExecutionTransformer.php +++ b/src/Instrument/Transformer/ConstructorExecutionTransformer.php @@ -14,11 +14,8 @@ use Go\Aop\Framework\ReflectionConstructorInvocation; use Go\Aop\InitializationAware; -use PhpParser\Node; -use PhpParser\Node\Expr\New_; use PhpParser\Node\Name; use PhpParser\NodeTraverser; -use PhpParser\NodeVisitor\FindingVisitor; /** * Transforms the source code to add an ability to intercept new instances creation @@ -57,15 +54,16 @@ public static function getInstance(): self */ public function transform(StreamMetaData $metadata): TransformerResultEnum { - $newExpressionFinder = new FindingVisitor(fn(Node $node) => $node instanceof New_); + // Skips `new` inside constant-expression contexts (parameter defaults, static var + // initializers, attribute arguments, constants, enum cases) — see issue #603. + $newExpressionFinder = new NewExpressionFinderVisitor(); // TODO: move this logic into walkSyntaxTree(Visitor $nodeVistor) method $traverser = new NodeTraverser(); $traverser->addVisitor($newExpressionFinder); $traverser->traverse($metadata->syntaxTree); - /** @var Node\Expr\New_[] $newExpressions */ - $newExpressions = $newExpressionFinder->getFoundNodes(); + $newExpressions = $newExpressionFinder->getFoundNewExpressions(); if (empty($newExpressions)) { return TransformerResultEnum::RESULT_ABSTAIN; diff --git a/src/Instrument/Transformer/NewExpressionFinderVisitor.php b/src/Instrument/Transformer/NewExpressionFinderVisitor.php new file mode 100644 index 00000000..d57649e5 --- /dev/null +++ b/src/Instrument/Transformer/NewExpressionFinderVisitor.php @@ -0,0 +1,104 @@ + + * + * This source file is subject to the license that is bundled + * with this source code in the file LICENSE. + */ + +namespace Go\Instrument\Transformer; + +use PhpParser\Node; +use PhpParser\Node\Attribute; +use PhpParser\Node\Const_; +use PhpParser\Node\Expr\New_; +use PhpParser\Node\Param; +use PhpParser\Node\PropertyItem; +use PhpParser\Node\StaticVar; +use PhpParser\Node\Stmt\EnumCase; +use PhpParser\NodeVisitorAbstract; + +/** + * Finds all `new` expressions that are legal to rewrite into runtime interceptor calls. + * + * Since PHP 8.1 `new` may appear inside constant-expression contexts: parameter default + * values, static variable initializers, attribute arguments and global constants (and + * php-parser also accepts it in property/class-constant defaults and enum case values). + * Such occurrences must stay untouched — the interceptor rewrite + * `...getInstance()->{Foo::class}(...)` is not a valid constant expression and would + * trigger a compile-time fatal error (https://github.com/goaop/framework/issues/603). + */ +final class NewExpressionFinderVisitor extends NodeVisitorAbstract +{ + /** + * @var list + */ + private array $newExpressions = []; + + /** + * Object ids of subtree roots that are constant-expression contexts + * + * @var array + */ + private array $constExprRoots = []; + + /** + * How deep the traversal currently is inside constant-expression subtrees + */ + private int $constExprDepth = 0; + + /** + * @return list All found `new` expressions outside of constant-expression contexts + */ + public function getFoundNewExpressions(): array + { + return $this->newExpressions; + } + + public function enterNode(Node $node): null + { + if ($node instanceof Attribute) { + // Attribute arguments are always constant expressions — skip the whole attribute. + // Property hooks on promoted parameters contain runtime code, so for the other + // containers only the initializer child expression is marked, not the container. + $this->constExprRoots[spl_object_id($node)] = true; + } else { + $constExpr = match (true) { + $node instanceof Param => $node->default, + $node instanceof StaticVar => $node->default, + $node instanceof PropertyItem => $node->default, + $node instanceof Const_ => $node->value, + $node instanceof EnumCase => $node->expr, + default => null, + }; + if ($constExpr !== null) { + $this->constExprRoots[spl_object_id($constExpr)] = true; + } + } + + if (isset($this->constExprRoots[spl_object_id($node)])) { + ++$this->constExprDepth; + } + + if ($this->constExprDepth === 0 && $node instanceof New_) { + $this->newExpressions[] = $node; + } + + return null; + } + + public function leaveNode(Node $node): null + { + $nodeId = spl_object_id($node); + if (isset($this->constExprRoots[$nodeId])) { + --$this->constExprDepth; + unset($this->constExprRoots[$nodeId]); + } + + return null; + } +} diff --git a/tests/Instrument/Transformer/ConstructorExecutionTransformerTest.php b/tests/Instrument/Transformer/ConstructorExecutionTransformerTest.php index 13ccfd7b..c5a84628 100644 --- a/tests/Instrument/Transformer/ConstructorExecutionTransformerTest.php +++ b/tests/Instrument/Transformer/ConstructorExecutionTransformerTest.php @@ -92,7 +92,41 @@ public static function listOfExpressions(): array [ '$n = new stdClass(new static::$object[0]->name)', '$n = \Go\Instrument\Transformer\ConstructorExecutionTransformer::getInstance()->{stdClass::class}(\Go\Instrument\Transformer\ConstructorExecutionTransformer::getInstance()->{static::$object[0]->name})' - ] + ], + // PHP 8.1 new in initializers (issue #603): `new` inside constant-expression + // contexts must stay untouched — the rewrite is not a valid constant expression. + 'parameter default value' => [ + 'function a($helper = new stdClass("x")) { return $helper; }', + 'function a($helper = new stdClass("x")) { return $helper; }' + ], + 'static variable initializer' => [ + 'function b() { static $memo = new \ArrayObject(); return $memo; }', + 'function b() { static $memo = new \ArrayObject(); return $memo; }' + ], + 'global constant initializer' => [ + 'const GLOBAL_SERVICE = new stdClass', + 'const GLOBAL_SERVICE = new stdClass' + ], + 'attribute argument' => [ + '#[SomeAttr(new stdClass)] function c() {}', + '#[SomeAttr(new stdClass)] function c() {}' + ], + 'parameter default kept while body is still rewritten' => [ + 'function d($helper = new stdClass) { return new stdClass; }', + 'function d($helper = new stdClass) { return \Go\Instrument\Transformer\ConstructorExecutionTransformer::getInstance()->{stdClass::class}; }' + ], + 'static variable kept while body is still rewritten' => [ + 'function e() { static $memo = new stdClass; $memo->x = new stdClass(); return $memo; }', + 'function e() { static $memo = new stdClass; $memo->x = \Go\Instrument\Transformer\ConstructorExecutionTransformer::getInstance()->{stdClass::class}(); return $memo; }' + ], + 'nested new inside parameter default' => [ + 'function f($helper = new stdClass(new stdClass())) {}', + 'function f($helper = new stdClass(new stdClass())) {}' + ], + 'promoted property hook body is still rewritten' => [ + 'class G { public function __construct(public stdClass $h = new stdClass { get { return new stdClass; } }) {} }', + 'class G { public function __construct(public stdClass $h = new stdClass { get { return \Go\Instrument\Transformer\ConstructorExecutionTransformer::getInstance()->{stdClass::class}; } }) {} }' + ], ]; } } From c52e07282825044c0dc6175ef55d20c4a5090c84 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 21:34:20 +0000 Subject: [PATCH 3/3] Demote intercepted promoted constructor properties in the woven trait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Property interception previously commented out the matched promoted parameter inside the constructor signature. A single-line constructor turned into a parse error (the line comment swallowed the closing parenthesis), and a multi-line constructor silently lost its parameters: the proxy kept the original signature and delegated to the now parameterless trait constructor, dropping all arguments and leaving typed properties uninitialized. The woven trait now demotes the promoted parameter to a plain parameter instead — only the promotion modifiers (visibility, asymmetric set visibility, readonly, final) are removed, keeping attributes, type and default value — and explicit '$this->prop = $prop;' assignments are injected on the opening brace line of the constructor body, preserving original line numbers. The assignments route through the proxy's generated property hooks, so writes performed during construction are intercepted like any other write. The proxy-side hook property now also carries the original default value: promoted properties report hasDefaultValue() as false because the default formally belongs to the parameter, so the generator reads it from the Param AST node. Covered by golden fixtures for multi-line and single-line constructors (including PHP 8.4 asymmetric visibility), a PHP 8.5-gated test for 'final' promoted parameters, and a functional weaving test with a real property aspect. Fixes #599 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WFMYyvE4hYRUoMS8mtKrHP --- .../Transformer/WeavingTransformer.php | 154 ++++++++++++++++++ .../AbstractInterceptedPropertyGenerator.php | 68 +++++--- .../src/Application/PromotedPropertyClass.php | 24 +++ .../Application/SingleLinePromotedClass.php | 13 ++ .../PromotedPropertyInterceptAspect.php | 26 +++ .../src/Kernel/DefaultAspectKernel.php | 2 + tests/Functional/ClassWeavingTest.php | 21 +++ .../Stubs/FinalPromotedClass85.php | 24 +++ .../Transformer/WeavingTransformerTest.php | 125 ++++++++++++++ .../_files/php80-promoted-property-proxy.php | 55 +++++++ ...80-promoted-property-single-line-proxy.php | 36 ++++ ...80-promoted-property-single-line-woven.php | 14 ++ .../php80-promoted-property-single-line.php | 13 ++ .../_files/php80-promoted-property-woven.php | 25 +++ .../_files/php80-promoted-property.php | 24 +++ 15 files changed, 599 insertions(+), 25 deletions(-) create mode 100644 tests/Fixtures/project/src/Application/PromotedPropertyClass.php create mode 100644 tests/Fixtures/project/src/Application/SingleLinePromotedClass.php create mode 100644 tests/Fixtures/project/src/Aspect/PromotedPropertyInterceptAspect.php create mode 100644 tests/Instrument/Transformer/Stubs/FinalPromotedClass85.php create mode 100644 tests/Instrument/Transformer/_files/php80-promoted-property-proxy.php create mode 100644 tests/Instrument/Transformer/_files/php80-promoted-property-single-line-proxy.php create mode 100644 tests/Instrument/Transformer/_files/php80-promoted-property-single-line-woven.php create mode 100644 tests/Instrument/Transformer/_files/php80-promoted-property-single-line.php create mode 100644 tests/Instrument/Transformer/_files/php80-promoted-property-woven.php create mode 100644 tests/Instrument/Transformer/_files/php80-promoted-property.php diff --git a/src/Instrument/Transformer/WeavingTransformer.php b/src/Instrument/Transformer/WeavingTransformer.php index 7ef2f3a3..4e11eaea 100644 --- a/src/Instrument/Transformer/WeavingTransformer.php +++ b/src/Instrument/Transformer/WeavingTransformer.php @@ -30,6 +30,7 @@ use Go\Proxy\EnumProxyGenerator; use Go\Proxy\FunctionProxyGenerator; use Go\Proxy\TraitProxyGenerator; +use PhpParser\Node\Param; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\EnumCase; use PhpParser\Node\Stmt\Property; @@ -613,6 +614,7 @@ private function commentOutInterceptedPropertiesInTraitBody( } $mask = ReflectionProperty::IS_PUBLIC | ReflectionProperty::IS_PROTECTED | ReflectionProperty::IS_PRIVATE; + $promotedAssignments = []; foreach ($class->getProperties($mask) as $property) { if (!isset($interceptedProperties[$property->getName()])) { continue; @@ -625,6 +627,15 @@ private function commentOutInterceptedPropertiesInTraitBody( if (!is_object($propertyNode) || !method_exists($propertyNode, 'getAttribute')) { continue; } + if ($propertyNode instanceof Param) { + // Promoted constructor property (issue #599): the declaration cannot be commented + // out — it doubles as the constructor parameter. Demote it to a plain parameter + // instead and assign it to the (proxy-declared) property in the constructor body. + $this->demotePromotedPropertyParameter($propertyNode, $streamMetaData); + $propertyName = $property->getName(); + $promotedAssignments[] = sprintf('$this->%1$s = $%1$s;', $propertyName); + continue; + } $start = $propertyNode->getAttribute('startTokenPos'); $end = $propertyNode->getAttribute('endTokenPos'); if (!is_int($start) || !is_int($end)) { @@ -632,6 +643,149 @@ private function commentOutInterceptedPropertiesInTraitBody( } $this->commentOutMovedPropertyTokenRange($class->name, $property->getName(), $start, $end, $streamMetaData); } + + if ($promotedAssignments !== []) { + $this->injectConstructorAssignments($class, $promotedAssignments, $streamMetaData); + } + } + + /** + * Demotes a promoted constructor property to a plain constructor parameter (issue #599). + * + * Removes only the promotion modifiers (visibility, asymmetric set-visibility, readonly, + * final) from the parameter tokens, keeping attributes, type, name, and default value. + * The property itself is re-declared with interception hooks in the proxy class, and the + * value is assigned in the constructor body (see injectConstructorAssignments), which + * routes the write through the proxy's set hook. + * + * Whitespace containing newlines is preserved so line numbers stay intact. + */ + private function demotePromotedPropertyParameter(Param $parameterNode, StreamMetaData $streamMetaData): void + { + $start = $parameterNode->getAttribute('startTokenPos'); + $end = $parameterNode->getAttribute('endTokenPos'); + if (!is_int($start) || !is_int($end)) { + return; + } + + $modifierTokenIds = [ + T_PUBLIC, T_PROTECTED, T_PRIVATE, + T_PUBLIC_SET, T_PROTECTED_SET, T_PRIVATE_SET, + T_READONLY, T_FINAL, + ]; + + $position = $start; + while ($position <= $end) { + if (!isset($streamMetaData->tokenStream[$position])) { + ++$position; + continue; + } + $token = $streamMetaData->tokenStream[$position]; + // Modifiers can only appear before the parameter variable — stop there so that + // tokens inside the default value expression are never touched + if ($token->id === T_VARIABLE) { + break; + } + // Skip parameter attribute groups entirely: '#[' opens a bracket context that can + // contain arbitrary nested brackets inside attribute arguments + if ($token->id === T_ATTRIBUTE) { + $bracketDepth = 1; + ++$position; + while ($position <= $end && $bracketDepth > 0) { + $innerText = isset($streamMetaData->tokenStream[$position]) ? $streamMetaData->tokenStream[$position]->text : ''; + if ($innerText === '[') { + ++$bracketDepth; + } elseif ($innerText === ']') { + --$bracketDepth; + } + ++$position; + } + continue; + } + if (in_array($token->id, $modifierTokenIds, true)) { + unset($streamMetaData->tokenStream[$position]); + // Also drop the following whitespace unless it holds a newline (line budget) + if (isset($streamMetaData->tokenStream[$position + 1])) { + $nextToken = $streamMetaData->tokenStream[$position + 1]; + if ($nextToken->id === T_WHITESPACE && strpbrk($nextToken->text, "\r\n") === false) { + unset($streamMetaData->tokenStream[$position + 1]); + } + } + } + ++$position; + } + } + + /** + * Injects property assignments at the very beginning of the constructor body. + * + * The assignments are appended to the opening '{' token of the constructor body, all on + * the same line, so the original line numbers of the constructor statements are preserved. + * + * @param non-empty-list $assignments Assignment statements like '$this->name = $name;' + */ + private function injectConstructorAssignments( + ReflectionClass $class, + array $assignments, + StreamMetaData $streamMetaData + ): void { + $constructor = $class->getConstructor(); + if ($constructor === null || !$constructor instanceof ReflectionMethod) { + return; + } + $constructorNode = $constructor->getNode(); + $start = $constructorNode->getAttribute('startTokenPos'); + $end = $constructorNode->getAttribute('endTokenPos'); + if (!is_int($start) || !is_int($end)) { + return; + } + + // The body '{' is the first '{' token after the parameter list closes (parenthesis + // depth back to zero). Hook bodies of promoted parameters contain '{' too, but they + // are always nested inside the parameter parentheses, so the depth guard skips them. + $position = $start; + $seenFunction = false; + $seenParameterList = false; + $parenthesisDepth = 0; + while ($position <= $end) { + if (!isset($streamMetaData->tokenStream[$position])) { + ++$position; + continue; + } + $token = $streamMetaData->tokenStream[$position]; + if (!$seenFunction) { + // Skip attribute groups before the 'function' keyword — their arguments + // may contain arbitrary parentheses + if ($token->id === T_ATTRIBUTE) { + $bracketDepth = 1; + ++$position; + while ($position <= $end && $bracketDepth > 0) { + $innerText = isset($streamMetaData->tokenStream[$position]) ? $streamMetaData->tokenStream[$position]->text : ''; + if ($innerText === '[') { + ++$bracketDepth; + } elseif ($innerText === ']') { + --$bracketDepth; + } + ++$position; + } + continue; + } + $seenFunction = ($token->id === T_FUNCTION); + ++$position; + continue; + } + if ($token->text === '(') { + ++$parenthesisDepth; + $seenParameterList = true; + } elseif ($token->text === ')') { + --$parenthesisDepth; + } elseif ($token->text === '{' && $seenParameterList && $parenthesisDepth === 0) { + $streamMetaData->tokenStream[$position]->text .= ' ' . implode(' ', $assignments); + + return; + } + ++$position; + } } private function commentOutMovedPropertyTokenRange( diff --git a/src/Proxy/Part/AbstractInterceptedPropertyGenerator.php b/src/Proxy/Part/AbstractInterceptedPropertyGenerator.php index 951ca2b8..93b2cfc4 100644 --- a/src/Proxy/Part/AbstractInterceptedPropertyGenerator.php +++ b/src/Proxy/Part/AbstractInterceptedPropertyGenerator.php @@ -55,31 +55,29 @@ protected function createBasePropertyGenerator(): PropertyGenerator if ($this->property->hasType()) { $generator->setType(TypeGenerator::fromReflectionType($this->property->getType())); } - if ($this->property->hasDefaultValue()) { - // When parser-reflection is loaded, prefer the raw AST default node over - // getDefaultValue(). This avoids parser-reflection bugs where getDefaultValue() - // crashes (uninitialized typed property for FCC) or returns null (Closure defaults), - // and correctly handles scalars, arrays, and FCC expressions uniformly. - if (method_exists($this->property, 'getNode')) { - $astNode = $this->property->getNode(); - $astDefault = ($astNode instanceof PropertyItem || $astNode instanceof Param) - ? $astNode->default - : null; - if ($astDefault !== null) { - $generator->setDefaultExpressionNode($astDefault); - } - } else { - $rawDefault = $this->property->getDefaultValue(); - if ($rawDefault instanceof \Closure) { - throw new \LogicException(sprintf( - 'Cannot generate proxy for property %s::$%s: PHP 8.5 Closure default values ' - . 'require goaop/parser-reflection for AST access.', - $this->property->getDeclaringClass()->getName(), - $this->property->getName() - )); - } - $generator->setDefaultValue($rawDefault); + // When parser-reflection is loaded, prefer the raw AST default node over + // getDefaultValue(). This avoids parser-reflection bugs where getDefaultValue() + // crashes (uninitialized typed property for FCC) or returns null (Closure defaults), + // and correctly handles scalars, arrays, and FCC expressions uniformly. + // The AST node is also the only source for promoted constructor property defaults: + // reflection reports hasDefaultValue() === false for them because the default + // formally belongs to the constructor parameter (see issue #599), but the proxy + // property must carry it so that the demoted woven trait plus proxy behave like + // the original promoted declaration. + $astDefault = $this->getAstDefaultNode(); + if ($astDefault !== null) { + $generator->setDefaultExpressionNode($astDefault); + } elseif ($this->property->hasDefaultValue() && !method_exists($this->property, 'getNode')) { + $rawDefault = $this->property->getDefaultValue(); + if ($rawDefault instanceof \Closure) { + throw new \LogicException(sprintf( + 'Cannot generate proxy for property %s::$%s: PHP 8.5 Closure default values ' + . 'require goaop/parser-reflection for AST access.', + $this->property->getDeclaringClass()->getName(), + $this->property->getName() + )); } + $generator->setDefaultValue($rawDefault); } $attributeGroups = AttributeGroupsGenerator::fromReflector($this->property); @@ -109,7 +107,27 @@ protected function isArrayTypedProperty(): bool protected function hasPotentiallyUninitializedTypedProperty(): bool { - return $this->property->hasType() && !$this->property->hasDefaultValue(); + return $this->property->hasType() + && !$this->property->hasDefaultValue() + && $this->getAstDefaultNode() === null; + } + + /** + * Returns the raw AST default value expression when parser-reflection is loaded. + * + * For promoted constructor properties the default lives on the Param node while + * reflection's hasDefaultValue() reports false, so the AST node is authoritative. + */ + private function getAstDefaultNode(): ?\PhpParser\Node\Expr + { + if (!method_exists($this->property, 'getNode')) { + return null; + } + $astNode = $this->property->getNode(); + + return ($astNode instanceof PropertyItem || $astNode instanceof Param) + ? $astNode->default + : null; } protected function createFieldAccessDocComment(string $variableName = 'fieldAccess', bool $isNullable = false): Doc diff --git a/tests/Fixtures/project/src/Application/PromotedPropertyClass.php b/tests/Fixtures/project/src/Application/PromotedPropertyClass.php new file mode 100644 index 00000000..a5c88b32 --- /dev/null +++ b/tests/Fixtures/project/src/Application/PromotedPropertyClass.php @@ -0,0 +1,24 @@ +name = trim($this->name); + } + + public function getName(): string + { + return $this->name; + } +} diff --git a/tests/Fixtures/project/src/Application/SingleLinePromotedClass.php b/tests/Fixtures/project/src/Application/SingleLinePromotedClass.php new file mode 100644 index 00000000..357ac7e5 --- /dev/null +++ b/tests/Fixtures/project/src/Application/SingleLinePromotedClass.php @@ -0,0 +1,13 @@ +name)")] + public function beforePromotedNameAccess(FieldAccess $access): void + { + // No-op: registration is asserted by functional tests + } + + #[Pointcut\Before("access(public Go\Tests\TestProject\Application\SingleLinePromotedClass->tag)")] + public function beforePromotedTagAccess(FieldAccess $access): void + { + // No-op: registration is asserted by functional tests + } +} diff --git a/tests/Fixtures/project/src/Kernel/DefaultAspectKernel.php b/tests/Fixtures/project/src/Kernel/DefaultAspectKernel.php index a1f0f291..12a3dfca 100644 --- a/tests/Fixtures/project/src/Kernel/DefaultAspectKernel.php +++ b/tests/Fixtures/project/src/Kernel/DefaultAspectKernel.php @@ -11,6 +11,7 @@ use Go\Tests\TestProject\Aspect\InitializationAspect; use Go\Tests\TestProject\Aspect\Issue293Aspect; use Go\Tests\TestProject\Aspect\LoggingAspect; +use Go\Tests\TestProject\Aspect\PromotedPropertyInterceptAspect; use Go\Tests\TestProject\Aspect\PropertyInterceptAspect; use Go\Tests\TestProject\Aspect\TraitCompositionAspect; use Go\Tests\TestProject\Aspect\WeavingAspect; @@ -27,6 +28,7 @@ protected function configureAop(AspectContainer $container): void $container->registerAspect(DoSomethingAspect::class); $container->registerAspect(ArrayPropertyInterceptAspect::class); $container->registerAspect(PropertyInterceptAspect::class); + $container->registerAspect(PromotedPropertyInterceptAspect::class); $container->registerAspect(Issue293Aspect::class); $container->registerAspect(InitializationAspect::class); $container->registerAspect(WeavingAspect::class); diff --git a/tests/Functional/ClassWeavingTest.php b/tests/Functional/ClassWeavingTest.php index 08fbd2de..85efa9c6 100644 --- a/tests/Functional/ClassWeavingTest.php +++ b/tests/Functional/ClassWeavingTest.php @@ -18,6 +18,8 @@ use Go\Tests\TestProject\Application\FinalClass; use Go\Tests\TestProject\Application\FooInterface; use Go\Tests\TestProject\Application\Main; +use Go\Tests\TestProject\Application\PromotedPropertyClass; +use Go\Tests\TestProject\Application\SingleLinePromotedClass; class ClassWeavingTest extends BaseFunctionalTestCase { @@ -87,6 +89,25 @@ public function testItDoesWeaveMethodWithComplexTypes(): void $this->assertMethodWoven(ClassWithComplexTypes::class, 'publicMethodWithDNFTypeReturn'); } + /** + * Promoted constructor properties must be weavable (issue #599): the promoted parameter + * is demoted to a plain parameter in the woven trait and the property is re-declared + * with interception hooks in the proxy — for multi-line and single-line constructors. + */ + public function testPromotedPropertyWeaving(): void + { + $this->assertPropertyWoven( + PromotedPropertyClass::class, + 'name', + 'Go\\Tests\\TestProject\\Aspect\\PromotedPropertyInterceptAspect->beforePromotedNameAccess' + ); + $this->assertPropertyWoven( + SingleLinePromotedClass::class, + 'tag', + 'Go\\Tests\\TestProject\\Aspect\\PromotedPropertyInterceptAspect->beforePromotedTagAccess' + ); + } + public function testArrayPropertyInterceptionAllowsIndirectModification(): void { $this->assertPropertyWoven( diff --git a/tests/Instrument/Transformer/Stubs/FinalPromotedClass85.php b/tests/Instrument/Transformer/Stubs/FinalPromotedClass85.php new file mode 100644 index 00000000..6d59a483 --- /dev/null +++ b/tests/Instrument/Transformer/Stubs/FinalPromotedClass85.php @@ -0,0 +1,24 @@ + + * + * This source file is subject to the license that is bundled + * with this source code in the file LICENSE. + */ + +namespace Go\Instrument\Transformer\Stubs; + +/** + * This file uses the PHP 8.5+ `final` promoted constructor property syntax and must + * only be loaded on PHP >= 8.5. Tests referencing it are gated with #[RequiresPhp]. + * + * Used for testing demotion of intercepted promoted properties (issue #599). + */ +class FinalPromotedClass85 +{ + public function __construct(final public string $token = 'secret') {} +} diff --git a/tests/Instrument/Transformer/WeavingTransformerTest.php b/tests/Instrument/Transformer/WeavingTransformerTest.php index ece9e7fe..8d36a39f 100644 --- a/tests/Instrument/Transformer/WeavingTransformerTest.php +++ b/tests/Instrument/Transformer/WeavingTransformerTest.php @@ -443,6 +443,131 @@ public function testWeaverMovesInterceptedPropertiesToProxyHooks(): void $this->assertStringContainsString("InterceptorInjector::forProperty(self::class, 'limited'", $proxyContent); } + /** + * Intercepted promoted constructor properties must be demoted to plain parameters in the + * woven trait — keeping type and default value — with explicit assignments injected at the + * start of the constructor body (issue #599). The proxy re-declares the property with + * interception hooks and must keep the original default value. + */ + public function testWeaverDemotesInterceptedPromotedProperties(): void + { + $transformer = $this->createTransformerWithAdvices([ + AspectContainer::PROPERTY_PREFIX => [ + 'name' => ['advisor.Go\Tests\TestProject\Application\PromotedPropertyClass->name' => true], + 'counter' => ['advisor.Go\Tests\TestProject\Application\PromotedPropertyClass->counter' => true], + ], + AspectContainer::METHOD_PREFIX => [ + '__construct' => ['advisor.Go\Tests\TestProject\Application\PromotedPropertyClass->__construct' => true], + 'getName' => ['advisor.Go\Tests\TestProject\Application\PromotedPropertyClass->getName' => true], + ], + ]); + + $metadata = $this->loadTestMetadata('php80-promoted-property'); + $transformer->transform($metadata); + + $actual = $this->normalizeWhitespaces($metadata->source); + $expected = $this->normalizeWhitespaces($this->loadTestMetadata('php80-promoted-property-woven')->source); + $this->assertEquals($expected, $actual); + + // Non-intercepted promoted parameter must stay promoted in the trait + $this->assertStringContainsString('protected ?\ArrayObject $bag = null', $actual); + + $matches = []; + $this->assertSame(1, preg_match("/AOP_CACHE_DIR . '(.+)';$/m", $actual, $matches)); + $actualProxyContent = $this->normalizeWhitespaces((string) file_get_contents('vfs://' . $matches[1])); + $expectedProxyContent = $this->normalizeWhitespaces($this->loadTestMetadata('php80-promoted-property-proxy')->source); + $this->assertEquals($expectedProxyContent, $actualProxyContent); + + // The proxy hook property must keep the original promoted default value + $this->assertStringContainsString("private string \$name = 'initial' {", $actualProxyContent); + } + + /** + * A promoted property inside a single-line constructor must weave without a parse error + * (issue #599). Commenting the parameter out used to swallow the closing ')' and '{'. + */ + public function testWeaverDemotesPromotedPropertyInSingleLineConstructor(): void + { + $transformer = $this->createTransformerWithAdvices([ + AspectContainer::PROPERTY_PREFIX => [ + 'tag' => ['advisor.Go\Tests\TestProject\Application\SingleLinePromotedClass->tag' => true], + ], + AspectContainer::METHOD_PREFIX => [ + '__construct' => ['advisor.Go\Tests\TestProject\Application\SingleLinePromotedClass->__construct' => true], + ], + ]); + + $metadata = $this->loadTestMetadata('php80-promoted-property-single-line'); + $transformer->transform($metadata); + + $actual = $this->normalizeWhitespaces($metadata->source); + $expected = $this->normalizeWhitespaces($this->loadTestMetadata('php80-promoted-property-single-line-woven')->source); + $this->assertEquals($expected, $actual); + + $matches = []; + $this->assertSame(1, preg_match("/AOP_CACHE_DIR . '(.+)';$/m", $actual, $matches)); + $actualProxyContent = $this->normalizeWhitespaces((string) file_get_contents('vfs://' . $matches[1])); + $expectedProxyContent = $this->normalizeWhitespaces($this->loadTestMetadata('php80-promoted-property-single-line-proxy')->source); + $this->assertEquals($expectedProxyContent, $actualProxyContent); + } + + /** + * PHP 8.5 `final` promoted constructor properties must demote cleanly: the woven trait + * drops both `final` and the visibility modifier, while the proxy re-declares the + * property as final with the original default value (issue #599). + */ + #[\PHPUnit\Framework\Attributes\RequiresPhp('>= 8.5.0')] + public function testWeaverDemotesFinalPromotedProperty(): void + { + $transformer = $this->createTransformerWithAdvices([ + AspectContainer::PROPERTY_PREFIX => [ + 'token' => ['advisor.Go\Instrument\Transformer\Stubs\FinalPromotedClass85->token' => true], + ], + AspectContainer::METHOD_PREFIX => [ + '__construct' => ['advisor.Go\Instrument\Transformer\Stubs\FinalPromotedClass85->__construct' => true], + ], + ]); + + $fileName = __DIR__ . '/Stubs/FinalPromotedClass85.php'; + $stream = fopen('php://filter/string.tolower/resource=' . $fileName, 'r'); + $metadata = new StreamMetaData($stream, (string) file_get_contents($fileName)); + fclose($stream); + $transformer->transform($metadata); + + $actual = $this->normalizeWhitespaces($metadata->source); + $this->assertStringContainsString( + "public function __construct(string \$token = 'secret') { \$this->token = \$token;}", + $actual + ); + + $matches = []; + $this->assertSame(1, preg_match("/AOP_CACHE_DIR . '(.+)';$/m", $actual, $matches)); + $proxyContent = $this->normalizeWhitespaces((string) file_get_contents('vfs://' . $matches[1])); + $this->assertStringContainsString("final public string \$token = 'secret' {", $proxyContent); + } + + /** + * Creates a WeavingTransformer whose advice matcher returns the given advices for any class. + */ + private function createTransformerWithAdvices(array $advices): WeavingTransformer + { + $adviceMatcher = $this->createMock(AdviceMatcherInterface::class); + $adviceMatcher->method('getAdvicesForClass')->willReturn($advices); + $adviceMatcher->method('getAdvicesForFunctions')->willReturn([]); + + $loader = $this + ->getMockBuilder(AspectLoader::class) + ->setConstructorArgs([$this->getContainerMock()]) + ->getMock(); + + return new WeavingTransformer( + $this->kernel, + $adviceMatcher, + $this->cachePathManager, + $loader + ); + } + /** * Testcase for multiple classes (@see https://github.com/lisachenko/go-aop-php/issues/71) */ diff --git a/tests/Instrument/Transformer/_files/php80-promoted-property-proxy.php b/tests/Instrument/Transformer/_files/php80-promoted-property-proxy.php new file mode 100644 index 00000000..28de8941 --- /dev/null +++ b/tests/Instrument/Transformer/_files/php80-promoted-property-proxy.php @@ -0,0 +1,55 @@ + $__joinPoint */ + static $__joinPoint = InterceptorInjector::forProperty(self::class, 'name', ['advisor.Go\Tests\TestProject\Application\PromotedPropertyClass->name']); + return $__joinPoint->__invoke($this, FieldAccessType::READ, $this->name); + } + set { + /** @var FieldAccess $__joinPoint */ + static $__joinPoint = InterceptorInjector::forProperty(self::class, 'name', ['advisor.Go\Tests\TestProject\Application\PromotedPropertyClass->name']); + $this->name = $__joinPoint->__invoke($this, FieldAccessType::WRITE, $value, $this->name); + } + } + final public private(set) int $counter = 1 { + get { + /** @var FieldAccess $__joinPoint */ + static $__joinPoint = InterceptorInjector::forProperty(self::class, 'counter', ['advisor.Go\Tests\TestProject\Application\PromotedPropertyClass->counter']); + return $__joinPoint->__invoke($this, FieldAccessType::READ, $this->counter); + } + set { + /** @var FieldAccess $__joinPoint */ + static $__joinPoint = InterceptorInjector::forProperty(self::class, 'counter', ['advisor.Go\Tests\TestProject\Application\PromotedPropertyClass->counter']); + $this->counter = $__joinPoint->__invoke($this, FieldAccessType::WRITE, $value, $this->counter); + } + } + public function __construct(string $name = 'initial', int $counter = 1, ?\ArrayObject $bag = null) + { + /** @var DynamicMethodInvocation $__joinPoint */ + static $__joinPoint = InterceptorInjector::forMethod(self::class, '__construct', ['advisor.Go\Tests\TestProject\Application\PromotedPropertyClass->__construct'], $this->__aop____construct(...)); + return $__joinPoint->__invoke($this, \array_slice([$name, $counter, $bag], 0, \func_num_args())); + } + public function getName(): string + { + /** @var DynamicMethodInvocation $__joinPoint */ + static $__joinPoint = InterceptorInjector::forMethod(self::class, 'getName', ['advisor.Go\Tests\TestProject\Application\PromotedPropertyClass->getName'], $this->__aop__getName(...)); + return $__joinPoint->__invoke($this); + } +} \ No newline at end of file diff --git a/tests/Instrument/Transformer/_files/php80-promoted-property-single-line-proxy.php b/tests/Instrument/Transformer/_files/php80-promoted-property-single-line-proxy.php new file mode 100644 index 00000000..6da7cc51 --- /dev/null +++ b/tests/Instrument/Transformer/_files/php80-promoted-property-single-line-proxy.php @@ -0,0 +1,36 @@ + $__joinPoint */ + static $__joinPoint = InterceptorInjector::forProperty(self::class, 'tag', ['advisor.Go\Tests\TestProject\Application\SingleLinePromotedClass->tag']); + return $__joinPoint->__invoke($this, FieldAccessType::READ, $this->tag); + } + set { + /** @var FieldAccess $__joinPoint */ + static $__joinPoint = InterceptorInjector::forProperty(self::class, 'tag', ['advisor.Go\Tests\TestProject\Application\SingleLinePromotedClass->tag']); + $this->tag = $__joinPoint->__invoke($this, FieldAccessType::WRITE, $value, $this->tag); + } + } + public function __construct(string $tag = 'default') + { + /** @var DynamicMethodInvocation $__joinPoint */ + static $__joinPoint = InterceptorInjector::forMethod(self::class, '__construct', ['advisor.Go\Tests\TestProject\Application\SingleLinePromotedClass->__construct'], $this->__aop____construct(...)); + return $__joinPoint->__invoke($this, \array_slice([$tag], 0, \func_num_args())); + } +} \ No newline at end of file diff --git a/tests/Instrument/Transformer/_files/php80-promoted-property-single-line-woven.php b/tests/Instrument/Transformer/_files/php80-promoted-property-single-line-woven.php new file mode 100644 index 00000000..e2195c20 --- /dev/null +++ b/tests/Instrument/Transformer/_files/php80-promoted-property-single-line-woven.php @@ -0,0 +1,14 @@ +tag = $tag;} +} +include_once AOP_CACHE_DIR . '/Transformer/_files/php80-promoted-property-single-line.php'; diff --git a/tests/Instrument/Transformer/_files/php80-promoted-property-single-line.php b/tests/Instrument/Transformer/_files/php80-promoted-property-single-line.php new file mode 100644 index 00000000..357ac7e5 --- /dev/null +++ b/tests/Instrument/Transformer/_files/php80-promoted-property-single-line.php @@ -0,0 +1,13 @@ +name = $name; $this->counter = $counter; + $this->name = trim($this->name); + } + + public function getName(): string + { + return $this->name; + } +} +include_once AOP_CACHE_DIR . '/Transformer/_files/php80-promoted-property.php'; diff --git a/tests/Instrument/Transformer/_files/php80-promoted-property.php b/tests/Instrument/Transformer/_files/php80-promoted-property.php new file mode 100644 index 00000000..a5c88b32 --- /dev/null +++ b/tests/Instrument/Transformer/_files/php80-promoted-property.php @@ -0,0 +1,24 @@ +name = trim($this->name); + } + + public function getName(): string + { + return $this->name; + } +}