From db6adf850ad513398432272887e6dc2a641f9034 Mon Sep 17 00:00:00 2001 From: agu2347 Date: Mon, 20 Jul 2026 06:42:58 +0000 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 (a shallow test-fixture based reproduction wasn't enough on its own, so I also built a throwaway Sphinx project on disk mirroring the issue's exact repro): 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 (e.g. `{{ value | upper }}`), 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 (to confirm circular-reference detection still triggers correctly for a dashed key too, exercising the fast path's reference tracking). Confirmed the test fails with the original code (produces the exact reported UndefinedError and leaves the dashed key unrendered) and passes with the fix. Ran the full existing test suite: 1241 passed (1240 baseline + 1 new), 12 skipped. The 4 remaining failures (missing linkify-it-py optional dependency, and pygments/docutils version-related rendering diffs) are present identically on a clean checkout of master, unrelated to this change. 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 )