diff --git a/CHANGELOG.md b/CHANGELOG.md index ada3a8dac0..b164bf0e05 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/clarity/src/vm/analysis/errors.rs b/clarity/src/vm/analysis/errors.rs index 29a5081626..692639467c 100644 --- a/clarity/src/vm/analysis/errors.rs +++ b/clarity/src/vm/analysis/errors.rs @@ -23,6 +23,127 @@ use crate::vm::types::{TraitIdentifier, TupleTypeSignature, TypeSignature, Value pub type CheckResult = Result; +/// 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 { + 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 { + 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 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 { match &self { - CheckErrors::BadSyntaxBinding => { - Some("binding syntax example: ((supply int) (ttl int))".into()) - } CheckErrors::BadLetSyntax => Some( "'let' syntax example: (let ((supply 1000) (ttl 60)) )".into(), ), diff --git a/clarity/src/vm/analysis/read_only_checker/mod.rs b/clarity/src/vm/analysis/read_only_checker/mod.rs index a244bf7101..97de5bd42c 100644 --- a/clarity/src/vm/analysis/read_only_checker/mod.rs +++ b/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])? { diff --git a/clarity/src/vm/analysis/tests/mod.rs b/clarity/src/vm/analysis/tests/mod.rs index 01d5e98136..8fe286ce30 100644 --- a/clarity/src/vm/analysis/tests/mod.rs +++ b/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] diff --git a/clarity/src/vm/analysis/type_checker/v2_05/mod.rs b/clarity/src/vm/analysis/type_checker/v2_05/mod.rs index 40bc742334..2638e6ae4e 100644 --- a/clarity/src/vm/analysis/type_checker/v2_05/mod.rs +++ b/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( diff --git a/clarity/src/vm/analysis/type_checker/v2_05/natives/mod.rs b/clarity/src/vm/analysis/type_checker/v2_05/natives/mod.rs index c0dfef5350..840a03c2dc 100644 --- a/clarity/src/vm/analysis/type_checker/v2_05/natives/mod.rs +++ b/clarity/src/vm/analysis/type_checker/v2_05/natives/mod.rs @@ -19,9 +19,10 @@ use stacks_common::types::StacksEpochId; use super::{ check_argument_count, check_arguments_at_least, no_type, TypeChecker, TypeResult, TypingContext, }; -use crate::vm::analysis::errors::{CheckError, CheckErrors}; +use crate::vm::analysis::errors::{CheckError, CheckErrors, SyntaxBindingErrorType}; use crate::vm::costs::cost_functions::ClarityCostFunction; use crate::vm::costs::{analysis_typecheck_cost, runtime_cost}; +use crate::vm::diagnostic::DiagnosableError; use crate::vm::functions::{handle_binding_list, NativeFunctions}; use crate::vm::types::{ BlockInfoProperty, FixedFunction, FunctionArg, FunctionSignature, FunctionType, PrincipalData, @@ -188,20 +189,24 @@ pub fn check_special_tuple_cons( let mut tuple_type_data = Vec::with_capacity(len); - handle_binding_list(args, |var_name, var_sexp| { - checker.type_check(var_sexp, context).and_then(|var_type| { - runtime_cost( - ClarityCostFunction::AnalysisTupleItemsCheck, - checker, - var_type.type_size()?, - )?; - tuple_type_data.push((var_name.clone(), var_type)); - Ok(()) - }) - })?; + handle_binding_list( + args, + SyntaxBindingErrorType::TupleCons, + |var_name, var_sexp| { + checker.type_check(var_sexp, context).and_then(|var_type| { + runtime_cost( + ClarityCostFunction::AnalysisTupleItemsCheck, + checker, + var_type.type_size()?, + )?; + tuple_type_data.push((var_name.clone(), var_type)); + Ok(()) + }) + }, + )?; let tuple_signature = TupleTypeSignature::try_from(tuple_type_data) - .map_err(|_e| CheckErrors::BadTupleConstruction)?; + .map_err(|e| CheckErrors::BadTupleConstruction(e.message()))?; Ok(TypeSignature::TupleType(tuple_signature)) } @@ -221,26 +226,30 @@ fn check_special_let( runtime_cost(ClarityCostFunction::AnalysisCheckLet, checker, args.len())?; - handle_binding_list(binding_list, |var_name, var_sexp| { - checker.contract_context.check_name_used(var_name)?; - if out_context.lookup_variable_type(var_name).is_some() { - return Err(CheckError::new(CheckErrors::NameAlreadyUsed( - var_name.to_string(), - ))); - } + handle_binding_list( + binding_list, + SyntaxBindingErrorType::Let, + |var_name, var_sexp| { + checker.contract_context.check_name_used(var_name)?; + if out_context.lookup_variable_type(var_name).is_some() { + return Err(CheckError::new(CheckErrors::NameAlreadyUsed( + var_name.to_string(), + ))); + } - let typed_result = checker.type_check(var_sexp, &out_context)?; + let typed_result = checker.type_check(var_sexp, &out_context)?; - runtime_cost( - ClarityCostFunction::AnalysisBindName, - checker, - typed_result.type_size()?, - )?; - out_context - .variable_types - .insert(var_name.clone(), typed_result); - Ok(()) - })?; + runtime_cost( + ClarityCostFunction::AnalysisBindName, + checker, + typed_result.type_size()?, + )?; + out_context + .variable_types + .insert(var_name.clone(), typed_result); + Ok(()) + }, + )?; checker.type_check_consecutive_statements(&args[1..args.len()], &out_context) } diff --git a/clarity/src/vm/analysis/type_checker/v2_05/tests/mod.rs b/clarity/src/vm/analysis/type_checker/v2_05/tests/mod.rs index 68c7bec3b6..8316931eed 100644 --- a/clarity/src/vm/analysis/type_checker/v2_05/tests/mod.rs +++ b/clarity/src/vm/analysis/type_checker/v2_05/tests/mod.rs @@ -16,7 +16,7 @@ use stacks_common::types::StacksEpochId; -use crate::vm::analysis::errors::CheckErrors; +use crate::vm::analysis::errors::{CheckErrors, SyntaxBindingError}; use crate::vm::analysis::mem_type_check; use crate::vm::analysis::type_checker::v2_05::TypeResult; use crate::vm::ast::build_ast; @@ -28,7 +28,6 @@ use crate::vm::types::{ FixedFunction, FunctionType, QualifiedContractIdentifier, TypeSignature, BUFF_32, BUFF_64, }; use crate::vm::ClarityVersion; - mod assets; mod contracts; @@ -647,8 +646,8 @@ fn test_simple_lets() { ]; let bad_expected = [ - CheckErrors::BadSyntaxBinding, - CheckErrors::BadSyntaxBinding, + CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_invalid_length(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_not_atom(0)), CheckErrors::TypeError(TypeSignature::IntType, TypeSignature::UIntType), ]; @@ -1237,7 +1236,7 @@ fn test_empty_tuple_should_fail() { ) .unwrap_err() .err, - CheckErrors::BadSyntaxBinding + CheckErrors::EmptyTuplesNotAllowed ); } @@ -2467,7 +2466,7 @@ fn test_buff_negative_len() { StacksEpochId::Epoch2_05, ) .unwrap_err(); - assert!(matches!(res.err, CheckErrors::BadSyntaxBinding)); + assert_eq!(res.err, CheckErrors::ValueOutOfBounds); } #[test] @@ -2481,7 +2480,7 @@ fn test_string_ascii_negative_len() { StacksEpochId::Epoch2_05, ) .unwrap_err(); - assert!(matches!(res.err, CheckErrors::BadSyntaxBinding)); + assert_eq!(res.err, CheckErrors::ValueOutOfBounds); } #[test] @@ -2495,5 +2494,5 @@ fn test_string_utf8_negative_len() { StacksEpochId::Epoch2_05, ) .unwrap_err(); - assert!(matches!(res.err, CheckErrors::BadSyntaxBinding)); + assert_eq!(res.err, CheckErrors::ValueOutOfBounds); } diff --git a/clarity/src/vm/analysis/type_checker/v2_1/mod.rs b/clarity/src/vm/analysis/type_checker/v2_1/mod.rs index 17ee17f615..25da96eb9b 100644 --- a/clarity/src/vm/analysis/type_checker/v2_1/mod.rs +++ b/clarity/src/vm/analysis/type_checker/v2_1/mod.rs @@ -27,7 +27,7 @@ use super::contexts::{TypeMap, TypingContext}; use super::ContractAnalysis; pub use crate::vm::analysis::errors::{ check_argument_count, check_arguments_at_least, check_arguments_at_most, CheckError, - CheckErrors, CheckResult, + CheckErrors, CheckResult, SyntaxBindingErrorType, }; use crate::vm::analysis::AnalysisDatabase; use crate::vm::costs::cost_functions::ClarityCostFunction; @@ -1251,8 +1251,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::Epoch21, args, &mut ()) - .map_err(|_| CheckErrors::BadSyntaxBinding)?; + let args = parse_name_type_pairs::<()>( + StacksEpochId::Epoch21, + args, + SyntaxBindingErrorType::Eval, + &mut (), + )?; if self.function_return_tracker.is_some() { return Err(CheckErrors::Expects( diff --git a/clarity/src/vm/analysis/type_checker/v2_1/natives/mod.rs b/clarity/src/vm/analysis/type_checker/v2_1/natives/mod.rs index 500632f0dd..65ba214359 100644 --- a/clarity/src/vm/analysis/type_checker/v2_1/natives/mod.rs +++ b/clarity/src/vm/analysis/type_checker/v2_1/natives/mod.rs @@ -20,9 +20,10 @@ use super::{ check_argument_count, check_arguments_at_least, check_arguments_at_most, compute_typecheck_cost, no_type, TypeChecker, TypeResult, TypingContext, }; -use crate::vm::analysis::errors::{CheckError, CheckErrors}; +use crate::vm::analysis::errors::{CheckError, CheckErrors, SyntaxBindingErrorType}; use crate::vm::costs::cost_functions::ClarityCostFunction; use crate::vm::costs::{analysis_typecheck_cost, runtime_cost, CostErrors, CostTracker}; +use crate::vm::diagnostic::DiagnosableError; use crate::vm::functions::{handle_binding_list, NativeFunctions}; use crate::vm::types::signatures::{ CallableSubtype, FunctionArgSignature, FunctionReturnsSignature, SequenceSubtype, ASCII_40, @@ -231,30 +232,36 @@ pub fn check_special_tuple_cons( let mut type_size = 0u32; let mut cons_error = Ok(()); - handle_binding_list(args, |var_name, var_sexp| { - checker.type_check(var_sexp, context).and_then(|var_type| { - runtime_cost( - ClarityCostFunction::AnalysisTupleItemsCheck, - checker, - var_type.type_size()?, - )?; - if type_size < MAX_VALUE_SIZE { - type_size = type_size - .saturating_add(var_name.len() as u32) - .saturating_add(var_name.len() as u32) - .saturating_add(var_type.type_size()?) - .saturating_add(var_type.size()?); - tuple_type_data.push((var_name.clone(), var_type)); - } else { - cons_error = Err(CheckErrors::BadTupleConstruction); - } - Ok(()) - }) - })?; + handle_binding_list( + args, + SyntaxBindingErrorType::TupleCons, + |var_name, var_sexp| { + checker.type_check(var_sexp, context).and_then(|var_type| { + runtime_cost( + ClarityCostFunction::AnalysisTupleItemsCheck, + checker, + var_type.type_size()?, + )?; + if type_size < MAX_VALUE_SIZE { + type_size = type_size + .saturating_add(var_name.len() as u32) + .saturating_add(var_name.len() as u32) + .saturating_add(var_type.type_size()?) + .saturating_add(var_type.size()?); + tuple_type_data.push((var_name.clone(), var_type)); + } else { + cons_error = Err(CheckErrors::BadTupleConstruction(format!( + "type size of {type_size} bytes exceeds maximum of {MAX_VALUE_SIZE} bytes" + ))); + } + Ok(()) + }) + }, + )?; cons_error?; let tuple_signature = TupleTypeSignature::try_from(tuple_type_data) - .map_err(|_e| CheckErrors::BadTupleConstruction)?; + .map_err(|e| CheckErrors::BadTupleConstruction(e.message()))?; Ok(TypeSignature::TupleType(tuple_signature)) } @@ -276,33 +283,37 @@ fn check_special_let( runtime_cost(ClarityCostFunction::AnalysisCheckLet, checker, args.len())?; let mut added_memory = 0u64; - handle_binding_list(binding_list, |var_name, var_sexp| { - checker.contract_context.check_name_used(var_name)?; - if out_context.lookup_variable_type(var_name).is_some() { - return Err(CheckError::new(CheckErrors::NameAlreadyUsed( - var_name.to_string(), - ))); - } + handle_binding_list( + binding_list, + SyntaxBindingErrorType::Let, + |var_name, var_sexp| { + checker.contract_context.check_name_used(var_name)?; + if out_context.lookup_variable_type(var_name).is_some() { + return Err(CheckError::new(CheckErrors::NameAlreadyUsed( + var_name.to_string(), + ))); + } - let typed_result = checker.type_check(var_sexp, &out_context)?; + let typed_result = checker.type_check(var_sexp, &out_context)?; - runtime_cost( - ClarityCostFunction::AnalysisBindName, - checker, - typed_result.type_size()?, - )?; - if checker.epoch.analysis_memory() { - let memory_use = u64::from(var_name.len()) - .checked_add(u64::from(typed_result.type_size()?)) - .ok_or_else(|| CostErrors::CostOverflow)?; - added_memory = added_memory - .checked_add(memory_use) - .ok_or_else(|| CostErrors::CostOverflow)?; - checker.add_memory(memory_use)?; - } - out_context.add_variable_type(var_name.clone(), typed_result, checker.clarity_version); - Ok(()) - })?; + runtime_cost( + ClarityCostFunction::AnalysisBindName, + checker, + typed_result.type_size()?, + )?; + if checker.epoch.analysis_memory() { + let memory_use = u64::from(var_name.len()) + .checked_add(u64::from(typed_result.type_size()?)) + .ok_or_else(|| CostErrors::CostOverflow)?; + added_memory = added_memory + .checked_add(memory_use) + .ok_or_else(|| CostErrors::CostOverflow)?; + checker.add_memory(memory_use)?; + } + out_context.add_variable_type(var_name.clone(), typed_result, checker.clarity_version); + Ok(()) + }, + )?; let res = checker.type_check_consecutive_statements(&args[1..args.len()], &out_context); if checker.epoch.analysis_memory() { diff --git a/clarity/src/vm/analysis/type_checker/v2_1/tests/mod.rs b/clarity/src/vm/analysis/type_checker/v2_1/tests/mod.rs index bb3e86a6ec..65fe442461 100644 --- a/clarity/src/vm/analysis/type_checker/v2_1/tests/mod.rs +++ b/clarity/src/vm/analysis/type_checker/v2_1/tests/mod.rs @@ -21,7 +21,7 @@ use rstest_reuse::{self, *}; use stacks_common::types::StacksEpochId; use super::CheckResult; -use crate::vm::analysis::errors::CheckErrors; +use crate::vm::analysis::errors::{CheckErrors, SyntaxBindingError}; use crate::vm::analysis::mem_type_check as mem_run_analysis; use crate::vm::analysis::type_checker::v2_1::TypeResult; use crate::vm::analysis::types::ContractAnalysis; @@ -979,8 +979,8 @@ fn test_simple_lets() { ]; let bad_expected = [ - CheckErrors::BadSyntaxBinding, - CheckErrors::BadSyntaxBinding, + CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_invalid_length(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_not_atom(0)), CheckErrors::TypeError(TypeSignature::IntType, TypeSignature::UIntType), ]; @@ -1948,7 +1948,7 @@ fn test_empty_tuple_should_fail() { assert_eq!( mem_type_check(contract_src).unwrap_err().err, - CheckErrors::BadSyntaxBinding + CheckErrors::EmptyTuplesNotAllowed, ); } @@ -3101,7 +3101,7 @@ fn test_buff_negative_len() { (func 0x00)"; let res = mem_type_check(contract_src).unwrap_err(); - assert!(matches!(res.err, CheckErrors::BadSyntaxBinding)); + assert_eq!(res.err, CheckErrors::ValueOutOfBounds); } #[test] @@ -3110,7 +3110,7 @@ fn test_string_ascii_negative_len() { (func \"\")"; let res = mem_type_check(contract_src).unwrap_err(); - assert!(matches!(res.err, CheckErrors::BadSyntaxBinding)); + assert_eq!(res.err, CheckErrors::ValueOutOfBounds); } #[test] @@ -3119,7 +3119,7 @@ fn test_string_utf8_negative_len() { (func u\"\")"; let res = mem_type_check(contract_src).unwrap_err(); - assert!(matches!(res.err, CheckErrors::BadSyntaxBinding)); + assert_eq!(res.err, CheckErrors::ValueOutOfBounds); } #[test] @@ -3654,3 +3654,77 @@ fn test_principal_admits() { assert!(res.is_err()); } } + +/// Comprehensive test of all of the bad syntax binding error variants we can detect. +/// Only concerns itself with simple errors (no nesting). +#[test] +fn test_simple_bad_syntax_bindings() { + let bad = [ + // bad let-binding -- binding item is not a list + "(let (oops (bar u1)) (ok true))", + // bad let-binding -- binding item is not a 2-element list + "(let ((oops u1 u2) (bar u1)) (ok true))", + // bad let-binding -- binding item name is not an atom + "(let ((1 2) (bar u1)) (ok true))", + // bad eval-binding -- binding item is not a list + "(define-private (foo oops (bar uint)) (ok true))", + // bad eval-binding -- binding item is not a 2-element list + "(define-private (foo (oops uint uint) (bar uint)) (ok true))", + // bad eval-binding -- binding item name is not an atom + "(define-private (foo (u1 uint) (bar uint)) (ok true))", + // bad tuple binding -- binding item is not a list + "(tuple oops (bar u1))", + // bad tuple binding -- binding item is not a 2-element list + "(tuple (oops u1 u2) (bar u1))", + // bad tuple binding -- binding item name is not an atom + "(tuple (u1 u2) (bar u1))", + // bad type signature (no longer a bad syntax binding error) + "(define-private (foo (bar (string-ascii -12))) (ok true))", + // bad type signature (no longer a bad syntax binding error) + "(from-consensus-buff? (tuple (a (string-ascii -12))) 0x00)", + ]; + let expected = [ + CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_not_list(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_invalid_length(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::let_binding_not_atom(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::eval_binding_not_list(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::eval_binding_invalid_length(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::eval_binding_not_atom(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::tuple_cons_not_list(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::tuple_cons_invalid_length(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::tuple_cons_not_atom(0)), + CheckErrors::ValueOutOfBounds, + CheckErrors::ValueOutOfBounds, + ]; + + for (bad_code, expected_err) in bad.iter().zip(expected.iter()) { + debug!("test simple bad syntax binding: '{}'", bad_code); + assert_eq!(expected_err, &type_check_helper(bad_code).unwrap_err().err); + } +} + +/// Nested type signature binding errors. +/// These are no longer BadSyntaxBinding errors, but are instead reported as their innermost +/// type-check error. +#[test] +fn test_nested_bad_type_signature_syntax_bindings() { + let bad = [ + // bad tuple type signature within a tower of tuples + "(define-public (foo (bar { a: { b: { c: (string-ascii -19) } } })) (ok true))", + // bad type signature within a tower of lists + "(define-public (foo (bar (list (list 10 (string-ascii -19))))) (ok true))", + // bad type signature within a tower of tuples + "(from-consensus-buff? { a : { b: { c: (string-ascii -19) } } } 0x00)", + ]; + + let expected = [ + CheckErrors::ValueOutOfBounds, + CheckErrors::InvalidTypeDescription, + CheckErrors::ValueOutOfBounds, + ]; + + for (bad_code, expected_err) in bad.iter().zip(expected.iter()) { + debug!("test nested bad syntax binding: '{}'", bad_code); + assert_eq!(expected_err, &type_check_helper(bad_code).unwrap_err().err); + } +} diff --git a/clarity/src/vm/errors.rs b/clarity/src/vm/errors.rs index d46f7a9ca1..852551e2ce 100644 --- a/clarity/src/vm/errors.rs +++ b/clarity/src/vm/errors.rs @@ -24,6 +24,7 @@ use stacks_common::types::chainstate::BlockHeaderHash; use super::ast::errors::ParseErrors; pub use crate::vm::analysis::errors::{ check_argument_count, check_arguments_at_least, check_arguments_at_most, CheckErrors, + SyntaxBindingError, SyntaxBindingErrorType, }; use crate::vm::ast::errors::ParseError; use crate::vm::contexts::StackTrace; diff --git a/clarity/src/vm/functions/define.rs b/clarity/src/vm/functions/define.rs index 1e11ff76e9..1e7907a708 100644 --- a/clarity/src/vm/functions/define.rs +++ b/clarity/src/vm/functions/define.rs @@ -20,6 +20,7 @@ use crate::vm::callables::{DefineType, DefinedFunction}; use crate::vm::contexts::{ContractContext, Environment, LocalContext}; use crate::vm::errors::{ check_argument_count, check_arguments_at_least, CheckErrors, InterpreterResult as Result, + SyntaxBindingErrorType, }; use crate::vm::eval; use crate::vm::representations::SymbolicExpressionType::Field; @@ -141,7 +142,8 @@ fn handle_define_function( check_legal_define(function_name, env.contract_context)?; - let arguments = parse_name_type_pairs(*env.epoch(), arg_symbols, env)?; + let arguments = + parse_name_type_pairs(*env.epoch(), arg_symbols, SyntaxBindingErrorType::Eval, env)?; for (argument, _) in arguments.iter() { check_legal_define(argument, env.contract_context)?; diff --git a/clarity/src/vm/functions/mod.rs b/clarity/src/vm/functions/mod.rs index 7c92d9a929..f28594920f 100644 --- a/clarity/src/vm/functions/mod.rs +++ b/clarity/src/vm/functions/mod.rs @@ -21,7 +21,7 @@ use crate::vm::costs::cost_functions::ClarityCostFunction; use crate::vm::costs::{constants as cost_constants, runtime_cost, CostTracker, MemoryConsumer}; use crate::vm::errors::{ check_argument_count, check_arguments_at_least, CheckErrors, Error, - InterpreterResult as Result, ShortReturnType, + InterpreterResult as Result, ShortReturnType, SyntaxBindingError, SyntaxBindingErrorType, }; pub use crate::vm::functions::assets::stx_transfer_consolidated; use crate::vm::representations::{ClarityName, SymbolicExpression, SymbolicExpressionType}; @@ -664,20 +664,29 @@ fn special_asserts( pub fn handle_binding_list( bindings: &[SymbolicExpression], + binding_error_type: SyntaxBindingErrorType, mut handler: F, ) -> std::result::Result<(), E> where F: FnMut(&ClarityName, &SymbolicExpression) -> std::result::Result<(), E>, E: From, { - for binding in bindings.iter() { - let binding_expression = binding.match_list().ok_or(CheckErrors::BadSyntaxBinding)?; + for (i, binding) in bindings.iter().enumerate() { + let binding_expression = binding.match_list().ok_or_else(|| { + CheckErrors::BadSyntaxBinding(SyntaxBindingError::NotList(binding_error_type, i)) + })?; if binding_expression.len() != 2 { - return Err(CheckErrors::BadSyntaxBinding.into()); + return Err( + CheckErrors::BadSyntaxBinding(SyntaxBindingError::InvalidLength( + binding_error_type, + i, + )) + .into(), + ); } - let var_name = binding_expression[0] - .match_atom() - .ok_or(CheckErrors::BadSyntaxBinding)?; + let var_name = binding_expression[0].match_atom().ok_or_else(|| { + CheckErrors::BadSyntaxBinding(SyntaxBindingError::NotAtom(binding_error_type, i)) + })?; let var_sexp = &binding_expression[1]; handler(var_name, var_sexp)?; @@ -687,11 +696,12 @@ where pub fn parse_eval_bindings( bindings: &[SymbolicExpression], + binding_error_type: SyntaxBindingErrorType, env: &mut Environment, context: &LocalContext, ) -> Result> { let mut result = Vec::with_capacity(bindings.len()); - handle_binding_list(bindings, |var_name, var_sexp| { + handle_binding_list(bindings, binding_error_type, |var_name, var_sexp| { eval(var_sexp, env, context).map(|value| result.push((var_name.clone(), value))) })?; @@ -719,7 +729,7 @@ fn special_let( let mut memory_use = 0; finally_drop_memory!( env, memory_use; { - handle_binding_list::<_, Error>(bindings, |binding_name, var_sexp| { + handle_binding_list::<_, Error>(bindings, SyntaxBindingErrorType::Let, |binding_name, var_sexp| { if is_reserved(binding_name, env.contract_context.get_clarity_version()) || env.contract_context.lookup_function(binding_name).is_some() || inner_context.lookup_variable(binding_name).is_some() { diff --git a/clarity/src/vm/functions/tuples.rs b/clarity/src/vm/functions/tuples.rs index 44519f1320..5aefe70e60 100644 --- a/clarity/src/vm/functions/tuples.rs +++ b/clarity/src/vm/functions/tuples.rs @@ -17,7 +17,7 @@ use crate::vm::costs::cost_functions::ClarityCostFunction; use crate::vm::costs::runtime_cost; use crate::vm::errors::{ check_argument_count, check_arguments_at_least, CheckErrors, InterpreterError, - InterpreterResult as Result, + InterpreterResult as Result, SyntaxBindingErrorType, }; use crate::vm::representations::SymbolicExpression; use crate::vm::types::{TupleData, TypeSignature, Value}; @@ -34,7 +34,7 @@ pub fn tuple_cons( check_arguments_at_least(1, args)?; - let bindings = parse_eval_bindings(args, env, context)?; + let bindings = parse_eval_bindings(args, SyntaxBindingErrorType::TupleCons, env, context)?; runtime_cost(ClarityCostFunction::TupleCons, env, bindings.len())?; TupleData::from_data(bindings).map(Value::from) diff --git a/clarity/src/vm/representations.rs b/clarity/src/vm/representations.rs index 08ddedf069..94134cb595 100644 --- a/clarity/src/vm/representations.rs +++ b/clarity/src/vm/representations.rs @@ -622,11 +622,25 @@ impl SymbolicExpression { None } } + + /// Encode this SymbolicExpression as a String suitable for logging an error (such as in + /// CheckErrors). The `developer-mode` feature includes the `span`. + #[cfg(feature = "developer-mode")] + pub fn as_error_string(&self) -> String { + format!("{} at {:?}", &self.expr, &self.span) + } + + /// Encode this SymbolicExpression as a String suitable for logging an error (such as in + /// CheckErrors). + #[cfg(not(feature = "developer-mode"))] + pub fn as_error_string(&self) -> String { + format!("{}", &self.expr) + } } -impl fmt::Display for SymbolicExpression { +impl fmt::Display for SymbolicExpressionType { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self.expr { + match self { SymbolicExpressionType::List(ref list) => { write!(f, "(")?; for item in list.iter() { @@ -647,12 +661,17 @@ impl fmt::Display for SymbolicExpression { SymbolicExpressionType::Field(ref value) => { write!(f, "<{value}>")?; } - }; - + } Ok(()) } } +impl fmt::Display for SymbolicExpression { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", &self.expr) + } +} + #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] pub struct Span { pub start_line: u32, diff --git a/clarity/src/vm/tests/datamaps.rs b/clarity/src/vm/tests/datamaps.rs index 90d9e2a642..74072b5062 100644 --- a/clarity/src/vm/tests/datamaps.rs +++ b/clarity/src/vm/tests/datamaps.rs @@ -17,7 +17,7 @@ use crate::vm::errors::Error; use crate::vm::types::{TupleData, Value}; #[cfg(test)] use crate::vm::{ - errors::{CheckErrors, ShortReturnType}, + errors::{CheckErrors, ShortReturnType, SyntaxBindingError}, types::{ListData, SequenceData, TupleTypeSignature, TypeSignature}, }; use crate::vm::{execute, ClarityName}; @@ -645,7 +645,7 @@ fn bad_define_maps() { "(define-map lists { name: int } { contents: (list 5 0 int) })", ]; let expected: Vec = vec![ - CheckErrors::BadSyntaxExpectedListOfPairs.into(), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::tuple_cons_invalid_length(0)).into(), CheckErrors::UnknownTypeName("contents".to_string()).into(), CheckErrors::ExpectedName.into(), CheckErrors::IncorrectArgumentCount(3, 4).into(), @@ -670,8 +670,8 @@ fn bad_tuples() { ]; let expected = vec![ CheckErrors::NameAlreadyUsed("name".into()), - CheckErrors::BadSyntaxBinding, - CheckErrors::BadSyntaxBinding, + CheckErrors::BadSyntaxBinding(SyntaxBindingError::tuple_cons_not_list(0)), + CheckErrors::BadSyntaxBinding(SyntaxBindingError::tuple_cons_invalid_length(1)), CheckErrors::NoSuchTupleField( "value".into(), TupleTypeSignature::try_from(vec![("name".into(), TypeSignature::IntType)]).unwrap(), diff --git a/clarity/src/vm/tests/defines.rs b/clarity/src/vm/tests/defines.rs index fea98b7298..235d1af885 100644 --- a/clarity/src/vm/tests/defines.rs +++ b/clarity/src/vm/tests/defines.rs @@ -25,6 +25,7 @@ use crate::vm::errors::{CheckErrors, Error}; use crate::vm::tests::test_clarity_versions; #[cfg(test)] use crate::vm::{ + analysis::errors::SyntaxBindingError, ast::{ build_ast, errors::{ParseError, ParseErrors}, @@ -279,7 +280,7 @@ fn test_define_parse_panic() { fn test_define_parse_panic_2() { let tests = "(define-private (a b (d)) 1)"; assert_eq_err( - CheckErrors::BadSyntaxExpectedListOfPairs, + CheckErrors::BadSyntaxBinding(SyntaxBindingError::eval_binding_not_list(0)), execute(tests).unwrap_err(), ); } diff --git a/clarity/src/vm/tests/mod.rs b/clarity/src/vm/tests/mod.rs index bf9cc68581..0ed4eb5d22 100644 --- a/clarity/src/vm/tests/mod.rs +++ b/clarity/src/vm/tests/mod.rs @@ -23,6 +23,7 @@ use super::ClarityVersion; use crate::vm::contexts::OwnedEnvironment; pub use crate::vm::database::BurnStateDB; use crate::vm::database::MemoryBackingStore; +use crate::vm::SymbolicExpression; #[cfg(test)] use crate::{vm::errors::Error, vm::types::Value}; @@ -223,3 +224,10 @@ pub fn test_only_mainnet_to_chain_id(mainnet: bool) -> u32 { CHAIN_ID_TESTNET } } + +impl SymbolicExpression { + pub fn with_id(mut self, id: u64) -> Self { + self.id = id; + self + } +} diff --git a/clarity/src/vm/types/signatures.rs b/clarity/src/vm/types/signatures.rs index a7fa8434f0..bacd9f1d6d 100644 --- a/clarity/src/vm/types/signatures.rs +++ b/clarity/src/vm/types/signatures.rs @@ -26,7 +26,7 @@ use lazy_static::lazy_static; use stacks_common::types::StacksEpochId; use crate::vm::costs::{runtime_cost, CostOverflowingMath}; -use crate::vm::errors::CheckErrors; +use crate::vm::errors::{CheckErrors, SyntaxBindingError, SyntaxBindingErrorType}; use crate::vm::representations::{ ClarityName, ContractName, SymbolicExpression, SymbolicExpressionType, TraitDefinition, CONTRACT_MAX_NAME_LENGTH, @@ -950,19 +950,6 @@ impl TupleTypeSignature { Ok(true) } - pub fn parse_name_type_pair_list( - epoch: StacksEpochId, - type_def: &SymbolicExpression, - accounting: &mut A, - ) -> Result { - if let SymbolicExpressionType::List(ref name_type_pairs) = type_def.expr { - let mapped_key_types = parse_name_type_pairs(epoch, name_type_pairs, accounting)?; - TupleTypeSignature::try_from(mapped_key_types) - } else { - Err(CheckErrors::BadSyntaxExpectedListOfPairs) - } - } - pub fn shallow_merge(&mut self, update: &mut TupleTypeSignature) { Arc::make_mut(&mut self.type_map).append(Arc::make_mut(&mut update.type_map)); } @@ -1507,7 +1494,12 @@ impl TypeSignature { type_args: &[SymbolicExpression], accounting: &mut A, ) -> Result { - let mapped_key_types = parse_name_type_pairs(epoch, type_args, accounting)?; + let mapped_key_types = parse_name_type_pairs( + epoch, + type_args, + SyntaxBindingErrorType::TupleCons, + accounting, + )?; let tuple_type_signature = TupleTypeSignature::try_from(mapped_key_types)?; Ok(TypeSignature::from(tuple_type_signature)) } @@ -1924,11 +1916,15 @@ use crate::vm::costs::cost_functions::ClarityCostFunction; use crate::vm::costs::CostTracker; use crate::vm::ClarityVersion; +/// Try to parse a list of (name_i, type_i) pairs into Vec<(ClarityName, TypeSignature)>. +/// On failure, return both the type-check error as well as the index of the symbolic expression which caused +/// the problem (for purposes of reporting the error). pub fn parse_name_type_pairs( epoch: StacksEpochId, name_type_pairs: &[SymbolicExpression], + binding_error_type: SyntaxBindingErrorType, accounting: &mut A, -) -> Result> { +) -> std::result::Result, CheckErrors> { // this is a pretty deep nesting here, but what we're trying to do is pick out the values of // the form: // ((name1 type1) (name2 type2) (name3 type3) ...) @@ -1936,28 +1932,37 @@ pub fn parse_name_type_pairs( use crate::vm::representations::SymbolicExpressionType::List; // step 1: parse it into a vec of symbolicexpression pairs. - let as_pairs: Result> = name_type_pairs + let as_pairs: std::result::Result, CheckErrors> = name_type_pairs .iter() - .map(|key_type_pair| { + .enumerate() + .map(|(i, key_type_pair)| { if let List(ref as_vec) = key_type_pair.expr { if as_vec.len() != 2 { - Err(CheckErrors::BadSyntaxExpectedListOfPairs) + Err(CheckErrors::BadSyntaxBinding( + SyntaxBindingError::InvalidLength(binding_error_type, i), + )) } else { Ok((&as_vec[0], &as_vec[1])) } } else { - Err(CheckErrors::BadSyntaxExpectedListOfPairs) + Err(SyntaxBindingError::NotList(binding_error_type, i).into()) } }) .collect(); // step 2: turn into a vec of (name, typesignature) pairs. - let key_types: Result> = (as_pairs?) + let key_types: std::result::Result, CheckErrors> = (as_pairs?) .iter() - .map(|(name_symbol, type_symbol)| { + .enumerate() + .map(|(i, (name_symbol, type_symbol))| { let name = name_symbol .match_atom() - .ok_or(CheckErrors::BadSyntaxExpectedListOfPairs)? + .ok_or_else(|| { + CheckErrors::BadSyntaxBinding(SyntaxBindingError::NotAtom( + binding_error_type, + i, + )) + })? .clone(); let type_info = TypeSignature::parse_type_repr(epoch, type_symbol, accounting)?; Ok((name, type_info))