chore: address PR feedback

Author: jcnelson

clarity/src/vm/analysis/errors.rs

@@ -29,7 +29,6 @@ pub enum SyntaxBindingErrorType {
     Let,
     Eval,
     TupleCons,
-    TypeDefinition,
 }
 
 impl fmt::Display for SyntaxBindingErrorType {
@@ -44,7 +43,6 @@ impl DiagnosableError for SyntaxBindingErrorType {
             Self::Let => "Let-binding".to_string(),
             Self::Eval => "Function argument definition".to_string(),
             Self::TupleCons => "Tuple constructor".to_string(),
-            Self::TypeDefinition => "Type definition".to_string(),
         }
     }
 
@@ -57,16 +55,11 @@ impl DiagnosableError for SyntaxBindingErrorType {
 #[derive(Debug, PartialEq)]
 pub enum SyntaxBindingError {
     /// binding list item is not a list
-    NotList(SyntaxBindingErrorType, usize, SymbolicExpression),
+    NotList(SyntaxBindingErrorType, usize),
     /// binding list item has an invalid length (e.g. not 2)
-    InvalidLength(SyntaxBindingErrorType, usize, SymbolicExpression),
+    InvalidLength(SyntaxBindingErrorType, usize),
     /// binding name is not an atom
-    NotAtom(SyntaxBindingErrorType, usize, SymbolicExpression),
-    /// second binding item is a type signature, and the type signature itself is bad.
-    /// NOTE: type signature parsing returns CheckErrors, so we cannot include a CheckErrors here
-    /// directly without creating a recursive type. Instead, we just report the `Display`
-    /// representation of the error here as the third item.
-    BadTypeSignature(usize, SymbolicExpression, String),
+    NotAtom(SyntaxBindingErrorType, usize),
 }
 
 impl fmt::Display for SyntaxBindingError {
@@ -78,30 +71,17 @@ impl fmt::Display for SyntaxBindingError {
 impl DiagnosableError for SyntaxBindingError {
     fn message(&self) -> String {
         match &self {
-            Self::NotList(err_type, item_index, item) => {
+            Self::NotList(err_type, item_index) => {
                 let item_no = item_index + 1;
-                format!(
-                    "{err_type} item #{item_no} is not a list: {}",
-                    item.as_error_string()
-                )
+                format!("{err_type} item #{item_no} is not a list",)
             }
-            Self::InvalidLength(err_type, item_index, item) => {
+            Self::InvalidLength(err_type, item_index) => {
                 let item_no = item_index + 1;
-                format!(
-                    "{err_type} item #{item_no} is not a two-element list: {}",
-                    item.as_error_string()
-                )
+                format!("{err_type} item #{item_no} is not a two-element list",)
             }
-            Self::NotAtom(err_type, item_index, item) => {
+            Self::NotAtom(err_type, item_index) => {
                 let item_no = item_index + 1;
-                format!(
-                    "{err_type} item #{item_no}'s name is not an atom: {}",
-                    item.as_error_string()
-                )
-            }
-            Self::BadTypeSignature(item_index, item, error_message) => {
-                let item_no = item_index + 1;
-                format!("Type definition item #{item_no} has an invalid type signature: {} (reason: {error_message})", item.as_error_string())
+                format!("{err_type} item #{item_no}'s name is not an atom",)
             }
         }
     }
@@ -112,49 +92,55 @@ impl DiagnosableError for SyntaxBindingError {
 }
 
 impl SyntaxBindingError {
-    /// Helper constructor for NotList(SyntaxBindingErrorType::Let, item_no, item)
-    pub fn let_binding_not_list(item_no: usize, item: SymbolicExpression) -> Self {
-        Self::NotList(SyntaxBindingErrorType::Let, item_no, item)
+    /// 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 InvalidLength(SyntaxBindingErrorType::Let, item_no, item)
-    pub fn let_binding_invalid_length(item_no: usize, item: SymbolicExpression) -> Self {
-        Self::InvalidLength(SyntaxBindingErrorType::Let, item_no, item)
+    /// 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 NotAtom(SyntaxBindingErrorType::Let, item_no, item)
-    pub fn let_binding_not_atom(item_no: usize, item: SymbolicExpression) -> Self {
-        Self::NotAtom(SyntaxBindingErrorType::Let, item_no, item)
+    /// 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 NotList(SyntaxBindingErrorType::Eval, item_no, item)
-    pub fn eval_binding_not_list(item_no: usize, item: SymbolicExpression) -> Self {
-        Self::NotList(SyntaxBindingErrorType::Eval, item_no, item)
+    /// 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 InvalidLength(SyntaxBindingErrorType::Eval, item_no, item)
-    pub fn eval_binding_invalid_length(item_no: usize, item: SymbolicExpression) -> Self {
-        Self::InvalidLength(SyntaxBindingErrorType::Eval, item_no, item)
+    /// 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 NotAtom(SyntaxBindingErrorType::Eval, item_no, item)
-    pub fn eval_binding_not_atom(item_no: usize, item: SymbolicExpression) -> Self {
-        Self::NotAtom(SyntaxBindingErrorType::Eval, item_no, item)
+    /// 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 NotList(SyntaxBindingErrorType::TupleCons, item_no, item)
-    pub fn tuple_cons_not_list(item_no: usize, item: SymbolicExpression) -> Self {
-        Self::NotList(SyntaxBindingErrorType::TupleCons, item_no, item)
+    /// 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 InvalidLength(SyntaxBindingErrorType::TupleCons, item_no, item)
-    pub fn tuple_cons_invalid_length(item_no: usize, item: SymbolicExpression) -> Self {
-        Self::InvalidLength(SyntaxBindingErrorType::TupleCons, item_no, item)
+    /// Helper constructor for NotAtom(SyntaxBindingErrorType::TupleCons, item_no)
+    pub fn tuple_cons_not_atom(item_no: usize) -> Self {
+        Self::NotAtom(SyntaxBindingErrorType::TupleCons, item_no)
     }
+}
 
-    /// Helper constructor for NotAtom(SyntaxBindingErrorType::TupleCons, item_no, item)
-    pub fn tuple_cons_not_atom(item_no: usize, item: SymbolicExpression) -> Self {
-        Self::NotAtom(SyntaxBindingErrorType::TupleCons, item_no, item)
+impl From<SyntaxBindingError> for CheckErrors {
+    fn from(e: SyntaxBindingError) -> Self {
+        Self::BadSyntaxBinding(e)
     }
 }
 
@@ -348,11 +334,6 @@ impl CheckErrors {
             CheckErrors::SupertypeTooLarge | CheckErrors::Expects(_)
         )
     }
-
-    /// Is the given error message due to a BadSyntaxBinding?
-    pub fn has_nested_bad_syntax_binding_message(msg: &str) -> bool {
-        msg.contains("invalid syntax binding: ")
-    }
 }
 
 impl CheckError {
@@ -378,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 {

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

@@ -327,19 +327,16 @@ impl<'a, 'b> ReadOnlyChecker<'a, 'b> {
 
                 for (i, pair) in binding_list.iter().enumerate() {
                     let pair_expression = pair.match_list().ok_or_else(|| {
-                        CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_not_list(
-                            i,
-                            pair.clone(),
-                        ))
+                        CheckError::with_expression(
+                            SyntaxBindingError::let_binding_not_list(i).into(),
+                            pair,
+                        )
                     })?;
                     if pair_expression.len() != 2 {
-                        return Err(CheckErrors::BadSyntaxBinding(
-                            SyntaxBindingError::let_binding_invalid_length(
-                                i,
-                                SymbolicExpression::list(pair_expression.to_vec()),
-                            ),
-                        )
-                        .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])? {
@@ -378,19 +375,16 @@ impl<'a, 'b> ReadOnlyChecker<'a, 'b> {
             TupleCons => {
                 for (i, pair) in args.iter().enumerate() {
                     let pair_expression = pair.match_list().ok_or_else(|| {
-                        CheckErrors::BadSyntaxBinding(SyntaxBindingError::tuple_cons_not_list(
-                            i,
-                            pair.clone(),
-                        ))
+                        CheckError::with_expression(
+                            SyntaxBindingError::tuple_cons_not_list(i).into(),
+                            pair,
+                        )
                     })?;
                     if pair_expression.len() != 2 {
-                        return Err(CheckErrors::BadSyntaxBinding(
-                            SyntaxBindingError::tuple_cons_invalid_length(
-                                i,
-                                SymbolicExpression::list(pair_expression.to_vec()),
-                            ),
-                        )
-                        .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

@@ -131,7 +131,8 @@ fn test_bad_tuple_construction() {
 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 syntax binding: Tuple constructor item #2 is not a two-element list: ( key-with-missing-value )."));
+    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/tests/mod.rs

@@ -21,14 +21,13 @@ use crate::vm::analysis::mem_type_check;
 use crate::vm::analysis::type_checker::v2_05::TypeResult;
 use crate::vm::ast::build_ast;
 use crate::vm::ast::errors::ParseErrors;
-use crate::vm::diagnostic::DiagnosableError;
 use crate::vm::types::SequenceSubtype::*;
 use crate::vm::types::StringSubtype::*;
 use crate::vm::types::TypeSignature::{BoolType, IntType, PrincipalType, UIntType};
 use crate::vm::types::{
     FixedFunction, FunctionType, QualifiedContractIdentifier, TypeSignature, BUFF_32, BUFF_64,
 };
-use crate::vm::{ClarityVersion, SymbolicExpression, Value};
+use crate::vm::ClarityVersion;
 mod assets;
 mod contracts;
 
@@ -647,16 +646,8 @@ fn test_simple_lets() {
     ];
 
     let bad_expected = [
-        CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_invalid_length(
-            0,
-            SymbolicExpression::list(vec![
-                SymbolicExpression::literal_value(Value::Int(1)).with_id(5)
-            ]),
-        )),
-        CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_not_atom(
-            0,
-            SymbolicExpression::literal_value(Value::Int(1)).with_id(5),
-        )),
+        CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_invalid_length(0)),
+        CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_not_atom(0)),
         CheckErrors::TypeError(TypeSignature::IntType, TypeSignature::UIntType),
     ];
 
@@ -1245,12 +1236,7 @@ fn test_empty_tuple_should_fail() {
         )
         .unwrap_err()
         .err,
-        CheckErrors::BadSyntaxBinding(SyntaxBindingError::BadTypeSignature(
-            0,
-            SymbolicExpression::list(vec![SymbolicExpression::atom("tuple".into()).with_id(8)])
-                .with_id(7),
-            CheckErrors::EmptyTuplesNotAllowed.message()
-        ))
+        CheckErrors::EmptyTuplesNotAllowed
     );
 }
 
@@ -2480,18 +2466,7 @@ fn test_buff_negative_len() {
         StacksEpochId::Epoch2_05,
     )
     .unwrap_err();
-    assert_eq!(
-        res.err,
-        CheckErrors::BadSyntaxBinding(SyntaxBindingError::BadTypeSignature(
-            0,
-            SymbolicExpression::list(vec![
-                SymbolicExpression::atom("buff".into()).with_id(8),
-                SymbolicExpression::literal_value(Value::Int(-12)).with_id(9),
-            ])
-            .with_id(7),
-            CheckErrors::ValueOutOfBounds.message()
-        ))
-    );
+    assert_eq!(res.err, CheckErrors::ValueOutOfBounds);
 }
 
 #[test]
@@ -2505,18 +2480,7 @@ fn test_string_ascii_negative_len() {
         StacksEpochId::Epoch2_05,
     )
     .unwrap_err();
-    assert_eq!(
-        res.err,
-        CheckErrors::BadSyntaxBinding(SyntaxBindingError::BadTypeSignature(
-            0,
-            SymbolicExpression::list(vec![
-                SymbolicExpression::atom("string-ascii".into()).with_id(8),
-                SymbolicExpression::literal_value(Value::Int(-12)).with_id(9),
-            ])
-            .with_id(7),
-            CheckErrors::ValueOutOfBounds.message()
-        ))
-    );
+    assert_eq!(res.err, CheckErrors::ValueOutOfBounds);
 }
 
 #[test]
@@ -2530,16 +2494,5 @@ fn test_string_utf8_negative_len() {
         StacksEpochId::Epoch2_05,
     )
     .unwrap_err();
-    assert_eq!(
-        res.err,
-        CheckErrors::BadSyntaxBinding(SyntaxBindingError::BadTypeSignature(
-            0,
-            SymbolicExpression::list(vec![
-                SymbolicExpression::atom("string-utf8".into()).with_id(8),
-                SymbolicExpression::literal_value(Value::Int(-12)).with_id(9),
-            ])
-            .with_id(7),
-            CheckErrors::ValueOutOfBounds.message()
-        ))
-    );
+    assert_eq!(res.err, CheckErrors::ValueOutOfBounds);
 }