Add length to spans and diagnostics

docs/testing.md

@@ -10,14 +10,14 @@ The syntax tests are testing that the compiler show the right error messages in
 The syntax tests are located in [internal/compiler/tests/syntax/](../internal/compiler/tests/syntax/) and it's driven by the
 [`syntax_tests.rs`](../internal/compiler/tests/syntax_tests.rs) file. More info in the comments of that file.
 
-In summary, each .slint files have comments with `^error` like so:
+In summary, each .slint files have comments with `> <error` like so:
 
 ```ignore
 foo bar
-//  ^error{parse error}
+//  > <error{parse error}
 ```
 
-Meaning that there must be an error on the line above at the location pointed by the caret.
+Meaning that there must be an error on the line above spanning `bar`, as indicated by the `>` and `<` arrows.
 
 Ideally, each error message must be tested like so.
 

internal/compiler/diagnostics.rs

@@ -10,12 +10,13 @@ use std::collections::BTreeSet;
 
 /// Span represent an error location within a file.
 ///
-/// Currently, it is just an offset in byte within the file.
+/// Currently, it is just an offset in byte within the file + the corresponding length.
 ///
 /// When the `proc_macro_span` feature is enabled, it may also hold a proc_macro span.
 #[derive(Debug, Clone)]
 pub struct Span {
     pub offset: usize,
+    pub length: usize,
     #[cfg(feature = "proc_macro_span")]
     pub span: Option<proc_macro::Span>,
 }
@@ -26,15 +27,16 @@ impl Span {
     }
 
     #[allow(clippy::needless_update)] // needed when `proc_macro_span` is enabled
-    pub fn new(offset: usize) -> Self {
-        Self { offset, ..Default::default() }
+    pub fn new(offset: usize, length: usize) -> Self {
+        Self { offset, length, ..Default::default() }
     }
 }
 
 impl Default for Span {
     fn default() -> Self {
         Span {
             offset: usize::MAX,
+            length: 0,
             #[cfg(feature = "proc_macro_span")]
             span: Default::default(),
         }
@@ -43,7 +45,7 @@ impl Default for Span {
 
 impl PartialEq for Span {
     fn eq(&self, other: &Span) -> bool {
-        self.offset == other.offset
+        self.offset == other.offset && self.length == other.length
     }
 }
 
@@ -297,6 +299,16 @@ impl Diagnostic {
         }
     }
 
+    /// Return the length of this diagnostic in characters.
+    pub fn length(&self) -> usize {
+        // The length should always be at least 1, even if the span indicates a
+        // length of 0, as otherwise there is no character to display the diagnostic on.
+        self.span.span.length.max(1)
+    }
+
+    // NOTE: The return-type differs from the Spanned trait.
+    // Because this is public API (Diagnostic is re-exported by the Interpreter), we cannot change
+    // this.
     /// return the path of the source file where this error is attached
     pub fn source_file(&self) -> Option<&Path> {
         self.span.source_file().map(|sf| sf.path())
@@ -333,6 +345,19 @@ pub fn diagnostic_line_column_with_format(
     sf.line_column(diagnostic.span.span.offset, format)
 }
 
+pub fn diagnostic_end_line_column_with_format(
+    diagnostic: &Diagnostic,
+    format: ByteFormat,
+) -> (usize, usize) {
+    let Some(sf) = &diagnostic.span.source_file else { return (0, 0) };
+    // The end_line_column is exclusive.
+    // Even if the span indicates a length of 0, the diagnostic should always
+    // return an end_line_column that is at least one offset further.
+    // Diagnostic::length ensures this.
+    let offset = diagnostic.span.span.offset + diagnostic.length();
+    sf.line_column(offset, format)
+}
+
 #[derive(Default)]
 pub struct BuildDiagnostics {
     inner: Vec<Diagnostic>,
@@ -432,11 +457,15 @@ impl BuildDiagnostics {
                     annotate_snippets::Group::with_title(message)
                 } else if let Some(sf) = &d.span.source_file {
                     if let Some(source) = &sf.source {
-                        let o = d.span.span.offset;
+                        let start_offset = d.span.span.offset;
+                        let end_offset = d.span.span.offset + d.length();
                         message.element(
                             annotate_snippets::Snippet::source(source)
                                 .path(sf.path.to_string_lossy())
-                                .annotation(annotate_snippets::AnnotationKind::Primary.span(o..o)),
+                                .annotation(
+                                    annotate_snippets::AnnotationKind::Primary
+                                        .span(start_offset..end_offset),
+                                ),
                         )
                     } else {
                         if let Some(ref mut handle_no_source) = handle_no_source {

internal/compiler/lexer.rs

@@ -200,6 +200,7 @@ pub fn lex(mut source: &str) -> Vec<crate::parser::Token> {
             kind: SyntaxKind::Whitespace,
             text: source[..3].into(),
             offset: 0,
+            length: 3,
             ..Default::default()
         });
         source = &source[3..];
@@ -211,6 +212,7 @@ pub fn lex(mut source: &str) -> Vec<crate::parser::Token> {
                 kind,
                 text: source[..len].into(),
                 offset,
+                length: len,
                 ..Default::default()
             });
             offset += len;

internal/compiler/object_tree.rs

@@ -691,7 +691,7 @@ impl GeometryProps {
 
 pub type BindingsMap = BTreeMap<SmolStr, RefCell<BindingExpression>>;
 
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct ElementDebugInfo {
     // The id qualified with the enclosing component name. Given `foo := Bar {}` this is `EnclosingComponent::foo`
     pub qualified_id: Option<SmolStr>,
@@ -799,7 +799,16 @@ pub struct Element {
 
 impl Spanned for Element {
     fn span(&self) -> crate::diagnostics::Span {
-        self.debug.first().map(|n| n.node.span()).unwrap_or_default()
+        self.debug
+            .first()
+            .map(|n| {
+                // If possible, only span the qualified name of the Element (i.e. the `MyElement`
+                // part of `MyElement { ... }`, as otherwise the span can get very large, which
+                // isn't useful for showing diagnostics.
+                // Only use the full span as the fallback.
+                n.node.QualifiedName().as_ref().map(Spanned::span).unwrap_or_else(|| n.node.span())
+            })
+            .unwrap_or_default()
     }
 
     fn source_file(&self) -> Option<&crate::diagnostics::SourceFile> {

internal/compiler/parser.rs

@@ -464,6 +464,7 @@ pub struct Token {
     pub kind: SyntaxKind,
     pub text: SmolStr,
     pub offset: usize,
+    pub length: usize,
     #[cfg(feature = "proc_macro_span")]
     pub span: Option<proc_macro::Span>,
 }
@@ -474,6 +475,7 @@ impl Default for Token {
             kind: SyntaxKind::Eof,
             text: Default::default(),
             offset: 0,
+            length: 0,
             #[cfg(feature = "proc_macro_span")]
             span: None,
         }
@@ -693,7 +695,7 @@ impl Parser for DefaultParser<'_> {
     fn error(&mut self, e: impl Into<String>) {
         let current_token = self.current_token();
         #[allow(unused_mut)]
-        let mut span = crate::diagnostics::Span::new(current_token.offset);
+        let mut span = crate::diagnostics::Span::new(current_token.offset, current_token.length);
         #[cfg(feature = "proc_macro_span")]
         {
             span.span = current_token.span;
@@ -712,7 +714,7 @@ impl Parser for DefaultParser<'_> {
     fn warning(&mut self, e: impl Into<String>) {
         let current_token = self.current_token();
         #[allow(unused_mut)]
-        let mut span = crate::diagnostics::Span::new(current_token.offset);
+        let mut span = crate::diagnostics::Span::new(current_token.offset, current_token.length);
         #[cfg(feature = "proc_macro_span")]
         {
             span.span = current_token.span;
@@ -947,7 +949,8 @@ impl NodeOrToken {
 
 impl Spanned for SyntaxNode {
     fn span(&self) -> crate::diagnostics::Span {
-        crate::diagnostics::Span::new(self.node.text_range().start().into())
+        let range = self.node.text_range();
+        crate::diagnostics::Span::new(range.start().into(), range.len().into())
     }
 
     fn source_file(&self) -> Option<&SourceFile> {
@@ -967,7 +970,8 @@ impl Spanned for Option<SyntaxNode> {
 
 impl Spanned for SyntaxToken {
     fn span(&self) -> crate::diagnostics::Span {
-        crate::diagnostics::Span::new(self.token.text_range().start().into())
+        let range = self.token.text_range();
+        crate::diagnostics::Span::new(range.start().into(), range.len().into())
     }
 
     fn source_file(&self) -> Option<&SourceFile> {

internal/compiler/tests/syntax/accessibility/accessible_properties.slint

@@ -2,41 +2,41 @@
 // SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
 
 Button1 := Rectangle {
-//      ^warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
+//      ><warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
     property <bool> cond;
     accessible-role: cond ? button : AccessibleRole.text;
-//                   ^error{The `accessible-role` property must be a constant expression}
+//                   >                                  <error{The `accessible-role` property must be a constant expression}
 
     rr := Rectangle {
         init => {
             self.accessible-role = AccessibleRole.text;
-//          ^error{The property must be known at compile time and cannot be changed at runtime}
+//          >                                        <error{The property must be known at compile time and cannot be changed at runtime}
         }
     }
 }
 
 Button2 := Rectangle {
-//      ^warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
+//      ><warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
     accessible-label: "the button";
-//                    ^error{The `accessible-label` property can only be set in combination to `accessible-role`}
+//                    >           <error{The `accessible-label` property can only be set in combination to `accessible-role`}
 }
 
 Button3 := Rectangle {
-//      ^warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
+//      ><warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
     Rectangle {
         accessible-role: text;
         accessible-label: "the button";
     }
 }
 
 export Test := Window {
-//          ^warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
+//          ><warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
 
     Button1 { }
     Button1 { accessible-description: "ok"; } // ok because Button1 has a role
     Button2 { accessible-role: none; }
     Button2 { }
     Button3 {}
     Button3 { accessible-description: "error";}
-//                                    ^error{The `accessible-description` property can only be set in combination to `accessible-role`}
+//                                    >      <error{The `accessible-description` property can only be set in combination to `accessible-role`}
 }

internal/compiler/tests/syntax/analysis/binding_loop1.slint

@@ -3,11 +3,11 @@
 
 
 WithStates := Rectangle {
-//         ^warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
+//         ><warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
     property <brush> extra_background;
     property <bool> condition;
     background: yellow; //FIXME: ideally we'd keep the span within the state
-//              ^error{The binding for the property 'background' is part of a binding loop (extra-background -> background)}
+//              >     <error{The binding for the property 'background' is part of a binding loop (extra-background -> background)}
     states [
         xxx when condition : {
             background: extra_background;
@@ -16,32 +16,32 @@ WithStates := Rectangle {
 }
 
 export Test := Rectangle {
-//          ^warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
+//          ><warning{':=' to declare a component is deprecated. The new syntax declare components with 'component MyComponent {'. Read the documentation for more info}
 
     property <int> a: 45 + root.b;
-//                    ^error{The binding for the property 'a' is part of a binding loop (root_window.d -> root_window.c -> root_window.b -> root_window.a)}
+//                    >          <error{The binding for the property 'a' is part of a binding loop (root_window.d -> root_window.c -> root_window.b -> root_window.a)}
     property <float> b: root.c;
-//                      ^error{The binding for the property 'b' is part of a binding loop (root_window.d -> root_window.c -> root_window.b -> root_window.a)}
+//                      >     <error{The binding for the property 'b' is part of a binding loop (root_window.d -> root_window.c -> root_window.b -> root_window.a)}
     property <int> c <=> d;
-//                   ^error{The binding for the property 'c' is part of a binding loop (root_window.d -> root_window.c -> root_window.b -> root_window.a)}
+//                   >    <error{The binding for the property 'c' is part of a binding loop (root_window.d -> root_window.c -> root_window.b -> root_window.a)}
     property <int> d: root.a + root.e;
-//                    ^error{The binding for the property 'd' is part of a binding loop (root_window.d -> root_window.c -> root_window.b -> root_window.a)}
+//                    >              <error{The binding for the property 'd' is part of a binding loop (root_window.d -> root_window.c -> root_window.b -> root_window.a)}
     property <int> e: root.b;
-//                    ^error{The binding for the property 'e' is part of a binding loop (root_window.e -> root_window.d -> root_window.c -> root_window.b)}
+//                    >     <error{The binding for the property 'e' is part of a binding loop (root_window.e -> root_window.d -> root_window.c -> root_window.b)}
     property <int> w: root.a + root.b; // This id not part of a loop
 
     property<bool> cond: xx.x == 0;
-//                       ^error{The binding for the property 'cond' is part of a binding loop (xx.y -> xx.x -> root_window.cond)}
+//                       >        <error{The binding for the property 'cond' is part of a binding loop (xx.y -> xx.x -> root_window.cond)}
 
     xx := Rectangle {
         x: y;
-//         ^error{The binding for the property 'x' is part of a binding loop (xx.y -> xx.x -> root_window.cond)}
+//         ><error{The binding for the property 'x' is part of a binding loop (xx.y -> xx.x -> root_window.cond)}
         y: root.cond ? 42px : 55px;
-//         ^error{The binding for the property 'y' is part of a binding loop (xx.y -> xx.x -> root_window.cond)}
+//         >                      <error{The binding for the property 'y' is part of a binding loop (xx.y -> xx.x -> root_window.cond)}
     }
 
     WithStates {
         extra_background: background;
-//                        ^error{The binding for the property 'extra-background' is part of a binding loop (extra-background -> background)}
+//                        >         <error{The binding for the property 'extra-background' is part of a binding loop (extra-background -> background)}
     }
 }