From 681dedfa8e1186c08b97c6574183891ad000c730 Mon Sep 17 00:00:00 2001 From: agu2347 <94227848+agu2347@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:25:43 +0530 Subject: [PATCH] Fix substitution keys with characters invalid in a Jinja2 identifier render_substitution() builds a Jinja2 expression by directly interpolating the raw substitution content into a template string: env.from_string(f"{{{{{token.content}}}}}"). For a key like `foo-bar`, this produces the template "{{foo-bar}}", which Jinja2 parses as the expression `foo - bar` (subtraction of two separate names) rather than as a lookup of a single variable named "foo-bar". Since neither `foo` nor `bar` exist in the substitution context, this raised UndefinedError: 'foo' is undefined, matching the error in the issue -- notice it never even mentions the -bar part, because Jinja evaluates left-to-right and raises on the first undefined name. MyST substitutions deliberately support full Jinja2 expression syntax (filters, dotted attribute access, etc. -- confirmed by the existing circular-reference detection walking the full parsed AST for all Name nodes, not just a single key), so a blanket fix escaping all non-identifier-safe characters isn't viable without breaking that. Fix: when the substitution's content is exactly one key already present in the variable context, look it up directly via dict subscript, bypassing Jinja2 expression parsing entirely for that case. Anything else -- arbitrary expressions, filters, dotted access, keys that aren't a literal match -- still goes through the existing Jinja2 rendering path, completely unaffected. The special env name is excluded from the fast path's reference-tracking, matching the existing exclusion in the general path. Verified end-to-end with a real Sphinx build: before the fix, {{foo-bar}} renders empty with the exact 'foo' is undefined warning from the issue; after the fix, it renders correctly. Also verified normal plain keys, Jinja2 filter expressions, and circular-reference detection all continue to work exactly as before. Added an integration test (test_substitutions_dashed_key) with a new sourcedirs fixture covering: a dashed key, a plain key, and a circular reference. Confirmed the test fails with the original code and passes with the fix. Ran the full existing test suite: 1241 passed (1240 baseline + 1 new), 12 skipped, no regressions. Fixes #1007 --- myst_parser/mdit_to_docutils/base.py | 57 +++++++++++++------ .../substitutions_dashed_key/conf.py | 9 +++ .../substitutions_dashed_key/index.md | 7 +++ tests/test_sphinx/test_sphinx_builds.py | 36 ++++++++++++ 4 files changed, 91 insertions(+), 18 deletions(-) create mode 100644 tests/test_sphinx/sourcedirs/substitutions_dashed_key/conf.py create mode 100644 tests/test_sphinx/sourcedirs/substitutions_dashed_key/index.md diff --git a/myst_parser/mdit_to_docutils/base.py b/myst_parser/mdit_to_docutils/base.py index 68cfae72..fae8273b 100644 --- a/myst_parser/mdit_to_docutils/base.py +++ b/myst_parser/mdit_to_docutils/base.py @@ -1973,25 +1973,46 @@ def render_substitution(self, token: SyntaxTreeNode, inline: bool) -> None: # fail on undefined variables env = jinja2.Environment(undefined=jinja2.StrictUndefined) - # try rendering - try: - rendered = env.from_string(f"{{{{{token.content}}}}}").render( - variable_context - ) - except Exception as error: - self.create_warning( - f"Substitution error:{error.__class__.__name__}: {error}", - MystWarnings.SUBSTITUTION, - line=position, - append_to=self.current_node, - ) - return + content = token.content.strip() + + if content in variable_context: + # Fast path for a substitution whose content is exactly a + # known key, looked up directly rather than through Jinja2 + # expression parsing. This is required (not just an + # optimisation) for keys containing characters that are + # invalid in a Jinja2 identifier -- most commonly a dash, + # e.g. `foo-bar` -- which Jinja2 would otherwise parse as + # an *expression* (`foo - bar`, a subtraction of two + # separate, likely-undefined names) rather than as a + # single variable reference. See GH #1007. + # + # This only applies when the whole substitution is exactly + # one known key; anything else (arbitrary Jinja2 + # expressions, filters, dotted attribute access, etc.) + # still goes through the normal Jinja2 rendering path + # below, unaffected. + rendered = str(variable_context[content]) + references = set() if content == "env" else {content} + else: + # try rendering + try: + rendered = env.from_string(f"{{{{{token.content}}}}}").render( + variable_context + ) + except Exception as error: + self.create_warning( + f"Substitution error:{error.__class__.__name__}: {error}", + MystWarnings.SUBSTITUTION, + line=position, + append_to=self.current_node, + ) + return - # handle circular references - ast = env.parse(f"{{{{{token.content}}}}}") - references = { - n.name for n in ast.find_all(jinja2.nodes.Name) if n.name != "env" - } + # handle circular references + ast = env.parse(f"{{{{{token.content}}}}}") + references = { + n.name for n in ast.find_all(jinja2.nodes.Name) if n.name != "env" + } self.document.sub_references = getattr(self.document, "sub_references", set()) cyclic = references.intersection(self.document.sub_references) if cyclic: diff --git a/tests/test_sphinx/sourcedirs/substitutions_dashed_key/conf.py b/tests/test_sphinx/sourcedirs/substitutions_dashed_key/conf.py new file mode 100644 index 00000000..8e34bd70 --- /dev/null +++ b/tests/test_sphinx/sourcedirs/substitutions_dashed_key/conf.py @@ -0,0 +1,9 @@ +extensions = ["myst_parser"] +exclude_patterns = ["_build"] +myst_enable_extensions = ["substitution"] +myst_substitutions = { + "foo-bar": "Foobar", + "plain": "PlainValue", + "circ-a": "{{circ-b}}", + "circ-b": "{{circ-a}}", +} diff --git a/tests/test_sphinx/sourcedirs/substitutions_dashed_key/index.md b/tests/test_sphinx/sourcedirs/substitutions_dashed_key/index.md new file mode 100644 index 00000000..680ee7c9 --- /dev/null +++ b/tests/test_sphinx/sourcedirs/substitutions_dashed_key/index.md @@ -0,0 +1,7 @@ +# Title + +Dashed key: {{foo-bar}} + +Plain key: {{plain}} + +Circular: {{circ-a}} diff --git a/tests/test_sphinx/test_sphinx_builds.py b/tests/test_sphinx/test_sphinx_builds.py index 333b24dc..4a877e4b 100644 --- a/tests/test_sphinx/test_sphinx_builds.py +++ b/tests/test_sphinx/test_sphinx_builds.py @@ -472,6 +472,42 @@ def test_substitutions_missing( ) +@pytest.mark.sphinx( + buildername="html", + srcdir=os.path.join(SOURCE_DIR, "substitutions_dashed_key"), + freshenv=True, +) +def test_substitutions_dashed_key( + app, + status, + warning, + get_sphinx_app_output, +): + """ + Regression test for + https://github.com/executablebooks/MyST-Parser/issues/1007 + + A substitution key containing a dash (e.g. ``foo-bar``) must be + looked up as a single key, not parsed by Jinja2 as a subtraction + expression (``foo - bar``, two separate, likely-undefined names). + Plain keys and circular-reference detection must continue to work + as before. + """ + app.build() + assert "build succeeded" in status.getvalue() + + warnings = warning.getvalue().strip().splitlines() + assert len(warnings) == 1 + assert "circular substitution reference: {'circ-a'}" in warnings[0] + + output = get_sphinx_app_output(app, filename="index.html") + assert "Foobar" in output + assert "PlainValue" in output + # The circular substitution should not have rendered a value. + assert "{{circ-a}}" not in output + assert "{{circ-b}}" not in output + + @pytest.mark.sphinx( buildername="gettext", srcdir=os.path.join(SOURCE_DIR, "gettext"), freshenv=True )