Skip to content

Commit ad5da5d

Browse files
authored
bug-fix: Fix self-loop cycle detections in type-system (#1725)
Prior: ``` class Foo { sub_foo Foo[] } class Bar { foo Foo } ``` would sometimes detect 2 cycles, `[Foo], [Foo, Bar]` but now we only detect 1 `[Foo]` <!-- ELLIPSIS_HIDDEN --> ---- > [!IMPORTANT] > Fixes cycle detection in `tarjan.rs` to ensure only unique cycles are detected, eliminating redundant cycle detections. > > - **Behavior**: > - Fixes cycle detection to ensure only unique cycles are detected in `tarjan.rs`. > - Previously detected redundant cycles like `[Foo], [Foo, Bar]`; now only `[Foo]` is detected. > - **Code Changes**: > - Removes redundant state updates in `strong_connect()` in `tarjan.rs`. > - Adds a test `find_cycles_names()` to verify unique cycle detection. > - **Tests**: > - Adds `find_cycles_names()` test to ensure only unique cycles are detected. > - Updates existing tests to reflect the new cycle detection behavior. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=BoundaryML%2Fbaml&utm_source=github&utm_medium=referral)<sup> for 35aa210. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
1 parent 5a93b0d commit ad5da5d

1 file changed

Lines changed: 55 additions & 5 deletions

File tree

engine/baml-lib/parser-database/src/tarjan.rs

Lines changed: 55 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,11 @@ impl<'g, V: Eq + Ord + Hash + Copy> Tarjan<'g, V> {
125125

126126
// Increment index and push node to stack.
127127
self.index += 1;
128+
129+
// Update state. We store this in a hash map
130+
// so we have to run the hashing algorithm every time we update the
131+
// state. Keep it to a minimum :)
132+
self.state.insert(node_id, node);
128133
self.stack.push(node_id);
129134

130135
// TODO: @antoniosarosi: HashSet is random, won't always iterate in the
@@ -139,8 +144,6 @@ impl<'g, V: Eq + Ord + Hash + Copy> Tarjan<'g, V> {
139144
// Grab owned state to circumvent borrow checker.
140145
let mut successor = self.state[successor_id];
141146
if successor.index == Self::UNVISITED {
142-
// Make sure state is updated before the recursive call.
143-
self.state.insert(node_id, node);
144147
self.strong_connect(*successor_id);
145148
// Grab updated state after recursive call.
146149
successor = self.state[successor_id];
@@ -150,9 +153,9 @@ impl<'g, V: Eq + Ord + Hash + Copy> Tarjan<'g, V> {
150153
}
151154
}
152155

153-
// Update state in case we haven't already. We store this in a hash map
154-
// so we have to run the hashing algorithm every time we update the
155-
// state. Keep it to a minimum :)
156+
// Re-insert the node's state as the state might have changed.
157+
// we need to do some memory gymnastics here to avoid needing a re-insert
158+
// of the state every time we update it.
156159
self.state.insert(node_id, node);
157160

158161
// Root node of a strongly connected component.
@@ -236,6 +239,53 @@ mod tests {
236239
.collect()
237240
}
238241

242+
#[test]
243+
fn find_cycles_names() {
244+
// Define the graph using a key-value type with string literals
245+
let graph_data = [
246+
(
247+
"ProcessNextStepArgs",
248+
vec!["JSONSchemaValue", "Tool", "Message"],
249+
),
250+
("JSONSchemaProperty", vec!["JSONSchemaValue"]),
251+
("Message", vec![]),
252+
("ParameterValue", vec![]),
253+
("Tool", vec!["JSONSchemaValue"]),
254+
("UserCommandQuestion", vec![]),
255+
("Parameter", vec![]),
256+
("UserTool", vec![]),
257+
("ScriptStep", vec!["JSONSchemaValue"]),
258+
("JSONSchemaValue", vec!["JSONSchemaValue"]),
259+
("UserCommandParameter", vec![]),
260+
(
261+
"StepDescriptionRequest",
262+
vec!["JSONSchemaValue", "UserTool", "ScriptStep"],
263+
),
264+
("GetUnstructuredContentArgs", vec![]),
265+
("SummarizedToolExecutionResults", vec!["StructuredResponse"]),
266+
("StructuredResponse", vec![]),
267+
("StepDescriptionResponse", vec![]),
268+
("GetUnstructuredContentResponse", vec![]),
269+
("ToolCall", vec!["ParameterValue"]),
270+
("SummarizeToolExecutionResultsArgs", vec!["Message"]),
271+
(
272+
"UserCommand",
273+
vec!["UserCommandQuestion", "UserCommandParameter"],
274+
),
275+
("ProcessNextStepResponse", vec!["ToolCall"]),
276+
];
277+
278+
// Transform the key-value type into an IndexMap with IndexSet
279+
let graph = HashMap::from_iter(
280+
graph_data
281+
.into_iter()
282+
.map(|(node, successors)| (node, HashSet::from_iter(successors.into_iter()))),
283+
);
284+
285+
let components = Tarjan::components(&graph);
286+
assert_eq!(components, &[&["JSONSchemaValue"]]);
287+
}
288+
239289
#[test]
240290
fn find_cycles() {
241291
let graph = graph(&[

0 commit comments

Comments
 (0)