Skip to content

fix(yaml) empty lines should not terminate block scalars - #4457

Open
pikammmmm wants to merge 2 commits into
highlightjs:mainfrom
pikammmmm:fix-yaml-block-scalar-empty-lines
Open

fix(yaml) empty lines should not terminate block scalars#4457
pikammmmm wants to merge 2 commits into
highlightjs:mainfrom
pikammmmm:fix-yaml-block-scalar-empty-lines

Conversation

@pikammmmm

Copy link
Copy Markdown

Fixes #4090.

The problem

A YAML block scalar stops being highlighted at the first empty line, and — worse — the remainder of the block is then re-parsed as YAML, so string content turns into keys.

Using the sample from the issue:

foo:
  bar: |
    still: a string

    not: anymore

not: anymore is plain text inside the string, but it is emitted as hljs-attr (a key). On a real-world file (chroma's .github/ISSUE_TEMPLATE/bug_report.yaml) the paragraph after the blank line is broken up into individual words, each wrapped in its own hljs-string span.

Cause

The multi-line string mode has only a begin regex and no end, so the entire highlighted span is whatever begin matches. Continuation lines had to match:

\2[^\n]+

that is, the block indent followed by at least one more character. An empty line has nothing after the indent, so it can never match and the block ends there.

This is observable: a "blank" line containing more spaces than the block indent works today, while a truly empty line, a shorter one, or one exactly as wide as the indent all terminate the block early.

The change

Empty lines (and lines containing only spaces) are now accepted as block content, both immediately after the header and between content lines:

-begin: '[\\|>]([1-9]?[+-])?[ ]*\\n( +)[^ ][^\\n]*\\n(\\2[^\\n]+\\n?)*'
+begin: '[\\|>]([1-9]?[+-])?[ ]*\\n(?:[ ]*\\n)*( +)[^ ][^\\n]*\\n((?:[ ]*\\n)*\\2[^\\n]+\\n?)*'

Two things were deliberate:

  • Blank lines are only consumed when another indented line follows. A blank line that separates the block from the next key is left outside the span, so that key still highlights as a key. The new fixture asserts this negative case explicitly (trailing-blank).
  • Both added groups are non-capturing, so the \2 backreference still refers to the indent. The capture count is unchanged at 3.

Because both additions are optional (*), the new pattern is a strict superset of the old one — it can only ever extend a match, never shrink or relocate one.

Testing

New fixture test/markup/yaml/block_empty_lines.txt covers the issue's own sample, a folded > scalar, a blank line right after the header, a spaces-only blank line, consecutive blank lines, and the trailing-blank guard described above.

  • Full suite with the change: 1594 passing, 0 failing
  • Full suite with the change reverted: 1593 passing, 1 failing — the sole failure is the new fixture, confirming it actually covers the bug
  • Differential check over 400 real-world .yaml/.yml files on disk, old build vs new: the underlying text is byte-identical in every file (markup-only changes), 37 files differ, and in all 37 the string coverage grew — no file lost coverage and no file gained a spurious key

Known limitations (pre-existing, not addressed here)

These were already broken before this change and are unchanged by it:

  • CRLF line endings[ ]*\n cannot cross the \r, so block scalars in CRLF files remain broken.
  • Blank lines containing a tab still terminate the block.

Both would need separate work; I kept this patch to the empty-line case in the issue.

Block scalar continuation lines had to match `\2[^\n]+` - the block
indent followed by at least one more character - so any genuinely empty
line ended the block early. The remainder of the scalar was then
re-parsed as YAML, turning string content into keys.

Empty lines (and lines of only spaces) are now treated as block content,
both after the header and between content lines. Blank lines are only
consumed when another indented line follows, so a blank line separating
the block from the next key is left alone.

Fixes highlightjs#4090
@joshgoebel

Copy link
Copy Markdown
Member

CRLF line endings — [ ]*\n cannot cross the \r, so block scalars in CRLF files remain broken.

What is the impact here with regard to #3298?

Will that help?

Comment thread src/languages/yaml.js
// follows, so that blank lines trailing the block are left alone.
className: 'string',
begin: '[\\|>]([1-9]?[+-])?[ ]*\\n( +)[^ ][^\\n]*\\n(\\2[^\\n]+\\n?)*'
begin: '[\\|>]([1-9]?[+-])?[ ]*\\n(?:[ ]*\\n)*( +)[^ ][^\\n]*\\n((?:[ ]*\\n)*\\2[^\\n]+\\n?)*'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you added non-capturing? Generally (other than multi-match) our engine doesn't care about capturing or not... is this intended to be a performance improvement or were you just not aware that "capture doesn't matter"?

I wonder if we could break this into multiple lines/regex (with comments) and use our regex.concact to build the final regex... it's getting a little hard to follow.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required here, not perf: \2 has to keep pointing at the indent group ( +). If the new groups capture, the indent becomes \3 and \2 refers to a group that can match zero times, so \2[^\n]+ degrades to "any line" and the block swallows what follows it — append baz: quux to the test sample and it gets pulled into the string.

Happy to rebuild it as commented pieces via regex.concat — say the word and I'll push that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, duh. :-)

say the word and I'll push that.

word.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(YAML) Multiline strings don't support empty lines

2 participants