refactor: Improve diagnostics (#158)

Author: zharinov

Makefile

@@ -10,7 +10,10 @@ test:
 	@cargo nextest run --no-fail-fast --hide-progress-bar --status-level none --failure-output final
 
 shot:
+	@# See AGENTS.md for diagnostic guidelines
+	@cargo nextest run --no-fail-fast --hide-progress-bar --status-level none --failure-output final || true
 	@cargo insta accept
+	@cargo nextest run --no-fail-fast --hide-progress-bar --status-level none --failure-output final
 
 coverage-lines:
 	@cargo llvm-cov --package plotnik-lib --text --show-missing-lines 2>/dev/null | grep '\.rs: [0-9]' | sed 's|.*/crates/|crates/|'

crates/plotnik-lib/src/diagnostics/message.rs

@@ -129,37 +129,57 @@ impl DiagnosticKind {
         matches!(self, Self::UnnamedDef)
     }
 
+    /// Default hint for this kind, automatically included in diagnostics.
+    /// Call sites can add additional hints for context-specific information.
+    pub fn default_hint(&self) -> Option<&'static str> {
+        match self {
+            Self::ExpectedSubtype => Some("e.g., `expression/binary_expression`"),
+            Self::ExpectedTypeName => Some("e.g., `::MyType` or `::string`"),
+            Self::ExpectedFieldName => Some("e.g., `!value`"),
+            Self::EmptyTree => Some("use `(_)` to match any named node, or `_` for any node"),
+            Self::TreeSitterSequenceSyntax => Some("use `{...}` for sequences"),
+            Self::MixedAltBranches => {
+                Some("use all labels for a tagged union, or none for a merged struct")
+            }
+            Self::RecursionNoEscape => {
+                Some("add a non-recursive branch to terminate: `[Base: ... Rec: (Self)]`")
+            }
+            Self::DirectRecursion => {
+                Some("recursive references must consume input before recursing")
+            }
+            _ => None,
+        }
+    }
+
     /// Base message for this diagnostic kind, used when no custom message is provided.
     pub fn fallback_message(&self) -> &'static str {
         match self {
-            // Unclosed delimiters - clear about what's missing
+            // Unclosed delimiters
             Self::UnclosedTree => "missing closing `)`",
             Self::UnclosedSequence => "missing closing `}`",
             Self::UnclosedAlternation => "missing closing `]`",
 
-            // Expected token errors - specific about what's needed
+            // Expected token errors
             Self::ExpectedExpression => "expected an expression",
-            Self::ExpectedTypeName => "expected type name after `::`",
-            Self::ExpectedCaptureName => "expected name after `@`",
+            Self::ExpectedTypeName => "expected type name",
+            Self::ExpectedCaptureName => "expected capture name",
             Self::ExpectedFieldName => "expected field name",
-            Self::ExpectedSubtype => "expected subtype after `/`",
-
-            // Invalid syntax - explain what's wrong
-            Self::EmptyTree => "empty parentheses are not allowed",
-            Self::BareIdentifier => "bare identifier is not a valid expression",
-            Self::InvalidSeparator => "separators are not needed",
-            Self::InvalidFieldEquals => "use `:` for field constraints, not `=`",
-            Self::InvalidSupertypeSyntax => "supertype syntax not allowed on references",
-            Self::InvalidTypeAnnotationSyntax => "use `::` for type annotations, not `:`",
-            Self::ErrorTakesNoArguments => "`(ERROR)` cannot have child nodes",
+            Self::ExpectedSubtype => "expected subtype name",
+
+            // Invalid syntax
+            Self::EmptyTree => "empty `()` is not allowed",
+            Self::BareIdentifier => "bare identifier is not valid",
+            Self::InvalidSeparator => "unexpected separator",
+            Self::InvalidFieldEquals => "use `:` instead of `=`",
+            Self::InvalidSupertypeSyntax => "references cannot have supertypes",
+            Self::InvalidTypeAnnotationSyntax => "use `::` for type annotations",
+            Self::ErrorTakesNoArguments => "`(ERROR)` cannot have children",
             Self::RefCannotHaveChildren => "references cannot have children",
-            Self::ErrorMissingOutsideParens => {
-                "`ERROR` and `MISSING` must be wrapped in parentheses"
-            }
-            Self::UnsupportedPredicate => "predicates like `#match?` are not supported",
+            Self::ErrorMissingOutsideParens => "special node requires parentheses",
+            Self::UnsupportedPredicate => "predicates are not supported",
             Self::UnexpectedToken => "unexpected token",
-            Self::CaptureWithoutTarget => "`@` must follow an expression to capture",
-            Self::LowercaseBranchLabel => "branch labels must be capitalized",
+            Self::CaptureWithoutTarget => "capture has no target",
+            Self::LowercaseBranchLabel => "branch label must start with uppercase",
 
             // Naming convention violations
             Self::CaptureNameHasDots => "capture names cannot contain `.`",
@@ -172,23 +192,25 @@ impl DiagnosticKind {
             Self::FieldNameHasHyphens => "field names cannot contain `-`",
             Self::FieldNameUppercase => "field names must be lowercase",
             Self::TypeNameInvalidChars => "type names cannot contain `.` or `-`",
-            Self::TreeSitterSequenceSyntax => "Tree-sitter sequence syntax",
+            Self::TreeSitterSequenceSyntax => "tree-sitter sequence syntax",
 
             // Semantic errors
-            Self::DuplicateDefinition => "name already defined",
+            Self::DuplicateDefinition => "duplicate definition",
             Self::UndefinedReference => "undefined reference",
             Self::MixedAltBranches => "cannot mix labeled and unlabeled branches",
-            Self::RecursionNoEscape => "infinite recursion: cycle has no escape path",
+            Self::RecursionNoEscape => "infinite recursion: no escape path",
             Self::DirectRecursion => "infinite recursion: cycle consumes no input",
-            Self::FieldSequenceValue => "field must match exactly one node",
+            Self::FieldSequenceValue => "field cannot match a sequence",
 
             // Type inference
-            Self::IncompatibleTypes => "incompatible types in alternation branches",
+            Self::IncompatibleTypes => "incompatible types",
             Self::MultiCaptureQuantifierNoName => {
-                "quantified expression with multiple captures requires `@name`"
+                "quantified expression with multiple captures requires a struct capture"
             }
             Self::UnusedBranchLabels => "branch labels have no effect without capture",
-            Self::StrictDimensionalityViolation => "quantifier requires row capture",
+            Self::StrictDimensionalityViolation => {
+                "quantifier with captures requires a struct capture"
+            }
             Self::DuplicateCaptureInScope => "duplicate capture in scope",
             Self::IncompatibleCaptureTypes => "incompatible capture types",
             Self::IncompatibleStructShapes => "incompatible struct shapes",
@@ -201,7 +223,7 @@ impl DiagnosticKind {
             Self::InvalidChildType => "node type not valid as child",
 
             // Structural
-            Self::UnnamedDef => "definitions must be named",
+            Self::UnnamedDef => "definition must be named",
         }
     }
 
@@ -212,9 +234,7 @@ impl DiagnosticKind {
             Self::RefCannotHaveChildren => {
                 "`{}` is a reference and cannot have children".to_string()
             }
-            Self::FieldSequenceValue => {
-                "field `{}` must match exactly one node, not a sequence".to_string()
-            }
+            Self::FieldSequenceValue => "field `{}` cannot match a sequence".to_string(),
 
             // Semantic errors with name context
             Self::DuplicateDefinition => "`{}` is already defined".to_string(),
@@ -249,15 +269,13 @@ impl DiagnosticKind {
             }
 
             // Type annotation specifics
-            Self::InvalidTypeAnnotationSyntax => {
-                "type annotations use `::`, not `:` — {}".to_string()
-            }
+            Self::InvalidTypeAnnotationSyntax => "use `::` for type annotations: {}".to_string(),
 
-            // Named def
-            Self::UnnamedDef => "definitions must be named — {}".to_string(),
+            // Named def (no custom message needed; suggestion goes in hint)
+            Self::UnnamedDef => self.fallback_message().to_string(),
 
             // Standard pattern: fallback + context
-            _ => format!("{}; {{}}", self.fallback_message()),
+            _ => format!("{}: {{}}", self.fallback_message()),
         }
     }
 

crates/plotnik-lib/src/diagnostics/mod.rs

@@ -245,7 +245,11 @@ impl<'a> DiagnosticBuilder<'a> {
         self
     }
 
-    pub fn emit(self) {
+    pub fn emit(mut self) {
+        // Prepend default hint if one exists for this kind
+        if let Some(default_hint) = self.message.kind.default_hint() {
+            self.message.hints.insert(0, default_hint.to_string());
+        }
         self.diagnostics.messages.push(self.message);
     }
 }

crates/plotnik-lib/src/diagnostics/tests.rs

@@ -89,7 +89,7 @@ fn builder_with_fix() {
 
     let result = diagnostics.printer(&map).render();
     insta::assert_snapshot!(result, @r"
-    error: use `:` for field constraints, not `=`; fixable
+    error: use `:` instead of `=`: fixable
       |
     1 | hello world
       | ^^^^^
@@ -207,7 +207,7 @@ fn printer_zero_width_span() {
 
     let result = diagnostics.printer(&map).render();
     insta::assert_snapshot!(result, @r"
-    error: expected an expression; zero width error
+    error: expected an expression: zero width error
       |
     1 | hello
       | ^

crates/plotnik-lib/src/parser/core.rs

@@ -260,7 +260,7 @@ impl<'src, 'diag> Parser<'src, 'diag> {
         true
     }
 
-    fn bump_as_error(&mut self) {
+    pub(super) fn bump_as_error(&mut self) {
         if !self.eof() {
             self.start_node(SyntaxKind::Error);
             self.bump();
@@ -298,11 +298,32 @@ impl<'src, 'diag> Parser<'src, 'diag> {
             .emit();
     }
 
+    pub(super) fn error_with_hint(&mut self, kind: DiagnosticKind, hint: impl Into<String>) {
+        let Some((range, suppression)) = self.get_error_ranges() else {
+            return;
+        };
+        self.diagnostics
+            .report(self.source_id, kind, range)
+            .hint(hint)
+            .suppression_range(suppression)
+            .emit();
+    }
+
     pub(super) fn error_and_bump(&mut self, kind: DiagnosticKind) {
         self.error(kind);
         self.bump_as_error();
     }
 
+    pub(super) fn error_and_bump_with_hint(
+        &mut self,
+        kind: DiagnosticKind,
+        hint: impl Into<String>,
+    ) {
+        self.error_with_hint(kind, hint);
+        self.bump_as_error();
+    }
+
+    #[allow(dead_code)]
     pub(super) fn error_and_bump_msg(&mut self, kind: DiagnosticKind, message: impl Into<String>) {
         self.error_msg(kind, message);
         self.bump_as_error();
@@ -359,7 +380,6 @@ impl<'src, 'diag> Parser<'src, 'diag> {
     pub(super) fn error_unclosed_delimiter(
         &mut self,
         kind: DiagnosticKind,
-        message: impl Into<String>,
         related_msg: impl Into<String>,
         open_range: TextRange,
     ) {
@@ -371,7 +391,6 @@ impl<'src, 'diag> Parser<'src, 'diag> {
         let full_range = TextRange::new(open_range.start(), current.end());
         self.diagnostics
             .report(self.source_id, kind, full_range)
-            .message(message)
             .related_to(self.source_id, open_range, related_msg)
             .emit();
     }

crates/plotnik-lib/src/parser/grammar/expressions.rs

@@ -22,7 +22,7 @@ impl Parser<'_, '_> {
             return false;
         }
 
-        self.error_and_bump_msg(
+        self.error_and_bump_with_hint(
             DiagnosticKind::UnexpectedToken,
             "try `(node)`, `[a b]`, `{a b}`, `\"literal\"`, or `_`",
         );
@@ -65,7 +65,7 @@ impl Parser<'_, '_> {
                 self.error_and_bump(DiagnosticKind::ErrorMissingOutsideParens);
             }
             _ => {
-                self.error_and_bump_msg(DiagnosticKind::UnexpectedToken, "not a valid expression");
+                self.error_and_bump(DiagnosticKind::UnexpectedToken);
             }
         }
 

crates/plotnik-lib/src/parser/grammar/fields.rs

@@ -42,10 +42,7 @@ impl Parser<'_, '_> {
             self.bump();
             self.validate_type_name(text, span);
         } else {
-            self.error_msg(
-                DiagnosticKind::ExpectedTypeName,
-                "e.g., `::MyType` or `::string`",
-            );
+            self.error(DiagnosticKind::ExpectedTypeName);
         }
 
         self.finish_node();
@@ -84,7 +81,7 @@ impl Parser<'_, '_> {
         self.expect(SyntaxKind::Negation, "'!' for negated field");
 
         if !self.currently_is(SyntaxKind::Id) {
-            self.error_msg(DiagnosticKind::ExpectedFieldName, "e.g., `!value`");
+            self.error(DiagnosticKind::ExpectedFieldName);
             self.finish_node();
             return;
         }
@@ -110,10 +107,13 @@ impl Parser<'_, '_> {
         }
 
         // Bare identifiers are not valid expressions; trees require parentheses
-        self.error_and_bump_msg(
-            DiagnosticKind::BareIdentifier,
-            "wrap in parentheses: `(identifier)`",
-        );
+        let span = self.current_span();
+        let text = token_text(self.source, &self.tokens[self.pos]);
+        self.diagnostics
+            .report(self.source_id, DiagnosticKind::BareIdentifier, span)
+            .fix("wrap in parentheses", format!("({})", text))
+            .emit();
+        self.bump_as_error();
     }
 
     /// Field constraint: `field_name: expr`
@@ -134,7 +134,7 @@ impl Parser<'_, '_> {
         if self.currently_is_one_of(EXPR_FIRST_TOKENS) {
             self.parse_expr_no_suffix();
         } else {
-            self.error_msg(DiagnosticKind::ExpectedExpression, "after `field:`");
+            self.error(DiagnosticKind::ExpectedExpression);
         }
 
         self.finish_node();
@@ -158,7 +158,7 @@ impl Parser<'_, '_> {
         if self.currently_is_one_of(EXPR_FIRST_TOKENS) {
             self.parse_expr();
         } else {
-            self.error_msg(DiagnosticKind::ExpectedExpression, "after `field =`");
+            self.error(DiagnosticKind::ExpectedExpression);
         }
 
         self.finish_node();

crates/plotnik-lib/src/parser/grammar/items.rs

@@ -30,7 +30,7 @@ impl Parser<'_, '_> {
                 let def_text = &self.source[usize::from(start)..usize::from(end)];
                 self.diagnostics
                     .report(self.source_id, DiagnosticKind::UnnamedDef, span)
-                    .message(format!("give it a name like `Name = {}`", def_text.trim()))
+                    .hint(format!("give it a name like `Name = {}`", def_text.trim()))
                     .emit();
             }
         }
@@ -83,10 +83,7 @@ impl Parser<'_, '_> {
         if self.currently_is_one_of(EXPR_FIRST_TOKENS) {
             self.parse_expr();
         } else {
-            self.error_msg(
-                DiagnosticKind::ExpectedExpression,
-                "after `=` in definition",
-            );
+            self.error(DiagnosticKind::ExpectedExpression);
         }
 
         self.finish_node();