Fix the patterns for code blocks and the handling of newlines when matching patterns - #9921
Fix the patterns for code blocks and the handling of newlines when matching patterns#9921DanTup wants to merge 2 commits into
Conversation
…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
There was a problem hiding this comment.
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.
| bool scanOnCurrentLine(RegExp pattern) { | ||
| if (!matchesOnCurrentLine(pattern)) return false; | ||
|
|
||
| return scan(pattern); | ||
| } |
There was a problem hiding this comment.
[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
- Prioritize logic, performance on the UI thread, and architectural consistency. (link)
There was a problem hiding this comment.
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.
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