Return self from every Expression builder; fold Eq/Gt into Comparison - #103
Merged
Conversation
The fluent builder on Expression is public @api, but nine of its fifteen combinators declared an @internal concrete node (Eq, Add, Call, ...) as their return type while the other six returned self. The surface both leaked internal symbols and contradicted itself — eq returned Eq but its sibling neq returned self. Every combinator now returns self. Once eq() and gt() no longer name Eq and Gt, those two classes have no reason to exist: they were @internal final subclasses of Comparison that added only a fixed constructor. They fold into Comparison, which is now final since all six operators are plain instances of it. Comparison::equals already matches on self, so a former Eq/Gt value and a plain Comparison compare equal — behavior is preserved. Structural BC break (return-type narrowing on public methods + class removals); correct for the 0.3.0 breaking release. Closes #92 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PbPoW5wtovYbi3R6z62fWT
MidnightDesign
deleted the
92-expression-builder-methods-return-internal-concrete-types-instead-of-self-fold-eqgt-into-comparison
branch
July 24, 2026 08:44
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.
Closes #92. Scoped for the 0.3.0 breaking release.
Problem
The fluent builder on
Expressionis our most-used public@apisurface, but nine of its fifteen combinators declared a class-level@internalnode as their return type while the other six returnedself. So the surface both leaked internal symbols and contradicted itself —eqreturnedEqbut its siblingneqreturnedself;gtreturnedGtbutlt/gte/ltereturnedself.Separately,
EqandGtwere BC shims:@internal finalsubclasses ofComparisonthat added nothing but a fixed constructor. They only still existed becauseExpression::eq()/gt()named them as return types beforeComparisonexisted.Changes
Expressionbuilder combinators now returnself. The rationale that was spelled out onnot()'s docblock is now the class-level rule, so it moved to the class docblock and the per-method note is gone.Expr::eq()/gt()returnComparisonand construct through the sharedcomparison()helper.EqandGtare removed; they fold intoComparison, which is nowfinal(all six operators are plain instances of it).Comparison::equals()already matches onself, so a formerEq/Gtvalue and a plainComparisoncompare equal — behavior is preserved.ComparisonandBinaryOperatorupdated to drop theEq/Gtreferences and the "four of six" framing.is_callable(), which only worked because the builders returned final concrete nodes. Withself(abstractExpression, possibly-callable via a hypothetical__invokesubclass) PHPStan widened$expr()tomixed. Reordered it to checkinstanceof Expressionfirst, mirroring the consuming test method.BC impact
Structural break — return-type narrowing on public methods plus two class removals. The Roave checker will flag it. Correct for a
0.MINORbump; frozen after 1.0.0, at which point we'd be stuck publishingEq/Add/… as permanent public types.Checks
cs-check,psalm,phpstan,check-deps,phpunit(1152 tests), andinfectionon changed lines (100% MSI) all pass locally.🤖 Generated with Claude Code