Fix unclosed style marker crash

machine/corpora/__init__.py

@@ -59,12 +59,8 @@
     normalize,
     unescape_spaces,
 )
-from .update_usfm_parser_handler import (
-    UpdateUsfmMarkerBehavior,
-    UpdateUsfmParserHandler,
-    UpdateUsfmRow,
-    UpdateUsfmTextBehavior,
-)
+from .update_usfm_behavior import UpdateUsfmMarkerBehavior, UpdateUsfmTextBehavior
+from .update_usfm_parser_handler import UpdateUsfmParserHandler, UpdateUsfmRow
 from .usfm_file_text import UsfmFileText
 from .usfm_file_text_corpus import UsfmFileTextCorpus
 from .usfm_memory_text import UsfmMemoryText

machine/corpora/paratext_project_text_updater_base.py

@@ -5,12 +5,8 @@
 from .paratext_project_file_handler import ParatextProjectFileHandler
 from .paratext_project_settings import ParatextProjectSettings
 from .paratext_project_settings_parser_base import ParatextProjectSettingsParserBase
-from .update_usfm_parser_handler import (
-    UpdateUsfmMarkerBehavior,
-    UpdateUsfmParserHandler,
-    UpdateUsfmRow,
-    UpdateUsfmTextBehavior,
-)
+from .update_usfm_behavior import UpdateUsfmMarkerBehavior, UpdateUsfmTextBehavior
+from .update_usfm_parser_handler import UpdateUsfmParserHandler, UpdateUsfmRow
 from .usfm_parser import parse_usfm
 from .usfm_token import UsfmTokenType
 from .usfm_tokenizer import UsfmToken, UsfmTokenizer

machine/corpora/place_markers_usfm_update_block_handler.py

@@ -4,7 +4,7 @@
 
 from ..translation.word_alignment_matrix import WordAlignmentMatrix
 from .segment_boundary_adjuster import SegmentBoundaryAdjuster
-from .update_usfm_parser_handler import UpdateUsfmMarkerBehavior
+from .update_usfm_behavior import UpdateUsfmMarkerBehavior
 from .usfm_token import UsfmToken, UsfmTokenType
 from .usfm_update_block import UsfmUpdateBlock
 from .usfm_update_block_element import UsfmUpdateBlockElement, UsfmUpdateBlockElementType
@@ -39,18 +39,7 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock:
             or alignment_info["alignment"].row_count == 0
             or alignment_info["alignment"].column_count == 0
             or not any(
-                (
-                    (
-                        e.type == UsfmUpdateBlockElementType.PARAGRAPH
-                        and alignment_info["paragraph_behavior"] == UpdateUsfmMarkerBehavior.PRESERVE
-                        and len(e.tokens) == 1
-                    )
-                    or (
-                        e.type == UsfmUpdateBlockElementType.STYLE
-                        and alignment_info["style_behavior"] == UpdateUsfmMarkerBehavior.PRESERVE
-                    )
-                )
-                for e in elements
+                e.is_placeable(alignment_info["paragraph_behavior"], alignment_info["style_behavior"]) for e in elements
             )
         ):
             return block
@@ -74,7 +63,8 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock:
                         elements.pop(i)
             elif not (
                 element.type == UsfmUpdateBlockElementType.EMBED
-                or (element.type == UsfmUpdateBlockElementType.TEXT and len(element.tokens[0].to_usfm().strip()) == 0)
+                or element.type == UsfmUpdateBlockElementType.OTHER
+                or (element.type == UsfmUpdateBlockElementType.TEXT and len(element.get_text().strip()) == 0)
             ):
                 eob_empty_paras = False
 
@@ -92,7 +82,7 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock:
         for element in elements:
             if element.type == UsfmUpdateBlockElementType.TEXT:
                 if element.marked_for_removal:
-                    text = element.tokens[0].to_usfm()
+                    text = element.get_text()
                     src_sent += text
 
                     # Track seen tokens
@@ -103,7 +93,7 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock:
                     if len(text.strip()) > 0:
                         src_tok_idx += 1
                 else:
-                    trg_sent += element.tokens[0].to_usfm()
+                    trg_sent += element.get_text()
 
             if element.marked_for_removal or (
                 element.type == UsfmUpdateBlockElementType.PARAGRAPH
@@ -163,6 +153,10 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock:
         to_insert.sort(key=lambda x: x[0])
         to_insert += [(len(trg_sent), element) for element in embed_elements + end_elements]
 
+        # In the case of unclosed markers, to_insert might be empty
+        if len(to_insert) == 0:
+            return block
+
         # Construct new text tokens to put between markers
         # and reincorporate headers and empty end-of-verse paragraph markers
         if to_insert[0][0] > 0:

machine/corpora/update_usfm_behavior.py

@@ -0,0 +1,12 @@
+from enum import Enum, auto
+
+
+class UpdateUsfmTextBehavior(Enum):
+    PREFER_EXISTING = auto()
+    PREFER_NEW = auto()
+    STRIP_EXISTING = auto()
+
+
+class UpdateUsfmMarkerBehavior(Enum):
+    PRESERVE = auto()
+    STRIP = auto()

machine/corpora/update_usfm_parser_handler.py

@@ -1,9 +1,9 @@
-from enum import Enum, auto
 from typing import Callable, Dict, Iterable, List, Optional, Sequence, Tuple, Union
 
 from ..scripture.verse_ref import IgnoreSegmentsVerseRef, VerseRef, Versification
 from .scripture_ref import ScriptureRef
 from .scripture_ref_usfm_parser_handler_base import ScriptureRefUsfmParserHandlerBase, ScriptureTextType
+from .update_usfm_behavior import UpdateUsfmMarkerBehavior, UpdateUsfmTextBehavior
 from .usfm_parser_state import UsfmParserState
 from .usfm_stylesheet import UsfmStylesheet
 from .usfm_tag import UsfmTextType
@@ -14,17 +14,6 @@
 from .usfm_update_block_handler import UsfmUpdateBlockHandler, UsfmUpdateBlockHandlerError
 
 
-class UpdateUsfmTextBehavior(Enum):
-    PREFER_EXISTING = auto()
-    PREFER_NEW = auto()
-    STRIP_EXISTING = auto()
-
-
-class UpdateUsfmMarkerBehavior(Enum):
-    PRESERVE = auto()
-    STRIP = auto()
-
-
 class _RowInfo:
     def __init__(self, row_index: int):
         self.row_index = row_index
@@ -235,7 +224,11 @@ def start_note(self, state: UsfmParserState, marker: str, caller: str, category:
 
     def end_note(self, state: UsfmParserState, marker: str, closed: bool) -> None:
         if closed:
-            self._collect_updatable_tokens(state)
+            # Mirror start_note: an embed in a duplicate verse is dropped, end marker included.
+            if self._duplicate_verse:
+                self._skip_updatable_tokens(state)
+            else:
+                self._collect_updatable_tokens(state)
 
         super().end_note(state, marker, closed)
 
@@ -264,14 +257,18 @@ def end_char(
         attributes: Sequence[UsfmAttribute],
         closed: bool,
     ) -> None:
-        if self._current_text_type == ScriptureTextType.EMBED:
-            self._collect_updatable_tokens(state)
-        else:
-            self._replace_with_new_tokens(state)
-            if self._style_behavior == UpdateUsfmMarkerBehavior.STRIP:
-                self._skip_updatable_tokens(state)
-            else:
+        # An implicitly closed character style has no end marker of its own, so the token at
+        # state.index belongs to whatever closed it (e.g. the next paragraph marker). Leave it
+        # for the callback that handles it, as end_note and end_sidebar already do.
+        if closed:
+            if self._current_text_type == ScriptureTextType.EMBED:
                 self._collect_updatable_tokens(state)
+            else:
+                self._replace_with_new_tokens(state)
+                if self._style_behavior == UpdateUsfmMarkerBehavior.STRIP:
+                    self._skip_updatable_tokens(state)
+                else:
+                    self._collect_updatable_tokens(state)
 
         super().end_char(state, marker, attributes, closed)
 

machine/corpora/usfm_update_block_element.py

@@ -1,6 +1,7 @@
 from dataclasses import dataclass
 from enum import Enum, auto
 
+from .update_usfm_behavior import UpdateUsfmMarkerBehavior
 from .usfm_token import UsfmToken
 
 
@@ -22,3 +23,17 @@ def get_tokens(self) -> list[UsfmToken]:
         if self.marked_for_removal:
             return []
         return self.tokens.copy()
+
+    def get_text(self) -> str:
+        return "".join(t.to_usfm() for t in self.tokens)
+
+    def is_placeable(
+        self, paragraph_behavior: UpdateUsfmMarkerBehavior, style_behavior: UpdateUsfmMarkerBehavior
+    ) -> bool:
+        if self.marked_for_removal:
+            return False
+        if self.type == UsfmUpdateBlockElementType.PARAGRAPH:
+            return paragraph_behavior == UpdateUsfmMarkerBehavior.PRESERVE and len(self.tokens) == 1
+        if self.type == UsfmUpdateBlockElementType.STYLE:
+            return style_behavior == UpdateUsfmMarkerBehavior.PRESERVE
+        return False

tests/corpora/test_place_markers_usfm_update_block_handler.py

@@ -729,6 +729,176 @@ def test_adjustment_of_placed_paragraph_marker() -> None:
     assert_usfm_equals(target, result)
 
 
+def test_unclosed_style_marker_in_non_verse_paragraph() -> None:
+    # An unclosed character style is closed implicitly by the next paragraph marker, which must
+    # not be pulled into the block being closed (and then dropped as part of the removed style).
+    source = "(A)"
+    pretranslation = "(A translated)"
+    align_info = PlaceMarkersAlignmentInfo(
+        source_tokens=[t for t in TOKENIZER.tokenize(source)],
+        translation_tokens=[t for t in TOKENIZER.tokenize(pretranslation)],
+        alignment=to_word_alignment_matrix("0-0 1-1 2-2"),
+        paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE,
+        style_behavior=UpdateUsfmMarkerBehavior.STRIP,
+    )
+    rows = [
+        UpdateUsfmRow(scr_ref("PSA 119:0/1:d"), str(pretranslation), metadata={"alignment_info": align_info}),
+        UpdateUsfmRow(scr_ref("PSA 119:1"), "New verse 1"),
+    ]
+    usfm = r"""\id PSA
+\c 119
+\d \bd (A)
+\q1
+\v 1 Verse 1
+"""
+
+    target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()])
+    result = r"""\id PSA
+\c 119
+\d (A translated)
+\q1
+\v 1 New verse 1
+"""
+    assert_usfm_equals(target, result)
+
+
+def test_unmatched_end_marker() -> None:
+    # A stray end marker has no matching start marker, so it is marked for removal even when
+    # styles are preserved. It must not be mistaken for a marker that can be placed.
+    source = "Section header"
+    pretranslation = "New section header"
+    align_info = PlaceMarkersAlignmentInfo(
+        source_tokens=[t for t in TOKENIZER.tokenize(source)],
+        translation_tokens=[t for t in TOKENIZER.tokenize(pretranslation)],
+        alignment=to_word_alignment_matrix("0-1 1-2"),
+        paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE,
+        style_behavior=UpdateUsfmMarkerBehavior.PRESERVE,
+    )
+    rows = [
+        UpdateUsfmRow(scr_ref("MAT 1:0/1:s"), str(pretranslation), metadata={"alignment_info": align_info}),
+        UpdateUsfmRow(scr_ref("MAT 1:1"), "New verse 1"),
+    ]
+    usfm = r"""\id MAT
+\c 1
+\s Section header\it*
+\p
+\v 1 Verse 1
+"""
+
+    target = update_usfm(
+        rows,
+        usfm,
+        style_behavior=UpdateUsfmMarkerBehavior.PRESERVE,
+        update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()],
+    )
+    result = r"""\id MAT
+\c 1
+\s New section header
+\p
+\v 1 New verse 1
+"""
+    assert_usfm_equals(target, result)
+
+
+def test_marker_behavior_disagrees_with_alignment_info() -> None:
+    # The behaviors in the alignment info are supplied by the caller and can disagree with the
+    # ones the updater was built with. The markers are already stripped, so there is nothing to
+    # place and the block is left alone.
+    source = "Section header"
+    pretranslation = "New section header"
+    align_info = PlaceMarkersAlignmentInfo(
+        source_tokens=[t for t in TOKENIZER.tokenize(source)],
+        translation_tokens=[t for t in TOKENIZER.tokenize(pretranslation)],
+        alignment=to_word_alignment_matrix("0-1 1-2"),
+        paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE,
+        style_behavior=UpdateUsfmMarkerBehavior.PRESERVE,
+    )
+    rows = [
+        UpdateUsfmRow(scr_ref("MAT 1:0/1:s"), str(pretranslation), metadata={"alignment_info": align_info}),
+        UpdateUsfmRow(scr_ref("MAT 1:1"), "New verse 1"),
+    ]
+    usfm = r"""\id MAT
+\c 1
+\s Section \it header\it*
+\p
+\v 1 Verse 1
+"""
+
+    target = update_usfm(
+        rows,
+        usfm,
+        style_behavior=UpdateUsfmMarkerBehavior.STRIP,
+        update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()],
+    )
+    result = r"""\id MAT
+\c 1
+\s New section header
+\p
+\v 1 New verse 1
+"""
+    assert_usfm_equals(target, result)
+
+
+def test_other_elements_do_not_affect_embed_placement() -> None:
+    # Attributes, milestones and the like are never transferred, so their presence must not
+    # change where anything else lands - here, an embed before end-of-verse paragraph markers.
+    source = "This is the first part. This is the second part."
+    pretranslation = "Esta es la primera parte. Esta es la segunda parte."
+    result = r"""\id MAT
+\c 1
+\v 1 Esta es la primera parte. Esta es la segunda parte. \f + \ft Footnote\f*
+\q1
+\q2
+"""
+    for milestone in ["", r" \ts-s\*"]:
+        align_info = PlaceMarkersAlignmentInfo(
+            source_tokens=[t for t in TOKENIZER.tokenize(source)],
+            translation_tokens=[t for t in TOKENIZER.tokenize(pretranslation)],
+            alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-3 4-4 5-5 6-6 7-7 8-8 9-9 10-10"),
+            paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE,
+            style_behavior=UpdateUsfmMarkerBehavior.STRIP,
+        )
+        rows = [UpdateUsfmRow(scr_ref("MAT 1:1"), str(pretranslation), metadata={"alignment_info": align_info})]
+        usfm = (
+            "\\id MAT\n\\c 1\n"
+            "\\v 1 This is the first part. This is the second part.\\f + \\ft Footnote\\f*\n"
+            f"\\q1{milestone}\n\\q2\n"
+        )
+
+        target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()])
+        assert_usfm_equals(target, result)
+
+
+def test_multiple_text_rows_in_verse_ranges_are_updated() -> None:
+    # Verse ranges contain multiple text rows, which must be processed as if they were a single row
+    source = "This is the first part. This is the second part."
+    pretranslation = "Esta es la primera parte. Esta es la segunda parte."
+    align_info = PlaceMarkersAlignmentInfo(
+        source_tokens=[t for t in TOKENIZER.tokenize(source)],
+        translation_tokens=[t for t in TOKENIZER.tokenize(pretranslation)],
+        alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-3 4-4 5-5 6-6 7-7 8-8 9-9 10-10"),
+        paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE,
+        style_behavior=UpdateUsfmMarkerBehavior.STRIP,
+    )
+    rows = [
+        UpdateUsfmRow(scr_ref("MAT 1:1"), "Esta es la primera parte.", metadata={"alignment_info": align_info}),
+        UpdateUsfmRow(scr_ref("MAT 1:2"), "Esta es la segunda parte.", metadata={"alignment_info": align_info}),
+    ]
+    usfm = r"""\id MAT
+\c 1
+\v 1-2 This is the first part.
+\p This is the second part.
+"""
+
+    target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()])
+    result = r"""\id MAT
+\c 1
+\v 1-2 Esta es la primera parte.
+\p Esta es la segunda parte.
+"""
+    assert_usfm_equals(target, result)
+
+
 def scr_ref(*refs: str) -> List[ScriptureRef]:
     return [ScriptureRef.parse(ref) for ref in refs]