command/views/json: Never generate invalid diagnostic snippet offsets#29048
Merged
apparentlymart merged 1 commit intomainfrom Jun 28, 2021
Merged
command/views/json: Never generate invalid diagnostic snippet offsets#29048apparentlymart merged 1 commit intomainfrom
apparentlymart merged 1 commit intomainfrom
Conversation
Because our snippet generator is trying to select whole lines to include in the snippet, it has some edge cases for odd situations where the relevant source range starts or ends directly at a newline, which were previously causing this logic to return out-of-bounds offsets into the code snippet string. Although arguably it'd be better for the original diagnostics to report more reasonable source ranges, it's better for us to report a slightly-inaccurate snippet than to crash altogether, and so we'll extend our existing range checks to check both bounds of the string and thus avoid downstreams having to deal with out-of-bounds indices. For completeness here I also added some similar logic to the human-oriented diagnostic formatter, which consumes the result of the JSON diagnostic builder. That's not really needed with the additional checks in the JSON diagnostic builder, but it's nice to reinforce that this code can't panic (in this way, at least) even if its input isn't valid.
mildwonkey
approved these changes
Jun 28, 2021
Contributor
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Because our snippet generator is trying to select whole lines to include in the snippet, it has some edge cases for odd situations where the relevant source range starts or ends directly at a newline, which were previously causing this logic to return out-of-bounds offsets into the code snippet string.
Newlines at the bounds of source ranges are inevitably a bit of an edge case when thinking about the line/column representation of positions rather than byte offsets, because newlines don't really exist as far as line/column representation is concerned: they are the boundary between the lines, not part of the lines themselves, and they therefore don't really have a real "column". Therefore we somewhat arbitrarily just decide to shift the range over to the start of the following line instead when presenting it, so we can still show something even if it's not exactly what the parser was gesturing at.
For completeness here I also added some similar logic to the human-oriented diagnostic formatter, which consumes the result of the JSON diagnostic builder. That's not really needed with the additional checks in the JSON diagnostic builder, but it's nice to reinforce that this code can't panic (in this way, at least) even if its input isn't valid.
This fixes #29041, making it no longer crash.
Arguably the source range of the error it is trying to return is also a bit off, but syntax errors are often like that because the HCL parser is just making a best guess about what the author's intention was. In any case if we want to improve that we'd be doing it over in HCL rather than in Terraform, so I've focused just on avoiding a crash here.