revset: bound recursion during resolution

[mirkomartn]

Fixes #10071.

An alternative would be to use stacker.

We might also consider making the limit configurable.

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (cli/src/config-schema.json)
• I have added/updated tests to cover my changes
• I fully understand the code that I am submitting (what it does,
  how it works, how it's organized), including any code drafted by an LLM.
• For any prose generated by an LLM, I have proof-read and copy-edited with
  an eye towards deleting anything that is irrelevant, clarifying anything
  that is confusing, and adding details that are relevant. This includes,
  for example, commit descriptions, PR descriptions, and code comments.

[PhilipMetzger]

Can you provide sufficient motivation in the commit message and move the fixes line into it to adhere to our commit guidellines. Thanks.

[yuja]

It's too late to reject deep expressions at the symbol resolution stage. We probably need a parsing-stage depth limit. PrattParser can also crash on deeply nested prefixes.

Some related discussion in pest-parser/pest#1129

[mirkomartn]

It's too late to reject deep expressions at the symbol resolution stage. We probably need a parsing-stage depth limit. PrattParser can also crash on deeply nested prefixes.

Some related discussion in pest-parser/pest#1129

Added a stacker-based check, so that we can check preemptively how much space is still left on the stack, before recursing deeper.

I left it at the same place (was just the least work to do so), but I think it should be very straightforward to add the same check at other places where there's currently unbounded recursion, if there's an agreement to go about it this way.

I'm also unclear on whether we should have a global bound on minimal space left on stack (what I think I'd prefer), or whether the limit could be specified at call-site. I currently set it to 20kb and I could confirm that the reproducer in the original issue no longer crashes (15kb on the other hand was too low and jj crashed).

[bos-voxel]

I applied this locally (macOS arm64) and ran the reproducer from #10071 plus some variants. It still overflows in every case, in both debug and release builds. Two separate causes:

1. The 20 KB red zone is smaller than one recursion level's frames in a debug build, so the check passes and the next level overflows. With RED_ZONE raised to 256 KB, depths 600 to 1000 return a clean error, so the stacker mechanism itself works.
2. Only symbol resolution is guarded. Even at 256 KB, depth 3000 crashes inside alias expansion (dsl_util::AliasExpander), which runs before resolution. The pest parser, lowering, the optimizer passes, and evaluation are also unguarded, and templates and filesets have the same problem.

Two smaller things: StackSizeUnknown should probably degrade to Ok(()) rather than fail every revset on platforms where stacker can't detect the limit, and the error text should say the expression is nested too deeply rather than mention a red zone.

Given all that, I think a deterministic nesting-depth limit in the three parsers (plus a cap in alias expansion) would be simpler and would cover every downstream pass, along the lines suggested in the issue thread.

[mirkomartn]

I applied this locally (macOS arm64) and ran the reproducer from #10071 plus some variants. It still overflows in every case, in both debug and release builds. Two separate causes:

Thanks for taking your time and checking!

1. The 20 KB red zone is smaller than one recursion level's frames in a debug build, so the check passes and the next level overflows. With RED_ZONE raised to 256 KB, depths 600 to 1000 return a clean error, so the stacker mechanism itself works.

Right, this was just a "works on my machine" example and I think we can and should have a conservatively high value for RED_ZONE (user can potentially always increase the stack size allocated by the OS).

2. Only symbol resolution is guarded. Even at 256 KB, depth 3000 crashes inside alias expansion (dsl_util::AliasExpander), which runs before resolution. The pest parser, lowering, the optimizer passes, and evaluation are also unguarded, and templates and filesets have the same problem.

Yes, that much was expected per the previous comment from @yuja, but as said, I first wanted to get an agreement on the mechanism and then apply it broadly.

Two smaller things: StackSizeUnknown should probably degrade to Ok(()) rather than fail every revset on platforms where stacker can't detect the limit, and the error text should say the expression is nested too deeply rather than mention a red zone.

Not sure I agree with the StackSizeUnknown handling, but it's really fringe anyway, so I won't argue with whatever a maintainer would choose.

Given all that, I think a deterministic nesting-depth limit in the three parsers (plus a cap in alias expansion) would be simpler and would cover every downstream pass, along the lines suggested in the issue thread.

Agree, but only that I would use a stacker mechanism instead of nesting-depth (which was what I originally went with). I think it's just more composable/generic and could potentially be used elsewhere. But again, don't feel strongly about it to argue for it.