Add length to spans and diagnostics #9703
Conversation
LeonMatthes
force-pushed
the
span-diagnostics
branch
from
71b1f70 to
8818370
Compare
internal/compiler/diagnostics.rs
Outdated
| /// Returns a tuple with the line (starting at 1) and column number (starting at 1) | ||
| /// | ||
| /// Can also return (0, 0) if the span is invalid | ||
| pub fn end_line_column(&self) -> (usize, usize) { |
There was a problem hiding this comment.
This function and the next one is public API in the interpreter, so this will need API review.
I think the length is probably a non-brainer, but this function is less obvious. Can we do without it?
There was a problem hiding this comment.
It is meant to mirror the line_column function above, to expose the same data as line_column, but for the end of the diagnostic.
Is there otherwise a sensible way to pass this information to the syntax_tests without making it public API?
In theory it can be calculated from the length and the line_column by parsing the file on the syntax test side, but that's probably somewhat cumbersome.
There was a problem hiding this comment.
@ogoffart Updated to use a similar free function to line_column_with_format
LeonMatthes
force-pushed
the
span-diagnostics
branch
from
8818370 to
0dd8527
Compare
This allows diagnostics to cover not just a single character, but the entire relevant code region. ChangeLog: Diagnostics now indicate the range of the error/warning, not just the starting character
If the end of a diagnostic falls on a newline CR of a CRLF, it should cover the entire CRLF, not just the CR.
LeonMatthes
force-pushed
the
span-diagnostics
branch
from
0dd8527 to
93dced2
Compare
ogoffart
left a comment
ogoffart
left a comment
There was a problem hiding this comment.
Looks good, thanks.
Only one comment about the impl Display and then i think we can merge this.
I really think we shouldn't highlight the full element especially for the loop error. But this can be done in a follow up.
| let end_offset = self.span.offset + self.span.length.max(1); | ||
| let (end_line, end_col) = sf.line_column(end_offset, ByteFormat::Utf8); | ||
| write!(f, "{}:{line}:{col}-{end_line}:{end_col}", sf.path.display()) |
There was a problem hiding this comment.
I think i'd prefer if this stay unchanged.
This is used to print the location when diagnostics are formated into text and i think it shouldn't have the end. Most compiler put only one.
| // <^^^warning{The binding for the property 'layout-organized-data' is part of a binding loop (lay1.width -> lay1.layout-cache-h -> x -> col -> lay1.layout-organized-data -> lay1.layoutinfo-h -> root.layoutinfo-h).↵This was allowed in previous version of Slint, but is deprecated and may cause panic at runtime} | ||
| // <^^^^warning{The binding for the property 'layoutinfo-h' is part of a binding loop (lay1.width -> lay1.layout-cache-h -> x -> col -> lay1.layout-organized-data -> lay1.layoutinfo-h -> root.layoutinfo-h).↵This was allowed in previous version of Slint, but is deprecated and may cause panic at runtime} | ||
| } | ||
| //<<<warning{The binding for the property 'layoutinfo-h' is part of a binding loop (lay1.width -> lay1.layout-cache-h -> x -> col -> lay1.layout-organized-data -> lay1.layoutinfo-h -> root.layoutinfo-h).↵This was allowed in previous version of Slint, but is deprecated and may cause panic at runtime} |
There was a problem hiding this comment.
Is this really better to have spans that span the entire element. For me that's just noise. IMHO we should restrict this warning to only span the element name and possibly it's id.
| let pos = lsp_types::Position::new( | ||
| (span.0 as u32).saturating_sub(1), | ||
| (span.1 as u32).saturating_sub(1), | ||
| fn to_range(start: (usize, usize), end: (usize, usize)) -> lsp_types::Range { |
There was a problem hiding this comment.
(pre-existing nitpick: this function should ideally document what these tuple are. Like is it one-based or 0-based and things. But since it is only used once anyway, it perhaps should be inlined in the caller and would make everything more clear.)
3 participants
Previously, diagnostics only indicated the first character of a warning/error.
While this is often sufficient, in many cases it leads to ambiguity as to what exactly causes the error.
This patch updates the
Spantype to add a length field and exposes this data to the diagnostic builder in the compiler and lsp.The patch is only large, as the syntax tests all needed to be updated. This was done via the automatic update mechanism.
The actual code changes are pretty straight-forward.
Also let me know whether the new syntax of
> <for the syntax tests is acceptable.Compiler Diagnostics
Before:
After:
LSP (Screenshots from NVIM)
Before:
After:
