Skip to content

Fix weaving of class attributes, promoted properties, and new-in-initializers - #613

Merged
lisachenko merged 3 commits into
masterfrom
claude/php85-audit-fix-weaving
Aug 28, 2026
Merged

Fix weaving of class attributes, promoted properties, and new-in-initializers#613
lisachenko merged 3 commits into
masterfrom
claude/php85-audit-fix-weaving

Conversation

@lisachenko

Copy link
Copy Markdown
Member

Summary

Fixes three confirmed weaving bugs found by the PHP 8.5 audit harness:

Fixes #598 / Fixes #599 / Fixes #603

#598 — Class-level attributes broke class→trait conversion

Root cause: a ClassLike node's startTokenPos in php-parser 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 {.

Fix: the scan now starts right after the node's last attribute group (WeavingTransformer::getPositionAfterAttributeGroups()). Class-level attributes stay untouched on the woven trait, where they are legal. This also repairs the enum path — the previously "passing" #[Loggable] enum BackedEnum functional fixture actually produced a woven trait that failed php -l; it now lints cleanly.

#599 — Property interception broke promoted constructor properties

Root cause: commentOutInterceptedPropertiesInTraitBody() line-commented the matched promoted parameter inside the constructor signature. Single-line constructors became parse errors (the comment swallowed the closing )), and multi-line constructors silently lost their parameters: the proxy kept the original signature and delegated to a now-parameterless trait constructor, dropping all arguments and leaving typed properties uninitialized. The proxy hook property also lost its default (public string $name { instead of public string $name = 'initial' {), because promoted properties report hasDefaultValue() === false.

Fix (demotion, as suggested in the issue):

  • The woven trait rewrites the promoted parameter to a plain parameter — only the promotion modifiers (public/protected/private, public(set)/protected(set)/private(set), readonly, final) are removed; attributes, type, and default are kept.
  • $this->prop = $prop; assignments are injected on the opening-brace line of the constructor body (line numbers preserved), routing construction-time writes through the proxy's generated set hook.
  • AbstractInterceptedPropertyGenerator now reads the default from the Param AST node, so the proxy hook property carries the original default value.

All modifier combinations demote cleanly, including PHP 8.4 asymmetric visibility (public private(set) int $v = 1) and PHP 8.5 final promoted parameters (final public string $x = 'y').

#603ConstructorExecutionTransformer rewrote new in constant expressions

Root cause: the finding pass collected every New_ node, including PHP 8.1 "new in initializers" occurrences (parameter defaults, static variable initializers, attribute arguments, global constants). The rewritten ...getInstance()->{Foo::class}(...) form is not a valid constant expression → compile-time fatal error.

Fix: a dedicated NewExpressionFinderVisitor tracks entry/exit of constant-expression subtrees (Param/StaticVar/PropertyItem/Const_ initializers, EnumCase values, whole Attribute nodes) and only collects new expressions outside of them. Runtime code nested inside those containers (e.g. property hook bodies on promoted parameters) is still rewritten.

Tests

  • New golden fixtures: php80-class-attribute (plain + argument attributes), php80-promoted-property (multi-line ctor with defaults, asymmetric visibility, and a non-intercepted promoted param that must stay promoted), php80-promoted-property-single-line — each asserting woven trait and generated proxy.
  • PHP 8.5-gated (#[RequiresPhp('>= 8.5.0')]) test for final promoted parameter demotion via a stub that is never eagerly loaded.
  • Functional coverage: new PromotedPropertyInterceptAspect + ClassWeavingTest::testPromotedPropertyWeaving weave the promoted-property classes with the real matcher on every functional run.
  • ConstructorExecutionTransformerTest provider extended with 8 const-expr cases (defaults, static vars, global const, attribute args, nested new, mixed same-file rewrites, hook bodies).

Test evidence

  • php8.5 vendor/bin/phpunit — OK, 2507 tests / 2991 assertions (1 pre-existing deprecation)
  • php8.4 vendor/bin/phpunit — OK, 2507 tests / 2985 assertions (2 skipped: PHP 8.5-gated)
  • php8.6 vendor/bin/phpunit (8.6.0beta2, informational) — OK, 2507 tests / 2991 assertions
  • php8.5 vendor/bin/phpstan analyze — level 10, no errors, no new baseline entries
  • Runtime smoke check through a real AspectKernel: new PromotedPropertyClass(' x ', 5)getName() === 'x', counter === 5; defaults 'initial'/1 applied when omitted; SingleLinePromotedClass default/custom tag both correct; instances implement Go\Aop\Proxy.

Generated by Claude Code

@lisachenko

Copy link
Copy Markdown
Member Author

Rebase please

claude added 3 commits August 28, 2026 21:42
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WFMYyvE4hYRUoMS8mtKrHP
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WFMYyvE4hYRUoMS8mtKrHP
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WFMYyvE4hYRUoMS8mtKrHP
@lisachenko
lisachenko force-pushed the claude/php85-audit-fix-weaving branch from 86448c3 to c52e072 Compare August 28, 2026 21:42

Copy link
Copy Markdown
Member Author

Rebased onto current master (c52e072). One conflict in WeavingTransformerTest.php (both #611 and this branch added a test at the same location) — resolved by keeping both tests. Full suite green on 8.4 (OK, 2523 tests, 4 skipped) and 8.5 (OK, 2523 tests, 3040 assertions, zero deprecations), PHPStan level 10 clean.


Generated by Claude Code

@lisachenko
lisachenko marked this pull request as ready for review August 28, 2026 21:52
@lisachenko
lisachenko merged commit 21ecc92 into master Aug 28, 2026
7 checks passed
@lisachenko
lisachenko deleted the claude/php85-audit-fix-weaving branch August 28, 2026 21:52
lisachenko pushed a commit that referenced this pull request Aug 28, 2026
Class-level attributes, promoted-property interception and new-in-const-expr
guards are fixed, so those fixtures are asserted clean now. The fixes exposed
two narrower follow-ups, reported as #615 (#[Attribute] classes cannot be
woven into traits) and #616 (new-in-initializer default copied onto the proxy
hook property), and pinned here.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WFMYyvE4hYRUoMS8mtKrHP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants