Enhance diagnostic formatter to include trivia between highlight nodes #2242
Conversation
- Separate logic for calculating highlight ranges from `DiagnosticsFormatter` into `SyntaxHighlightRangeCalculator`. - Introduce an `ExtendedHighlight` private struct to manage trivia settings for each highlight. - Extend logic to consider leading and trailing trivia when highlighting consecutive nodes. - The `computeHighlightRanges` method now takes into account special cases for trivia between consecutive highlight nodes. - Add unit tests to cover all paths in `SyntaxHighlightRangeCalculator`. - Add documentation about trivia handling, explaining the conditions under which trivia should be included or excluded.
| /// Flag to indicate whether trailing trivia should be included in the highlight range. | ||
| var afterTrailingTrivia = false |
There was a problem hiding this comment.
I would prefer this to be beforeTrailingTrivia because that matches the positionBeforeTrailingTrivia method.
| /// | ||
| /// - Note: | ||
| /// 1. If the highlight is the first in the line, its leading trivia is always excluded. | ||
| /// 2. If a highlight's end aligns with the start of the next highlight, their shared trivia settings are adjusted. |
There was a problem hiding this comment.
| /// 2. If a highlight's end aligns with the start of the next highlight, their shared trivia settings are adjusted. | |
| /// 2. If a highlight's end aligns with the start of the next highlight, the trivia in between them is also highlighted. |
| let firstIndex = extendedHighlights.startIndex | ||
| let lastIndex = extendedHighlights.index(before: extendedHighlights.endIndex) | ||
|
|
||
| // Special case: The first highlight should always include leading trivia. |
There was a problem hiding this comment.
| // Special case: The first highlight should always include leading trivia. | |
| // Special case: The first highlight should always exclude leading trivia. |
| afterLeadingTrivia = true | ||
| } | ||
|
|
||
| // Special case: The last highlight should never include trailing trivia. |
There was a problem hiding this comment.
| // Special case: The last highlight should never include trailing trivia. | |
| // Special case: The last highlight should always exclude trailing trivia. |
| // Initialize an array of extended highlights, based on provided Syntax highlights. | ||
| var extendedHighlights = annotatedSourceLine.highlights.map { ExtendedHighlight(highlight: $0) } | ||
|
|
||
| // Loop through each extended highlight to fine-tune its trivia settings. | ||
| for extendedHighlightsIndex in extendedHighlights.indices { | ||
| adjustTriviaSettingsForHighlight( | ||
| atIndex: extendedHighlightsIndex, | ||
| in: &extendedHighlights, | ||
| usingSourceLocationConverter: sourceLocationConverter | ||
| ) | ||
| } |
There was a problem hiding this comment.
I am not a fan of creating the extendedHighlights array and then fixing it up later. What I would do instead is:
Iterate over the highlighted nodes. If the node’s positionBeforeTrailingTrivia is equal to the last node’s positionAfterTrailingTrivia, extend the last highlight range in the array. If not, add a new entry to it.
| at lineNumber: Int, | ||
| fromTree tree: some SyntaxProtocol, | ||
| usingSourceLocationConverter sourceLocationConverter: SourceLocationConverter | ||
| ) -> [Range<Int>] { |
There was a problem hiding this comment.
I’m not really a fan of how this takes a line number and returns an array of Ints because it’s ambiguous whether these integers are offsets within the source string or the line and whether we are counting in UTF-8 (str.utf8.count), UTF-16 (str.utf16.count) or grapheme clusters (str.count).
I would prefer if this method didn’t take any line number and instead returned [Range<AbsolutePosition>]. IMO that’s the most generic way to write this function and converting the positions to [Range<SourceLocation>] and then filtering out the highlights that aren’t relevant for the current line.
Or am I missing something?
| "@PropertyWrapper private(set) var variable: Decimal = 123 * 456" | ||
| // 123456789012345678901234567890 |
There was a problem hiding this comment.
Instead of adding this comment, what do you think about changing assertSyntaxHighlightRanges to actually underline the highlighted ranges? Either using a DiagnosticDecorator or using a hand-rolled function? I just think that it would make reading the tests a bit easier.
DiagnosticsFormatterintoSyntaxHighlightRangeCalculator.ExtendedHighlightprivate struct to manage trivia settings for each highlight.computeHighlightRangesmethod now takes into account special cases for trivia between consecutive highlight nodes.SyntaxHighlightRangeCalculator.Resolves #2208