New world: single-pass expression processing through ExpressionResult callbacks#5838
Draft
ondrejmirtes wants to merge 8 commits into
Draft
New world: single-pass expression processing through ExpressionResult callbacks#5838ondrejmirtes wants to merge 8 commits into
ondrejmirtes wants to merge 8 commits into
Conversation
Carry a lazy typeCallback and specifyTypesCallback on ExpressionResult so an expression's Type and SpecifiedTypes are available after processExprNode finishes, without re-traversing via MutatingScope::getType or TypeSpecifier. - ExpressionResult: getType/getNativeType/getTypeForScope (#5224-style single scope-arg callback), getSpecifiedTypes, and getTruthyScope/getFalseyScope rebuilt on top of it. When a handler has not supplied a callback, getType falls back to $scope->getType (legacy bridge: works under PHPSTAN_FNSR=0, hits the guard under FNSR=1). - Store ExpressionResult per Expr; FiberScope::getType/getNativeType now suspend for the whole ExpressionResult (ExpressionResultForExprRequest, renamed) and resume at the end of processExprNode via storeResult; base storeResult populates storage so findResult works without fibers too. - ImplicitToStringCallHelper takes the resolved Type from the caller's child ExpressionResult; findEarlyTerminatingExpr takes the result type. - Migrate ScalarHandler, VariableHandler and AssignHandler to supply the type callback (Assign reads its RHS type/native-type from the stored result). echo '1' passes at level 8 under the guard; FNSR=0 stays on the legacy path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
processAssignVar received the AssignOp node itself as the assigned expression (e.g. `$sum += $v`), which is never processed through processExprNode and so has no stored ExpressionResult. findResult returned null and threw. Only plain Assign values are stored; fall back to the legacy scope type/native-type for the rest (works under PHPSTAN_FNSR=0, hits the guard under FNSR=1 as expected for not-yet-migrated AssignOp). Restores FNSR=0 parity with baseline across try/catch, closures, foreach, match, properties and AssignOp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…orld The new world is cut away from the old: typeCallback/specifyTypesCallback carry copied-and-adjusted code, never delegating to resolveType()/ specifyTypes() (which are deleted in 3.0). ResultAwareScope is used only at the sanctioned boundaries: extension invocations and ParametersAcceptorSelector (+ TypeSpecifier conditional-return/assert helpers until ported). - ResultAwareScope: non-suspending adapter for code that receives a Scope mid-analysis. getType() tiers: ExpressionTypeResolverExtensions -> scope- tracked holder -> known child ExpressionResults -> inline re-processing of the (possibly synthetic) expression -> guarded legacy bridge. Derivation-safe (pushInFunctionCall/popInFunctionCall carry the adapter context, mirroring FiberScope); native variant via doNotTreatPhpDocTypesAsCertain override. - TypeSpecifier::specifyTypesInCondition head-checks: ResultAwareScope recursion stays in the new world; FiberScope (rules, e.g. ImpossibleCheckTypeHelper) suspends for the ExpressionResult. - FiberScope: getExpressionResult() extracted; doNotTreatPhpDocTypesAsCertain stays fiber-aware. - ScalarHandler: specifyTypesCallback via DefaultNarrowingHelper (new-world copy of default truthy/falsey narrowing using the expression's own type). - AssignHandler: the processAssignVar callback result carries the assigned value's type (hasTypeCallback() contract, AssignOp wraps with expr only and bridges); nested assigns flow through result delegation (no unwrapAssign on the type path, no storage lookups); specifyTypesForAssign covers null context (RHS result narrowing minus the assigned var) and variable targets (default narrowing with the RHS type); conditional-expression holders gated old-world-only with a TODO. - FuncCallHandler: resolveTypeViaResults/specifyTypesViaResults new-world copies; dynamic name uses the name ExpressionResult; call_user_func/clone synthetics processed inline; getFunctionThrowPoint takes a lazy return-type callback and gives throw-type extensions the adapter; processArgs takes the callable-arg type from the result. - NewWorld::isEnabled() transitional switch; NewWorldTypeInferenceTest (temporary, deleted when the suite is green under the guard) - 13 assertions green in both worlds; FNSR=0 parity verified on stress files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…essionResults TDD leg driven by NewWorldTypeInferenceTest (33 assertions, green under the guard and under PHPSTAN_FNSR=0 alike): - MutatingScope::applySpecifiedTypes: new-world replacement for filterBySpecifiedTypes. Original types resolved in tiers (extension registry -> scope-tracked holders -> caller-supplied ExpressionResults -> guarded bridge); add/remove math mirrored locally; the conditional-holder matching tail extracted into a method shared with filterBySpecifiedTypes. ExpressionResult::getTruthyScope/getFalseyScope (memoized) and the per-statement createNull narrowing run on it. - VariableHandler: copied typeCallback (dynamic names bridge until equality migrates) + default-narrowing specifyTypesCallback — `if ($v)` narrows in both branches. - TypeExprHandler/NativeTypeExprHandler migrated (type = the wrapped type); synthetic fiber requests are processed on the plain scope — a FiberScope would suspend from within (this looped infinitely in the asserts flow). - FuncCallHandler: conditional-return-type and asserts narrowing copied in (specifyTypesFrom*ViaResults) instead of delegating to TypeSpecifier internals; @api create()/specifyTypesInCondition() with the adapter remain the entry points. getFunctionThrowPoint takes a lazy return-type callback. - AssignHandler: conditional-expression holders ported to the new world — truthy/falsey projection and falsey-scalar equality holders are built from the assigned value's ExpressionResult with a per-entry type resolver (assigned result -> tracked holders -> skip unpriceable entries such as conditional-return narrowing of inner call arguments). AssignRef narrows with default narrowing and reads the intertwined variable types from the value result. Ternary/Match holders stay old-world until those handlers migrate. - NodeScopeResolver: If/elseif condition types from the condition ExpressionResult (new world; old world keeps pre-scope semantics); processArgs reuses per-arg result types for callable-arg and impure-callee invalidation. - No TODO markers remain in new-world code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cles Running with the guard exceptions disabled (or PHPSTAN_FNSR=0) is the mixed mode: new world where migrated, old world everywhere else. Several seams hard-failed or recursed instead of bridging: - TypeSpecifier head-checks: when the ExpressionResult has no specifyTypesCallback, fall through to the guarded old-world dispatcher (FiberScope unwraps to its MutatingScope; ResultAwareScope is kept so inner lookups stay unguarded) instead of throwing "not migrated". - FuncCallHandler: adapters are seeded with a self-result carrying the call's own type/specify callbacks, so is_int()-family return type extensions going through ImpossibleCheckTypeHelper are answered from the call's own narrowing instead of re-processing the call - which recursed forever. - ResultAwareScope: on-demand processing marks the expression in flight on the storage (duplicates inherit the marks), so descendants detect ancestor cycles and degrade to the guarded bridge - PreDec entries re-entered the assign holders otherwise. Missing adapter context (scope-mutation paths beyond pushInFunctionCall) degrades to the bridge instead of throwing. - AssignHandler: holder entries are priced through a fresh adapter (tiers + cycle guard) instead of raw re-processing; assigns with a not-yet-migrated value keep their old-world holders via the guarded bridge. - FiberNodeScopeResolver: don't spin up fibers for NoopNodeCallback. NodeScopeResolverTest in mixed mode: 1650 tests run to completion, 1605 green; the 45 type diffs are split between new-world improvements (multi-assign narrows intermediate variables soundly) and inference gaps to triage. NewWorldTypeInferenceTest stays 33/33 under the guard and under FNSR=0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mnants Mixed-mode self-analysis (make phpstan with the guards disabled) runs in ~40s and now reports zero findings in the new-world code: - AssignHandler: the holder entry resolver always produces a type (the adapter prices everything), so the nullable signature and dead null checks go away; callable signatures documented; named argument for SpecifiedTypes. - FuncCallHandler: instanceof ConstantBooleanType replaced with Type::isTrue()/isFalse() in the conditional-return and asserts copies; getFunctionThrowPoint callback documented. - ExpressionResult: getNativeType checks the promoted scope instance. - ExpressionResultStorage: the before-scope API is dead since fibers resume on results - removed; fiber generics docblocks made parser-friendly. - FiberScope: dead preprocessScope removed (narrowing replay is gone). - FiberNodeScopeResolver: null-guard the parked-fiber pop, annotate the new Fiber. - phpstan-baseline: two more useless string casts in the applySpecifiedTypes copies of the filterBySpecifiedTypes loops. Remaining mixed-mode self-analysis findings (24) are inference divergences in existing src code - same triage bucket as the 45 NodeScopeResolverTest diffs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
What this is
The next stage of the ExprHandler refactoring: stop traversing the AST multiple times per expression. Today
NodeScopeResolverupdates the Scope,MutatingScope::resolveTypere-walks the expression for itsType, andTypeSpecifier::specifyTypesInConditionre-walks it again for narrowing — forcing pathologies likeBooleanAndHandler::resolveTypere-runningprocessExprNodeon a throwaway storage just to rebuild a truthy scope it already had.The new world: after
processExprNodefinishes, theExpressionResultcarries not just the scope but lazytypeCallback/specifyTypesCallbackwired by the handler at the moment it had its children's results and the correct intermediate scopes in hand. Rules and extensions get answers through two adapters:FiberScope(rule node-callbacks suspend until the result exists) andResultAwareScope(extensions invoked mid-analysis never suspend — children are already processed; synthetics are processed inline).Full design doc with motivations, settled decisions, inventory and status log:
NEW_WORLD.md(branch-lifetime document).Enforcement
Scope::getType()/getNativeType()/getKeepVoidType()andTypeSpecifier::specifyTypesInCondition()throw unlessPHPSTAN_FNSR=0. The old world stays fully functional underPHPSTAN_FNSR=0(the PHP < 8.1 path until 3.0, where it is mass-deleted together with allresolveType/specifyTypesmethods,filterBySpecifiedTypes,filterByTruthy/FalseyValueand the dispatcher).Working agreements baked into the branch
resolveType/specifyTypes. Duplication until 3.0 is accepted.ResultAwareScopeonly at sanctioned boundaries: extension invocations andParametersAcceptorSelector(the@apiTypeSpecifier::create()/specifyTypesInCondition()with the adapter remain legitimate entry points).ExpressionResults are threaded through closures — never fetched fromExpressionResultStorage(storage is the fiber rendezvous only).NewWorldTypeInferenceTest; each new-world branch gets a probing assert, verified identical in both worlds.Migrated so far
Scalar,Variable,Assign(incl. conditional-expression holders with a per-entry type resolver;unwrapAssignis no longer needed on the type path — nested assigns flow through result delegation),FuncCall(return type extensions,selectFromArgs, conditional return types,@phpstan-assert— the latter two copied in as*ViaResults),TypeExpr/NativeTypeExpr.MutatingScope::applySpecifiedTypes— original types resolved in tiers (extension registry → scope-tracked holders → caller-supplied results → guarded bridge); shares the conditional-holder matching tail withfilterBySpecifiedTypes.getTruthyScope/getFalseyScopeand per-statement narrowing run on it, soif/elsenarrowing works end-to-end in the new world.ExpressionResults (resume at the end ofprocessExprNode), synthetic expressions processed on demand on the plain scope,If_/elseif condition types andprocessArgsarg types from results,findEarlyTerminatingExprreordered.Verification
NewWorldTypeInferenceTest(temporary — delete once the whole suite is green under the guard): 33 assertions, green both under the guard and underPHPSTAN_FNSR=0, covering scalars, nested/by-ref assigns, params, extension-driven calls,if/elsenarrowing ($v = 1; if ($v)), assign-in-condition, function asserts, conditional return types, and holder-driven narrowing ($len = strlen($s); if ($len)→$sisnon-empty-string).PHPSTAN_FNSR=0parity against the pre-branch baseline verified on stress files (try/catch, closures,foreach,match, properties, AssignOp).Intentionally red
The pre-existing test suite is red under the guard until the rewrite completes — that is the migration pressure by design. Next natural handlers:
BinaryOpequality/comparisons (unlocks dynamic variable names and Ternary/Match holders),BooleanAnd/Or(deletes the flagship re-walk andBOOLEAN_EXPRESSION_MAX_PROCESS_DEPTH),Ternary/Coalesce.🤖 Generated with Claude Code