Preserve private property scopes and add advice property accessors

PROPERTY_ACCESS.md

@@ -0,0 +1,82 @@
+# Accessing private properties from advice
+
+Private properties keep their original visibility and declaring scope when a class
+is woven. This fixes [#6](https://github.com/okapi-web/php-aop/issues/6): a parent and
+child may legally declare private properties with the same name and different
+types. They also retain independent values when their types are identical.
+
+## Migration
+
+Advice that previously read or wrote a private property directly through
+`$invocation->getSubject()` should use `$invocation->properties()` instead. This applies
+to all private properties, including properties whose names are currently unique.
+Public and protected properties retain their existing behavior. Method interception
+is unchanged.
+
+```php
+// Before: $subject->data = ['updated'];
+$invocation->properties()->data = ['updated'];
+$data = $invocation->properties()->data;
+```
+
+Use the original class that declares the property, without `__AopProxied`. For a
+property supplied by a trait, use the class that uses the trait. The scope argument
+can be omitted if the name identifies one property in the object's hierarchy.
+Duplicate independent declarations require an explicit scope:
+
+```php
+$invocation->properties(ArgvInput::class)->tokens = ['parent'];
+$invocation->properties(CompletionInput::class)->tokens = 'child';
+```
+
+The accessor supports array mutation, references, `isset`, and `unset`:
+
+```php
+$properties = $invocation->properties();
+$properties->data[] = 'appended';
+$reference =& $properties->data;
+isset($properties->data);
+unset($properties->data);
+```
+
+`properties()` is a view of the existing subject, not a replacement for it.
+`getSubject()` still returns the same object. Subject type identity, internal method
+calls, and method interception are unchanged. In static advice, the accessor uses
+the invocation's class; instance properties require an object.
+
+The lower-level `PropertyAccess` API is also available outside an invocation. For
+static properties, pass an object or class name as the first argument:
+
+```php
+use Okapi\Aop\PropertyAccess;
+
+$value = PropertyAccess::get(Service::class, 'configuration', Service::class);
+PropertyAccess::set(Service::class, 'configuration', $value, Service::class);
+```
+
+Property access uses PHP reflection and closures bound to the declaring scope,
+preserves declared types, and accesses declared storage directly for initialized
+properties. After explicitly unsetting a property, PHP may invoke the subject's
+`__set` when writing it again; the accessor preserves that native behavior, which
+may leave the declared storage uninitialized if the setter does not restore it.
+The lower-level `get()` returns a value; the invocation accessor supports references.
+A missing property or an invalid
+declaring scope throws `ReflectionException`. An ambiguous name or a class-name
+argument for an instance property throws `LogicException`. Uninitialized typed
+properties still throw `Error` on read without initializing them. `isset` returns
+false and `unset` does nothing for absent properties; ambiguous names still throw.
+Static properties cannot be unset. Writes must name a declared property; the
+accessor does not create dynamic properties. PHP also prevents taking references
+to readonly properties on subjects whose readonly declarations remain intact.
+
+## Why private declarations must remain private
+
+A parent can be loaded before its descendants are known. Making even a currently
+unique private property public can invalidate a child loaded later. Checking only
+known collisions would make behavior depend on load order and cached code.
+Generated magic accessors are also unsafe as a general compatibility layer: their
+signatures can conflict with a descendant's own magic methods. Explicit access
+avoids changing either inheritance contract.
+
+Clear the configured AOP cache when upgrading so existing generated classes are
+rebuilt. Deploy this change with the advice migration above.

README.md

@@ -689,8 +689,11 @@ $firstLog = $logs[0];
 - Intercept "private" and "protected" methods
   (Will show errors in IDEs)
 
-- Access "private" and "protected" properties and methods of the subject
-  (Will show errors in IDEs)
+- Access private properties through [`$invocation->properties()`](PROPERTY_ACCESS.md),
+  with an explicit declaring class when names overlap in an inheritance hierarchy
+
+- Access protected properties and private/protected methods of the subject
+  (Direct access may show errors in IDEs)
 
 - Intercept "final" methods and classes
 

docs/superpowers/plans/2026-09-05-private-properties.md

@@ -0,0 +1,35 @@
+# Private property inheritance implementation plan
+
+## Design
+
+Fix #6 without merging independent private property slots. A parent can be loaded
+before any descendant is known, so private declarations must remain private even
+when no collision is currently visible. An explicit PropertyAccess API selects an
+original declaring class
+for ambiguous names and supports static properties. Public/protected properties and
+method interception retain their existing behavior. Generated constructors must not
+declare promoted properties a second time.
+
+## Tasks
+
+- [x] Add failing functional tests for different/same types, parent-first loading,
+  promoted properties, property mutation, and explicit scoped access.
+- [x] Preserve private property declarations in ProxiedClassModifier; remove
+  promotion from generated forwarding constructors in WovenClassBuilder.
+- [x] Add PropertyAccess, reporting ambiguous names instead of selecting silently.
+  Independent review showed generated magic accessors could introduce child-method
+  signature fatals; the safe candidate omits them and documents migration of advice.
+- [x] Cover traits, static properties, errors, and existing magic behavior.
+- [x] Document access semantics and migration and obtain independent review.
+- [x] Run Tests and Performance on PHP 8.1–8.5: 73 functional/integration tests
+  and 45 performance tests per version, no failures. Existing incomplete tests,
+  PHP 8.1 readonly-class skip, and dependency deprecations remain.
+- [ ] Verify origin and upstream CI after the publishing decision.
+- [ ] Merge the upstream PR referencing #6 only after successful checks.
+
+## Publishing decision
+
+The user approved migration to `$invocation->properties($declaringClass)`.
+The accessor wraps property access only; it never replaces the subject or adds
+magic methods to the subject's inheritance hierarchy. Reads/writes, array mutation,
+references, isset/unset, and static invocation access have functional coverage.

src/Core/Transform/ProxiedClassModifier.php

@@ -191,6 +191,24 @@ private function unReadOnlyClasses(): void
      */
     private function changeVisibility(): void
     {
+        // A descendant may be loaded after this class. Keep every private property
+        // in its declaring scope, even when no same-name property is known yet.
+        foreach ($this->sourceFileNode->getDescendantNodes() as $node) {
+            $modifiers = match (true) {
+                $node instanceof Node\PropertyDeclaration => $node->modifiers ?? [],
+                $node instanceof Node\Parameter => array_filter([
+                    $node->visibilityToken,
+                    ...($node->modifiers ?? []),
+                ]),
+                default => [],
+            };
+            foreach ($modifiers as $modifier) {
+                if ($modifier->kind === TokenKind::PrivateKeyword) {
+                    $this->alreadyProcessed[] = $modifier;
+                }
+            }
+        }
+
         $this->tokenCallbacks[] = function (Token $token) {
             if ($token->kind === TokenKind::PrivateKeyword
                 || $token->kind === TokenKind::ProtectedKeyword

src/Core/Transform/WovenClassBuilder.php

@@ -7,6 +7,7 @@
 use Nette\PhpGenerator\ClassType;
 use Nette\PhpGenerator\Factory;
 use Nette\PhpGenerator\Method;
+use Nette\PhpGenerator\Parameter;
 use Nette\PhpGenerator\PhpNamespace;
 use Nette\PhpGenerator\PromotedParameter;
 use Nette\PhpGenerator\Property;
@@ -225,11 +226,23 @@ private function buildMethod(BetterReflectionMethod $refMethod): Method
 
         $methodName = $refMethod->getName();
 
-        foreach ($method->getParameters() as $parameter) {
+        $parameters = $method->getParameters();
+        foreach ($parameters as $name => $parameter) {
             if ($parameter instanceof PromotedParameter) {
-                $parameter->setReadOnly(false);
+                // Promotion belongs to the original constructor, which the
+                // interceptor invokes. A forwarding method must not own a second slot.
+                $plain = new Parameter($parameter->getName());
+                $plain->setType($parameter->getType());
+                $plain->setNullable($parameter->isNullable());
+                $plain->setReference($parameter->isReference());
+                $plain->setAttributes($parameter->getAttributes());
+                if ($parameter->hasDefaultValue()) {
+                    $plain->setDefaultValue($parameter->getDefaultValue());
+                }
+                $parameters[$name] = $plain;
             }
         }
+        $method->setParameters($parameters);
 
         // Add "return" if the method has a return type
         $return = (string)$method->getReturnType() !== 'void' ? 'return ' : '';

src/Invocation/MethodInvocation.php

@@ -115,6 +115,12 @@ public function getSubject(): ?object
         return $this->subject;
     }
 
+    /** Access the subject's properties, optionally in an original declaring scope. */
+    public function properties(?string $declaringClass = null): PropertyAccessor
+    {
+        return new PropertyAccessor($this->subject ?? $this->className, $declaringClass);
+    }
+
     /**
      * Get the original subject class name of the invocation.
      *

src/Invocation/PropertyAccessor.php

@@ -0,0 +1,34 @@
+<?php
+namespace Okapi\Aop\Invocation;
+
+use Okapi\Aop\PropertyAccess;
+
+/** A property view of the subject; never a replacement for the subject itself. */
+final class PropertyAccessor
+{
+    public function __construct(
+        private readonly object|string $subject,
+        private readonly ?string $declaringClass = null,
+    ) {}
+
+    public function &__get(string $name): mixed
+    {
+        $value =& PropertyAccess::reference($this->subject, $name, $this->declaringClass);
+        return $value;
+    }
+
+    public function __set(string $name, mixed $value): void
+    {
+        PropertyAccess::set($this->subject, $name, $value, $this->declaringClass);
+    }
+
+    public function __isset(string $name): bool
+    {
+        return PropertyAccess::isSet($this->subject, $name, $this->declaringClass);
+    }
+
+    public function __unset(string $name): void
+    {
+        PropertyAccess::remove($this->subject, $name, $this->declaringClass);
+    }
+}

src/PropertyAccess.php

@@ -0,0 +1,132 @@
+<?php
+namespace Okapi\Aop;
+
+use LogicException;
+use Closure;
+use Error;
+use Okapi\Aop\Core\Cache\CachePaths;
+use ReflectionClass;
+use ReflectionException;
+use ReflectionProperty;
+
+/** Access a property without changing its declaring scope or storage. */
+final class PropertyAccess
+{
+    /**
+     * Read a value, optionally selecting its original declaring class.
+     *
+     * @throws ReflectionException If the property or declaring scope does not exist.
+     * @throws LogicException If the name is ambiguous or an instance is required.
+     */
+    public static function get(object|string $subject, string $name, ?string $declaringClass = null): mixed
+    {
+        $property = self::resolve($subject, $name, $declaringClass);
+        if (!$property->isInitialized(is_object($subject) ? $subject : null)) {
+            throw new Error('Property ' . $property->getDeclaringClass()->getName()
+                . '::$' . $name . ' must not be accessed before initialization');
+        }
+        return $property->getValue(is_object($subject) ? $subject : null);
+    }
+
+    /**
+     * Write a value using PHP's property type checks.
+     *
+     * @throws ReflectionException If the property or declaring scope does not exist.
+     * @throws LogicException If the name is ambiguous or an instance is required.
+     */
+    public static function set(object|string $subject, string $name, mixed $value, ?string $declaringClass = null): void
+    {
+        $property = self::resolve($subject, $name, $declaringClass);
+        $property->setValue(is_object($subject) ? $subject : null, $value);
+    }
+
+    /** @internal Support indirect writes through an invocation's property accessor. */
+    public static function &reference(object|string $subject, string $name, ?string $declaringClass = null): mixed
+    {
+        $property = self::resolve($subject, $name, $declaringClass);
+        // Do not initialize nullable properties or invoke __get after explicit unset.
+        if (!$property->isInitialized(is_object($subject) ? $subject : null)) {
+            throw new Error('Typed property ' . $property->getDeclaringClass()->getName()
+                . '::$' . $name . ' must not be accessed before initialization');
+        }
+        $scope = $property->getDeclaringClass()->getName();
+        $read = $property->isStatic()
+            ? Closure::bind(static function &() use ($name) { return self::$$name; }, null, $scope)
+            : Closure::bind(function &() use ($name) { return $this->$name; }, $subject, $scope);
+        $value =& $read();
+        return $value;
+    }
+
+    /** @internal */
+    public static function isSet(object|string $subject, string $name, ?string $declaringClass = null): bool
+    {
+        try {
+            $property = self::resolve($subject, $name, $declaringClass);
+        } catch (ReflectionException) {
+            return false;
+        }
+        $object = is_object($subject) ? $subject : null;
+        return $property->isInitialized($object) && $property->getValue($object) !== null;
+    }
+
+    /** @internal */
+    public static function remove(object|string $subject, string $name, ?string $declaringClass = null): void
+    {
+        try {
+            $property = self::resolve($subject, $name, $declaringClass);
+        } catch (ReflectionException) {
+            return;
+        }
+        if ($property->isStatic()) {
+            throw new Error("Cannot unset static property \$$name.");
+        }
+        if (!$property->isInitialized($subject)) {
+            return;
+        }
+        $remove = Closure::bind(function () use ($name): void {
+            unset($this->$name);
+        }, $subject, $property->getDeclaringClass()->getName());
+        $remove();
+    }
+
+    private static function resolve(object|string $subject, string $name, ?string $declaringClass = null): ReflectionProperty
+    {
+        $matches = [];
+        $scope = $declaringClass === null ? null : ltrim($declaringClass, '\\');
+        $class = new ReflectionClass($subject);
+        do {
+            $originalName = $class->getName();
+            if (str_ends_with($originalName, CachePaths::PROXIED_SUFFIX)) {
+                $originalName = substr($originalName, 0, -strlen(CachePaths::PROXIED_SUFFIX));
+            }
+            if ($scope !== null && strcasecmp($scope, $originalName) !== 0) {
+                continue;
+            }
+            foreach ($class->getProperties() as $property) {
+                if ($property->getName() !== $name || $property->getDeclaringClass()->getName() !== $class->getName()) {
+                    continue;
+                }
+                // Non-private instance overrides share storage. Redeclared static
+                // properties and private declarations each have independent slots.
+                $independent = $property->isPrivate() || $property->isStatic();
+                if (!$independent && isset($matches['inherited'])) {
+                    continue;
+                }
+                $key = $independent ? $class->getName() : 'inherited';
+                $matches[$key] = $property;
+            }
+        } while ($class = $class->getParentClass());
+
+        if (!$matches) {
+            throw new ReflectionException("Property \$$name does not exist in the requested scope.");
+        }
+        if (count($matches) > 1) {
+            throw new LogicException("Property \$$name is ambiguous; pass its original declaring class to PropertyAccess::get()/set().");
+        }
+        $property = reset($matches);
+        if (is_string($subject) && !$property->isStatic()) {
+            throw new LogicException("An object is required to access instance property \$$name.");
+        }
+        return $property;
+    }
+}

tests/Functional/AdviceBehavior/Include/Aspect/DatabaseModifierAspect.php

@@ -4,7 +4,7 @@
 
 use Okapi\Aop\Attributes\After;
 use Okapi\Aop\Attributes\Aspect;
-use Okapi\Aop\Invocation\AfterMethodInvocation;
+use Okapi\Aop\Invocation\AfterMethodInvocation;
 use Okapi\Aop\Tests\Functional\AdviceBehavior\Include\Target\SecureDatabaseService;
 
 #[Aspect]
@@ -16,13 +16,10 @@ class: SecureDatabaseService::class,
     )]
     public function modifyData(AfterMethodInvocation $invocation): void
     {
-        /** @var SecureDatabaseService $subject */
-        $subject = $invocation->getSubject();
-
-        $subject->data = [
+        $invocation->properties()->data = [
             'd' => 4,
             'e' => 5,
             'f' => 6,
-        ];
+        ];
     }
 }