tamer: ld::xmle::lower: Use asg::graph::visit::topo::topo_sort

This integrates the new topological sort, replacing the previous
implementation in the linker.

This will now allow encapsulating the graph, finally, and ensures that
future changes can be fully maintained within the `asg` module.

More cleanup will come over time.

DEV-13162

tamer/src/ld/xmle/lower.rs

@@ -21,22 +21,15 @@
 //!
 //! See the [parent module](super) for more information.
 
-use std::iter::once;
-
 use super::section::{SectionsError, XmleSections};
 use crate::{
-    asg::{Asg, Ident, IdentKind, Object, ObjectIndex},
+    asg::{visit::topo_sort, Asg, AsgError, Ident, Object},
     diagnose::{Annotate, Diagnostic},
     diagnostic_unreachable,
-    fmt::{
-        AndConjList, DisplayWrapper, JoinListWrap, ListDisplayWrapper, Raw,
-        TtQuote,
-    },
     parse::util::SPair,
     span::UNKNOWN_SPAN,
     sym::{st, GlobalSymbolResolve, SymbolId},
 };
-use petgraph::visit::DfsPostOrder;
 
 // Result of [`sort`].
 pub type SortResult<T> = Result<T, SortError>;
@@ -50,23 +43,16 @@ pub fn sort<'a, S: XmleSections<'a>>(asg: &'a Asg, mut dest: S) -> SortResult<S>
 where
     S: XmleSections<'a>,
 {
-    // TODO: we should check for cycles as we sort (as the POC did).
-    check_cycles(asg)?;
-
-    // TODO: Encapsulate petgraph.
-    // This is technically a topological sort, but functions have cycles.
-    let mut dfs = DfsPostOrder::new(&asg.graph, asg.root(UNKNOWN_SPAN).into());
-
     // These are always generated by the map compiler,
     //   but do not have edges that would allow them to be properly ordered
     //   (adding an edge to every map object would be wasteful).
     dest.push(get_ident(asg, st::L_MAP_UUUHEAD))?;
     dest.push(get_ident(asg, st::L_RETMAP_UUUHEAD))?;
 
-    while let Some(index) = dfs.next(&asg.graph) {
-        // TODO: `new` really ought to be private to the `asg` module,
-        //   so let's work on fully encapsulating Petgraph.
-        let oi = ObjectIndex::<Object>::new(index, UNKNOWN_SPAN);
+    let roots = [asg.root(UNKNOWN_SPAN).widen()].into_iter();
+
+    for result in topo_sort(asg, roots) {
+        let oi = result.map_err(AsgError::UnsupportedCycle)?;
 
         match asg.get(oi).expect("missing object") {
             Object::Root(_) => (),
@@ -109,70 +95,6 @@ where
         })
 }
 
-/// Check graph for cycles
-///
-/// We want to catch any cycles before we start using the graph.
-///   Unfortunately, we need to allow cycles for our [`IdentKind::Func`]
-///   so we cannot use the typical algorithms in a straightforward manner.
-///
-/// We loop through all SCCs and check that they are not all functions. If
-///   they are, we ignore the cycle, otherwise we will return an error.
-fn check_cycles(asg: &Asg) -> SortResult<()> {
-    // While `tarjan_scc` does do a topological sort, it does not suit our
-    // needs because we need to filter out some allowed cycles. It would
-    // still be possible to use this, but we also need to only check nodes
-    // that are attached to our "roots". We are doing our own sort and as of
-    // the initial writing, this does not have a significant performance
-    // impact.
-    let sccs = petgraph::algo::tarjan_scc(&asg.graph);
-
-    let cycles: Vec<Vec<SPair>> = sccs
-        .into_iter()
-        .filter_map(|scc| {
-            // For single-node SCCs, we just need to make sure they are
-            // not neighbors with themselves.
-            if scc.len() == 1
-                && !asg.graph.neighbors(scc[0]).any(|nx| nx == scc[0])
-            {
-                return None;
-            }
-
-            let is_all_funcs = scc.iter().all(|ni| {
-                let oi = ObjectIndex::<Object>::new(*ni, UNKNOWN_SPAN);
-                let ident = asg.get(oi).expect("missing node");
-                matches!(
-                    ident.as_ident_ref().and_then(Ident::kind),
-                    Some(IdentKind::Func(..))
-                )
-            });
-
-            if is_all_funcs {
-                None
-            } else {
-                // TODO: ...these aren't references, they're the actual
-                //   identifiers,
-                //     so the diagnostic message isn't that great of a guide
-                //     yet!
-                //   Use reference spans once they're available.
-                let cycles = scc
-                    .iter()
-                    .map(|ni| ObjectIndex::<Object>::new(*ni, UNKNOWN_SPAN))
-                    .filter_map(|oi| {
-                        asg.get(oi).unwrap().as_ident_ref().map(Ident::name)
-                    })
-                    .collect();
-                Some(cycles)
-            }
-        })
-        .collect();
-
-    if cycles.is_empty() {
-        Ok(())
-    } else {
-        Err(SortError::Cycles(cycles))
-    }
-}
-
 /// Error during graph sorting.
 ///
 /// These errors reflect barriers to meaningfully understanding the
@@ -185,7 +107,7 @@ pub enum SortError {
     SectionsError(SectionsError),
 
     /// The graph has a cyclic dependency.
-    Cycles(Vec<Vec<SPair>>),
+    AsgError(AsgError),
 }
 
 impl From<SectionsError> for SortError {
@@ -194,32 +116,17 @@ impl From<SectionsError> for SortError {
     }
 }
 
+impl From<AsgError> for SortError {
+    fn from(value: AsgError) -> Self {
+        Self::AsgError(value)
+    }
+}
+
 impl std::fmt::Display for SortError {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
         match self {
-            Self::SectionsError(err) => err.fmt(f),
-            Self::Cycles(cycles) => {
-                let cycles_fmt = cycles
-                    .iter()
-                    .map(|cycle| {
-                        let cycle_fmt = cycle
-                            .iter()
-                            .rev()
-                            .chain(once(cycle.last().unwrap()))
-                            .collect::<Vec<_>>();
-
-                        // TODO: Wrappers ought to support nested lists.
-                        JoinListWrap::<" -> ", Raw>::wrap(&cycle_fmt)
-                            .to_string()
-                    })
-                    .collect::<Vec<_>>();
-
-                write!(
-                    f,
-                    "circular dependencies: {}",
-                    AndConjList::<TtQuote>::wrap(&cycles_fmt),
-                )
-            }
+            Self::SectionsError(e) => e.fmt(f),
+            Self::AsgError(e) => e.fmt(f),
         }
     }
 }
@@ -236,50 +143,7 @@ impl Diagnostic for SortError {
 
         match self {
             SectionsError(e) => e.describe(),
-
-            // TODO: In future it'd be far less confusing to have a single
-            //   error per cycle.
-            Cycles(cycles) => cycles
-                .iter()
-                .flat_map(|cycle| {
-                    let n = cycle.len();
-                    let ident = cycle.last().unwrap();
-
-                    cycle.iter().rev().enumerate().map(|(i, spair)| {
-                        spair.note(match i {
-                            0 => format!(
-                                "[0/{n}] the cycle begins here, depending on..."
-                            ),
-                            _ => format!(
-                                "[{i}/{n}] ...this identifier, which depends on..."
-                            )
-                        })
-                    })
-                        .chain(once(ident.error(format!(
-                            "[{n}/{n}] ...the first identifier once again, \
-                                creating the cycle"
-                        ))))
-                        .chain(vec![
-                            ident.help(format!(
-                                "the value of {} cannot be computed because its",
-                                TtQuote::wrap(ident),
-                            )),
-                            ident.help(
-                                "  definition requires first computing itself.",
-                            ),
-                            ident.help(
-                                "in the future the above output will emphasize the "
-                            ),
-                            ident.help(
-                                "  references to the identifiers rather than their "
-                            ),
-                            ident.help(
-                                "  definition sites."
-                            )
-                        ])
-                        .collect::<Vec<_>>()
-                })
-                .collect(),
+            AsgError(e) => e.describe(),
         }
     }
 }

tamer/src/ld/xmle/lower/test.rs

@@ -19,9 +19,8 @@
 
 use super::*;
 use crate::{
-    asg::{AsgError, FragmentText, Ident, Source},
+    asg::{AsgError, FragmentText, Ident, IdentKind, ObjectIndex, Source},
     ld::xmle::{section::PushResult, Sections},
-    num::{Dim, Dtype},
     parse::util::SPair,
     span::dummy::*,
     sym::GlobalSymbolIntern,
@@ -275,538 +274,9 @@ fn graph_sort_simple_cycle() -> SortResult<()> {
 
     let result = sort(&asg, Sections::new());
 
-    let expected = vec![[oi_dep, oi_sym]
-        .into_iter()
-        .map(|o| asg.get(o).unwrap().name())
-        .collect::<Vec<_>>()];
-
     match result {
         Ok(_) => panic!("sort did not detect cycle"),
-        Err(SortError::Cycles(scc)) => assert_eq!(expected, scc),
-        Err(e) => panic!("unexpected error: {}", e),
-    }
-
-    Ok(())
-}
-
-#[test]
-fn graph_sort_two_simple_cycles() -> SortResult<()> {
-    let mut asg = make_asg();
-
-    let sym = SPair("sym".into(), S1);
-    let sym2 = SPair("sym2".into(), S2);
-    let dep = SPair("dep".into(), S3);
-    let dep2 = SPair("dep2".into(), S4);
-
-    let oi_sym = declare(
-        &mut asg,
-        sym,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("foo"))
-    .unwrap();
-
-    let oi_sym2 = declare(
-        &mut asg,
-        sym2,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("bar"))
-    .unwrap();
-
-    let oi_dep = declare(
-        &mut asg,
-        dep,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("baz"))
-    .unwrap();
-
-    let oi_dep2 = declare(
-        &mut asg,
-        dep2,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("huh"))
-    .unwrap();
-
-    oi_sym.add_opaque_dep(&mut asg, oi_dep);
-    oi_dep.add_opaque_dep(&mut asg, oi_sym);
-    oi_sym2.add_opaque_dep(&mut asg, oi_dep2);
-    oi_dep2.add_opaque_dep(&mut asg, oi_sym2);
-
-    oi_sym.root(&mut asg);
-
-    let result = sort(&asg, Sections::new());
-
-    let expected = [[oi_dep, oi_sym], [oi_dep2, oi_sym2]]
-        .into_iter()
-        .map(|cycle| {
-            cycle
-                .into_iter()
-                .map(|o| asg.get(o).unwrap().name())
-                .collect::<Vec<_>>()
-        })
-        .collect::<Vec<_>>();
-
-    match result {
-        Ok(_) => panic!("sort did not detect cycle"),
-        Err(SortError::Cycles(scc)) => assert_eq!(expected, scc),
-        Err(e) => panic!("unexpected error: {}", e),
-    }
-
-    Ok(())
-}
-
-#[test]
-fn graph_sort_no_cycle_with_edge_to_same_node() -> SortResult<()> {
-    let mut asg = make_asg();
-
-    let sym = SPair("sym".into(), S1);
-    let dep = SPair("dep".into(), S2);
-
-    let sym_node = declare(
-        &mut asg,
-        sym,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("foo"))
-    .unwrap();
-
-    let dep_node = declare(
-        &mut asg,
-        dep,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("bar"))
-    .unwrap();
-
-    sym_node.add_opaque_dep(&mut asg, dep_node);
-    sym_node.add_opaque_dep(&mut asg, dep_node);
-
-    sym_node.root(&mut asg);
-
-    let result = sort(&asg, Sections::new());
-
-    match result {
-        Ok(_) => (),
-        Err(e) => panic!("unexpected error: {}", e),
-    }
-
-    Ok(())
-}
-
-#[test]
-fn graph_sort_cycle_with_a_few_steps() -> SortResult<()> {
-    let mut asg = make_asg();
-
-    let sym1 = SPair("sym1".into(), S1);
-    let sym2 = SPair("sym2".into(), S2);
-    let sym3 = SPair("sym3".into(), S3);
-
-    let sym1_node = declare(
-        &mut asg,
-        sym1,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("foo"))
-    .unwrap();
-
-    let sym2_node = declare(
-        &mut asg,
-        sym2,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("bar"))
-    .unwrap();
-
-    let sym3_node = declare(
-        &mut asg,
-        sym3,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("baz"))
-    .unwrap();
-
-    sym1_node.add_opaque_dep(&mut asg, sym2_node);
-    sym2_node.add_opaque_dep(&mut asg, sym3_node);
-    sym3_node.add_opaque_dep(&mut asg, sym1_node);
-
-    sym1_node.root(&mut asg);
-
-    let result = sort(&asg, Sections::new());
-
-    let expected = vec![[sym3_node, sym2_node, sym1_node]
-        .into_iter()
-        .map(|o| asg.get(o).unwrap().name())
-        .collect::<Vec<_>>()];
-
-    match result {
-        Ok(_) => panic!("sort did not detect cycle"),
-        Err(SortError::Cycles(scc)) => assert_eq!(expected, scc),
-        Err(e) => panic!("unexpected error: {}", e),
-    }
-
-    Ok(())
-}
-
-#[test]
-fn graph_sort_cyclic_function_with_non_function_with_a_few_steps(
-) -> SortResult<()> {
-    let mut asg = make_asg();
-
-    let sym1 = SPair("sym1".into(), S1);
-    let sym2 = SPair("sym2".into(), S2);
-    let sym3 = SPair("sym3".into(), S3);
-
-    let sym1_node = declare(
-        &mut asg,
-        sym1,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("foo"))
-    .unwrap();
-
-    let sym2_node = declare(
-        &mut asg,
-        sym2,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("bar"))
-    .unwrap();
-
-    let sym3_node = declare(
-        &mut asg,
-        sym3,
-        IdentKind::Func(Dim::Scalar, Dtype::Empty),
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("baz"))
-    .unwrap();
-
-    sym1_node.add_opaque_dep(&mut asg, sym2_node);
-    sym2_node.add_opaque_dep(&mut asg, sym3_node);
-    sym3_node.add_opaque_dep(&mut asg, sym1_node);
-
-    sym1_node.root(&mut asg);
-
-    let result = sort(&asg, Sections::new());
-
-    let expected = vec![[sym3_node, sym2_node, sym1_node]
-        .into_iter()
-        .map(|o| asg.get(o).unwrap().name())
-        .collect::<Vec<_>>()];
-
-    match result {
-        Ok(_) => panic!("sort did not detect cycle"),
-        Err(SortError::Cycles(scc)) => assert_eq!(expected, scc),
-        Err(e) => panic!("unexpected error: {}", e),
-    }
-
-    Ok(())
-}
-
-#[test]
-fn graph_sort_cyclic_bookended_by_functions() -> SortResult<()> {
-    let mut asg = make_asg();
-
-    let sym1 = SPair("sym1".into(), S1);
-    let sym2 = SPair("sym2".into(), S2);
-    let sym3 = SPair("sym3".into(), S3);
-
-    let sym1_node = declare(
-        &mut asg,
-        sym1,
-        IdentKind::Func(Dim::Scalar, Dtype::Empty),
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("foo"))
-    .unwrap();
-
-    let sym2_node = declare(
-        &mut asg,
-        sym2,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("bar"))
-    .unwrap();
-
-    let sym3_node = declare(
-        &mut asg,
-        sym3,
-        IdentKind::Func(Dim::Scalar, Dtype::Empty),
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("baz"))
-    .unwrap();
-
-    sym1_node.add_opaque_dep(&mut asg, sym2_node);
-    sym2_node.add_opaque_dep(&mut asg, sym3_node);
-    sym3_node.add_opaque_dep(&mut asg, sym1_node);
-
-    sym1_node.root(&mut asg);
-
-    let result = sort(&asg, Sections::new());
-
-    let expected = vec![[sym3_node, sym2_node, sym1_node]
-        .into_iter()
-        .map(|o| asg.get(o).unwrap().name())
-        .collect::<Vec<_>>()];
-
-    match result {
-        Ok(_) => panic!("sort did not detect cycle"),
-        Err(SortError::Cycles(scc)) => assert_eq!(expected, scc),
-        Err(e) => panic!("unexpected error: {}", e),
-    }
-
-    Ok(())
-}
-
-#[test]
-fn graph_sort_cyclic_function_ignored() -> SortResult<()> {
-    let mut asg = make_asg();
-
-    let sym = SPair("sym".into(), S1);
-    let dep = SPair("dep".into(), S2);
-
-    let sym_node = declare(
-        &mut asg,
-        sym,
-        IdentKind::Func(Dim::Scalar, Dtype::Empty),
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("foo"))
-    .unwrap();
-
-    let dep_node = declare(
-        &mut asg,
-        dep,
-        IdentKind::Func(Dim::Scalar, Dtype::Empty),
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("bar"))
-    .unwrap();
-
-    sym_node.add_opaque_dep(&mut asg, dep_node);
-    dep_node.add_opaque_dep(&mut asg, sym_node);
-
-    sym_node.root(&mut asg);
-
-    let result = sort(&asg, Sections::new());
-
-    match result {
-        Ok(_) => (),
-        Err(e) => panic!("unexpected error: {}", e),
-    }
-
-    Ok(())
-}
-
-#[test]
-fn graph_sort_cyclic_function_is_bookended() -> SortResult<()> {
-    let mut asg = make_asg();
-
-    let sym1 = SPair("sym1".into(), S1);
-    let sym2 = SPair("sym2".into(), S2);
-    let sym3 = SPair("sym3".into(), S3);
-
-    let sym1_node = declare(
-        &mut asg,
-        sym1,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("foo"))
-    .unwrap();
-
-    let sym2_node = declare(
-        &mut asg,
-        sym2,
-        IdentKind::Func(Dim::Scalar, Dtype::Empty),
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("bar"))
-    .unwrap();
-
-    let sym3_node = declare(
-        &mut asg,
-        sym3,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("baz"))
-    .unwrap();
-
-    sym1_node.add_opaque_dep(&mut asg, sym2_node);
-    sym2_node.add_opaque_dep(&mut asg, sym3_node);
-    sym3_node.add_opaque_dep(&mut asg, sym1_node);
-
-    sym1_node.root(&mut asg);
-
-    let result = sort(&asg, Sections::new());
-
-    let expected = vec![[sym3_node, sym2_node, sym1_node]
-        .into_iter()
-        .map(|o| asg.get(o).unwrap().name())
-        .collect::<Vec<_>>()];
-
-    match result {
-        Ok(_) => panic!("sort did not detect cycle"),
-        Err(SortError::Cycles(scc)) => assert_eq!(expected, scc),
-        Err(e) => panic!("unexpected error: {}", e),
-    }
-
-    Ok(())
-}
-
-#[test]
-fn graph_sort_ignore_non_linked() -> SortResult<()> {
-    let mut asg = make_asg();
-
-    let sym = SPair("sym".into(), S1);
-    let dep = SPair("dep".into(), S2);
-    let ignored = SPair("ignored".into(), S3);
-
-    let sym_node = declare(
-        &mut asg,
-        sym,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("foo"))
-    .unwrap();
-
-    let dep_node = declare(
-        &mut asg,
-        dep,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("bar"))
-    .unwrap();
-
-    let ignored_node = declare(
-        &mut asg,
-        ignored,
-        IdentKind::Tpl,
-        Source {
-            virtual_: true,
-            ..Default::default()
-        },
-    )
-    .unwrap()
-    .set_fragment(&mut asg, FragmentText::from("baz"))
-    .unwrap();
-
-    sym_node.add_opaque_dep(&mut asg, dep_node);
-    ignored_node.add_opaque_dep(&mut asg, sym_node);
-
-    sym_node.root(&mut asg);
-
-    let result = sort(&asg, Sections::new());
-
-    match result {
-        Ok(_) => (),
+        Err(SortError::AsgError(AsgError::UnsupportedCycle(_))) => (),
         Err(e) => panic!("unexpected error: {}", e),
     }