Proposed change
Give ExprType a maximum nesting depth (e.g. 64 — generous next to
parse's existing limit of 10 and the spec's 2-level list limit) and
enforce it in the constructors, which become fallible:
pub fn list(elem: ExprType) -> Result<ExprType, TypeError>;
pub fn union(types: Vec<ExprType>) -> Result<ExprType, TypeError>;
pub fn unresolved(constraint: ExprType) -> Result<ExprType, TypeError>;
pub fn signature(params: Vec<ExprType>, ret: ExprType) -> Result<ExprType, TypeError>;
pub fn new(code: TypeCode, params: Vec<ExprType>) -> Result<ExprType, TypeError>;
All construction already funnels through one internal point, so the check
itself is a few lines. Returning Result rather than panicking follows the
ecosystem's smart-constructor idiom (CString::new, regex::Regex::new;
cf. chrono deprecating its panicking date constructors) and matches this
crate's own Float64::new and make_list. Panicking instead is not an
option: panics surface uncatchably through the FFI boundaries of the
PyO3/wasm bindings.
This is a breaking API change. The bulk of the work is mechanical:
~140 call sites in workspace src (mostly already in Result-returning
contexts, so ?) and ~210 in tests (unwrap()). Pre-release is the
cheapest this break will ever be.
Motivation
The constructors currently place no bound on nesting depth, while every
traversal over a type — satisfies, PartialEq, Hash, Display, and
the derived Drop — is recursive. A sufficiently deep type overflows the
stack, which aborts the process (not a catchable panic):
let mut ty = ExprType::INT;
for _ in 0..50_000 {
ty = ExprType::list(ty);
}
ExprValue::Int(1).coerce(&ty, PathFormat::Posix); // stack overflow, abort
// `ty == ty`, hashing `ty`, or even dropping `ty` abort the same way
ExprType::parse is already bounded, and the PyO3/wasm bindings construct
types from strings through parse, so this is reachable only by Rust API
callers building types programmatically.
Bounding depth at construction makes the unsafe state unrepresentable and
protects every recursive consumer at once. The alternatives protect less:
depth checks at API boundaries (e.g. coerce, SymbolTable::set) would
still let over-deep types abort via ==/Hash/Drop, and rewriting all
six traversals iteratively would trade the crate's clearest code for
manual worklist implementations of PartialEq, Hash, Drop, and
friends.
Proposed change
Give
ExprTypea maximum nesting depth (e.g. 64 — generous next toparse's existing limit of 10 and the spec's 2-level list limit) andenforce it in the constructors, which become fallible:
All construction already funnels through one internal point, so the check
itself is a few lines. Returning Result rather than panicking follows the
ecosystem's smart-constructor idiom (CString::new, regex::Regex::new;
cf. chrono deprecating its panicking date constructors) and matches this
crate's own Float64::new and make_list. Panicking instead is not an
option: panics surface uncatchably through the FFI boundaries of the
PyO3/wasm bindings.
This is a breaking API change. The bulk of the work is mechanical:
~140 call sites in workspace src (mostly already in Result-returning
contexts, so ?) and ~210 in tests (unwrap()). Pre-release is the
cheapest this break will ever be.
Motivation
The constructors currently place no bound on nesting depth, while every
traversal over a type — satisfies, PartialEq, Hash, Display, and
the derived Drop — is recursive. A sufficiently deep type overflows the
stack, which aborts the process (not a catchable panic):
ExprType::parse is already bounded, and the PyO3/wasm bindings construct
types from strings through parse, so this is reachable only by Rust API
callers building types programmatically.
Bounding depth at construction makes the unsafe state unrepresentable and
protects every recursive consumer at once. The alternatives protect less:
depth checks at API boundaries (e.g. coerce, SymbolTable::set) would
still let over-deep types abort via ==/Hash/Drop, and rewriting all
six traversals iteratively would trade the crate's clearest code for
manual worklist implementations of PartialEq, Hash, Drop, and
friends.