diff --git a/tdom/parser.py b/tdom/parser.py index 2eec36d8..92fab52d 100644 --- a/tdom/parser.py +++ b/tdom/parser.py @@ -4,7 +4,12 @@ from string.templatelib import Interpolation, Template from .htmlspec import VOID_ELEMENTS +from .parser_utils import HTMLAttribute from .placeholders import PlaceholderConfig, PlaceholderState +from .source import ( + FrozenPosition, + TagSourceInfo, +) from .template_utils import TemplateRef, combine_template_refs from .tnodes import ( TAttribute, @@ -21,19 +26,47 @@ TText, ) -type HTMLAttribute = tuple[str, str | None] -type HTMLAttributesDict = dict[str, str | None] + +@dataclass(frozen=True, slots=True) +class OpenTagSourceInfo: + """ + Retained tag information from the parsed source. + + @NOTE: These properties DEPEND on the placeholder configuration because + they can contain embedded placeholders. + """ + + starttag_text: str + " Entire starttag as parsed, includes placeholders, . " + raw_attrs: tuple[HTMLAttribute, ...] + " Attrs as parsed, includes placeholders. " + startend: bool + " Was parsed as startend tag, ie. . " + starttag_pos: FrozenPosition + " Position of the parser when the element starttag was parsed. " + + def close(self, endtag_pos: FrozenPosition | None = None) -> TagSourceInfo: + return TagSourceInfo( + starttag_text=self.starttag_text, + raw_attrs=self.raw_attrs, + startend=self.startend, + starttag_pos=self.starttag_pos, + endtag_pos=endtag_pos, + ) @dataclass class OpenTElement: tag: str attrs: tuple[TAttribute, ...] + parser_pos: FrozenPosition + sinfo: OpenTagSourceInfo children: list[TNode] = field(default_factory=list) @dataclass class OpenTFragment: + parser_pos: FrozenPosition | None = None children: list[TNode] = field(default_factory=list) @@ -45,10 +78,12 @@ class OpenTComponent: offset_into_children_start_s: int """The offset INTO the starting string where the component's children template starts.""" attrs: tuple[TAttribute, ...] + parser_pos: FrozenPosition + sinfo: OpenTagSourceInfo # @NOTE: The `children` are discarded after parsing and are just used to - # track template consistency. If the component is processed and - # returns its children template then that template will be - # re-parsed (or pulled from the cache). + # track template consistency or assist with error reporting. If the + # component is processed and returns its children template then that + # template will be re-parsed (or pulled from the cache). children: list[TNode] = field(default_factory=list) @@ -72,6 +107,11 @@ class SourceTracker: def interpolations(self) -> tuple[Interpolation, ...]: return self.template.interpolations + def values_match(self, i_index1: int, i_index2: int) -> bool: + return ( + self.interpolations[i_index1].value == self.interpolations[i_index2].value + ) + def advance_interpolation(self) -> int: """Call before processing an interpolation to move to the next one.""" self.i_index += 1 @@ -95,12 +135,20 @@ def format_starttag(self, i_index: int) -> str: """Format a component start tag for error messages.""" return self.get_expression(i_index, fallback_prefix="component-starttag") + def format_endtag(self, i_index: int) -> str: + return self.get_expression(i_index, fallback_prefix="component-endtag") + class TemplateParser(HTMLParser): root: OpenTFragment stack: list[OpenTag] placeholders: PlaceholderState source: SourceTracker | None + " Map from completed tnodes back to their opentag for error reporting. " + tcomponent_children: dict[TComponent, list[TNode]] + "List of children for each finished tcomponent, stored at closing. " + sinfo_table: dict[FrozenPosition, TagSourceInfo] + " Tags with more source info than just a position are tracked in this mapping. " def __init__(self, *, convert_charrefs: bool = True): # This calls HTMLParser.reset() which we override to set up our state. @@ -118,6 +166,18 @@ def append_child(self, child: TNode) -> None: parent = self.get_parent() parent.children.append(child) + def get_parser_pos(self) -> FrozenPosition: + """ + Get the current position of the parser. + + @NOTE: This position is relative to text embedded with placeholders but + can be translated back to the position within the original template. + Since it *IS* relative to placeholders, ie. "SLOTS", this position is + unique across a "family" of templates with the same structure. + """ + line, offset = self.getpos() + return FrozenPosition(line=line, offset=offset) + # ------------------------------------------ # Attribute Helpers # ------------------------------------------ @@ -159,12 +219,25 @@ def make_tattrs(self, attrs: Sequence[HTMLAttribute]) -> tuple[TAttribute, ...]: # Tag Helpers # ------------------------------------------ - def make_open_tag(self, tag: str, attrs: Sequence[HTMLAttribute]) -> OpenTag: + def make_open_tag( + self, tag: str, attrs: Sequence[HTMLAttribute], startend: bool = False + ) -> OpenTag: """Build an OpenTag from a raw tag and attribute tuples.""" tag_ref = self.placeholders.remove_placeholders(tag) - if tag_ref.is_literal: - return OpenTElement(tag=tag, attrs=self.make_tattrs(attrs)) + parser_pos = self.get_parser_pos() + open_tag = OpenTElement( + tag=tag, + attrs=self.make_tattrs(attrs), + sinfo=OpenTagSourceInfo( + starttag_text=self.get_starttag_text(), + raw_attrs=tuple(attrs), + startend=startend, + starttag_pos=parser_pos, + ), + parser_pos=parser_pos, + ) + return open_tag if not tag_ref.is_singleton: raise ValueError( @@ -189,11 +262,9 @@ def make_open_tag(self, tag: str, attrs: Sequence[HTMLAttribute]) -> OpenTag: # @NOTE: This must be called when the tag is handled since it is # populated based on the most recently finished start tag. Otherwise # the value will be out of sync. - starttag_text = self.get_starttag_text() - if starttag_text is None: - raise AssertionError( - f"Expected startag_text to be set when parsing component at {i_index}." - ) + starttag_text = self.get_starttag_text( + f"Expected startag_text to be set when parsing component at {i_index}." + ) tattrs = self.make_tattrs(attrs) @@ -204,12 +275,21 @@ def make_open_tag(self, tag: str, attrs: Sequence[HTMLAttribute]) -> OpenTag: starttag_text=starttag_text, ) - return OpenTComponent( + parser_pos = self.get_parser_pos() + open_tag = OpenTComponent( start_i_index=i_index, children_start_s_index=children_start_s_index, offset_into_children_start_s=offset_into_children_start_s, attrs=tattrs, + parser_pos=parser_pos, + sinfo=OpenTagSourceInfo( + starttag_text=starttag_text, + raw_attrs=tuple(attrs), + startend=startend, + starttag_pos=parser_pos, + ), ) + return open_tag def compute_offset_into_children_start_s( self, @@ -258,33 +338,56 @@ def compute_offset_into_children_start_s( return len(tag_ref.strings[-1]) def finalize_tag( - self, open_tag: OpenTag, endtag_i_index: int | None = None + self, + open_tag: OpenTag, + endtag_i_index: int | None = None, + endtag_pos: FrozenPosition | None = None, ) -> TNode: """Finalize an OpenTag into a TNode.""" + source = self.get_source() match open_tag: - case OpenTElement(tag=tag, attrs=attrs, children=children): - return TElement(tag=tag, attrs=attrs, children=tuple(children)) - case OpenTFragment(children=children): - return TFragment(children=tuple(children)) + case OpenTElement( + tag=tag, + attrs=attrs, + children=children, + parser_pos=parser_pos, + sinfo=sinfo, + ): + tnode = TElement( + tag=tag, + attrs=attrs, + children=tuple(children), + parser_pos=parser_pos, + ) + self.sinfo_table[parser_pos] = sinfo.close(endtag_pos=endtag_pos) + case OpenTFragment(children=children, parser_pos=parser_pos): + tnode = TFragment(children=tuple(children), parser_pos=parser_pos) case OpenTComponent( start_i_index=start_i_index, children_start_s_index=children_start_s_index, offset_into_children_start_s=offset_into_children_start_s, attrs=attrs, + parser_pos=parser_pos, + sinfo=sinfo, + children=children, ): children_ref = self.extract_component_children_ref( start_i_index=start_i_index, endtag_i_index=endtag_i_index, children_start_s_index=children_start_s_index, offset_into_children_start_s=offset_into_children_start_s, - template=self.get_source().template, + template=source.template, ) - return TComponent( + tnode = TComponent( start_i_index=start_i_index, end_i_index=endtag_i_index, children_ref=children_ref, attrs=attrs, + parser_pos=parser_pos, ) + self.sinfo_table[parser_pos] = sinfo.close(endtag_pos=endtag_pos) + self.tcomponent_children[tnode] = children + return tnode def extract_component_children_ref( self, @@ -339,7 +442,7 @@ def extract_component_children_ref( def validate_end_tag(self, tag: str, open_tag: OpenTag) -> int | None: """Validate that closing tag matches open tag. Return component end index if applicable.""" - assert self.source, "Parser source tracker not initialized." + source = self.get_source() tag_ref = self.placeholders.remove_placeholders(tag) match open_tag: @@ -359,18 +462,61 @@ def validate_end_tag(self, tag: str, open_tag: OpenTag) -> int | None: case OpenTComponent(start_i_index=start_i_index): if tag_ref.is_literal: - raise ValueError( - f"Mismatched closing tag for component starting at {self.source.format_starttag(start_i_index)}." + starttag = source.format_starttag(start_i_index) + e = ValueError( + f"Mismatched closing tag for component with tag {{{starttag}}}." ) + if self.has_ambiguous_forward_slash(open_tag.sinfo): + e.add_note( + f'Did you mean to quote the last attribute or put a space before "/>" for "<{{{starttag}}} .../>"?' + ) + raise e if not tag_ref.is_singleton: raise ValueError( "Component end tags must have exactly one interpolation." ) - # HERE BE DRAGONS: the interpolation at end_i_index shuld be a - # component callable that matches the start tag. We do not check - # any of this in the parser, instead relying on higher layers. return tag_ref.i_indexes[0] + def get_starttag_text(self, msg: str = "Expecting starttag text to be set.") -> str: + """ + Wrap get_starttag_text and just raise if None is returned. + + Do this so we don't guard for `None` everywhere. + """ + starttag_text = super().get_starttag_text() + if starttag_text is None: + raise AssertionError(msg) + return starttag_text + + def has_ambiguous_forward_slash( + self, sinfo: OpenTagSourceInfo | TagSourceInfo | None + ) -> bool: + """ + Detect when an unquoted attribute value consumes a trailing "/" that + *might* have been meant to attempt to self-close a tag, ie. "/>". + + This can come up with literal values or values with interpolations. + + Such as "
" or "<{Component} title=test/>". + + Or more often "<{Component} title={title}/>" which should be corrected + with "<{Component} title={title} />". + """ + if sinfo is not None: + return ( + # has attributes + len(sinfo.raw_attrs) > 0 + # last attr not bare attribute + and sinfo.raw_attrs[-1][1] is not None + # last char of last attr is "/" + and sinfo.raw_attrs[-1][1][-1] == "/" + # parsed starttag ends with "/>" + and sinfo.starttag_text.endswith("/>") + # if parsed as startend then its not ambiguous + and not sinfo.startend + ) + return False + # ------------------------------------------ # HTMLParser tag callbacks # ------------------------------------------ @@ -385,19 +531,60 @@ def handle_starttag(self, tag: str, attrs: Sequence[HTMLAttribute]) -> None: def handle_startendtag(self, tag: str, attrs: Sequence[HTMLAttribute]) -> None: """Dispatch a self-closing tag, `` to specialized handlers.""" - open_tag = self.make_open_tag(tag, attrs) + open_tag = self.make_open_tag(tag, attrs, startend=True) final_tag = self.finalize_tag(open_tag) self.append_child(final_tag) def handle_endtag(self, tag: str) -> None: if not self.stack: - raise ValueError(f"Unexpected closing tag with no open tag.") - + tag_ref = self.placeholders.copy().remove_placeholders(tag) + if tag_ref.is_literal: + raise ValueError(f"Unexpected closing tag with no open tag.") + if not tag_ref.is_singleton: + # @TODO: Also it doesn't match anything + raise ValueError( + "Component end tags must have exactly one interpolation." + ) + # Component tag endtag but no component tag is open... + unmatched_endtag = self.get_source().format_endtag(tag_ref.i_indexes[0]) + raise ValueError( + f"Unexpected closing component tag with no open tag." + ) open_tag = self.stack.pop() endtag_i_index = self.validate_end_tag(tag, open_tag) - final_tag = self.finalize_tag(open_tag, endtag_i_index) + final_tag = self.finalize_tag( + open_tag, endtag_i_index=endtag_i_index, endtag_pos=self.get_parser_pos() + ) self.append_child(final_tag) + def get_closed_tcomps( + self, root: OpenTag | None, recurse_component_children: bool = False + ) -> list[TComponent]: + """ + Get TComponents that were closed during parsing starting from `root`. + + If `root` is None then use the parser's default `root`. + + TComponents should be returned in the order they were closed in: + from first closed to last closed. + + @NOTE: That the root is an `OpenTag` but its `children` are actually `TNode`s. + """ + if root is None: + root = self.root + tcomps = [] + nodes = list(root.children) + while nodes: + node = nodes.pop() + if isinstance(node, TComponent): + tcomps.append(node) + if recurse_component_children: + children = self.tcomponent_children.get(node, []) + nodes.extend(children) + elif isinstance(node, (TElement, TFragment)): + nodes.extend(node.children) + return tcomps + # ------------------------------------------ # HTMLParser other callbacks # ------------------------------------------ @@ -406,15 +593,18 @@ def handle_data(self, data: str) -> None: ref = self.placeholders.remove_placeholders(data) parent = self.get_parent() if parent.children and isinstance(parent.children[-1], TText): + prior_text = parent.children[-1] parent.children[-1] = TText( - ref=combine_template_refs(parent.children[-1].ref, ref) + ref=combine_template_refs(prior_text.ref, ref), + # Keep starting position of the prior text + parser_pos=prior_text.parser_pos, ) else: - self.append_child(TText(ref=ref)) + self.append_child(TText(ref=ref, parser_pos=self.get_parser_pos())) def handle_comment(self, data: str) -> None: ref = self.placeholders.remove_placeholders(data) - comment = TComment(ref) + comment = TComment(ref, parser_pos=self.get_parser_pos()) self.append_child(comment) def handle_decl(self, decl: str) -> None: @@ -423,7 +613,7 @@ def handle_decl(self, decl: str) -> None: raise ValueError("Interpolations are not allowed in declarations.") elif decl.upper().startswith("DOCTYPE "): doctype_content = decl[7:].strip() - doctype = TDocumentType(doctype_content) + doctype = TDocumentType(doctype_content, parser_pos=self.get_parser_pos()) self.append_child(doctype) else: raise NotImplementedError( @@ -436,6 +626,8 @@ def reset(self): self.stack = [] self.placeholders = PlaceholderState() self.source = None + self.sinfo_table = {} + self.tcomponent_children = {} def close(self) -> None: if self.waiting_for_data(): @@ -449,7 +641,49 @@ def close(self) -> None: "Parser expects more data, is the template valid html?" ) if self.stack: - raise ValueError("Invalid HTML structure: unclosed tags remain.") + source = self.get_source() + e = ValueError("Invalid HTML structure: unclosed tags remain.") + # @TODO: We need to determine which tags this might apply to, + # this only applies to components. + parent = self.stack[-1] + if isinstance(parent, OpenTComponent) and self.has_ambiguous_forward_slash( + parent.sinfo + ): + # CASE: "<{C1} attr={value}/>" -- meant to self-close + # Maybe user meant to self-close? + starttag = source.format_starttag(parent.start_i_index) + e.add_note( + f'Did you mean to quote the last attribute or put a space before "/>" for "<{{{starttag}}} .../>"?' + ) + else: + # CASE: t"<{C2}><{C1} attr=/>" + # Maybe user meant to self-close <{C1} ...>, but closed by leaving <{C2}...> open? + # CASE: t"<{C3}><{C2}><{C1} attr=/>" + for comp in reversed( + self.get_closed_tcomps(parent, recurse_component_children=True) + ): + if ( + comp.end_i_index is not None + and comp.start_i_index != comp.end_i_index + and not source.values_match( + comp.start_i_index, comp.end_i_index + ) + ): + sinfo = ( + self.sinfo_table.get(comp.parser_pos) + if comp.parser_pos is not None + else None + ) + starttag = source.format_starttag(comp.start_i_index) + endtag = source.format_endtag(comp.end_i_index) + e.add_note( + f"Component start tag, <{{{starttag}}}>, and end tag, , have values that do not match." + ) + if self.has_ambiguous_forward_slash(sinfo): + e.add_note( + f'Did you mean to quote the last attribute or put a space before "/>" for "<{{{starttag}}} .../>"?' + ) + raise e if not self.placeholders.is_empty: raise ValueError("Some placeholders were never resolved.") super().close() diff --git a/tdom/parser_test.py b/tdom/parser_test.py index d1650ae0..523b918f 100644 --- a/tdom/parser_test.py +++ b/tdom/parser_test.py @@ -4,6 +4,7 @@ from .parser import TemplateParser from .placeholders import make_placeholder_config +from .source import FrozenPosition from .template_utils import TemplateRef from .tnodes import ( TComment, @@ -468,6 +469,14 @@ def Component(): _ = TemplateParser.parse(t"<{Component}>") +def test_unmatched_end_component_tag_error(): + def Component(): + pass + + with pytest.raises(ValueError, match="Unexpected closing component tag"): + _ = TemplateParser.parse(t"") + + def test_placeholder_collision_avoidance(): config = make_placeholder_config() # This test is to ensure that our placeholder detection avoids collisions @@ -602,3 +611,81 @@ def test_extract_with_templated_attr_gt_char(self, Component): strings=("
Hello, World!
",), i_indexes=() ), ) + + +class TestComponentUnquotedAttrValueWithAmbiguousSlash: + @pytest.fixture + def comp_maker(self): + def maker(suffix=None): + def _Comp(children: Template, title: str) -> Template: + return children + + if suffix is not None: + _Comp.__name__ = f"{_Comp.__name__}__{suffix}" + return _Comp + + return maker + + @pytest.fixture + def Comp(self): + def _Comp(children: Template, title: str) -> Template: + return children + + return _Comp + + @pytest.fixture + def Comp2(self): + def _Comp2(children: Template, title: str) -> Template: + return children + + return _Comp2 + + def test_comp_unquoted_attr_value_error_root(self, Comp): + with pytest.raises( + ValueError, match="Did you mean to quote the last attribute" + ): + _ = TemplateParser.parse(t"<{Comp} title=today/>") + + def test_comp_unquoted_attr_value_error_nested_in_el(self, Comp): + with pytest.raises( + ValueError, match="Did you mean to quote the last attribute" + ): + _ = TemplateParser.parse(t"
<{Comp} title=today/>
") + + def test_comp_unquoted_attr_value_error_single_nested_in_comp(self, Comp, Comp2): + with pytest.raises( + ValueError, match="Did you mean to quote the last attribute" + ): + _ = TemplateParser.parse(t"<{Comp2}><{Comp} title=today/>") + + def test_comp_unquoted_attr_value_error_double_nested_in_comp(self, comp_maker): + Comp1, Comp2, Comp3 = comp_maker("1"), comp_maker("2"), comp_maker("3") + with pytest.raises( + ValueError, match="Did you mean to quote the last attribute" + ): + _ = TemplateParser.parse( + t"<{Comp2}><{Comp1}><{Comp3} title=today/>" + ) + + +def PositionComp() -> Template: + return t"" + + +def test_tnode_parser_position(): + for tnode_type, fragment in ( + (TElement, t""), + (TComment, t""), + (TDocumentType, t""), + (TComponent, t"<{PositionComp}>"), + (TText, t"Just a simple text."), + ): + tnode = TemplateParser.parse(t"
" + fragment + t"
") + assert ( + isinstance(tnode, TElement) + and tnode.tag == "div" + and len(tnode.children) == 1 + ) + el = tnode.children[0] + assert isinstance(el, tnode_type) + assert el.parser_pos == FrozenPosition(line=1, offset=len("
")) diff --git a/tdom/parser_utils.py b/tdom/parser_utils.py new file mode 100644 index 00000000..f8365871 --- /dev/null +++ b/tdom/parser_utils.py @@ -0,0 +1 @@ +type HTMLAttribute = tuple[str, str | None] diff --git a/tdom/placeholders.py b/tdom/placeholders.py index 1cf47128..eabe4583 100644 --- a/tdom/placeholders.py +++ b/tdom/placeholders.py @@ -61,6 +61,9 @@ class PlaceholderState: config: PlaceholderConfig = field(default_factory=make_placeholder_config) """Collection of currently 'known and active' placeholder indexes.""" + def copy(self): + return PlaceholderState(known=self.known.copy(), config=self.config) + @property def is_empty(self) -> bool: return len(self.known) == 0 diff --git a/tdom/source.py b/tdom/source.py new file mode 100644 index 00000000..7ff90726 --- /dev/null +++ b/tdom/source.py @@ -0,0 +1,47 @@ +from dataclasses import dataclass + +from .parser_utils import HTMLAttribute + + +@dataclass(slots=True, frozen=True) +class FrozenPosition: + "A immutable position in a block of source code." + + line: int = 1 + " Line of code, starts at 1. " + offset: int = 0 + " Offset from the start of the line, starts at 0. " + + +@dataclass(slots=True) +class Position: + "A position in a block of source code." + + line: int = 1 + " Line of code, starts at 1. " + offset: int = 0 + " Offset from the start of the line, starts at 0. " + + def freeze(self) -> FrozenPosition: + return FrozenPosition(line=self.line, offset=self.offset) + + +@dataclass(frozen=True, slots=True) +class TagSourceInfo: + """ + Retained tag information from the parsed source. + + @NOTE: These properties DEPEND on the placeholder configuration because + they can contain embedded placeholders. + """ + + starttag_text: str + " Entire starttag as parsed, includes placeholders, . " + raw_attrs: tuple[HTMLAttribute, ...] + " Attrs as parsed, includes placeholders. " + startend: bool + " Was parsed as startend tag, ie. . " + starttag_pos: FrozenPosition + " Position of the parser when the element starttag was parsed. " + endtag_pos: FrozenPosition | None = None + " Position of the parser when the element endtag was parsed. " diff --git a/tdom/tnodes.py b/tdom/tnodes.py index 3afb1063..cee6d6da 100644 --- a/tdom/tnodes.py +++ b/tdom/tnodes.py @@ -1,6 +1,7 @@ import typing as t from dataclasses import dataclass, field +from .source import FrozenPosition from .template_utils import TemplateRef @@ -45,6 +46,8 @@ def __str__(self) -> str: class TText(TNode): ref: TemplateRef + parser_pos: FrozenPosition | None = field(default=None, compare=False) + @classmethod def empty(cls) -> t.Self: return cls(TemplateRef.empty()) @@ -58,6 +61,8 @@ def literal(cls, text: str) -> t.Self: class TComment(TNode): ref: TemplateRef + parser_pos: FrozenPosition | None = field(default=None, compare=False) + @classmethod def literal(cls, text: str) -> t.Self: return cls(TemplateRef.literal(text)) @@ -67,11 +72,15 @@ def literal(cls, text: str) -> t.Self: class TDocumentType(TNode): text: str + parser_pos: FrozenPosition | None = field(default=None, compare=False) + @dataclass(slots=True, frozen=True) class TFragment(TNode): children: tuple[TNode, ...] = field(default_factory=tuple) + parser_pos: FrozenPosition | None = field(default=None, compare=False) + @dataclass(slots=True, frozen=True) class TElement(TNode): @@ -79,6 +88,8 @@ class TElement(TNode): attrs: tuple[TAttribute, ...] = field(default_factory=tuple) children: tuple[TNode, ...] = field(default_factory=tuple) + parser_pos: FrozenPosition | None = field(default=None, compare=False) + @dataclass(slots=True, frozen=True) class TComponent(TNode): @@ -95,5 +106,7 @@ class TComponent(TNode): attrs: tuple[TAttribute, ...] = field(default_factory=tuple) + parser_pos: FrozenPosition | None = field(default=None, compare=False) + type TTag = TElement | TComponent | TFragment