Skip to content

Fix the patterns for code blocks and the handling of newlines when matching patterns - #9921

Open
DanTup wants to merge 2 commits into
flutter:masterfrom
DanTup:fix-span-parser-newline-handling
Open

Fix the patterns for code blocks and the handling of newlines when matching patterns#9921
DanTup wants to merge 2 commits into
flutter:masterfrom
DanTup:fix-span-parser-newline-handling

Conversation

@DanTup

@DanTup DanTup commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

TextMate grammars are matched by-line and therefore can never match \n. However our Dart implementation allowed this, which resulted in some bugs when code blocks are not closed.

This updates the grammar to instead use $ to match the end of the line, and updates the parser to only ever match on the current line and not consumer the newline. With this change, the result matches the output of the TypeScript implementation used for testing in Dart-Code.

The source PR for this version of the grammar is at dart-lang/dart-syntax-highlight#91 (I'm filing these together because one repo owns the syntax and one owns the parser).

The one change to a golden file is expected, the /// shouldn't have been classed as code/variable.

See Dart-Code/Dart-Code#6130
See Dart-Code/Dart-Code#6131

…tching patterns

TextMate grammars are matched by-line and therefore can never match `\n`. However our Dart implementation allowed this, which resulted in some bugs when code blocks are not closed.

This updates the grammar to instead use $ to match the end of the line, and updates the parser to only ever match on the current line and not consumer the newline. With this change, the result matches the output of the TypeScript implementation used for testing in Dart-Code.

The source PR for this version of the grammar is at dart-lang/dart-syntax-highlight#91 (I'm filing these together because one repo owns the syntax and one owns the parser).

See Dart-Code/Dart-Code#6130
See Dart-Code/Dart-Code#6131

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Dart syntax highlighting parser to match regular expressions on a single line at a time, preventing patterns from consuming text across line breaks. This is implemented by introducing matchesOnCurrentLine and scanOnCurrentLine helper methods on LineScanner. The review feedback suggests optimizing scanOnCurrentLine to avoid a redundant regular expression match by directly advancing the scanner's position using lastMatch!.end to improve UI thread performance.

Comment on lines +788 to +792
bool scanOnCurrentLine(RegExp pattern) {
if (!matchesOnCurrentLine(pattern)) return false;

return scan(pattern);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

[CONCERN] In scanOnCurrentLine, calling scan(pattern) after matchesOnCurrentLine(pattern) causes the regular expression to be matched twice. Since matchesOnCurrentLine already performs the match and updates lastMatch, we can directly advance the scanner's position to lastMatch!.end to avoid this redundant overhead.

This is important for maintaining high performance on the UI thread during syntax highlighting.

  bool scanOnCurrentLine(RegExp pattern) {
    if (!matchesOnCurrentLine(pattern)) return false;

    position = lastMatch!.end;
    return true;
  }
References
  1. Prioritize logic, performance on the UI thread, and architectural consistency. (link)

@DanTup DanTup Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work, it causes a lot of test failures. My guess is that it's because _column is also tracking the position via some whitespace-handling in scan(). Unless there's evidence of bad performance, I think we should try to avoid duplicating that logic here.

@DanTup

DanTup commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! I'll wait for dart-lang/dart-syntax-highlight#91 to be reviewed before landing this, in case there are any comments to address there (this PRs effectively contain the same change.. both repos have the grammar + the parser).

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.

2 participants