From bcd515f78ed6c8b15a14b57bf8a512388b054d53 Mon Sep 17 00:00:00 2001 From: James Prior Date: Wed, 25 Dec 2024 19:22:30 +0000 Subject: [PATCH 1/5] Pass current line number to expression parsers --- liquid/builtin/statement.py | 4 +++- liquid/builtin/tags/assign_tag.py | 10 +++++++--- liquid/builtin/tags/echo_tag.py | 6 +++--- liquid/builtin/tags/for_tag.py | 5 ++++- liquid/builtin/tags/if_tag.py | 5 ++++- liquid/builtin/tags/tablerow_tag.py | 4 +++- liquid/builtin/tags/unless_tag.py | 4 +++- liquid/environment.py | 15 ++++++++------- liquid/extra/tags/if_expressions.py | 23 +++++++++++++---------- pyproject.toml | 9 ++++++--- 10 files changed, 54 insertions(+), 31 deletions(-) diff --git a/liquid/builtin/statement.py b/liquid/builtin/statement.py index 3043492..a70bf43 100644 --- a/liquid/builtin/statement.py +++ b/liquid/builtin/statement.py @@ -60,4 +60,6 @@ class Statement(Tag): def parse(self, stream: TokenStream) -> StatementNode: tok = stream.current expect(stream, TOKEN_STATEMENT) - return self.node_class(tok, self.env.parse_filtered_expression_value(tok.value)) + return self.node_class( + tok, self.env.parse_filtered_expression_value(tok.value, tok.linenum) + ) diff --git a/liquid/builtin/tags/assign_tag.py b/liquid/builtin/tags/assign_tag.py index 874c0ff..96cd536 100644 --- a/liquid/builtin/tags/assign_tag.py +++ b/liquid/builtin/tags/assign_tag.py @@ -65,8 +65,8 @@ class AssignTag(Tag): block = False node_class = AssignNode - def _parse_expression(self, value: str) -> Expression: - return self.env.parse_filtered_expression_value(value) + def _parse_expression(self, value: str, linenum: int) -> Expression: + return self.env.parse_filtered_expression_value(value, linenum) def parse(self, stream: TokenStream) -> AssignNode: expect(stream, TOKEN_TAG, value=TAG_ASSIGN) @@ -83,5 +83,9 @@ def parse(self, stream: TokenStream) -> AssignNode: ) return self.node_class( - tok, AssignmentExpression(name, self._parse_expression(right)) + tok, + AssignmentExpression( + name, + self._parse_expression(right, stream.current.linenum), + ), ) diff --git a/liquid/builtin/tags/echo_tag.py b/liquid/builtin/tags/echo_tag.py index 1187386..467b726 100644 --- a/liquid/builtin/tags/echo_tag.py +++ b/liquid/builtin/tags/echo_tag.py @@ -30,8 +30,8 @@ class EchoTag(Tag): block = False node_class = EchoNode - def _parse_expression(self, value: str) -> Expression: - return self.env.parse_filtered_expression_value(value) + def _parse_expression(self, value: str, linenum: int) -> Expression: + return self.env.parse_filtered_expression_value(value, linenum) def parse(self, stream: TokenStream) -> Node: # noqa: D102 expect(stream, TOKEN_TAG, value=TAG_ECHO) @@ -42,5 +42,5 @@ def parse(self, stream: TokenStream) -> Node: # noqa: D102 expr: Expression = NIL else: expect(stream, TOKEN_EXPRESSION) - expr = self._parse_expression(stream.current.value) + expr = self._parse_expression(stream.current.value, tok.linenum) return self.node_class(tok, expression=expr) diff --git a/liquid/builtin/tags/for_tag.py b/liquid/builtin/tags/for_tag.py index 38066b6..307fb75 100644 --- a/liquid/builtin/tags/for_tag.py +++ b/liquid/builtin/tags/for_tag.py @@ -334,7 +334,10 @@ def parse(self, stream: TokenStream) -> Node: stream.next_token() expect(stream, TOKEN_EXPRESSION) - expr = self.env.parse_loop_expression_value(stream.current.value) + expr = self.env.parse_loop_expression_value( + stream.current.value, + stream.current.linenum, + ) stream.next_token() block = parser.parse_block(stream, ENDFORBLOCK) diff --git a/liquid/builtin/tags/if_tag.py b/liquid/builtin/tags/if_tag.py index e927496..77eee8c 100644 --- a/liquid/builtin/tags/if_tag.py +++ b/liquid/builtin/tags/if_tag.py @@ -185,7 +185,10 @@ def __init__(self, env: Environment): def parse_expression(self, stream: TokenStream) -> Expression: """Pare a boolean expression from a stream of tokens.""" expect(stream, TOKEN_EXPRESSION) - return self.env.parse_boolean_expression_value(stream.current.value) + return self.env.parse_boolean_expression_value( + stream.current.value, + stream.current.linenum, + ) def parse(self, stream: TokenStream) -> Node: expect(stream, TOKEN_TAG, value=TAG_IF) diff --git a/liquid/builtin/tags/tablerow_tag.py b/liquid/builtin/tags/tablerow_tag.py index 1dd6882..c3d7e3b 100644 --- a/liquid/builtin/tags/tablerow_tag.py +++ b/liquid/builtin/tags/tablerow_tag.py @@ -267,7 +267,9 @@ def parse(self, stream: TokenStream) -> TablerowNode: stream.next_token() expect(stream, TOKEN_EXPRESSION) - loop_expression = self.env.parse_loop_expression_value(stream.current.value) + loop_expression = self.env.parse_loop_expression_value( + stream.current.value, stream.current.linenum + ) stream.next_token() block = parser.parse_block(stream, END_TAGBLOCK) diff --git a/liquid/builtin/tags/unless_tag.py b/liquid/builtin/tags/unless_tag.py index 5a0ac8c..ee71dc1 100644 --- a/liquid/builtin/tags/unless_tag.py +++ b/liquid/builtin/tags/unless_tag.py @@ -184,7 +184,9 @@ def __init__(self, env: Environment): def parse_expression(self, stream: TokenStream) -> Expression: """Parse a boolean expression from a stream of tokens.""" expect(stream, TOKEN_EXPRESSION) - return self.env.parse_boolean_expression_value(stream.current.value) + return self.env.parse_boolean_expression_value( + stream.current.value, stream.current.linenum + ) def parse(self, stream: TokenStream) -> Union[UnlessNode, IllegalNode]: expect(stream, TOKEN_TAG, value=TAG_UNLESS) diff --git a/liquid/environment.py b/liquid/environment.py index 113c125..e90a86d 100644 --- a/liquid/environment.py +++ b/liquid/environment.py @@ -529,7 +529,8 @@ async def analyze_tags_async( ) def make_globals( - self, globals: Optional[Mapping[str, object]] = None # noqa: A002 + self, + globals: Optional[Mapping[str, object]] = None, # noqa: A002 ) -> Dict[str, object]: """Combine environment globals with template globals.""" if globals: @@ -577,12 +578,12 @@ def set_expression_cache_size(self, maxsize: int = 0) -> None: def _get_expression_parsers( self, cache_size: int = 0 ) -> Tuple[ - Callable[[str], "BooleanExpression"], - Callable[[str], "BooleanExpression"], - Callable[[str], "FilteredExpression"], - Callable[[str], "FilteredExpression"], - Callable[[str], "FilteredExpression"], - Callable[[str], "LoopExpression"], + Callable[[str, int], "BooleanExpression"], + Callable[[str, int], "BooleanExpression"], + Callable[[str, int], "FilteredExpression"], + Callable[[str, int], "FilteredExpression"], + Callable[[str, int], "FilteredExpression"], + Callable[[str, int], "LoopExpression"], ]: if cache_size >= 1: return ( diff --git a/liquid/extra/tags/if_expressions.py b/liquid/extra/tags/if_expressions.py index 650d633..e6b8318 100644 --- a/liquid/extra/tags/if_expressions.py +++ b/liquid/extra/tags/if_expressions.py @@ -26,7 +26,7 @@ def parse(self, stream: TokenStream) -> StatementNode: tok = stream.current expect(stream, TOKEN_STATEMENT) return StatementNode( - tok, self.env.parse_conditional_expression_value(tok.value) + tok, self.env.parse_conditional_expression_value(tok.value, tok.linenum) ) @@ -35,8 +35,8 @@ class InlineIfAssignTag(AssignTag): inline `if` expressions. """ - def _parse_expression(self, value: str) -> Expression: - return self.env.parse_conditional_expression_value(value) + def _parse_expression(self, value: str, linenum: int) -> Expression: + return self.env.parse_conditional_expression_value(value, linenum) class InlineIfEchoTag(EchoTag): @@ -44,8 +44,8 @@ class InlineIfEchoTag(EchoTag): inline `if` expressions. """ - def _parse_expression(self, value: str) -> Expression: - return self.env.parse_conditional_expression_value(value) + def _parse_expression(self, value: str, linenum: int) -> Expression: + return self.env.parse_conditional_expression_value(value, linenum) class InlineIfStatementWithParens(Statement): @@ -58,7 +58,10 @@ def parse(self, stream: TokenStream) -> StatementNode: tok = stream.current expect(stream, TOKEN_STATEMENT) return StatementNode( - tok, self.env.parse_conditional_expression_value_with_parens(tok.value) + tok, + self.env.parse_conditional_expression_value_with_parens( + tok.value, tok.linenum + ), ) @@ -68,8 +71,8 @@ class InlineIfAssignTagWithParens(AssignTag): terms with parentheses. """ - def _parse_expression(self, value: str) -> Expression: - return self.env.parse_conditional_expression_value_with_parens(value) + def _parse_expression(self, value: str, linenum: int) -> Expression: + return self.env.parse_conditional_expression_value_with_parens(value, linenum) class InlineIfEchoTagWithParens(EchoTag): @@ -78,5 +81,5 @@ class InlineIfEchoTagWithParens(EchoTag): terms with parentheses. """ - def _parse_expression(self, value: str) -> Expression: - return self.env.parse_conditional_expression_value_with_parens(value) + def _parse_expression(self, value: str, linenum: int) -> Expression: + return self.env.parse_conditional_expression_value_with_parens(value, linenum) diff --git a/pyproject.toml b/pyproject.toml index 8a1a1a2..01d60bc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -3,7 +3,7 @@ build-backend = "hatchling.build" requires = ["hatchling"] [project] -authors = [{name = "James Prior", email = "jamesgr.prior@gmail.com"}] +authors = [{ name = "James Prior", email = "jamesgr.prior@gmail.com" }] classifiers = [ "Development Status :: 5 - Production/Stable", "Intended Audience :: Developers", @@ -18,7 +18,11 @@ classifiers = [ "Programming Language :: Python :: 3.11", "Programming Language :: Python :: 3.12", ] -dependencies = ["python-dateutil>=2.8.1", "typing-extensions>=4.2.0", "importlib-resources>=5.10.0"] +dependencies = [ + "python-dateutil>=2.8.1", + "typing-extensions>=4.2.0", + "importlib-resources>=5.10.0", +] description = "A Python engine for the Liquid template language." dynamic = ["version"] license = "MIT" @@ -150,7 +154,6 @@ select = [ "SLF", "T10", "T20", - "TCH", "YTT", ] # TODO: review ignores From 960538aa3a5469c1b2e26147bf5881d27a398e71 Mon Sep 17 00:00:00 2001 From: James Prior Date: Wed, 25 Dec 2024 19:46:36 +0000 Subject: [PATCH 2/5] Test with Ubuntu 22.04 for Python 3.7 --- .github/workflows/tests.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 96c09f9..1f6ec43 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -8,7 +8,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] + os: [ubuntu-22.04, windows-latest, macos-latest] python-version: ["3.7.17", "3.8", "3.9", "3.10", "3.11", "3.12", "3.13"] exclude: - os: macos-latest From ebb8e528a2e35442055b5d3d771a93012e977ba6 Mon Sep 17 00:00:00 2001 From: James Prior Date: Wed, 25 Dec 2024 20:11:24 +0000 Subject: [PATCH 3/5] Found a few more --- liquid/builtin/tags/decrement_tag.py | 4 +++- liquid/builtin/tags/increment_tag.py | 4 +++- liquid/builtin/tags/render_tag.py | 5 ++++- liquid/extra/tags/if_not.py | 4 +++- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/liquid/builtin/tags/decrement_tag.py b/liquid/builtin/tags/decrement_tag.py index 7874f97..4981cb3 100644 --- a/liquid/builtin/tags/decrement_tag.py +++ b/liquid/builtin/tags/decrement_tag.py @@ -63,7 +63,9 @@ def parse(self, stream: TokenStream) -> DecrementNode: tok=tok, identifier=str( parse_unchained_identifier( - ExprTokenStream(tokenize(stream.current.value)) + ExprTokenStream( + tokenize(stream.current.value, stream.current.linenum) + ) ) ), ) diff --git a/liquid/builtin/tags/increment_tag.py b/liquid/builtin/tags/increment_tag.py index 66b3339..3e2f6e4 100644 --- a/liquid/builtin/tags/increment_tag.py +++ b/liquid/builtin/tags/increment_tag.py @@ -59,7 +59,9 @@ def parse(self, stream: TokenStream) -> IncrementNode: tok=tok, identifier=str( parse_unchained_identifier( - ExprTokenStream(tokenize(stream.current.value)) + ExprTokenStream( + tokenize(stream.current.value, stream.current.linenum) + ) ) ), ) diff --git a/liquid/builtin/tags/render_tag.py b/liquid/builtin/tags/render_tag.py index de58e9e..491f76f 100644 --- a/liquid/builtin/tags/render_tag.py +++ b/liquid/builtin/tags/render_tag.py @@ -1,4 +1,5 @@ """Parse tree node and tag definition for the built in "render" tag.""" + import sys from typing import Dict from typing import List @@ -249,7 +250,9 @@ class RenderTag(Tag): def parse(self, stream: TokenStream) -> Node: tok = next(stream) expect(stream, TOKEN_EXPRESSION) - expr_stream = ExprTokenStream(tokenize(stream.current.value)) + expr_stream = ExprTokenStream( + tokenize(stream.current.value, stream.current.linenum) + ) # Need a string. 'render' does not accept identifiers that resolve to a string. # This is the name of the template to be included. diff --git a/liquid/extra/tags/if_not.py b/liquid/extra/tags/if_not.py index 821db7c..2a31eb6 100644 --- a/liquid/extra/tags/if_not.py +++ b/liquid/extra/tags/if_not.py @@ -20,4 +20,6 @@ class IfNotTag(IfTag): def parse_expression(self, stream: TokenStream) -> Expression: """Pare a boolean expression from a stream of tokens.""" expect(stream, TOKEN_EXPRESSION) - return parse_boolean_expression_with_parens(stream.current.value) + return parse_boolean_expression_with_parens( + stream.current.value, stream.current.linenum + ) From 381fd147ef55309ddb125888d401e292ccdb8ed8 Mon Sep 17 00:00:00 2001 From: James Prior Date: Thu, 26 Dec 2024 08:35:53 +0000 Subject: [PATCH 4/5] Add more tests --- tests/test_malformed.py | 100 ++++++++++++++++++++++++++++++++++------ 1 file changed, 86 insertions(+), 14 deletions(-) diff --git a/tests/test_malformed.py b/tests/test_malformed.py index 0505ce8..068ee97 100644 --- a/tests/test_malformed.py +++ b/tests/test_malformed.py @@ -17,6 +17,7 @@ from liquid.exceptions import FilterArgumentError from liquid.exceptions import LiquidSyntaxError from liquid.exceptions import LiquidTypeError +from liquid.exceptions import LiquidWarning from liquid.exceptions import TemplateNotFound from liquid.exceptions import lookup_warning from liquid.extra import IfNotTag @@ -34,20 +35,20 @@ class Case(NamedTuple): expect_render: str = "" @property - def exceptions(self): + def exceptions(self) -> Tuple[Type[Error], ...]: if isinstance(self.expect_exception, tuple): return self.expect_exception return (self.expect_exception,) @property - def warnings(self): + def warnings(self) -> Tuple[Type[LiquidWarning], ...]: return tuple(lookup_warning(e) for e in self.exceptions) class MalformedTemplateTestCase(TestCase): """Malformed template test case.""" - def setUp(self): + def setUp(self) -> None: self.global_context = { "product": { "some-tags": ["hello", "there"], @@ -56,7 +57,7 @@ def setUp(self): "tag": "goodbye", } - def _test(self, test_cases: Iterable[Case], mode: Mode = Mode.STRICT): + def _test(self, test_cases: Iterable[Case], mode: Mode = Mode.STRICT) -> None: """Helper method for running lists of `Case`s in each render mode.""" self._test_with_env(Environment(tolerance=mode), test_cases) @@ -76,7 +77,7 @@ def _test(self, test_cases: Iterable[Case], mode: Mode = Mode.STRICT): env, [case for case in test_cases if "||" not in case.template] ) - def _test_with_env(self, env: Environment, test_cases: Iterable[Case]): + def _test_with_env(self, env: Environment, test_cases: Iterable[Case]) -> None: for case in test_cases: with self.subTest(msg=case.description, mode=env.mode): if env.mode == Mode.STRICT: @@ -104,7 +105,9 @@ def _test_with_env(self, env: Environment, test_cases: Iterable[Case]): result = template.render() self.assertEqual(result, case.expect_render) - def _test_partial(self, test_cases: Iterable[Case], templates: Dict[str, str]): + def _test_partial( + self, test_cases: Iterable[Case], templates: Dict[str, str] + ) -> None: """Helper method for testing lists of 'include' or 'render' cases.""" env = Environment(loader=DictLoader(templates)) for case in test_cases: @@ -133,7 +136,7 @@ def _test_partial(self, test_cases: Iterable[Case], templates: Dict[str, str]): result = template.render() self.assertEqual(result, case.expect_render) - def test_liquid_syntax(self): + def test_liquid_syntax(self) -> None: """Test that we fail early and loud when parsing a malformed template.""" test_cases = [ Case( @@ -390,14 +393,14 @@ def test_liquid_syntax(self): self._test(test_cases, mode=Mode.WARN) self._test(test_cases, mode=Mode.LAX) - def test_liquid_syntax_from_template_api(self): + def test_liquid_syntax_from_template_api(self) -> None: """Test that syntax errors are raised by default when using the `Template` API.""" with self.assertRaises(LiquidSyntaxError): # Missing colon before filter argument Template(r"{{ a | sort foo }}") - def test_unrecoverable_syntax_errors(self): + def test_unrecoverable_syntax_errors(self) -> None: """Test that we fail early and loud when parsing a malformed template.""" test_cases = [ Case( @@ -412,6 +415,13 @@ def test_unrecoverable_syntax_errors(self): expect_exception=LiquidSyntaxError, expect_msg="expected '%}', found 'eof', on line 1", ), + ] + + self._test(test_cases, mode=Mode.STRICT) + + def test_error_line_numbers(self) -> None: + """Test that we get the correct line number in error messages.""" + test_cases = [ Case( description="issue #162", template="Hello, \nMy name is {{{ name }}", @@ -419,16 +429,78 @@ def test_unrecoverable_syntax_errors(self): expect_msg="unexpected '{', on line 2", ), Case( - description="issue #162", + description="echo", template="Hello, \nMy name is {% echo { name %}", expect_exception=LiquidSyntaxError, expect_msg="unexpected '{', on line 2", ), + Case( + description="assign", + template="Hello, \n\n {% assign x = { name %}", + expect_exception=LiquidSyntaxError, + expect_msg="unexpected '{', on line 3", + ), + Case( + description="decrement", + template="Hello, \n\n {% decrement { x %}", + expect_exception=LiquidSyntaxError, + expect_msg="unexpected '{', on line 3", + ), + Case( + description="increment", + template="Hello, \n\n {% increment { x %}", + expect_exception=LiquidSyntaxError, + expect_msg="unexpected '{', on line 3", + ), + Case( + description="for", + template="Hello, \n {% for x \nin { y %}{% endfor %}", + expect_exception=LiquidSyntaxError, + expect_msg="unexpected '{', on line 3", + ), + Case( + description="if", + template="Hello, \n {% if x == { y %}{% endif %}", + expect_exception=LiquidSyntaxError, + expect_msg="unexpected '{', on line 2", + ), + Case( + description="elsif", + template="Hello, \n {% if x == y %}\n{% elsif z < { %}{% endif %}", + expect_exception=LiquidSyntaxError, + expect_msg="unexpected '{', on line 3", + ), + Case( + description="render", + template="Hello, \n {% render 'foo' x: { %}", + expect_exception=LiquidSyntaxError, + expect_msg="unexpected '{', on line 2", + ), + Case( + description="tablerow", + template="Hello, \n {% tablerow x in { y %}{% endtablerow %}", + expect_exception=LiquidSyntaxError, + expect_msg="unexpected '{', on line 2", + ), + Case( + description="unless", + template="Hello, \n {% unless x == { y %}{% endunless %}", + expect_exception=LiquidSyntaxError, + expect_msg="unexpected '{', on line 2", + ), + Case( + description="unless elsif", + template=( + "Hello, \n {% unless x == y %}\n{% elsif z < { %}{% endunless %}" + ), + expect_exception=LiquidSyntaxError, + expect_msg="unexpected '{', on line 3", + ), ] self._test(test_cases, mode=Mode.STRICT) - def test_bad_include(self): + def test_bad_include(self) -> None: """Test that we gracefully handle include errors.""" test_cases = [ Case( @@ -498,7 +570,7 @@ def test_bad_include(self): self._test_partial(test_cases, templates) - def test_bad_render(self): + def test_bad_render(self) -> None: """Test that we gracefully handle render errors.""" test_cases = [ Case( @@ -559,7 +631,7 @@ def test_bad_render(self): self._test_partial(test_cases, templates) - def test_resume_block(self): + def test_resume_block(self) -> None: """Test that we continue to execute a block after a single statement error.""" source = ( r"{% if true %}" @@ -594,7 +666,7 @@ def test_resume_block(self): self.assertEqual(result, "before error after error") - def test_invalid_identifiers(self): + def test_invalid_identifiers(self) -> None: """Test that we gracefully handle invalid identifiers.""" test_cases = [ Case( From 0f729c0f30fef969b5476d9c41062cf3bfce0e9c Mon Sep 17 00:00:00 2001 From: James Prior Date: Thu, 26 Dec 2024 08:44:02 +0000 Subject: [PATCH 5/5] Update change log --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index d56f9f2..84164fd 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,6 +5,7 @@ **Fixes** - Fixed `{% case %}` / `{% when %}` behavior. When using [`liquid.future.Environment`](https://jg-rp.github.io/liquid/api/future-environment), we now render any number of `{% else %}` blocks and allow `{% when %}` tags to appear after `{% else %}` tags. The default `Environment` continues to raise a `LiquidSyntaxError` in such cases. +- Fixed line numbers in some error messages. When parsing some Liquid expressions, we were always getting a line number of `1` in the event of a syntax error. See [issue #162](https://github.com/jg-rp/liquid/issues/162). **Changed**