Skip to content

Return null from the limit descent's default, not the node (#829, #833) - #836

Merged
Rafael-SOWNet merged 1 commit into
masterfrom
fix/limit-base-returns-null
Aug 9, 2026
Merged

Return null from the limit descent's default, not the node (#829, #833)#836
Rafael-SOWNet merged 1 commit into
masterfrom
fix/limit-base-returns-null

Conversation

@Rafael-SOWNet

Copy link
Copy Markdown
Collaborator

Fixes #829 and #833 at the root, and stops six more nodes crashing that #831 does not reach.

The default was the bug

internal virtual Entity? ComputeLimitDivideEtImpera(Variable x, Entity dist, ApproachFrom side)
    => new Limitf(this, x, dist, side);

That is the cycle AGENTS.md names, with this exact expression as its example:

Say "no answer" by returning null, not by handing back an unevaluated node of the expression you were asked about. Limitf(this, ...) looks like the honest answer and is in fact a cycle: the caller evaluates it to compare, evaluating computes the limit, and computing arrives back where it started. That overflows the stack, which kills the process rather than raising anything catchable.

So a node crashed by inheriting the default — by not being mentioned anywhere. Measured on 532d89cf, each of these kills the process:

limit(floor(x), x, 0)     limit(min(x, 1), x, 0)    limit(phi(x), x, 0)
limit(ceil(x), x, 0)      limit(max(x, 1), x, 0)
limit(round(x), x, 0)     limit(gcd(x, 2), x, 0)

The first six arrived with #827 and #828 — mine, and I skipped the ComputeLimitDivideEtImpera override that AddingNode.cs lists at step 3d. phi predates 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 with is { } or switch on the result, and the signature was already Entity?. 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

LimitTerminatesOnEveryNodeTest reflects over every sealed Entity subtype 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 floor and ceil real limit answers and fixes the OverflowException of #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, gcd and phi still killing the process, which is why this is separate rather than folded into it.

🤖 Generated with Claude Code

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>
@Rafael-SOWNet
Rafael-SOWNet merged commit 8e7792a into master Aug 9, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Limit over floor or ceil overflows the stack and kills the process

1 participant