GH-85: Refuse a value the constructor parameter rejects - #140
Merged
Conversation
Types are resolved docblock-first, which is right when the docblock REFINES the
native declaration - `array` narrowed to `string[]` is the library's core
capability - and wrong when it WIDENS it. A docblock cannot grant a value the
target itself rejects.
The two lanes failed differently. Assigned to a property, the value goes through
the accessor and the write guard already turns the refusal into a reported
mismatch, so that lane never escaped. Passed to a constructor parameter there is
no assignment to intercept: the value reached `new $className()` and raised a
native TypeError outside the report - the contract break §4 names.
So the check sits at the hand-over, against each parameter's own declaration,
through the new NativeTypeMatcher. It is not a null check: it refuses every proven
violation of the native type, covering three shapes verified to escape today, each
pinned by a test that fails without the guard with exactly the TypeError it
describes:
- `@var int|null` over a promoted `int`, given null
- `@var int|string` over a promoted `int`, given a string
- a property carrying no type metadata at all, feeding a natively typed
parameter - nothing contradicts anything here, which is why keying on the
docblock would have missed it
A refused VARIADIC element is dropped on its own, keeping its valid siblings and
reporting under its own index, matching the collection element loop's "one bad
entry must not discard the rest" rule. A required parameter whose value is refused
records the refusal and then falls through to the lanes an absent value takes -
the same two-error, no-object outcome it already produces when its value fails
ordinary conversion, so the guard adds a lane rather than a new failure shape.
The matcher answers one-sidedly by construction: only a proven violation refuses,
and a declaration it cannot judge passes. A missed violation degrades to the error
PHP would have raised anyway; a fabricated one would refuse a valid value with no
way around it. It resolves `self`/`parent`/`static` against the declaring class,
because whether reflection spells a relative type as `self` or as the class it
stands for changed in PHP 8.5 - keying on the spelling would wave every value
through on 8.3 and 8.4, where reflection still reports the literal keyword.
Cross-checking nullability inside the type resolver was the other candidate and is
wrong: `ReflectionProperty::getType()` is the storage type, not the type the write
has to satisfy. A setter, or a `set` hook, may accept more than the field it
writes, so narrowing there refuses values the class documents itself as taking. A
regression test pins that direction.
The ci:test suite is green across the 8.3, 8.4 and 8.5 legs (549 tests, exit 0).
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
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
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 #85.
Property types are resolved docblock-first, which is right when the docblock refines the native declaration —
arraynarrowed tostring[]is the library's core capability — and wrong when it widens it. A docblock cannot grant a value the target itself rejects.The defect
The two lanes failed differently:
TypeMismatchException— that lane never escaped.new $className(...)and raised a nativeTypeErroroutside the mapping report — the contract break AGENTS.md §4 names (a nativeTypeError/ValueError/ArgumentCountErrorreaching the caller is invisible to error collection).The fix
A new
NativeTypeMatcherchecks each constructor argument against the parameter's own native declaration at the hand-over, before the spread. A proven violation is recorded as aTypeMismatchExceptionand then falls through to the lanes an absent value takes — a defaulted parameter takes its default, a required one recordsMissingConstructorArgumentExceptionand the object is not built (the same two-error, no-object outcome a required parameter already produces when its value fails ordinary conversion).It is not a null check: it refuses every proven widening, verified across three escaping shapes, each pinned by a test that fails without the guard with exactly the native
TypeErrorit describes:@var int|nullover a promotedint, givennull@var int|stringover a promotedint, given a stringFurther behaviour:
falseonly for a proven violation,truefor anything it cannot judge. A missed violation degrades to the error PHP would have raised anyway; a fabricated one would refuse a valid value with no way around it.self/parent/staticagainst the declaring class, because PHP 8.5 changed whether reflection spells a relative type as the keyword or the class it stands for — keying on the spelling would wave every value through on 8.3/8.4, where reflection still reports the literal keyword.Cross-checking nullability inside the type resolver was the rejected alternative:
ReflectionProperty::getType()is the storage type, not the type the write must satisfy — a setter or asethook may accept more than the field it writes, so narrowing there would refuse values the class documents itself as taking. A regression test pins that direction.Verification
composer ci:testgreen across the 8.3, 8.4 and 8.5 legs (549 tests).NativeTypeMatcheris unit-tested per type category (builtins in both directions, nullable, union, intersection, DNF,describe()rendering); the constructor-guard behaviour is pinned end-to-end.🤖 Generated with Claude Code