Improve error message for unclosed components whose attribute value might have consumed the slash. #140
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,12 @@ | |
|
|
||
| @dataclass | ||
| class OpenTElement: | ||
| starttag_text: str | ||
| " Entire starttag as parsed, includes placeholders, used for debugging. " | ||
| raw_attrs: Sequence[HTMLAttribute] | ||
| " Attrs as parsed, includes placeholders, used for debugging. " | ||
| startend: bool | ||
| " Was parsed as startend tag, ie. <tag />, used for debugging. " | ||
| tag: str | ||
| attrs: tuple[TAttribute, ...] | ||
| children: list[TNode] = field(default_factory=list) | ||
|
|
@@ -39,6 +45,12 @@ class OpenTFragment: | |
|
|
||
| @dataclass | ||
| class OpenTComponent: | ||
| starttag_text: str | ||
| " Entire starttag as parsed, includes placeholders, used for debugging. " | ||
| raw_attrs: Sequence[HTMLAttribute] | ||
| " Attrs as parsed, includes placeholders, used for debugging. " | ||
| startend: bool | ||
| " Was parsed as startend tag, ie. <tag />, used for debugging. " | ||
| start_i_index: int | ||
| children_start_s_index: int | ||
| """The strings index where the component's children template starts.""" | ||
|
|
@@ -72,6 +84,26 @@ class SourceTracker: | |
| def interpolations(self) -> tuple[Interpolation, ...]: | ||
| return self.template.interpolations | ||
|
|
||
| def _check_indices(self, index1: int, index2: int): | ||
| last_index = len(self.interpolations) - 1 | ||
| if max(index1, index2) > last_index or min(index1, index2) < 0: | ||
| raise ValueError( | ||
| f"Interpolation indices exceed bounds: {index1} {index2}: [0...{last_index}]" | ||
| ) | ||
|
|
||
| def expressions_match(self, i_index1: int, i_index2: int) -> bool: | ||
| self._check_indices(i_index1, i_index2) | ||
| return ( | ||
| self.interpolations[i_index1].expression | ||
| == self.interpolations[i_index2].expression | ||
| ) | ||
|
|
||
| def values_match(self, i_index1: int, i_index2: int) -> bool: | ||
| self._check_indices(i_index1, i_index2) | ||
| 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 | ||
|
|
@@ -159,12 +191,20 @@ def make_tattrs(self, attrs: Sequence[HTMLAttribute]) -> tuple[TAttribute, ...]: | |
| # Tag Helpers | ||
| # ------------------------------------------ | ||
|
|
||
| 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( | ||
| starttag_text=self.get_starttag_text(), | ||
| raw_attrs=attrs, | ||
| startend=startend, | ||
| tag=tag, | ||
| attrs=self.make_tattrs(attrs), | ||
| ) | ||
|
|
||
| if not tag_ref.is_singleton: | ||
| raise ValueError( | ||
|
|
@@ -189,11 +229,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( | ||
| f"Expected startag_text to be set when parsing component at {i_index}." | ||
| ) | ||
|
|
||
| tattrs = self.make_tattrs(attrs) | ||
|
|
||
|
|
@@ -205,6 +243,9 @@ def make_open_tag(self, tag: str, attrs: Sequence[HTMLAttribute]) -> OpenTag: | |
| ) | ||
|
|
||
| return OpenTComponent( | ||
| starttag_text=starttag_text, | ||
| raw_attrs=attrs, | ||
| startend=startend, | ||
| start_i_index=i_index, | ||
| children_start_s_index=children_start_s_index, | ||
| offset_into_children_start_s=offset_into_children_start_s, | ||
|
|
@@ -339,7 +380,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.""" | ||
| source = self.get_source() | ||
| tag_ref = self.placeholders.remove_placeholders(tag) | ||
|
|
||
| match open_tag: | ||
|
|
@@ -359,18 +400,73 @@ 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: | ||
| starttag = source.format_starttag(start_i_index) | ||
| e = ValueError( | ||
| f"Mismatched closing tag </{tag}> for component with tag {{{starttag}}}." | ||
| ) | ||
| if self.has_ambiguous_forward_slash(open_tag): | ||
| 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." | ||
| ) | ||
| if not source.expressions_match( | ||
|
Contributor
There was a problem hiding this comment. ...but there's a catch, which is that "cache only on def C1():
pass
def C2():
pass
proxy = CachedTemplateParserProxy()
proxy.to_tnode(t"<{C1}></{C1}>") # works as expected
proxy.to_tnode(t"<{C1}></{C2}>") # doesn't raise because we hit the cacheI'm not sure exactly how to think about this. I know some check is necessary to handle your I wonder if we could compute
Contributor
There was a problem hiding this comment. (Random aside: is the expression check before the value check a perf thing? In the common case, it seems good. But I suppose we can craft identical expressions that may not be equivalent, like
Contributor
Author
There was a problem hiding this comment. I guess, in this case, I am ok NOT involving this in the caching story. I think it is okay to say that the open tag value and the closing tag value for component tags need to have the same value if you want it parsed correctly ( or always?). After the "first" time whenever that is as there might be a startup stampede then we'll just assume you passed in the right values otherwise the behavior is "unpredictable". The only part I don't like is digging in the interpolation values "in general". I think if for example you wanted to pre-parse the templates then you could pass dummy values that follow that rule and it should still work. Otherwise we end up with essentially this situation An alternative which might be possible is that when the inevitable failure occurs then we backtrack and try to rerun through the tags and decipher which component was incorrectly closed with a different component for whatever reason, the common case probably being this: |
||
| open_tag.start_i_index, tag_ref.i_indexes[0] | ||
| ) and not source.values_match( | ||
| open_tag.start_i_index, tag_ref.i_indexes[0] | ||
| ): | ||
| e = TypeError( | ||
| "Component start and end tags must contain the same callable." | ||
| ) | ||
| if self.has_ambiguous_forward_slash(open_tag): | ||
| starttag = source.format_starttag(start_i_index) | ||
| e.add_note( | ||
| f'Did you mean to quote the last attribute or put a space before "/>" for "<{{{starttag}}} .../>"?' | ||
| ) | ||
| raise e | ||
| 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, open_tag: OpenTag) -> 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 "<div title=test/>" or "<{Component} title=test/>". | ||
|
|
||
| Or more often "<{Component} title={title}/>" which should be corrected | ||
| with "<{Component} title={title} />". | ||
| """ | ||
| if isinstance(open_tag, (OpenTElement, OpenTComponent)): | ||
| return ( | ||
| # has attributes | ||
| len(open_tag.raw_attrs) > 0 | ||
| # last attr not bare attribute | ||
| and open_tag.raw_attrs[-1][1] is not None | ||
| # last char of last attr is "/" | ||
| and open_tag.raw_attrs[-1][1][-1] == "/" | ||
| # parsed starttag ends with "/>" | ||
| and open_tag.starttag_text.endswith("/>") | ||
| # if parsed as startend then its not ambiguous | ||
| and not open_tag.startend | ||
| ) | ||
| return False | ||
|
|
||
| # ------------------------------------------ | ||
| # HTMLParser tag callbacks | ||
| # ------------------------------------------ | ||
|
|
@@ -385,7 +481,7 @@ 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, `<tag />` to specialized handlers.""" | ||
| open_tag = self.make_open_tag(tag, attrs, startend=True) | ||
| final_tag = self.finalize_tag(open_tag) | ||
| self.append_child(final_tag) | ||
|
|
||
|
|
@@ -449,7 +545,21 @@ def close(self) -> None: | |
| "Parser expects more data, is the template valid html?" | ||
| ) | ||
| if self.stack: | ||
| e = ValueError("Invalid HTML structure: unclosed tags remain.") | ||
| # Check for tags that might have meant to self-close but whose | ||
| # unquoted last attribute value consumed a "/", ie. <div id=app/>. | ||
| parent = self.stack[-1] | ||
| # @TODO: We need to determine which tags this might apply to, this only applies to components. | ||
|
Contributor
There was a problem hiding this comment. Yeah this PR sequences before the work that would answer this. But it does feel like where we land is: anything self closable (components here; in a future PR, void tags; even further, foreign tags once the parser becomes smart about such things) gets one message; everyone else gets the other.
Contributor
Author
There was a problem hiding this comment. Yeah I think actually void tags wouldn't get this message because they don't go on the stack but yes we'd probably add foreign tags. Just to clarify: This error is happening regardless of the slash but we try to make "Did you mean ..." help message for a potentially hard to debug case but the "unclosed tags remain" is always happening. They could have meant to use the slash as an attribute value but ... just forgot to close the component (or foreign tag). Ie. |
||
| if isinstance(parent, OpenTComponent) and self.has_ambiguous_forward_slash( | ||
| parent | ||
| ): | ||
| starttag = ( | ||
| f"{{{self.get_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} .../>"?' | ||
| ) | ||
| raise e | ||
|
Contributor
Author
There was a problem hiding this comment. @davepeck This block now performs the component start/end values check but where an exception is already going to happen (we closed the parser but tags are still open on the stack). So the parsing would have already failed. I think that should still keep our cache solely dependent on the shape because the values check does not affect what succeeds it is only used to provided better error reporting. Does that make sense? |
||
| if not self.placeholders.is_empty: | ||
| raise ValueError("Some placeholders were never resolved.") | ||
| super().close() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.