Return null from the limit descent's default, not the node (#829, #833) - #836
Merged
Conversation
ComputeLimitDivideEtImpera defaulted to new Limitf(this, x, dist, side). That is the cycle AGENTS.md names, with this exact expression as its example: the caller evaluates the returned node to compare it, evaluating a Limitf computes the limit, and computing arrives back at the default. It overflows the stack, which kills the process rather than raising anything a caller can catch. So a node crashed by inheriting the default -- by not being mentioned anywhere. Seven did: limit(floor(x), x, 0) stack overflow, process dies limit(ceil(x), x, 0) stack overflow limit(round(x), x, 0) stack overflow limit(min(x, 1), x, 0) stack overflow limit(max(x, 1), x, 0) stack overflow limit(gcd(x, 2), x, 0) stack overflow limit(phi(x), x, 0) stack overflow The first six arrived with #827 and #828, which added the nodes without the ComputeLimitDivideEtImpera override that AddingNode.cs lists at step 3d. phi predates both and is #833, so this is not only my regression -- the default has been a landmine for every node that ever inherited it, which is what #704 was. Returning null is what AGENTS.md prescribes and what every caller already expects: each of the eleven call sites tests the result with `is { }` or switches on it, and the signature was already Entity?. All seven now come back as an unevaluated limit node, which is the honest "I could not settle this", and nothing else moves -- 5928 tests passed before this change and after it. LimitTerminatesOnEveryNodeTest enumerates the node types by reflection rather than listing them, because the defect is in what a node inherits by not being mentioned, and a per-node test cannot catch that: 45 types, each over three destinations and three sides, asserting only that the call returns. Checked against the unfixed code, where the run aborts. This does not replace #831, which gives floor and ceil real limit answers and fixes the OverflowException of #830. That adds capability; this removes the way to crash. They compose, and the order does not matter. Tests: 5973 passing, 0 failed, 14 skipped; F# 130. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 9, 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.
Fixes #829 and #833 at the root, and stops six more nodes crashing that #831 does not reach.
The default was the bug
That is the cycle AGENTS.md names, with this exact expression as its example:
So a node crashed by inheriting the default — by not being mentioned anywhere. Measured on
532d89cf, each of these kills the process:The first six arrived with #827 and #828 — mine, and I skipped the
ComputeLimitDivideEtImperaoverride thatAddingNode.cslists at step 3d.phipredates both and is #833, so the default has been a landmine for every node that ever inherited it. That is what #704 was.The fix is the one AGENTS.md prescribes
Return
null. Every caller already expects it: all eleven call sites test withis { }or switch on the result, and the signature was alreadyEntity?. All seven expressions now return an unevaluated limit node — the honest "I could not settle this".It costs nothing. 5928 tests passed before this change and 5928 after; the extra 45 are the new test below.
The test enumerates rather than lists
LimitTerminatesOnEveryNodeTestreflects over every sealedEntitysubtype constructible from a variable — 45 of them — and asserts only that a limit returns, over three destinations and three sides.That shape is the point. The defect is in what a node inherits by not being mentioned, so a per-node test cannot catch it: the node that crashes is precisely the one nobody wrote a test for. A node added tomorrow is covered the day it is added.
Checked against the unfixed code, where the run aborts rather than fails — a stack overflow cannot be caught, so the signal is the test host dying. Brutal, but detected.
Relationship to #831
This does not replace #831 and does not conflict with it (different files). #831 gives
floorandceilreal limit answers and fixes theOverflowExceptionof #830 — it adds capability. This removes the ability to crash. They compose, and the merge order does not matter.Worth noting that #831 alone would leave
round,min,max,gcdandphistill killing the process, which is why this is separate rather than folded into it.🤖 Generated with Claude Code