Repository: zkat/miette
Pull request #424 (2025-02-19 04:10:20)

fix(highlight): increase syntax highlighter config priority
This PR increases the `highlighter` config priority over the `rgb_colors` config. It resolves https://github.com/zkat/miette/issues/337.

Currently, enabling the `syntect-highlighter` feature is not enough to actually enable syntax highlighting, because the default `rgb_colors` config disallows it (more info and current workarounds in https://github.com/zkat/miette/issues/337). It would be desirable if users could use syntax highlighting simply by adding the `syntect-highlighter` feature.

Also, the current behavior is not consistent. For instance, when a custom highlighter is set (instead of syntect's), `rgb_colors` is already disregarded. When a custom highlighter is set and `syntect-highlighter` is disabled, even the `color` config is disregarded (it shouldn't be).

I kept 2 commits that may or may not be reviewed separately:

1. The first commit just adds new tests to assert the current (unexpected) behavior. It makes no changes in behavior.
2. The second commit performs the actual fix/refactor.

I did this so the change in behavior is explicit in the diff between the commits, by observing how the tests were changed. By the way, this is a breaking change since default behavior is changed.


Patch:
diff --git a/README.md b/README.md
index bd2e887..4829d03 100644
--- a/README.md
+++ b/README.md
@@ -51,6 +51,7 @@ diagnostic error code: ruget::api::bad_json
   - [... handler options](#-handler-options)
   - [... dynamic diagnostics](#-dynamic-diagnostics)
   - [... syntax highlighting](#-syntax-highlighting)
+  - [... primary label](#-primary-label)
   - [... collection of labels](#-collection-of-labels)
 - [Acknowledgements](#acknowledgements)
 - [License](#license)
@@ -689,6 +690,37 @@ trait to [`MietteHandlerOpts`] by calling the
 [`with_syntax_highlighting`](MietteHandlerOpts::with_syntax_highlighting)
 method. See the [`highlighters`] module docs for more details.
 
+#### ... primary label
+
+You can use the `primary` parameter to `label` to indicate that the label
+is the primary label.
+
+```rust
+#[derive(Debug, Diagnostic, Error)]
+#[error("oops!")]
+struct MyError {
+    #[label(primary, "main issue")]
+    primary_span: SourceSpan,
+
+    #[label("other label")]
+    other_span: SourceSpan,
+}
+```
+
+The `primary` parameter can be used at most once:
+
+```rust
+#[derive(Debug, Diagnostic, Error)]
+#[error("oops!")]
+struct MyError {
+    #[label(primary, "main issue")]
+    primary_span: SourceSpan,
+
+    #[label(primary, "other label")] // Error: Cannot have more than one primary label.
+    other_span: SourceSpan,
+}
+```
+
 #### ... collection of labels
 
 When the number of labels is unknown, you can use a collection of `SourceSpan`
diff --git a/src/diagnostic_chain.rs b/src/diagnostic_chain.rs
index 1e5e0c2..996fdba 100644
--- a/src/diagnostic_chain.rs
+++ b/src/diagnostic_chain.rs
@@ -74,7 +74,7 @@ impl<'a> ErrorKind<'a> {
     }
 }
 
-impl<'a> std::fmt::Debug for ErrorKind<'a> {
+impl std::fmt::Debug for ErrorKind<'_> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             ErrorKind::Diagnostic(d) => d.fmt(f),
@@ -83,7 +83,7 @@ impl<'a> std::fmt::Debug for ErrorKind<'a> {
     }
 }
 
-impl<'a> std::fmt::Display for ErrorKind<'a> {
+impl std::fmt::Display for ErrorKind<'_> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             ErrorKind::Diagnostic(d) => d.fmt(f),
diff --git a/src/eyreish/ptr.rs b/src/eyreish/ptr.rs
index fa954d1..40c9c7b 100644
--- a/src/eyreish/ptr.rs
+++ b/src/eyreish/ptr.rs
@@ -69,9 +69,9 @@ where
     lifetime: PhantomData<&'a T>,
 }
 
-impl<'a, T> Copy for Ref<'a, T> where T: ?Sized {}
+impl<T> Copy for Ref<'_, T> where T: ?Sized {}
 
-impl<'a, T> Clone for Ref<'a, T>
+impl<T> Clone for Ref<'_, T>
 where
     T: ?Sized,
 {
@@ -132,9 +132,9 @@ where
     lifetime: PhantomData<&'a mut T>,
 }
 
-impl<'a, T> Copy for Mut<'a, T> where T: ?Sized {}
+impl<T> Copy for Mut<'_, T> where T: ?Sized {}
 
-impl<'a, T> Clone for Mut<'a, T>
+impl<T> Clone for Mut<'_, T>
 where
     T: ?Sized,
 {
@@ -173,7 +173,7 @@ where
     }
 }
 
-impl<'a, T> Mut<'a, T> {
+impl<T> Mut<'_, T> {
     pub(crate) unsafe fn read(self) -> T {
         self.ptr.as_ptr().read()
     }
diff --git a/src/handler.rs b/src/handler.rs
index 4af5999..c6ac348 100644
--- a/src/handler.rs
+++ b/src/handler.rs
@@ -1,5 +1,3 @@
-use std::fmt;
-
 use crate::highlighters::Highlighter;
 use crate::highlighters::MietteHighlighter;
 use crate::protocol::Diagnostic;
@@ -9,6 +7,8 @@ use crate::NarratableReportHandler;
 use crate::ReportHandler;
 use crate::ThemeCharacters;
 use crate::ThemeStyles;
+use cfg_if::cfg_if;
+use std::fmt;
 
 /// Settings to control the color format used for graphical rendering.
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Default)]
@@ -99,7 +99,9 @@ impl MietteHandlerOpts {
     /// Setting this option will not force color output. In all cases, the
     /// current color configuration via
     /// [`color()`](MietteHandlerOpts::color()) takes precedence over
-    /// highlighter configuration.
+    /// highlighter configuration. However, this option does take precedence over
+    /// [`rgb_colors()`](MietteHandlerOpts::rgb_colors()) (meaning syntax highlighting will be
+    /// enabled regardless of the value of [`MietteHandlerOpts::rgb_colors`]).
     pub fn with_syntax_highlighting(
         mut self,
         highlighter: impl Highlighter + Send + Sync + 'static,
@@ -203,6 +205,8 @@ impl MietteHandlerOpts {
     /// first place. That is handled by the [`MietteHandlerOpts::color`]
     /// setting. If colors are not being used, the value of `rgb_colors` has
     /// no effect.
+    ///
+    /// It also does not control colors when a syntax highlighter is in use.
     pub fn rgb_colors(mut self, color: RgbColors) -> Self {
         self.rgb_colors = color;
         self
@@ -292,31 +296,13 @@ impl MietteHandlerOpts {
             } else {
                 ThemeStyles::none()
             };
-            #[cfg(not(feature = "syntect-highlighter"))]
-            let highlighter = self.highlighter.unwrap_or_else(MietteHighlighter::nocolor);
-            #[cfg(feature = "syntect-highlighter")]
-            let highlighter = if self.color == Some(false) {
-                MietteHighlighter::nocolor()
-            } else if self.color == Some(true) || syscall::supports_color() {
-                match self.highlighter {
-                    Some(highlighter) => highlighter,
-                    None => match self.rgb_colors {
-                        // Because the syntect highlighter currently only supports 24-bit truecolor,
-                        // respect RgbColor::Never by disabling the highlighter.
-                        // TODO: In the future, find a way to convert the RGB syntect theme
-                        // into an ANSI color theme.
-                        RgbColors::Never => MietteHighlighter::nocolor(),
-                        _ => MietteHighlighter::syntect_truecolor(),
-                    },
-                }
-            } else {
-                MietteHighlighter::nocolor()
-            };
+            let highlighter_opt =
+                HighlighterOption::select(self.color, self.highlighter, syscall::supports_color());
             let theme = self.theme.unwrap_or(GraphicalTheme { characters, styles });
             let mut handler = GraphicalReportHandler::new_themed(theme)
                 .with_width(width)
                 .with_links(linkify);
-            handler.highlighter = highlighter;
+            handler.highlighter = highlighter_opt.into();
             if let Some(with_cause_chain) = self.with_cause_chain {
                 if with_cause_chain {
                     handler = handler.with_cause_chain();
@@ -425,6 +411,55 @@ impl ReportHandler for MietteHandler {
     }
 }
 
+enum HighlighterOption {
+    Disable,
+    EnableCustom(MietteHighlighter),
+    #[cfg(feature = "syntect-highlighter")]
+    EnableSyntect,
+}
+
+impl HighlighterOption {
+    fn select(
+        color: Option<bool>,
+        highlighter: Option<MietteHighlighter>,
+        supports_color: bool,
+    ) -> HighlighterOption {
+        if color == Some(false) || (color == None && !supports_color) {
+            return HighlighterOption::Disable;
+        }
+        highlighter
+            .map(HighlighterOption::EnableCustom)
+            .unwrap_or_default()
+    }
+}
+
+impl Default for HighlighterOption {
+    fn default() -> Self {
+        cfg_if! {
+            if #[cfg(feature = "syntect-highlighter")] {
+                // Because the syntect highlighter currently only supports 24-bit truecolor,
+                // it supersedes and ignores the `rgb_colors` config.
+                // TODO: In the future, if we find a way to convert the RGB syntect theme
+                // into an ANSI color theme, we can take `rgb_colors` into account.
+                HighlighterOption::EnableSyntect
+            } else {
+                HighlighterOption::Disable
+            }
+        }
+    }
+}
+
+impl From<HighlighterOption> for MietteHighlighter {
+    fn from(opt: HighlighterOption) -> Self {
+        match opt {
+            HighlighterOption::Disable => MietteHighlighter::nocolor(),
+            HighlighterOption::EnableCustom(highlighter) => highlighter,
+            #[cfg(feature = "syntect-highlighter")]
+            HighlighterOption::EnableSyntect => MietteHighlighter::syntect_truecolor(),
+        }
+    }
+}
+
 mod syscall {
     use cfg_if::cfg_if;
 
@@ -450,7 +485,6 @@ mod syscall {
         }
     }
 
-    #[cfg(feature = "syntect-highlighter")]
     #[inline]
     pub(super) fn supports_color() -> bool {
         cfg_if! {
@@ -484,3 +518,108 @@ mod syscall {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::highlighters::BlankHighlighter;
+    use cfg_if::cfg_if;
+
+    #[test]
+    fn test_highlighter_option() {
+        // Syntax highlighting is enabled depending on several variables:
+        // - The `color` config
+        // - The `highlighter` config
+        // - Whether the `syntect-highlighter` feature is enabled
+        // - Whether the terminal supports color
+        //
+        // This test asserts the expected highlighter depending on combinations of those variables.
+
+        macro_rules! assert_highlighter_opt {
+            (opts = $opts:expr, supports_color = $sup_color:literal, expected = $expected:pat $(,)?) => {
+                assert_highlighter_opt!(
+                    opts = $opts,
+                    supports_color = $sup_color,
+                    expected_with_syntect = $expected,
+                    expected_without_syntect = $expected,
+                );
+            };
+
+            (
+                opts = $opts:expr,
+                supports_color = $sup_color:literal,
+                expected_with_syntect = $expected_with:pat,
+                expected_without_syntect = $expected_without:pat $(,)?
+            ) => {{
+                let highlighter_opt =
+                    HighlighterOption::select($opts.color, $opts.highlighter, $sup_color);
+                cfg_if! {
+                    if #[cfg(feature = "syntect-highlighter")] {
+                        assert!(matches!(highlighter_opt, $expected_with));
+                    } else {
+                        assert!(matches!(highlighter_opt, $expected_without));
+                    }
+                }
+            }};
+        }
+
+        // When color is explicitly disabled, highlighting is also always disabled.
+        assert_highlighter_opt!(
+            opts = MietteHandlerOpts::new().color(false),
+            supports_color = true,
+            expected = HighlighterOption::Disable,
+        );
+
+        // When color is unset and the terminal doesn't support color, highlighting is disabled.
+        assert_highlighter_opt!(
+            opts = MietteHandlerOpts::new(),
+            supports_color = false,
+            expected = HighlighterOption::Disable,
+        );
+
+        // With explicit or implicit color support, highlighting is automatically enabled when
+        // `syntect-highlighter` is enabled.
+        assert_highlighter_opt!(
+            opts = MietteHandlerOpts::new().color(true),
+            supports_color = false,
+            expected_with_syntect = HighlighterOption::EnableSyntect,
+            expected_without_syntect = HighlighterOption::Disable,
+        );
+        assert_highlighter_opt!(
+            opts = MietteHandlerOpts::new(),
+            supports_color = true,
+            expected_with_syntect = HighlighterOption::EnableSyntect,
+            expected_without_syntect = HighlighterOption::Disable,
+        );
+
+        // With explicit or implicit color support, if custom highlighting is set, it's enabled.
+        assert_highlighter_opt!(
+            opts = MietteHandlerOpts::new()
+                .color(true)
+                .with_syntax_highlighting(BlankHighlighter),
+            supports_color = false,
+            expected = HighlighterOption::EnableCustom(_),
+        );
+        assert_highlighter_opt!(
+            opts = MietteHandlerOpts::new().with_syntax_highlighting(BlankHighlighter),
+            supports_color = true,
+            expected = HighlighterOption::EnableCustom(_),
+        );
+
+        // Setting `RgbColors::Never` has no effect when syntax highlighting is enabled.
+        assert_highlighter_opt!(
+            opts = MietteHandlerOpts::new()
+                .color(true)
+                .rgb_colors(RgbColors::Never),
+            supports_color = false,
+            expected_with_syntect = HighlighterOption::EnableSyntect,
+            expected_without_syntect = HighlighterOption::Disable,
+        );
+        assert_highlighter_opt!(
+            opts = MietteHandlerOpts::new().rgb_colors(RgbColors::Never),
+            supports_color = true,
+            expected_with_syntect = HighlighterOption::EnableSyntect,
+            expected_without_syntect = HighlighterOption::Disable,
+        );
+    }
+}
diff --git a/src/handlers/graphical.rs b/src/handlers/graphical.rs
index 3e9c790..faac695 100644
--- a/src/handlers/graphical.rs
+++ b/src/handlers/graphical.rs
@@ -33,6 +33,7 @@ pub struct GraphicalReportHandler {
     pub(crate) with_cause_chain: bool,
     pub(crate) wrap_lines: bool,
     pub(crate) break_words: bool,
+    pub(crate) with_primary_span_start: bool,
     pub(crate) word_separator: Option<textwrap::WordSeparator>,
     pub(crate) word_splitter: Option<textwrap::WordSplitter>,
     pub(crate) highlighter: MietteHighlighter,
@@ -61,6 +62,7 @@ impl GraphicalReportHandler {
             with_cause_chain: true,
             wrap_lines: true,
             break_words: true,
+            with_primary_span_start: true,
             word_separator: None,
             word_splitter: None,
             highlighter: MietteHighlighter::default(),
@@ -80,6 +82,7 @@ impl GraphicalReportHandler {
             tab_width: 4,
             wrap_lines: true,
             with_cause_chain: true,
+            with_primary_span_start: true,
             break_words: true,
             word_separator: None,
             word_splitter: None,
@@ -119,6 +122,20 @@ impl GraphicalReportHandler {
         self
     }
 
+    /// Include the line and column for the the start of the primary span when the
+    /// snippet extends multiple lines
+    pub fn with_primary_span_start(mut self) -> Self {
+        self.with_primary_span_start = true;
+        self
+    }
+
+    /// Do not include the line and column for the the start of the primary span
+    /// when the snippet extends multiple lines
+    pub fn without_primary_span_start(mut self) -> Self {
+        self.with_primary_span_start = false;
+        self
+    }
+
     /// Whether to include [`Diagnostic::url()`] in the output.
     ///
     /// Disabling this is not recommended, but can be useful for more easily
@@ -664,26 +681,34 @@ impl GraphicalReportHandler {
         };
 
         if let Some(source_name) = primary_contents.name() {
-            writeln!(
-                f,
-                "[{}]",
-                format_args!(
-                    "{}:{}:{}",
-                    source_name,
-                    primary_contents.line() + 1,
-                    primary_contents.column() + 1
-                )
-                .style(self.theme.styles.link)
-            )?;
-        } else if lines.len() <= 1 {
-            writeln!(f, "{}", self.theme.characters.hbar.to_string().repeat(3))?;
-        } else {
+            if self.with_primary_span_start {
+                writeln!(
+                    f,
+                    "[{}]",
+                    format_args!(
+                        "{}:{}:{}",
+                        source_name,
+                        primary_contents.line() + 1,
+                        primary_contents.column() + 1
+                    )
+                    .style(self.theme.styles.link)
+                )?;
+            } else {
+                writeln!(
+                    f,
+                    "[{}]",
+                    format_args!("{}", source_name,).style(self.theme.styles.link)
+                )?;
+            }
+        } else if self.with_primary_span_start && lines.len() > 1 {
             writeln!(
                 f,
                 "[{}:{}]",
                 primary_contents.line() + 1,
                 primary_contents.column() + 1
             )?;
+        } else {
+            writeln!(f, "{}", self.theme.characters.hbar.to_string().repeat(3))?;
         }
 
         // Now it's time for the fun part--actually rendering everything!
diff --git a/src/protocol.rs b/src/protocol.rs
index 7f09d4d..c2df294 100644
--- a/src/protocol.rs
+++ b/src/protocol.rs
@@ -134,7 +134,7 @@ impl From<&str> for Box<dyn Diagnostic> {
     }
 }
 
-impl<'a> From<&str> for Box<dyn Diagnostic + Send + Sync + 'a> {
+impl From<&str> for Box<dyn Diagnostic + Send + Sync + '_> {
     fn from(s: &str) -> Self {
         From::from(String::from(s))
     }
diff --git a/src/source_impls.rs b/src/source_impls.rs
index e362b4a..9c26b71 100644
--- a/src/source_impls.rs
+++ b/src/source_impls.rs
@@ -104,7 +104,7 @@ impl SourceCode for [u8] {
     }
 }
 
-impl<'src> SourceCode for &'src [u8] {
+impl SourceCode for &[u8] {
     fn read_span<'a>(
         &'a self,
         span: &SourceSpan,
@@ -143,7 +143,7 @@ impl SourceCode for str {
 }
 
 /// Makes `src: &'static str` or `struct S<'a> { src: &'a str }` usable.
-impl<'s> SourceCode for &'s str {
+impl SourceCode for &str {
     fn read_span<'a>(
         &'a self,
         span: &SourceSpan,


Test patch:
diff --git a/tests/graphical.rs b/tests/graphical.rs
index b80e16d..93c9bce 100644
--- a/tests/graphical.rs
+++ b/tests/graphical.rs
@@ -2508,3 +2508,63 @@ fn after_invalid_unicode() -> Result<(), MietteError> {
 
     Ok(())
 }
+
+#[test]
+fn option_include_primary_span_start() {
+    #[derive(Debug, Clone, Diagnostic, Error)]
+    #[error("decoding error")]
+    #[diagnostic(code(decode_err))]
+    struct E {
+        #[label("valid data here")]
+        src: SourceSpan,
+    }
+
+    let invalid_source: &[u8] = b"malformed\nh\xf0\x93\x8aXYZ";
+
+    let (x_index, _) = invalid_source
+        .iter()
+        .enumerate()
+        .find(|&(_, &x)| x == b'X')
+        .unwrap();
+
+    // make err pointing at the X
+    let err = E {
+        src: SourceSpan::from((x_index, 1)),
+    };
+
+    let result = fmt_report_with_settings(
+        Report::new(err.clone()).with_source_code(invalid_source),
+        |handler| handler,
+    );
+
+    let expected = "decode_err
+
+  × decoding error
+   ╭─[2:5]
+ 1 │ malformed
+ 2 │ h�XYZ
+   ·   ┬
+   ·   ╰── valid data here
+   ╰────
+";
+
+    assert_eq!(expected, result);
+
+    let result = fmt_report_with_settings(
+        Report::new(err).with_source_code(invalid_source),
+        |handler| handler.without_primary_span_start(),
+    );
+
+    let expected = "decode_err
+
+  × decoding error
+   ╭────
+ 1 │ malformed
+ 2 │ h�XYZ
+   ·   ┬
+   ·   ╰── valid data here
+   ╰────
+";
+
+    assert_eq!(expected, result);
+}