Fix/bad syntax binding error variants

CHANGELOG.md

@@ -5,7 +5,15 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to the versioning scheme outlined in the [README.md](README.md).
 
+## [unreleased]
+
+### Changed
+
+- Clarity errors pertaining to syntax binding errors have been made more
+  expressive (#6337)
+
 ## [3.2.0.0.1]
+
 ### Added
 
 - Adds node-config-docsgen to automatically create config documentation (#6227)

clarity/src/vm/analysis/errors.rs

@@ -23,6 +23,127 @@ use crate::vm::types::{TraitIdentifier, TupleTypeSignature, TypeSignature, Value
 
 pub type CheckResult<T> = Result<T, CheckError>;
 
+/// What kind of syntax binding was found to be in error?
+#[derive(Debug, PartialEq, Clone, Copy)]
+pub enum SyntaxBindingErrorType {
+    Let,
+    Eval,
+    TupleCons,
+}
+
+impl fmt::Display for SyntaxBindingErrorType {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", &self.message())
+    }
+}
+
+impl DiagnosableError for SyntaxBindingErrorType {
+    fn message(&self) -> String {
+        match &self {
+            Self::Let => "Let-binding".to_string(),
+            Self::Eval => "Function argument definition".to_string(),
+            Self::TupleCons => "Tuple constructor".to_string(),
+        }
+    }
+
+    fn suggestion(&self) -> Option<String> {
+        None
+    }
+}
+
+/// Syntax binding error types
+#[derive(Debug, PartialEq)]
+pub enum SyntaxBindingError {
+    /// binding list item is not a list
+    NotList(SyntaxBindingErrorType, usize),
+    /// binding list item has an invalid length (e.g. not 2)
+    InvalidLength(SyntaxBindingErrorType, usize),
+    /// binding name is not an atom
+    NotAtom(SyntaxBindingErrorType, usize),
+}
+
+impl fmt::Display for SyntaxBindingError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{self:?}")
+    }
+}
+
+impl DiagnosableError for SyntaxBindingError {
+    fn message(&self) -> String {
+        match &self {
+            Self::NotList(err_type, item_index) => {
+                let item_no = item_index + 1;
+                format!("{err_type} item #{item_no} is not a list",)
+            }
+            Self::InvalidLength(err_type, item_index) => {
+                let item_no = item_index + 1;
+                format!("{err_type} item #{item_no} is not a two-element list",)
+            }
+            Self::NotAtom(err_type, item_index) => {
+                let item_no = item_index + 1;
+                format!("{err_type} item #{item_no}'s name is not an atom",)
+            }
+        }
+    }
+
+    fn suggestion(&self) -> Option<String> {
+        None
+    }
+}
+
+impl SyntaxBindingError {
+    /// Helper constructor for NotList(SyntaxBindingErrorType::Let, item_no)
+    pub fn let_binding_not_list(item_no: usize) -> Self {
+        Self::NotList(SyntaxBindingErrorType::Let, item_no)
+    }
+
+    /// Helper constructor for InvalidLength(SyntaxBindingErrorType::Let, item_no)
+    pub fn let_binding_invalid_length(item_no: usize) -> Self {
+        Self::InvalidLength(SyntaxBindingErrorType::Let, item_no)
+    }
+
+    /// Helper constructor for NotAtom(SyntaxBindingErrorType::Let, item_no)
+    pub fn let_binding_not_atom(item_no: usize) -> Self {
+        Self::NotAtom(SyntaxBindingErrorType::Let, item_no)
+    }
+
+    /// Helper constructor for NotList(SyntaxBindingErrorType::Eval, item_no)
+    pub fn eval_binding_not_list(item_no: usize) -> Self {
+        Self::NotList(SyntaxBindingErrorType::Eval, item_no)
+    }
+
+    /// Helper constructor for InvalidLength(SyntaxBindingErrorType::Eval, item_no)
+    pub fn eval_binding_invalid_length(item_no: usize) -> Self {
+        Self::InvalidLength(SyntaxBindingErrorType::Eval, item_no)
+    }
+
+    /// Helper constructor for NotAtom(SyntaxBindingErrorType::Eval, item_no)
+    pub fn eval_binding_not_atom(item_no: usize) -> Self {
+        Self::NotAtom(SyntaxBindingErrorType::Eval, item_no)
+    }
+
+    /// Helper constructor for NotList(SyntaxBindingErrorType::TupleCons, item_no)
+    pub fn tuple_cons_not_list(item_no: usize) -> Self {
+        Self::NotList(SyntaxBindingErrorType::TupleCons, item_no)
+    }
+
+    /// Helper constructor for InvalidLength(SyntaxBindingErrorType::TupleCons, item_no)
+    pub fn tuple_cons_invalid_length(item_no: usize) -> Self {
+        Self::InvalidLength(SyntaxBindingErrorType::TupleCons, item_no)
+    }
+
+    /// Helper constructor for NotAtom(SyntaxBindingErrorType::TupleCons, item_no)
+    pub fn tuple_cons_not_atom(item_no: usize) -> Self {
+        Self::NotAtom(SyntaxBindingErrorType::TupleCons, item_no)
+    }
+}
+
+impl From<SyntaxBindingError> for CheckErrors {
+    fn from(e: SyntaxBindingError) -> Self {
+        Self::BadSyntaxBinding(e)
+    }
+}
+
 #[derive(Debug, PartialEq)]
 pub enum CheckErrors {
     // cost checker errors
@@ -101,8 +222,7 @@ pub enum CheckErrors {
     ExpectedTuple(TypeSignature),
     NoSuchTupleField(String, TupleTypeSignature),
     EmptyTuplesNotAllowed,
-    BadTupleConstruction,
-    TupleExpectsPairs,
+    BadTupleConstruction(String),
 
     // variables
     NoSuchDataVariable(String),
@@ -152,8 +272,7 @@ pub enum CheckErrors {
     BadLetSyntax,
 
     // generic binding syntax
-    BadSyntaxBinding,
-    BadSyntaxExpectedListOfPairs,
+    BadSyntaxBinding(SyntaxBindingError),
 
     MaxContextDepthReached,
     UndefinedFunction(String),
@@ -240,6 +359,18 @@ impl CheckError {
         self.diagnostic.spans = exprs.iter().map(|e| e.span().clone()).collect();
         self.expressions.replace(exprs.to_vec());
     }
+
+    pub fn with_expression(err: CheckErrors, expr: &SymbolicExpression) -> Self {
+        let mut r = Self::new(err);
+        r.set_expression(expr);
+        r
+    }
+}
+
+impl From<(SyntaxBindingError, &SymbolicExpression)> for CheckError {
+    fn from(e: (SyntaxBindingError, &SymbolicExpression)) -> Self {
+        Self::with_expression(CheckErrors::BadSyntaxBinding(e.0), e.1)
+    }
 }
 
 impl fmt::Display for CheckErrors {
@@ -362,7 +493,6 @@ impl DiagnosableError for CheckErrors {
             CheckErrors::MemoryBalanceExceeded(a, b) => format!("contract execution cost exceeded memory budget: {a:?} > {b:?}"),
             CheckErrors::InvalidTypeDescription => "supplied type description is invalid".into(),
             CheckErrors::EmptyTuplesNotAllowed => "tuple types may not be empty".into(),
-            CheckErrors::BadSyntaxExpectedListOfPairs => "bad syntax: function expects a list of pairs to bind names, e.g., ((name-0 a) (name-1 b) ...)".into(),
             CheckErrors::UnknownTypeName(name) => format!("failed to parse type: '{name}'"),
             CheckErrors::ValueTooLarge => "created a type which was greater than maximum allowed value size".into(),
             CheckErrors::ValueOutOfBounds => "created a type which value size was out of defined bounds".into(),
@@ -390,8 +520,7 @@ impl DiagnosableError for CheckErrors {
             CheckErrors::BadTupleFieldName => "invalid tuple field name".into(),
             CheckErrors::ExpectedTuple(type_signature) => format!("expecting tuple, found '{type_signature}'"),
             CheckErrors::NoSuchTupleField(field_name, tuple_signature) => format!("cannot find field '{field_name}' in tuple '{tuple_signature}'"),
-            CheckErrors::BadTupleConstruction => "invalid tuple syntax, expecting list of pair".into(),
-            CheckErrors::TupleExpectsPairs => "invalid tuple syntax, expecting pair".into(),
+            CheckErrors::BadTupleConstruction(message) => format!("invalid tuple syntax: {message}"),
             CheckErrors::NoSuchDataVariable(var_name) => format!("use of unresolved persisted variable '{var_name}'"),
             CheckErrors::BadTransferSTXArguments => "STX transfer expects an int amount, from principal, to principal".into(),
             CheckErrors::BadTransferFTArguments => "transfer expects an int amount, from principal, to principal".into(),
@@ -428,7 +557,7 @@ impl DiagnosableError for CheckErrors {
             CheckErrors::MaxLengthOverflow => format!("expecting a value <= {}", u32::MAX),
             CheckErrors::BadLetSyntax => "invalid syntax of 'let'".into(),
             CheckErrors::CircularReference(references) => format!("detected circular reference: ({})", references.join(", ")),
-            CheckErrors::BadSyntaxBinding => "invalid syntax binding".into(),
+            CheckErrors::BadSyntaxBinding(binding_error) => format!("invalid syntax binding: {}", &binding_error.message()),
             CheckErrors::MaxContextDepthReached => "reached depth limit".into(),
             CheckErrors::UndefinedVariable(var_name) => format!("use of unresolved variable '{var_name}'"),
             CheckErrors::UndefinedFunction(var_name) => format!("use of unresolved function '{var_name}'"),
@@ -476,9 +605,6 @@ impl DiagnosableError for CheckErrors {
 
     fn suggestion(&self) -> Option<String> {
         match &self {
-            CheckErrors::BadSyntaxBinding => {
-                Some("binding syntax example: ((supply int) (ttl int))".into())
-            }
             CheckErrors::BadLetSyntax => Some(
                 "'let' syntax example: (let ((supply 1000) (ttl 60)) <next-expression>)".into(),
             ),

clarity/src/vm/analysis/read_only_checker/mod.rs

@@ -19,6 +19,7 @@ use stacks_common::types::StacksEpochId;
 
 pub use super::errors::{
     check_argument_count, check_arguments_at_least, CheckError, CheckErrors, CheckResult,
+    SyntaxBindingError,
 };
 use super::AnalysisDatabase;
 use crate::vm::analysis::types::{AnalysisPass, ContractAnalysis};
@@ -324,10 +325,18 @@ impl<'a, 'b> ReadOnlyChecker<'a, 'b> {
 
                 let binding_list = args[0].match_list().ok_or(CheckErrors::BadLetSyntax)?;
 
-                for pair in binding_list.iter() {
-                    let pair_expression = pair.match_list().ok_or(CheckErrors::BadSyntaxBinding)?;
+                for (i, pair) in binding_list.iter().enumerate() {
+                    let pair_expression = pair.match_list().ok_or_else(|| {
+                        CheckError::with_expression(
+                            SyntaxBindingError::let_binding_not_list(i).into(),
+                            pair,
+                        )
+                    })?;
                     if pair_expression.len() != 2 {
-                        return Err(CheckErrors::BadSyntaxBinding.into());
+                        return Err(CheckError::with_expression(
+                            SyntaxBindingError::let_binding_invalid_length(i).into(),
+                            &SymbolicExpression::list(pair_expression.to_vec()),
+                        ));
                     }
 
                     if !self.check_read_only(&pair_expression[1])? {
@@ -364,11 +373,18 @@ impl<'a, 'b> ReadOnlyChecker<'a, 'b> {
                 self.check_expression_application_is_read_only(args)
             }
             TupleCons => {
-                for pair in args.iter() {
-                    let pair_expression =
-                        pair.match_list().ok_or(CheckErrors::TupleExpectsPairs)?;
+                for (i, pair) in args.iter().enumerate() {
+                    let pair_expression = pair.match_list().ok_or_else(|| {
+                        CheckError::with_expression(
+                            SyntaxBindingError::tuple_cons_not_list(i).into(),
+                            pair,
+                        )
+                    })?;
                     if pair_expression.len() != 2 {
-                        return Err(CheckErrors::TupleExpectsPairs.into());
+                        return Err(CheckError::with_expression(
+                            SyntaxBindingError::tuple_cons_invalid_length(i).into(),
+                            &SymbolicExpression::list(pair_expression.to_vec()),
+                        ));
                     }
 
                     if !self.check_read_only(&pair_expression[1])? {

clarity/src/vm/analysis/tests/mod.rs

@@ -123,14 +123,16 @@ fn test_no_such_tuple_field() {
 fn test_bad_tuple_construction() {
     let snippet = "(tuple (key 1) (key 2))";
     let err = mem_type_check(snippet).unwrap_err();
-    assert!(format!("{}", err.diagnostic).contains("invalid tuple syntax, expecting list of pair"));
+    assert!(format!("{}", err.diagnostic)
+        .contains("invalid tuple syntax: defining 'key' conflicts with previous value."));
 }
 
 #[test]
 fn test_tuple_expects_pairs() {
     let snippet = "(tuple (key 1) (key-with-missing-value))";
     let err = mem_type_check(snippet).unwrap_err();
-    assert!(format!("{}", err.diagnostic).contains("invalid tuple syntax, expecting pair"));
+    assert!(format!("{}", err.diagnostic)
+        .contains("invalid syntax binding: Tuple constructor item #2 is not a two-element list."));
 }
 
 #[test]

clarity/src/vm/analysis/type_checker/v2_05/mod.rs

@@ -28,6 +28,7 @@ use super::contexts::{TypeMap, TypingContext};
 use super::ContractAnalysis;
 pub use crate::vm::analysis::errors::{
     check_argument_count, check_arguments_at_least, CheckError, CheckErrors, CheckResult,
+    SyntaxBindingErrorType,
 };
 use crate::vm::analysis::AnalysisDatabase;
 use crate::vm::costs::cost_functions::ClarityCostFunction;
@@ -555,8 +556,12 @@ impl<'a, 'b> TypeChecker<'a, 'b> {
         let function_name = function_name
             .match_atom()
             .ok_or(CheckErrors::BadFunctionName)?;
-        let args = parse_name_type_pairs::<()>(StacksEpochId::Epoch2_05, args, &mut ())
-            .map_err(|_| CheckErrors::BadSyntaxBinding)?;
+        let args = parse_name_type_pairs::<()>(
+            StacksEpochId::Epoch2_05,
+            args,
+            SyntaxBindingErrorType::Eval,
+            &mut (),
+        )?;
 
         if self.function_return_tracker.is_some() {
             return Err(CheckErrors::Expects(