SpannedError: Store position_start and position_end #569
Conversation
| @@ -78,7 +78,8 @@ impl<'de> Deserializer<'de> { | |||
|
|
|||
| Err(SpannedError { | |||
| code: err.into(), | |||
| position: Position::from_src_end(valid_input), | |||
| position_start: Position { line: 1, col: 1 }, | |||
There was a problem hiding this comment.
I was wondering if we should store the span as a range ... though with what range? I don't think we're fully consistent yet with where the error ranges start and stop, so we could use this opportunity to properly define if the start and end are open or closed
There was a problem hiding this comment.
Regarding precise range positions: SpannedError::position was previously defined as the line/col position of Parser.cursor at the point where the parser encountered an error. This ends up being at the end of the offending bytes, so we set SpannedError::position_end to this same line/col position. (Just thinking out loud...)
We compute position_start by storing a second cursor within the Parser, prev_cursor, and we set it to the old cursor value whenever we advance_bytes:
I think this is pretty logical in terms of the precise placement of ranges, what do you reckon?
There was a problem hiding this comment.
That seems fine, I was wondering more about what kind of range to use. For an error, it seems clear that the start should be inclusive. Do we want the end to be inclusive or exclusive? Perhaps the error could be represented using e.g. std::ops::Range (start..end).
| @@ -68,22 +68,25 @@ fn test_struct() { | |||
| "NewType", | |||
| Err(SpannedError { | |||
| code: Error::ExpectedNamedStructLike("NewType"), | |||
| position: Position { line: 1, col: 8 }, | |||
| position_start: Position { line: 1, col: 1 }, | |||
There was a problem hiding this comment.
Verifying that these spans make sense is starting to get quite tricky. Perhaps we could add a helper that formats a simple highlight e.g.
A(42: "hi")
^^
or that just extracts the substring of the error, here 42, so we can better visually check if the error spans are correct
There was a problem hiding this comment.
Since the error doesn't store the source, it could probably be a test-only helper
|
Thanks @pfnsec for working on this feature! I like the general idea and mostly had some small design questions and thoughts on how we might be able to better test the spans. |
Co-authored-by: Juniper Tyree <50025784+juntyr@users.noreply.github.com>
pfnsec
force-pushed
the
feat/spanned-error-range
branch
from
2373f64 to
ccb2de8
Compare
pfnsec
force-pushed
the
feat/spanned-error-range
branch
from
ccb2de8 to
c6fbefb
Compare
egads! :P |
|
@pfnsec Perhaps you could add a cargo clean between each step of the CI check job? |
|
I meant further above, lines 77, 80, and 85 where the check, clippy, and test steps are. Could you add a clean step between those? |
pfnsec
force-pushed
the
feat/spanned-error-range
branch
from
6478a11 to
7eda5b6
Compare
|
Ah, my bad. Like this? |
CHANGELOG.mdHi! We're working on basic language server support for RON, both for the core syntax and as a small lib for easily bringing up custom language servers based on custom Rust types. In order to support this, we propose storing the error position in
SpannedErroras a start and end position, instead of a single position. This PR implements all the necessary logic for this proposal, and fixes all tests to validate correctly against the newSpannedErrorformat.Here's what our budding language server work looks like with our PR:
Note that this PR doesn't include the language server work itself.
In theory, this should make the modified unit tests more robust also, since they now validate against 2 positions instead of just 1.
I think it would be better to merge #567 before this PR if approved, and I'll go in and double check my changes against the new HEAD and make sure all of the tests are still passing.
P.S. thanks for RON, it's great!