Repository navigation
verifyAll() returns how many checks it verified - #154
Merged
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…d minor The compatibility policy amended in this PR admits widening a return type where no caller can observe the change; teach the gate to recognise exactly that case: every finding must be a 'changed from void to the non-covariant' widening, the named class must reflect as final against the checkout, and the top CHANGELOG heading must be a minor above the latest tag. Anything else still fails. CHANGELOG declares 1.1.0 - the minor this ships in.
rasuvaeff
added a commit
that referenced
this pull request
Oct 13, 2026
Inline forwarding adopted any clone of the running double's class, and one generated class serves every double of a type: a real body handed another double of its own class — `merge(self $other)` — and cloning that one gave the copy bodies to run with no state copy to run them over. Where the class declares a typed property that is PHP's own "must not be accessed before initialization" out of the clone; where every property has a declared default it is a quiet wrong answer computed from the declaration instead of the donor. Both were a regression: proxied, the copy answered defaults. `Runtime::isCopyOf()` now compares the two property tables with `get_mangled_object_vars()` — the only reading that reaches a parent's private properties, answers a hooked property with its backing store rather than running the doubled class's `get` in the middle of `__clone()`, has no entry for a virtual one, and omits what nobody initialized. A copy is property-identical to its source at this moment, because a generated `__clone()` does nothing but call `adoptClone()`. Two doubles of one class in the same state stay indistinguishable, because `__clone()` is handed no reference to its source: such a copy is still adopted and answers for real, over the state it already holds. That residual is documented where the rule is stated — both READMEs, the forwarding guide, the CHANGELOG — rather than called harmless. A `clone $double` written in the test's own `answers()` closure was adopted too, although the documented rule makes it a fresh double: the closure runs inside dispatch, which a real body can reach. `Runtime::$inlineFloor` hides the frames underneath for exactly the closure's span, so only what the closure itself enters — `callOriginal()`, which pushes a frame of its own — is visible to a clone made there. Five of the seven new tests fail or error on the previous commit: both forms of the foreign clone, the test closure under a real body, the comparison reading a hooked property through its backing store, and a plain property declared after a hooked and a virtual one deciding the verdict. The other two pin mechanisms that did not exist before it — the floor coming back after a throwing stub, and the hook that counts its own calls to show neither the state copy nor the comparison runs one. composer build green (Unit 1173, Integration 27, examples 8), full mutation 3586 mutants / 249 escaped / MSI 93% against the gate of 92, rector clean, roave reports only the pre-existing verifyAll(): void -> int from #154, bin/package-audit 0 errors, docs build and claims green. `make docs-vale` needs a vale binary this host has not got; the docs job lints the new prose. Perf was not re-measured.
rasuvaeff
added a commit
that referenced
this pull request
Oct 13, 2026
* Inline forwarding: a class double runs the real bodies on itself
A forwarding double of a class no longer proxies the real bodies to the
instance it was given: the state that instance held is copied onto the
double, and an unmatched call runs the doubled class's own body with `$this`
still the double. A self-call inside real code therefore re-enters the
double — it meets stubs, an armed protocol and the call log — which is what a
`makePartial()` port needs and what the standing limitation ("understudy
proxies an object; it does not instrument one") described. An interface
double has no bodies to reach and proxies exactly as before.
The body is entered through `ReflectionMethod::invokeArgs()` on a reflection
of the doubled class, not through a generated `parent::` closure. The closure
variant was built and measured first: handed to the dispatcher as an argument
of every generated method, it cost +10% per call on every class double,
forwarding or not, because the argument is evaluated before dispatch can
decide it is not needed — and `perf/` doubles only interfaces, so the harness
cannot see it. With the reflection the generated code is byte-identical to
master. A generated `__understudyReal_*()` sibling is the third way, and it
is closed by golden rule 3. The language claim — non-virtual entry, virtual
self-calls, protected bodies, by-reference parameters, `$this` identity — is
pinned by spikes/08-inline-forwarding on 8.3, 8.4 and 8.5.
Decisions behind the shape:
- Inline needs the instance to be exactly the doubled class. A donor that is
a subclass carries overrides the doubled class's bodies do not contain, and
running those instead would silently change which code executes, so it
keeps the proxy form.
- `callOriginal()` follows the double's own form, because after the copy the
donor's state is no longer the current one.
- A clone made while a real body runs inherits that double's forwarding:
without it a `: self` wither, or a body merely computing over an internal
copy, would hold a double answering defaults in the middle of real code.
Expectations are still not copied, and a clone the test makes is untouched.
- A `new static` cannot be adopted — its constructor built its own state — so
it is refused by registration rather than by return type: an instance no
context knows is broken for every caller.
- Re-pointing an inline double at another instance is refused. A `readonly`
property cannot be written twice, and a second copy would discard what the
real code has written since.
Measured: mutation 3573 mutants / 93% MSI (gate 92), against 3481 / 93% on
master; the class-double dispatch loop moves -3.4%, -4.8%, +3.4% across three
paired runs, and `perf`/`perf-memory` are unchanged. Suites green on 8.3, 8.4
and 8.5.
* Ask the 8.4-only ReflectionProperty members by name, so psalm passes on 8.3
`hasHooks()`, `isVirtual()`, `getRawValue()` and `setRawValue()` exist from
PHP 8.4, and the state copy reaches them behind a `PHP_VERSION_ID` guard — but
Psalm reflects the real `ReflectionProperty` of the PHP it runs on, so naming
them directly is an `UndefinedMethod` under 8.3. `composer build` was green on
a machine with 8.5 and red in the `PHP 8.3` and `Prefer lowest` matrix jobs,
which is where this was caught.
They are now asked through two private helpers that take the member name as a
parameter, so nothing can resolve it — the idiom
`Codegen\PropertyDefaults::isWritableFromOutside()` already uses for the same
three members. Verified by running psalm under php:8.3-cli and php:8.4-cli,
not only under the 8.5 the build image carries.
* Adopt a clone only when it is a copy of the double whose body is running
Inline forwarding adopted any clone of the running double's class, and one
generated class serves every double of a type: a real body handed another
double of its own class — `merge(self $other)` — and cloning that one gave
the copy bodies to run with no state copy to run them over. Where the class
declares a typed property that is PHP's own "must not be accessed before
initialization" out of the clone; where every property has a declared
default it is a quiet wrong answer computed from the declaration instead of
the donor. Both were a regression: proxied, the copy answered defaults.
`Runtime::isCopyOf()` now compares the two property tables with
`get_mangled_object_vars()` — the only reading that reaches a parent's
private properties, answers a hooked property with its backing store rather
than running the doubled class's `get` in the middle of `__clone()`, has no
entry for a virtual one, and omits what nobody initialized. A copy is
property-identical to its source at this moment, because a generated
`__clone()` does nothing but call `adoptClone()`.
Two doubles of one class in the same state stay indistinguishable, because
`__clone()` is handed no reference to its source: such a copy is still
adopted and answers for real, over the state it already holds. That
residual is documented where the rule is stated — both READMEs, the
forwarding guide, the CHANGELOG — rather than called harmless.
A `clone $double` written in the test's own `answers()` closure was adopted
too, although the documented rule makes it a fresh double: the closure runs
inside dispatch, which a real body can reach. `Runtime::$inlineFloor` hides
the frames underneath for exactly the closure's span, so only what the
closure itself enters — `callOriginal()`, which pushes a frame of its own —
is visible to a clone made there.
Five of the seven new tests fail or error on the previous commit: both
forms of the foreign clone, the test closure under a real body, the
comparison reading a hooked property through its backing store, and a plain
property declared after a hooked and a virtual one deciding the verdict.
The other two pin mechanisms that did not exist before it — the floor
coming back after a throwing stub, and the hook that counts its own calls
to show neither the state copy nor the comparison runs one.
composer build green (Unit 1173, Integration 27, examples 8), full mutation
3586 mutants / 249 escaped / MSI 93% against the gate of 92, rector clean,
roave reports only the pre-existing verifyAll(): void -> int from #154,
bin/package-audit 0 errors, docs build and claims green. `make docs-vale`
needs a vale binary this host has not got; the docs job lints the new prose.
Perf was not re-measured.
* Type the clone fixtures by class name: a `self` parameter is a fatal below 8.5
The fixtures of the previous commit declared `mergeWith(self $other)`, and
the suite died on PHP 8.3 and 8.4 before reaching a single test:
Declaration of …\Generated\Understudy_c28436a2f78b6ee1::mergeWith(
Understudy_c28436a2f78b6ee1|ArgumentMatcher|Absent $other = …): string
must be compatible with …\Fixture\Fwd\Pipeline::mergeWith(
Pipeline $other): string
The generated override renders the parameter as `self`, which inside the
generated class is the generated class — a subclass of the doubled one, so
the widened union excludes the very type the contract accepts. PHP 8.5 lets
it through, which is why the authoring host and the 8.5 job were green while
every other job was not.
That is a codegen gap of its own, not something this branch introduced: no
fixture had a `self`-typed parameter before. It is filed separately, and the
regression fixtures here name their class explicitly, which is what a user's
class usually does anyway.
Unit suite 1173 green on 8.3, 8.4 and 8.5 (the first two in `php:8.N-cli`
directly, since `composer:2` is 8.5 and cannot see this); `composer build`
and `bin/package-audit` green.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #153
What
Understudy::verifyAll()now returnsint<0, max>— how many checks it verified — instead ofvoid, so a runner adapter can count real assertions instead of a flat one per test (understudy-phpunit'sassertPostConditions()doesaddToAssertionCount(1)whether the test armed one expectation or fifteen).One check, precisely
expect(), ortimes()on a stub);strictStubs, where its use is what is checked;A failed check throws, so the number only ever counts checks that passed. Checks
verify()made earlier are not part of it: those answered at their own call sites, andverifyAll()does not re-decide them.Compatibility
void → intis source-compatible for every existing caller: the method answered nothing before, so no caller could have read the answer, andUnderstudyisfinal, so no override shadows it.composer bc-check -- --from=v1.0.1agrees:The compatibility policy in both READMEs previously said "retyping an existing public member is a major-version change", which would have made this a 2.0 for a change no caller can observe. The policy now says a return type may be widened in a minor release where no caller can observe the change (and the same reasoning admits a parameter widened to a supertype every existing argument already satisfies); narrowing is never minor. The paragraph quotes this very change as its first application.
Notes for reviewers
checkOrdering()split intoorderedExpectations()(the collector) +orderingFailure()(the renderer), because the count needs to know whether an ordering constraint existed without re-walking states.allVerified()keeps its behaviour:orderingFailure(orderedExpectations($state)).failuresIn()returnsarray{list<VerificationFailure>, int<0, max>}now — counted by what is checked, not what passed: a failed check throws before the number exists anyway.docs/src/api/**regenerated withmake docs-api; the wide diff is the catch-up of version strings and satellite references from the already-committeddocs/.api-workspace/composer.lockmove, plus the realverifyAll()docblock change.max(1, n), understudy-testo metric+n) follow once this merges; the adapter contract suites in this repo's CI run against core HEAD either way.