Skip to content

fix(preprocessor): recompute method override flags after all files prepared - #42

Merged
matyhtf merged 1 commit into
swoole:masterfrom
xiaoyin199:fix/method-override-order
Sep 1, 2026
Merged

fix(preprocessor): recompute method override flags after all files prepared#42
matyhtf merged 1 commit into
swoole:masterfrom
xiaoyin199:fix/method-override-order

Conversation

@xiaoyin199

Copy link
Copy Markdown
Contributor

Problem

Preprocessor::prepareClassMethod() registers method-override flags in two directions, but neither covers the "sandwich" preprocessing order (ancestor → leaf → intermediate class):

  1. Upward marking: when a subclass is prepared, it walks up symbols->parent() and marks already-registered ancestor methods as overridden — but the isset() guard only reaches already-prepared ancestors, and an unregistered link in the parent chain terminates the loop early.
  2. Downward lookup (isMethodOverriddenInSubClasses()): when an ancestor is prepared, it scans subclasses — but only sees already-prepared subclasses.

With source order Base → Leaf → Mid:

  • Base prepared (1st): downward lookup, Leaf not processed yet → flag stays false
  • Leaf prepared (2nd): walks up, symbols->parent('app\mid') not registered yet (Mid is 3rd) → chain breaks after one step, never reaches Base
  • Mid prepared (3rd): only marks upward for its own declared methods; perform is not among them → Base's flag stays false forever

Later, MethodCallTrait::findNativeMethod() queries isOverrideMethod('app\base::perform'), gets false, and devirtualizes the call into a direct native call — subclass overrides never execute.

Minimal reproduction

Four files; the sources order is part of the trigger (Leaf before Mid):

<?php
// src/a.php — ancestor
namespace App;

class Base {
    protected function perform() { return 'base'; }
    public function delete() { return $this->perform(); }  // late-bound call
}
<?php
// src/b.php — leaf (overrides the method)
namespace App;

class Leaf extends Mid {
    protected function perform() { return 'leaf'; }
}
<?php
// src/c.php — intermediate (the "sandwich" middle layer)
namespace App;

class Mid extends Base {}
<?php
// src/entry.php
function main(int $argc, array $argv): void {
    echo (new \App\Leaf())->delete(), "\n";
}
# typephp.yml — note the order: b.php (leaf) before c.php (intermediate)
name: order-test
build-mode: bin
version: 0.1.0
output: build/order-test
build-dir: build/typephp
no-progress: true
sources:
  - ./src/entry.php
  - ./src/a.php
  - ./src/b.php
  - ./src/c.php

Actual: prints baseLeaf::perform() is ignored. In the generated C++, the perform() call inside Base::delete() is devirtualized to a direct native call (php_app__base__perform(this_)).

Expected (Zend parity): prints leaf.

Control experiment (both verified): swapping only the order of b/c flips the result — a → b → c ❌ prints base, a → c → b ✅ prints leaf.

Real-world impact

Hit in a real Hyperf project, and it is a data-correctness bug: Hyperf\Database\Model\SoftDeletes::performDeleteOnModel() overrides the same-named base Model method to implement soft deletes. After the faulty devirtualization, delete() calls the base implementation directly, so soft-deletable models are physically deleted (delete from ... where id = ? instead of update deleted_at). The same pattern exists in Hyperf\ModelCache\Cacheable (overrides query()/newQuery()/increment() etc., silently disabling the cache) and Hyperf\DbConnection\Traits\HasContainer (overrides getConnection()).

In the real project the trigger order came from directory alphabetical order: app/Model/Branch.php (B) is prepared before app/Model/Model.php (M), while the vendor ancestor classes precede the whole app/ tree — exactly the sandwich.

Fix

After all files are prepared and before conversion starts, recompute the override flags once against the complete inheritance graph (idempotent, +29 lines). With the patch, the reproduction prints leaf, and the full Hyperf project passes end-to-end smoke tests (soft delete writes deleted_at correctly; login/list endpoints all OK).

Environment

  • TypePHP Compiler (AOT) v0.6.8 (HEAD 7bdb6dc)
  • libphp: PHP 8.4.24 NTS
  • OS: WSL2 Ubuntu x86_64

@matyhtf

matyhtf commented Sep 1, 2026

Copy link
Copy Markdown
Member

Thank you for the detailed analysis and the real-world reproduction. The root cause is correct, and I verified that this patch fixes the normal project/CLI pipeline.

There is still one compatibility gap that needs to be addressed before merging:

The public prepareFile() -> convertFile() path is not fixed

recomputeMethodOverrideFlags() is currently called only from SourcePipelineTrait::prepare(). However, prepareFile() and convertFile() are public compiler APIs and are also used directly by our compiler tests and embedding code.

I reproduced the same Base -> Leaf -> Mid declaration order through that direct API. The generated C++ still contains:

php_app__base__perform(this_)

so the call is still incorrectly devirtualized.

Please move this finalization into a common pre-conversion path used by both convert() and convertFile(). For example, it could be integrated into the existing declaration/symbol finalization stage, with a dirty/finalized flag so it runs only when the prepared class graph has changed.

Algorithm

The current implementation scans all descendants for every method whose flag is false. This can become quadratic on a large class hierarchy. Once preprocessing is complete, a simpler approach is:

  1. Iterate over every declared method key.
  2. Walk that method's complete parent chain.
  3. Mark every existing ancestor method with the same name as overridden.

This follows the existing upward-marking semantics and is approximately method count x inheritance depth.

Tests

Please add:

  • an end-to-end PHPT covering the exact multi-file Base -> Leaf -> Mid source order;
  • the normal Base -> Mid -> Leaf order as a control;
  • a PHPUnit/code-generation test using the direct prepareFile() -> convertFile() API.

The issue report and investigation are excellent; once the shared finalization path is fixed, this should be a solid correctness fix.

The classMethodOverride registration in prepareClassMethod() depends on
file preprocessing order. With a "sandwich" order (ancestor first, leaf
second, intermediate class last), the ancestor method's override flag is
missed: the upward marking cannot cross the not-yet-registered
intermediate class, and the downward subclass lookup ran before the leaf
was prepared. findNativeMethod() then devirtualizes the late-bound call
into a direct native call, silently ignoring the override. In Hyperf this
turns SoftDeletes::delete() into a physical DELETE.

Finalize the flags once the complete class graph is known:
finalizeMethodOverrideFlags() walks every declared method's complete
parent chain and marks each existing ancestor method of the same name as
overridden (method count x inheritance depth). It runs from
Translator::convertFile() next to finalizeDeclarationExpressions(), so
both the project pipeline and the public prepareFile()/convertFile() API
share the same pre-conversion finalization, guarded by a dirty/finalized
flag reset in prepareFile().

Tests:
- tests/compiler/devirtualize/override-order-sandwich.phpt (fails on
  master with "base", passes with "leaf")
- tests/compiler/devirtualize/override-order-normal.phpt (control)
- phpunit/src/DevirtualizeOrderTest.php driving the public
  prepareFile()/convertFile() API in both orders, asserting the
  generated Base::delete() body dispatches dynamically
@xiaoyin199
xiaoyin199 force-pushed the fix/method-override-order branch from d7e207b to ae3d936 Compare September 1, 2026 05:57
@xiaoyin199

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — all three points are addressed in the updated commit:

Shared finalization path

finalizeMethodOverrideFlags() now lives next to finalizeDeclarationExpressions() and is called from Translator::convertFile(), right after the declaration-expression finalization — so both the project pipeline (prepare() → convert() → convertFile()) and the direct prepareFile() → convertFile() API go through the same pre-conversion finalization.

It follows the existing dirty/finalized pattern: prepareFile() sets methodOverrideFlagsFinalized = false whenever the prepared class graph changes (same place declarationExpressionsFinalized is reset), and the finalizer returns early once finalized. The dedicated call in SourcePipelineTrait::prepare() is removed.

Algorithm

Replaced the descendant scan with your suggested upward walk: for every declared method key, walk its complete parent chain and mark each existing ancestor method of the same name as overridden. Order-independent, ~ method count × inheritance depth.

Tests

  • tests/compiler/devirtualize/override-order-sandwich.phpt — ancestor → leaf → intermediate declaration order, expects leaf (fails with base without the fix)
  • tests/compiler/devirtualize/override-order-normal.phpt — ancestor → intermediate → leaf as control
  • phpunit/src/DevirtualizeOrderTest.php — drives the public prepareFile() → convertFile() API with three fixture files in sandwich order (plus the normal-order control), and asserts the generated body of Base::delete() dispatches dynamically (callScoped) instead of calling php_ordertest__base__perform directly

Verified: on master both PHPUnit sandwich assertions and the sandwich PHPT fail (the generated Base::delete() body contains the direct php_ordertest__base__perform(this_) call); with this patch all of them pass, along with the existing tests/compiler/devirtualize/ suite (6/6) and the full PHPUnit suite (same 7 pre-existing environment-related failures as master: 4× ExitCodeTest toolchain, 3× PythonModuleTest). The original Hyperf SoftDeletes scenario was smoke-tested end-to-end with the previous revision of this fix.

@matyhtf
matyhtf merged commit 8c0d52d into swoole:master Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants