Repository: brendanzab/codespan (rust)

Issue:
File column/line should consistently point to the first primary label instead of the first label
Currently when you have a diagnostic, and that diagnostic contains 2 labels, say, one secondary at `1:1` and one primary at `6:3`, the column/line at the top of the diagnostic on rich rendering shows `1:1`, this should instead point to the first primary label, which seems to be the case for short rendering. When you use short rendering it points to `6:3`, this should be made consistent across rendering types.

Pull request description:
locus at first primary label
This should resolve #259.

This solution only works if the first primary label occurs before any secondary labels which appear earlier in the source code. This would e.g. also work if all primary labels were added before the secondary labels. I think that is more or less the convention? 

If this behaviour is adopted, it should be documented somewhere. Where would that best be?

Alternatively the labels could be sorted before being processed. But I think that might be a performance hit.

Patch:
diff --git a/codespan-lsp/src/lib.rs b/codespan-lsp/src/lib.rs
index 26f3937..6e79f84 100644
--- a/codespan-lsp/src/lib.rs
+++ b/codespan-lsp/src/lib.rs
@@ -229,8 +229,9 @@ where
     let source = source.as_ref();
 
     let line_span = files.line_range(file_id, position.line as usize).unwrap();
+    let line_str = source.get(line_span.clone()).unwrap();
 
-    let byte_offset = character_to_line_offset(source, position.character)?;
+    let byte_offset = character_to_line_offset(line_str, position.character)?;
 
     Ok(line_span.start + byte_offset)
 }
@@ -314,7 +315,8 @@ test
     #[test]
     fn unicode_get_position() {
         let mut files = SimpleFiles::new();
-        let file_id = files.add("unicode", UNICODE);
+        let file_id = files.add("unicode", UNICODE.to_string());
+        let file_id2 = files.add("unicode newline", "\n".to_string() + UNICODE);
 
         let result = byte_index_to_position(&files, file_id, 5);
         assert_eq!(
@@ -333,5 +335,14 @@ test
                 character: 6,
             })
         );
+
+        let result = byte_index_to_position(&files, file_id2, 11);
+        assert_eq!(
+            result,
+            Ok(LspPosition {
+                line: 1,
+                character: 6,
+            })
+        );
     }
 }
diff --git a/codespan-reporting/assets/readme_preview.svg b/codespan-reporting/assets/readme_preview.svg
index 5fd415e..b06a903 100644
--- a/codespan-reporting/assets/readme_preview.svg
+++ b/codespan-reporting/assets/readme_preview.svg
@@ -52,7 +52,7 @@
   <foreignObject x="0" y="0" width="882" height="335">
     <div xmlns="http://www.w3.org/1999/xhtml">
       <pre><span class="fg red bold bright">error[E0308]</span><span class="bold bright">: `case` clauses have incompatible types</span>
-   <span class="fg blue">┌─</span> FizzBuzz.fun:10:15
+   <span class="fg blue">┌─</span> FizzBuzz.fun:16:16
    <span class="fg blue">│</span>  
 <span class="fg blue">10</span> <span class="fg blue">│</span>   fizz₂ : Nat → String
    <span class="fg blue">│</span>                 <span class="fg blue">------</span> <span class="fg blue">expected type `String` found here</span>
diff --git a/codespan-reporting/examples/readme_preview.rs b/codespan-reporting/examples/readme_preview.rs
index aa3d8cf..2cf96f6 100644
--- a/codespan-reporting/examples/readme_preview.rs
+++ b/codespan-reporting/examples/readme_preview.rs
@@ -81,7 +81,10 @@ fn main() -> anyhow::Result<()> {
         Opts::Svg => {
             let mut buffer = Vec::new();
             let mut writer = HtmlEscapeWriter::new(SvgWriter::new(&mut buffer));
-            let config = codespan_reporting::term::Config::default();
+            let config = codespan_reporting::term::Config {
+                styles: codespan_reporting::term::Styles::with_blue(Color::Blue),
+                ..codespan_reporting::term::Config::default()
+            };
 
             for diagnostic in &diagnostics {
                 term::emit(&mut writer, &config, &file, &diagnostic)?;
diff --git a/codespan-reporting/src/diagnostic.rs b/codespan-reporting/src/diagnostic.rs
index 559c899..8f017ba 100644
--- a/codespan-reporting/src/diagnostic.rs
+++ b/codespan-reporting/src/diagnostic.rs
@@ -50,7 +50,7 @@ impl PartialOrd for Severity {
     }
 }
 
-#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd)]
 #[cfg_attr(feature = "serialization", derive(Serialize, Deserialize))]
 pub enum LabelStyle {
     /// Labels that describe the primary cause of a diagnostic.
@@ -112,6 +112,8 @@ impl<FileId> Label<FileId> {
 
 /// Represents a diagnostic message that can provide information like errors and
 /// warnings to the user.
+///
+/// The position of a Diagnostic is considered to be the position of the [`Label`] with a style of [`LabelStyle::primary`] that has the smallest start position.
 #[derive(Clone, Debug)]
 #[cfg_attr(feature = "serialization", derive(Serialize, Deserialize))]
 pub struct Diagnostic<FileId> {
diff --git a/codespan-reporting/src/term/views.rs b/codespan-reporting/src/term/views.rs
index a67ce43..0e75977 100644
--- a/codespan-reporting/src/term/views.rs
+++ b/codespan-reporting/src/term/views.rs
@@ -45,6 +45,7 @@ where
             location: Location,
             num_multi_labels: usize,
             lines: BTreeMap<usize, Line<'diagnostic>>,
+            max_label_style: LabelStyle,
         }
 
         impl<'diagnostic, FileId> LabeledFile<'diagnostic, FileId> {
@@ -103,14 +104,20 @@ where
                 .find(|labeled_file| label.file_id == labeled_file.file_id)
             {
                 Some(labeled_file) => {
-                    if labeled_file.start > label.range.start {
+                    // another diagnostic also referenced this file
+                    if labeled_file.start > label.range.start
+                        && labeled_file.max_label_style >= label.style
+                    {
+                        // this label indicates an earlier start and has at least the same level of style
                         labeled_file.start = label.range.start;
                         labeled_file.location =
                             files.location(label.file_id, label.range.start).unwrap();
+                        labeled_file.max_label_style = label.style;
                     }
                     labeled_file
                 }
                 None => {
+                    // no other diagnostic referenced this file yet
                     labeled_files.push(LabeledFile {
                         file_id: label.file_id,
                         start: label.range.start,
@@ -118,6 +125,7 @@ where
                         location: files.location(label.file_id, label.range.start).unwrap(),
                         num_multi_labels: 0,
                         lines: BTreeMap::new(),
+                        max_label_style: label.style,
                     });
                     labeled_files.last_mut().unwrap()
                 }


Test patch:
diff --git a/codespan-reporting/tests/snapshots/term__fizz_buzz__rich_color.snap b/codespan-reporting/tests/snapshots/term__fizz_buzz__rich_color.snap
index a051eef..79dded2 100644
--- a/codespan-reporting/tests/snapshots/term__fizz_buzz__rich_color.snap
+++ b/codespan-reporting/tests/snapshots/term__fizz_buzz__rich_color.snap
@@ -3,7 +3,7 @@ source: codespan-reporting/tests/term.rs
 expression: TEST_DATA.emit_color(&config)
 ---
 {fg:Red bold bright}error[E0308]{bold bright}: `case` clauses have incompatible types{/}
-  {fg:Blue}┌─{/} FizzBuzz.fun:3:15
+  {fg:Blue}┌─{/} FizzBuzz.fun:8:12
   {fg:Blue}│{/}  
 {fg:Blue}3{/} {fg:Blue}│{/}   fizz₁ : Nat → String
   {fg:Blue}│{/}                 {fg:Blue}------{/} {fg:Blue}expected type `String` found here{/}
@@ -20,7 +20,7 @@ expression: TEST_DATA.emit_color(&config)
        found type `Nat`
 
 {fg:Red bold bright}error[E0308]{bold bright}: `case` clauses have incompatible types{/}
-   {fg:Blue}┌─{/} FizzBuzz.fun:10:15
+   {fg:Blue}┌─{/} FizzBuzz.fun:16:16
    {fg:Blue}│{/}  
 {fg:Blue}10{/} {fg:Blue}│{/}   fizz₂ : Nat → String
    {fg:Blue}│{/}                 {fg:Blue}------{/} {fg:Blue}expected type `String` found here{/}
diff --git a/codespan-reporting/tests/snapshots/term__fizz_buzz__rich_no_color.snap b/codespan-reporting/tests/snapshots/term__fizz_buzz__rich_no_color.snap
index dac0107..a0b7260 100644
--- a/codespan-reporting/tests/snapshots/term__fizz_buzz__rich_no_color.snap
+++ b/codespan-reporting/tests/snapshots/term__fizz_buzz__rich_no_color.snap
@@ -3,7 +3,7 @@ source: codespan-reporting/tests/term.rs
 expression: TEST_DATA.emit_no_color(&config)
 ---
 error[E0308]: `case` clauses have incompatible types
-  ┌─ FizzBuzz.fun:3:15
+  ┌─ FizzBuzz.fun:8:12
   │  
 3 │   fizz₁ : Nat → String
   │                 ------ expected type `String` found here
@@ -20,7 +20,7 @@ error[E0308]: `case` clauses have incompatible types
        found type `Nat`
 
 error[E0308]: `case` clauses have incompatible types
-   ┌─ FizzBuzz.fun:10:15
+   ┌─ FizzBuzz.fun:16:16
    │  
 10 │   fizz₂ : Nat → String
    │                 ------ expected type `String` found here
diff --git a/codespan-reporting/tests/snapshots/term__multiline_overlapping__rich_color.snap b/codespan-reporting/tests/snapshots/term__multiline_overlapping__rich_color.snap
index fee20e1..10701fb 100644
--- a/codespan-reporting/tests/snapshots/term__multiline_overlapping__rich_color.snap
+++ b/codespan-reporting/tests/snapshots/term__multiline_overlapping__rich_color.snap
@@ -3,7 +3,7 @@ source: codespan-reporting/tests/term.rs
 expression: TEST_DATA.emit_color(&config)
 ---
 {fg:Red bold bright}error[E0308]{bold bright}: match arms have incompatible types{/}
-  {fg:Blue}┌─{/} codespan/src/file.rs:1:9
+  {fg:Blue}┌─{/} codespan/src/file.rs:4:34
   {fg:Blue}│{/}    
 {fg:Blue}1{/} {fg:Blue}│{/}   {fg:Blue}╭{/}         match line_index.compare(self.last_line_index()) {
 {fg:Blue}2{/} {fg:Blue}│{/}   {fg:Blue}│{/}             Ordering::Less => Ok(self.line_starts()[line_index.to_usize()]),
diff --git a/codespan-reporting/tests/snapshots/term__multiline_overlapping__rich_no_color.snap b/codespan-reporting/tests/snapshots/term__multiline_overlapping__rich_no_color.snap
index 8f82c15..f6802c8 100644
--- a/codespan-reporting/tests/snapshots/term__multiline_overlapping__rich_no_color.snap
+++ b/codespan-reporting/tests/snapshots/term__multiline_overlapping__rich_no_color.snap
@@ -3,7 +3,7 @@ source: codespan-reporting/tests/term.rs
 expression: TEST_DATA.emit_no_color(&config)
 ---
 error[E0308]: match arms have incompatible types
-  ┌─ codespan/src/file.rs:1:9
+  ┌─ codespan/src/file.rs:4:34
   │    
 1 │   ╭         match line_index.compare(self.last_line_index()) {
 2 │   │             Ordering::Less => Ok(self.line_starts()[line_index.to_usize()]),
diff --git a/codespan-reporting/tests/snapshots/term__overlapping__rich_color.snap b/codespan-reporting/tests/snapshots/term__overlapping__rich_color.snap
index 92f061f..d29a429 100644
--- a/codespan-reporting/tests/snapshots/term__overlapping__rich_color.snap
+++ b/codespan-reporting/tests/snapshots/term__overlapping__rich_color.snap
@@ -3,7 +3,7 @@ source: codespan-reporting/tests/term.rs
 expression: TEST_DATA.emit_color(&config)
 ---
 {fg:Red bold bright}error[E0666]{bold bright}: nested `impl Trait` is not allowed{/}
-  {fg:Blue}┌─{/} nested_impl_trait.rs:5:46
+  {fg:Blue}┌─{/} nested_impl_trait.rs:5:56
   {fg:Blue}│{/}
 {fg:Blue}5{/} {fg:Blue}│{/} fn bad_in_ret_position(x: impl Into<u32>) -> impl Into<{fg:Red}impl Debug{/}> { x }
   {fg:Blue}│{/}                                              {fg:Blue}----------{fg:Red}^^^^^^^^^^{fg:Blue}-{/}
@@ -21,7 +21,7 @@ expression: TEST_DATA.emit_color(&config)
   {fg:Blue}│{/}                  {fg:Blue}help: replace with the correct return type: `i32`{/}
 
 {fg:Red bold bright}error[E0121]{bold bright}: the type placeholder `_` is not allowed within types on item signatures{/}
-  {fg:Blue}┌─{/} typeck_type_placeholder_item.rs:2:24
+  {fg:Blue}┌─{/} typeck_type_placeholder_item.rs:2:25
   {fg:Blue}│{/}
 {fg:Blue}2{/} {fg:Blue}│{/} fn fn_test2(x: i32) -> ({fg:Red}_{/}, {fg:Red}_{/}) { (x, x) }
   {fg:Blue}│{/}                        {fg:Blue}-{fg:Red}^{fg:Blue}--{fg:Red}^{fg:Blue}-{/}
diff --git a/codespan-reporting/tests/snapshots/term__overlapping__rich_no_color.snap b/codespan-reporting/tests/snapshots/term__overlapping__rich_no_color.snap
index e4a944d..32f8eec 100644
--- a/codespan-reporting/tests/snapshots/term__overlapping__rich_no_color.snap
+++ b/codespan-reporting/tests/snapshots/term__overlapping__rich_no_color.snap
@@ -3,7 +3,7 @@ source: codespan-reporting/tests/term.rs
 expression: TEST_DATA.emit_no_color(&config)
 ---
 error[E0666]: nested `impl Trait` is not allowed
-  ┌─ nested_impl_trait.rs:5:46
+  ┌─ nested_impl_trait.rs:5:56
   │
 5 │ fn bad_in_ret_position(x: impl Into<u32>) -> impl Into<impl Debug> { x }
   │                                              ----------^^^^^^^^^^-
@@ -21,7 +21,7 @@ error[E0121]: the type placeholder `_` is not allowed within types on item signa
   │                  help: replace with the correct return type: `i32`
 
 error[E0121]: the type placeholder `_` is not allowed within types on item signatures
-  ┌─ typeck_type_placeholder_item.rs:2:24
+  ┌─ typeck_type_placeholder_item.rs:2:25
   │
 2 │ fn fn_test2(x: i32) -> (_, _) { (x, x) }
   │                        -^--^-
diff --git a/codespan-reporting/tests/snapshots/term__position_indicator__rich_no_color.snap b/codespan-reporting/tests/snapshots/term__position_indicator__rich_no_color.snap
new file mode 100644
index 0000000..35217e4
--- /dev/null
+++ b/codespan-reporting/tests/snapshots/term__position_indicator__rich_no_color.snap
@@ -0,0 +1,14 @@
+---
+source: codespan-reporting/tests/term.rs
+expression: TEST_DATA.emit_no_color(&config)
+---
+warning[ParserWarning]: The strict mode declaration in the body of function `foo` is redundant, as the outer scope is already in strict mode
+  ┌─ tests/main.js:4:3
+  │
+1 │ "use strict";
+  │ ------------ Strict mode is first declared here
+  ·
+4 │   "use strict";
+  │   ^^^^^^^^^^^^ This strict mode declaration is redundant
+
+
diff --git a/codespan-reporting/tests/snapshots/term__position_indicator__short_no_color.snap b/codespan-reporting/tests/snapshots/term__position_indicator__short_no_color.snap
new file mode 100644
index 0000000..3ec348d
--- /dev/null
+++ b/codespan-reporting/tests/snapshots/term__position_indicator__short_no_color.snap
@@ -0,0 +1,6 @@
+---
+source: codespan-reporting/tests/term.rs
+expression: TEST_DATA.emit_no_color(&config)
+---
+tests/main.js:4:3: warning[ParserWarning]: The strict mode declaration in the body of function `foo` is redundant, as the outer scope is already in strict mode
+
diff --git a/codespan-reporting/tests/snapshots/term__same_line__rich_color.snap b/codespan-reporting/tests/snapshots/term__same_line__rich_color.snap
index 59bd616..38addbb 100644
--- a/codespan-reporting/tests/snapshots/term__same_line__rich_color.snap
+++ b/codespan-reporting/tests/snapshots/term__same_line__rich_color.snap
@@ -3,7 +3,7 @@ source: codespan-reporting/tests/term.rs
 expression: TEST_DATA.emit_color(&config)
 ---
 {fg:Red bold bright}error[E0499]{bold bright}: cannot borrow `v` as mutable more than once at a time{/}
-  {fg:Blue}┌─{/} one_line.rs:3:5
+  {fg:Blue}┌─{/} one_line.rs:3:12
   {fg:Blue}│{/}
 {fg:Blue}3{/} {fg:Blue}│{/}     v.push({fg:Red}v{/}.pop().unwrap());
   {fg:Blue}│{/}     {fg:Blue}-{/} {fg:Blue}----{/} {fg:Red}^{/} {fg:Red}second mutable borrow occurs here{/}
diff --git a/codespan-reporting/tests/snapshots/term__same_line__rich_no_color.snap b/codespan-reporting/tests/snapshots/term__same_line__rich_no_color.snap
index 7342f3f..75511a4 100644
--- a/codespan-reporting/tests/snapshots/term__same_line__rich_no_color.snap
+++ b/codespan-reporting/tests/snapshots/term__same_line__rich_no_color.snap
@@ -3,7 +3,7 @@ source: codespan-reporting/tests/term.rs
 expression: TEST_DATA.emit_no_color(&config)
 ---
 error[E0499]: cannot borrow `v` as mutable more than once at a time
-  ┌─ one_line.rs:3:5
+  ┌─ one_line.rs:3:12
   │
 3 │     v.push(v.pop().unwrap());
   │     - ---- ^ second mutable borrow occurs here
diff --git a/codespan-reporting/tests/term.rs b/codespan-reporting/tests/term.rs
index df1b263..4a0fa26 100644
--- a/codespan-reporting/tests/term.rs
+++ b/codespan-reporting/tests/term.rs
@@ -845,6 +845,41 @@ mod unicode_spans {
     test_emit!(short_no_color);
 }
 
+mod position_indicator {
+    use super::*;
+
+    lazy_static::lazy_static! {
+        static ref TEST_DATA: TestData<'static, SimpleFile<&'static str, String>> = {
+            let file = SimpleFile::new(
+                "tests/main.js",
+                [
+                    "\"use strict\";",
+                    "let zero=0;",
+                    "function foo() {",
+                    "  \"use strict\";",
+                    "  one=1;",
+                    "}",
+                ].join("\n"),
+            );
+            let diagnostics = vec![
+                Diagnostic::warning()
+                    .with_code("ParserWarning")
+                    .with_message("The strict mode declaration in the body of function `foo` is redundant, as the outer scope is already in strict mode")
+                    .with_labels(vec![
+                        Label::primary((), 45..57)
+                            .with_message("This strict mode declaration is redundant"),
+                        Label::secondary((), 0..12)
+                            .with_message("Strict mode is first declared here"),
+                    ]),
+            ];
+            TestData{files: file, diagnostics }
+        };
+    }
+
+    test_emit!(rich_no_color);
+    test_emit!(short_no_color);
+}
+
 mod multiline_omit {
     use super::*;