Skip to content

Commit ecde5ae

Browse files
stroxlermeta-codesync[bot]
authored andcommitted
Fix a bug in narrow limit
Summary: The existing handling of the narrow depth limit is usesless! We wind up clearing narrows whenever we're about to set a narrow, which is actually a no-op: - We've already gotten the previous narrow when we computed the new narrow - And then we clear the narrows, but we immediately overwrite it with the new narrow - That means we haven't done anything at all in terms of how the bindings work, all we're accomplishing is writing to the same struct field twice in succession In addition, we know that 512 narrows is very likely too much, it will consume a prohibitively large amount of stack space. The examples we've seen where the limit gets hit are always generated files with a huge number of narrows at the top level (and we probably would do best not narrowing at all for all such examples seen so far), so I think a limit of 100 makes good sense - in practice I don't think non-generated code would hit this limit, but it saves a lot more stack usage when we do hit it. This commit changes the logic so that we intercept narrowing depth in `idx()` and we just ignore narrows at that point. The resulting behavior will be a bit different because now we'll completely ignore narrows when we hit the limit (as opposed to trying to reset at the raw value and then start narrowing again). I don't know if this behavior is better per se but I think it's easier for a user to understand, and again I think the limit is probably only going to be hit on generated code anyway in practice. Reviewed By: migeed-z Differential Revision: D91717810 fbshipit-source-id: 173319aec29d11232ff31b3b457b8316636c1ddf
1 parent 7aaabbd commit ecde5ae

File tree

2 files changed

+18
-19
lines changed

2 files changed

+18
-19
lines changed

pyrefly/lib/binding/scope.rs

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -472,7 +472,10 @@ impl Flow {
472472
/// Bound names can accumulate facet narrows from long assignment chains (e.g. huge
473473
/// literal dictionaries). Limiting how many consecutive narrows we remember keeps
474474
/// the flow graph shallow enough to avoid recursive explosions in the solver.
475-
const MAX_FLOW_NARROW_DEPTH: usize = 512;
475+
///
476+
/// When this limit is reached, lookups return the base value instead of the narrow
477+
/// chain, breaking the recursion. This is checked in `FlowInfo::idx()`.
478+
const MAX_FLOW_NARROW_DEPTH: usize = 100;
476479

477480
/// Flow information about a name. At least one of `narrow` and `value` will always
478481
/// be non-None (although in some cases the value may have FlowStyle::Uninitialized,
@@ -563,12 +566,14 @@ impl FlowInfo {
563566
}
564567
}
565568

566-
fn clear_narrow(&mut self) {
567-
self.narrow = None;
568-
self.narrow_depth = 0;
569-
}
570-
571569
fn idx(&self) -> Idx<Key> {
570+
// When the narrow depth limit is exceeded, return the base value instead
571+
// of the narrow chain to break recursion in the solver.
572+
if self.narrow_depth >= MAX_FLOW_NARROW_DEPTH
573+
&& let Some(FlowValue { idx, .. }) = &self.value
574+
{
575+
return *idx;
576+
}
572577
match (&self.narrow, &self.value) {
573578
(Some(FlowNarrow { idx, .. }), _) => *idx,
574579
(None, Some(FlowValue { idx, .. })) => *idx,
@@ -1679,11 +1684,7 @@ impl Scopes {
16791684
e.insert(FlowInfo::new_narrow(idx));
16801685
}
16811686
Entry::Occupied(mut e) => {
1682-
let mut info = e.get().clone();
1683-
if info.narrow_depth >= MAX_FLOW_NARROW_DEPTH {
1684-
info.clear_narrow();
1685-
}
1686-
*e.get_mut() = info.updated_narrow(idx, in_loop);
1687+
*e.get_mut() = e.get().updated_narrow(idx, in_loop);
16871688
}
16881689
}
16891690
}

pyrefly/lib/test/subscript_narrow.rs

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -404,13 +404,12 @@ def use2(mapping: NotDict) -> None:
404404

405405
// This test verifies behavior related to MAX_FLOW_NARROW_DEPTH in scope.rs.
406406
// By assigning to the same key repeatedly, we increment the narrow depth.
407-
// The depth limit should break the narrow chain to prevent stack overflow,
408-
// but currently the limit (512) is too high and the check happens too late.
407+
// After exceeding the depth limit (100), the narrow chain is broken and we
408+
// fall back to the base type to prevent stack overflow during solving.
409409
testcase!(
410-
bug = "MAX_FLOW_NARROW_DEPTH check is ineffective; should limit to ~100 and check in idx()",
411410
test_many_consecutive_subscript_assigns,
412411
r#"
413-
from typing import assert_type, Literal
412+
from typing import assert_type
414413
415414
def test() -> None:
416415
d: dict[str, int] = {}
@@ -519,9 +518,8 @@ def test() -> None:
519518
d["k"] = 102
520519
d["k"] = 103
521520
d["k"] = 104
522-
# After 105 assignments, the narrow depth limit should be exceeded and we
523-
# should fall back to `int`. But currently we still track Literal[104]
524-
# because MAX_FLOW_NARROW_DEPTH=512 is too high and the check is ineffective.
525-
assert_type(d["k"], Literal[104])
521+
# After 105 assignments, the narrow depth limit (100) is exceeded and we
522+
# fall back to `int` instead of tracking the literal value.
523+
assert_type(d["k"], int)
526524
"#,
527525
);

0 commit comments

Comments
 (0)