Repository: typst/typst
File: crates/typst-pdf/src/tags/tree/build.rs (Rust)

Code before:
struct StackEntry {
    /// The location of the stack entry. If this is `None` the stack entry has
    /// to be manually popped.
    loc: Option<Location>,
    id: GroupId,
    prog_idx: u32,
}

pub fn build(document: &PagedDocument, options: &PdfOptions) -> SourceResult<Tree> {
    let mut tree = TreeBuilder::new(document, options);
    for page in document.pages.iter() {
        visit_frame(&mut tree, &page.frame)?;
    }

    if let Some(last) = tree.stack.last() {
        assert_internal(tree.stack.is_empty(), "tags weren't properly closed")
            .at(tree.groups.get(last.id).span)?;
    }
    assert_internal(tree.unfinished_stacks.is_empty(), "tags weren't properly closed")
        .at(Span::detached())?;
    assert_internal(
        Some(&GroupId::ROOT) == tree.progressions.last(),
        "tags weren't properly closed",
    )
    .at(Span::detached())?;

    // Insert logical children into the tree.
    #[allow(clippy::iter_over_hash_type)]
    for (loc, children) in tree.logical_children.iter() {
        let located = (tree.groups.by_loc(loc))
            .expect_internal("parent group")
            .at(Span::detached())?;

        if options.is_pdf_ua() && located.multiple_parents {
            let validator = options.standards.config.validator().as_str();
            let group = tree.groups.get(located.id);
            bail!(
                group.span,
                "{validator} error: ambiguous logical parent";
                hint: "please report this as a bug"
            );
        }

        for child in children.iter() {
            let child = tree.groups.get_mut(*child);

            let GroupKind::LogicalChild(inherit, logical_parent) = &mut child.kind else {
                unreachable!()
            };
            *logical_parent = located.id;


Review comment (suggestion):
I only just noticed the very small differences here.

1. The `tree.groups.get(last.id).span` should be fine since even if that somehow panics for a malformed ID, we'd have panicked anyway because the stack can't be empty if `tree.stack.last() is Some(last)`. In fact `assert_internal` could actually just be a new `panic_internal` here?

2. `tree.progressions.first()` became `Some(&GroupId::ROOT)`. Is that 100% always the same? Otherwise, I'd prefer to not touch it for this PR as it seems slightly unrelated and I want to really keep diffs as minimal as possible for PRs that will be backported. If it's just a nice clean-up, it could also easily be changed in a separate PR.

Code after:
#[derive(Debug, Copy, Clone)]
struct StackEntry {
    /// The location of the stack entry. If this is `None` the stack entry has
    /// to be manually popped.
    loc: Option<Location>,
    id: GroupId,
    prog_idx: u32,
}

pub fn build(document: &PagedDocument, options: &PdfOptions) -> SourceResult<Tree> {
    let mut tree = TreeBuilder::new(document, options);
    for page in document.pages.iter() {
        visit_frame(&mut tree, &page.frame)?;
    }

    if let Some(last) = tree.stack.last() {
        panic_internal("tags weren't properly closed")
            .at(tree.groups.get(last.id).span)?;
    }
    assert_internal(tree.unfinished_stacks.is_empty(), "tags weren't properly closed")
        .at(Span::detached())?;
    assert_internal(
        tree.progressions.first() == tree.progressions.last(),
        "tags weren't properly closed",
    )
    .at(Span::detached())?;

    // Insert logical children into the tree.
    #[allow(clippy::iter_over_hash_type)]
    for (loc, children) in tree.logical_children.iter() {
        let located = (tree.groups.by_loc(loc))
            .expect_internal("parent group")
            .at(Span::detached())?;

        if options.is_pdf_ua() && located.multiple_parents {
            let validator = options.standards.config.validator().as_str();
            let group = tree.groups.get(located.id);
            bail!(
                group.span,
                "{validator} error: ambiguous logical parent";
                hint: "please report this as a bug"
            );
        }

        for child in children.iter() {
            let child = tree.groups.get_mut(*child);

            let GroupKind::LogicalChild(inherit, logical_parent) = &mut child.kind else {
                unreachable!()
            };
            *logical_parent = located.id;