🐛 FIX: report accurate source lines for directives and text

Fix three classes of wrong source-line attribution:

- Directive option warnings (unknown key, invalid value, bad YAML,
  comments) now point at the offending option's own source line,
  for both `---` and `:option:` block styles, instead of the
  directive's opening line.
  Unknown option keys are now warned once per key, at its own line.
  The `---` options block is now split line-based: any text trailing
  a closing delimiter still becomes body content (as previously),
  but is kept as its own line, so it and all subsequent body lines
  map exactly to their source lines.
  A new `options_to_tokens` API in `myst_parser.parsers.options`
  exposes the tokenizer's per-key positions, and its `State` now
  records the lines of `#` comments
  (`options_to_items` is now a thin wrapper around it).
  Directives run from synthesized, non-source content (HTML images
  and admonitions) attribute option warnings to the element's line.

- Directives with body content on the opening line (no-argument
  directives such as `{note}`) reported every body warning one line
  too low; `body_offset` is now `-1` for the merged first line.

- `document.current_line` was set once per directive and never
  reset, so docutils' `Node.setup_child` stamped the most recent
  directive's line (or 0) onto every subsequently-created line-less
  node, e.g. every `Text` node - the root cause of tools such as
  sphinxcontrib-spelling and gettext reporting 'line 0' or the last
  directive's line. The renderer now keeps `current_line` in sync
  as nodes are created.

Also:

- `block_text` passed to directives is now the full directive text
  (opening fence, options, body, closing fence), matching what an
  equivalent rST directive receives, so third-party directives that
  inspect it see rST-consistent content.
- `include` with `:literal:` now sets the literal block's line to
  `start-line + 1` (docutils parity) instead of a hardcoded 1.

Closes #546

Author: chrisjsewell

AGENTS.md

@@ -142,15 +142,20 @@ def parse_directive_text(
     first_line: str,
     content: str,
     *,
+    line: int | None = None,
     validate_options: bool = True,
-) -> tuple[list[str], dict[str, Any], list[str], int]:
+    additional_options: dict[str, str] | None = None,
+) -> DirectiveParsingResult:
     """Parse directive text into its components.
 
     :param directive_class: The directive class to parse for.
     :param first_line: The first line (arguments).
     :param content: The directive content.
+    :param line: The 1-based source line of the directive's opening line.
     :param validate_options: Whether to validate options against the directive spec.
-    :return: Tuple of (arguments, options, body_lines, body_offset).
+    :param additional_options: Additional options to add to the directive.
+    :return: A ``DirectiveParsingResult`` with fields
+        ``arguments``, ``options``, ``body``, ``body_offset`` and ``warnings``.
     :raises MarkupError: If the directive text is malformed.
     """
     ...

docs/configuration.md

@@ -211,5 +211,13 @@ suppress_warnings = ["myst.header"]
 
 Or use `--myst-suppress-warnings="myst.header"` for the [docutils CLI](myst-docutils).
 
+:::{note}
+Two known limitations apply to the source lines reported by warnings:
+warnings for inline syntax (such as roles and links) report the first line of the enclosing paragraph,
+rather than the exact line of the syntax,
+and warnings within the rendered output of a [substitution](syntax/substitutions) may map beyond the substitution's own location,
+if that output spans more lines than the source.
+:::
+
 ```{myst-warnings}
 ```

myst_parser/mdit_to_docutils/base.py

@@ -321,7 +321,9 @@ def nested_render_text(
         if tokens and tokens[0].type == "front_matter":
             tokens.pop(0)
 
-        # update the line numbers
+        # update the line numbers,
+        # note the maps stay 0-based here;
+        # ``_render_tokens`` applies the final 0-based to 1-based conversion
         for token in tokens:
             if token.map:
                 token.map = [token.map[0] + lineno, token.map[1] + lineno]
@@ -374,6 +376,10 @@ def add_line_and_source_path(self, node, token: SyntaxTreeNode) -> None:
         """Copy the line number and document source path to the docutils node."""
         with suppress(ValueError):
             node.line = token_line(token)
+            # keep the document's current_line in sync, so that docutils'
+            # ``Node.setup_child`` stamps sensible lines onto line-less
+            # children (such as ``Text`` nodes), rather than a stale value
+            self.document.current_line = node.line
         node.source = self.document["source"]
 
     def add_line_and_source_path_r(
@@ -1783,12 +1789,22 @@ def render_directive(
         :param arguments: The remaining text on the same line as the directive name.
         """
         position = token_line(token)
+        # reconstruct the directive's full text, so that ``block_text`` aligns
+        # with what an equivalent rST directive would receive
+        # (best-effort: the parser strips fence indentation and normalises
+        # the info string, so this may not be byte-identical to the source)
+        content_text = token.content
+        if content_text and not content_text.endswith("\n"):
+            # e.g. for an unterminated fence at the end of the document
+            content_text += "\n"
+        block_text = f"{token.markup}{token.info}\n{content_text}{token.markup}\n"
         nodes_list = self.run_directive(
             name,
             arguments,
             token.content,
             position,
             additional_options=additional_options,
+            block_text=block_text,
         )
         self.current_node += nodes_list
 
@@ -1799,6 +1815,8 @@ def run_directive(
         content: str,
         position: int,
         additional_options: dict[str, str] | None = None,
+        *,
+        block_text: str | None = None,
     ) -> list[nodes.Element]:
         """Run a directive and return the generated nodes.
 
@@ -1809,6 +1827,8 @@ def run_directive(
         :param position: The line number of the first line
         :param additional_options: Additional options to add to the directive,
             above those parsed from the content.
+        :param block_text: The full unparsed text of the directive,
+            defaulting to ``content`` if not given.
 
         """
         self.document.current_line = position
@@ -1838,7 +1858,10 @@ def run_directive(
                 directive_class,
                 first_line,
                 content,
-                line=position,
+                # when no block_text is given, the content is synthesized
+                # (e.g. from HTML attributes) and has no source lines,
+                # so option warnings fall back to the directive's position
+                line=position if block_text is not None else None,
                 additional_options=additional_options,
             )
         except MarkupError as error:
@@ -1883,7 +1906,7 @@ def run_directive(
                 # the line offset of the first line of the content
                 content_offset=parsed.body_offset,
                 # a string containing the entire directive
-                block_text="\n".join(parsed.body),
+                block_text=content if block_text is None else block_text,
                 state=state,
                 state_machine=state_machine,
             )

myst_parser/mocking.py

@@ -451,7 +451,8 @@ def run(self) -> list[nodes.Element]:
             literal_block = nodes.literal_block(
                 file_content, source=str(path), classes=self.options.get("class", [])
             )
-            literal_block.line = 1  # TODO don;t think this should be 1?
+            # docutils sets this to the ``start-line`` option (or 0) + 1
+            literal_block.line = (self.options.get("start-line") or 0) + 1
             self.add_name(literal_block)
             if "number-lines" in self.options:
                 # note starting in docutils 0.22 this option is now an integer instead of a string, see: https://github.com/live-clones/docutils/commit/f39ac1413e56a330c8fea6e0d080fed0ff2b8483

myst_parser/parsers/directives.py

@@ -36,7 +36,6 @@
 
 from __future__ import annotations
 
-import re
 from collections.abc import Callable
 from dataclasses import dataclass
 from textwrap import dedent
@@ -50,7 +49,7 @@
 
 from myst_parser.warnings_ import MystWarnings
 
-from .options import TokenizeError, options_to_items
+from .options import TokenizeError, options_to_tokens
 
 YAML_LOAD_ERRORS = (yaml.YAMLError, ValueError, RecursionError)
 """Errors that ``yaml.safe_load`` can raise on invalid input:
@@ -76,7 +75,12 @@ class DirectiveParsingResult:
     body: list[str]
     """The lines of body content"""
     body_offset: int
-    """The number of lines to the start of the body content."""
+    """The number of lines from the directive's opening line
+    to the first line of the body content.
+
+    This is ``-1`` when body content starts on the opening line itself
+    (only possible for directives that take no arguments).
+    """
     warnings: list[ParseWarnings]
     """List of non-fatal errors encountered during parsing.
     (message, line_number)
@@ -144,7 +148,9 @@ def parse_directive_text(
                     )
                 )
             body_lines.insert(0, first_line)
-            content_offset = 0
+            # the merged first body line sits on the opening line itself,
+            # one line *before* where body content normally starts
+            content_offset -= 1
         arguments = []
     else:
         arguments = parse_directive_arguments(directive_class, first_line)
@@ -181,21 +187,40 @@ def _parse_directive_options(
 ) -> _DirectiveOptions:
     """Parse (and validate) the directive option section.
 
-    :returns: (content, options, validation_errors)
+    :param content: All text after the directive's opening line,
+        possibly starting with an option block
+    :param directive_class: The directive class to validate options against
+    :param as_yaml: Whether to parse the options block with the full YAML spec,
+        rather than validating against the directive's option specification
+        (used by myst-nb)
+    :param line: The 1-based line number of the directive's opening line,
+        or None if unknown
+    :param additional_options: Additional options for the directive,
+        which the options block takes priority over
     """
     options_block: None | str = None
+    options_position: int | None = None
+    """The 1-based source line of the first line of the options block."""
     if content.startswith("---"):
-        line = None if line is None else line + 1
-        content = "\n".join(content.splitlines()[1:])
-        match = re.search(r"^-{3,}", content, re.MULTILINE)
-        if match:
-            options_block = content[: match.start()]
-            content = content[match.end() + 1 :]  # TODO advance line number
+        options_position = None if line is None else line + 2
+        content_lines = content.splitlines()[1:]
+        for index, content_line in enumerate(content_lines):
+            if content_line.startswith("---"):
+                options_block = "\n".join(content_lines[:index])
+                # any text after the closing delimiter's dashes becomes
+                # the first line of the body content (and, being kept as
+                # its own line, maps exactly to its source line)
+                remainder = content_line.lstrip("-")
+                remainder = remainder.removeprefix(" ")
+                rest = content_lines[index + 1 :]
+                content = "\n".join([remainder, *rest] if remainder else rest)
+                break
         else:
-            options_block = content
+            options_block = "\n".join(content_lines)
             content = ""
         options_block = dedent(options_block)
     elif content.lstrip().startswith(":"):
+        options_position = None if line is None else line + 1
         content_lines = content.splitlines()
         yaml_lines = []
         while content_lines:
@@ -214,12 +239,19 @@ def _parse_directive_options(
         yaml_errors: list[ParseWarnings] = []
         try:
             yaml_options = yaml.safe_load(options_block or "") or {}
-        except YAML_LOAD_ERRORS:
+        except YAML_LOAD_ERRORS as exc:
             yaml_options = {}
+            yaml_error_line = options_position
+            if (
+                options_position is not None
+                and isinstance(exc, yaml.MarkedYAMLError)
+                and exc.problem_mark is not None
+            ):
+                yaml_error_line = options_position + exc.problem_mark.line
             yaml_errors.append(
                 ParseWarnings(
                     "Invalid options format (bad YAML)",
-                    line,
+                    yaml_error_line,
                     MystWarnings.DIRECTIVE_OPTION,
                 )
             )
@@ -228,7 +260,7 @@ def _parse_directive_options(
             yaml_errors.append(
                 ParseWarnings(
                     "Invalid options format (not a dict)",
-                    line,
+                    options_position,
                     MystWarnings.DIRECTIVE_OPTION,
                 )
             )
@@ -237,32 +269,53 @@ def _parse_directive_options(
     validation_errors: list[ParseWarnings] = []
 
     options: dict[str, str] = {}
+    option_lines: dict[str, int] = {}
+    """0-based line of each key, within the options block
+    (both block styles keep a 1:1 line correspondence with the source).
+    """
     if options_block is not None:
         try:
-            _options, state = options_to_items(options_block)
-            options = dict(_options)
+            option_tokens, state = options_to_tokens(options_block)
         except TokenizeError as err:
             return _DirectiveOptions(
                 content,
                 options,
                 [
                     ParseWarnings(
                         f"Invalid options format: {err.problem}",
-                        line,
+                        None
+                        if options_position is None
+                        else options_position + err.problem_mark.line,
                         MystWarnings.DIRECTIVE_OPTION,
                     )
                 ],
                 has_options_block,
             )
+        for key_token, value_token in option_tokens:
+            options[key_token.value] = (
+                value_token.value if value_token is not None else ""
+            )
+            option_lines[key_token.value] = key_token.start.line
         if state.has_comments:
             validation_errors.append(
                 ParseWarnings(
                     "Directive options has # comments, which may not be supported in future versions.",
-                    line,
+                    None
+                    if options_position is None
+                    else options_position
+                    + (state.comment_lines[0] if state.comment_lines else 0),
                     MystWarnings.DIRECTIVE_OPTION_COMMENTS,
                 )
             )
 
+    def _option_line(name: str) -> int | None:
+        """The 1-based source line of an option key, or None if unknown
+        (e.g. keys injected via ``additional_options`` have no source line).
+        """
+        if options_position is None or name not in option_lines:
+            return None
+        return options_position + option_lines[name]
+
     if issubclass(directive_class, TestDirective):
         # technically this directive spec only accepts one option ('option')
         # but since its for testing only we accept all options
@@ -274,14 +327,19 @@ def _parse_directive_options(
 
     # check options against spec
     options_spec: dict[str, Callable] = directive_class.option_spec
-    unknown_options: list[str] = []
     new_options: dict[str, Any] = {}
     value: str | None
     for name, value in options.items():
         try:
             converter = options_spec[name]
         except KeyError:
-            unknown_options.append(name)
+            validation_errors.append(
+                ParseWarnings(
+                    f"Unknown option key: {name!r} (allowed: {sorted(options_spec)})",
+                    _option_line(name),
+                    MystWarnings.DIRECTIVE_OPTION,
+                )
+            )
             continue
         if not value:
             # restructured text parses empty option values as None
@@ -296,23 +354,13 @@ def _parse_directive_options(
             validation_errors.append(
                 ParseWarnings(
                     f"Invalid option value for {name!r}: {value}: {error}",
-                    line,
+                    _option_line(name),
                     MystWarnings.DIRECTIVE_OPTION,
                 )
             )
         else:
             new_options[name] = converted_value
 
-    if unknown_options:
-        validation_errors.append(
-            ParseWarnings(
-                f"Unknown option keys: {sorted(unknown_options)} "
-                f"(allowed: {sorted(options_spec)})",
-                line,
-                MystWarnings.DIRECTIVE_OPTION,
-            )
-        )
-
     return _DirectiveOptions(content, new_options, validation_errors, has_options_block)