Repository navigation
fix(core): reject transient reuse after persistent! and clean up leaked PHP type errors - #2229
Merged
Merged
Conversation
…ed PHP type errors Surfaced by the clojure-test-suite CI job (#2223). - Transients now guard their lifecycle: a shared TransientStateTrait invalidates the collection in persistent(), so conj!/assoc!/dissoc!/ disj!/pop! (or a second persistent!) after persistent! throw instead of silently mutating structure shared with the persisted value. Applied to TransientVector, TransientMapWrapper (hash/array maps + hash-sets) and TransientSortedMap (sorted maps + sets). Persistent ops on transients stay lenient by design: the fast into/group-by paths reduce conj/assoc over a transient accumulator. - nan?/NaN? on nil returns false without leaking is_nan() deprecation. - take's lazy 2-arg form rejects a non-integer count with a clean InvalidArgumentException instead of leaking Seq::take() TypeError. - SequenceGenerator::toIterable() throws a clean InvalidArgumentException for a non-seqable value instead of leaking PHP's iterable return-type TypeError (covers remove, select-keys and other for/generator consumers).
Chemaclass
force-pushed
the
fix/clojure-suite-clean-errors
branch
from
May 29, 2026 20:28
59ff326 to
69eeb0f
Compare
Chemaclass
added a commit
that referenced
this pull request
May 29, 2026
The suite now runs 0 failures against main (#2229 fixed the real bugs; phel-lang/clojure-test-suite#20 marked the intentional :phel divergences), so drop the soft-gate: - add a pull_request trigger so the job reports on PRs - remove continue-on-error (failures now fail the job) - pin the suite checkout to the merged suite SHA (7d3db18b) for reproducible PR/push runs; the nightly cron keeps tracking main to surface upstream drift Relates to #2223.
Closed
4 tasks
Chemaclass
added a commit
that referenced
this pull request
May 29, 2026
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.
🤔 Background
The nightly
clojure-test-suiteCI job (informational,continue-on-error) runs the Clojure-compatibility suite against Phel. Triaging its failures (#2223) surfaced a handful of genuine Phel bugs rather than intentional leniency: transients could be reused afterpersistent!, and several bad-type inputs leaked raw PHPTypeError/ deprecation text instead of a clean Phel exception.Companion PR phel-lang/clojure-test-suite#20 marks the intentional divergences as
:phel; this PR fixes the real bugs.💡 Goal
Match Clojure where it makes sense without making Phel worse: enforce the transient/persistent contract and surface clean errors, while keeping Phel's deliberate lenient behaviours (nil-punning, polymorphic
conj/assocover a transient accumulator) intact.🔖 Changes
Phel\Lang\Collections\TransientStateTrait:persistent()invalidates the transient, and every mutator checks it first.conj!/assoc!/dissoc!/disj!/pop!— or a secondpersistent!— afterpersistent!now throwRuntimeException("Transient used after persistent! call") instead of silently mutating structure shared with the persisted value. Applied toTransientVector,TransientMapWrapper(hash/array maps + hash-sets) andTransientSortedMap(sorted maps + sets via delegation).conj/assoc/dissocstill accept transients — the fastinto/group-bypaths reduce them over a transient accumulator, so this polymorphism is load-bearing. (Clojure throws here; kept as a:pheldivergence in the suite.)nan?/NaN?onnilreturnsfalsecleanly instead of leakingis_nan(): Passing null ... is deprecated.takelazy (2-arg) form rejects a non-integer count with a cleanInvalidArgumentExceptioninstead of leakingSeq::take(): Argument #1 ($n) must be of type int.SequenceGenerator::toIterable()throws a cleanInvalidArgumentExceptionfor a non-seqable value instead of leaking PHP'sReturn value must be of type Traversable|arrayTypeError(coversremove,select-keysand otherfor/ generator consumers).TransientVectorTestinvalidation cases; newtests/phel/core/transient-safety.phelandtests/phel/core/clean-type-errors.phel.CHANGELOG.md+src/php/Lang/CLAUDE.mdupdated.Relates to #2223.
Full
composer testgreen; the clojure-test-suite run drops to 0 failures (with #20).