Repository navigation
fix: bound the rendering window when formatting an error line - #264
Open
minh-anm-nguyen wants to merge 1 commit into
Open
minh-anm-nguyen wants to merge 1 commit into
minh-anm-nguyen wants to merge 1 commit into
Conversation
formatLines() trims a window around the reported column when the offending line is longer than 40 characters. `end` was bounded above by the line's length, but `start` was only bounded below, so a column past the end of its line -- which is what gets reported when a token is left unterminated there -- indexed past the end of the string. Rendering a syntax error could therefore take the interpreter down instead of printing the error. Normalize the reported column once into a zero-based caret clamped to the line, where a caret equal to the line's length denotes the position just past the final character, and derive the window from it so that start <= caret <= end <= size() holds throughout. The window arithmetic still runs on the clamped one-based column, so the trimmed output stays identical for every input that already rendered correctly. Also return early for a null source, which Location::Empty() produces, and for line zero, which would reach one before the first line. Add a regression test that calls formatLines() directly rather than driving it through a malformed pattern, so the coverage does not quietly disappear if the lexer's column bookkeeping ever changes.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
formatLines()renders the offending source line with a caret under the reported column, trimming a window around it when the line is longer than 40 characters:endis bounded above by the line's length.startis bounded only below, bystart > 0, and never above.location.columncan legitimately exceed the length of the line it names, which is what gets reported when a token is left unterminated at the end of a line, and thenstartindexes past the end of the string.The effect is that a malformed pattern can take the interpreter down while it is building the message that would have explained the problem. Under a hardened standard library
std::string::operator[]trips its precondition and aborts; otherwise it is an out-of-range read.Two nearby locations reach for something that is not there either:
location.source == nullptr, whichLocation::Empty()produces, is dereferenced bylocation.source->content.location.line == 0reacheslines[location.line - 1], one before the first line.arrowPositionis also computed by unsigned subtraction in two places that can wrap, after whichstd::string(lineNumberPrefix.length() + arrowPosition, ' ')asks for an enormous string.The fix normalizes the reported column once into a zero-based caret clamped to the line, where a caret equal to the line's length denotes the position just past the final character, which is the right place for an end-of-input marker. The window is then derived so that
start <= caret <= end <= errorLine.size()holds throughout.The window arithmetic still runs on the clamped one-based column, so the trimmed output and the caret placement stay byte-identical for every input that already rendered correctly. Only the previously broken cases change.
The regression test calls
formatLines()directly rather than driving it through a malformed pattern. A pattern-driven test would depend on how the lexer numbers columns, and would quietly stop covering this code if that bookkeeping were ever tidied up.