hirosystems / clarinet

Write, test and deploy high-quality smart contracts to the Stacks blockchain and Bitcoin.
https://hiro.so/clarinet
GNU General Public License v3.0
307 stars 141 forks source link

Incorrect number of arguments #228

Closed friedger closed 2 years ago

friedger commented 2 years ago

Incorrect number of arguments are not detected by clarinet check.

Contract:

(define-map user-profile principal principal)
(define-public (delete)
  (map-delete user-profile tx-sender contract-caller))

clarinet check passes

However, calling delete in clarinet console fails

Runtime Error: Unchecked(IncorrectArgumentCount(2, 3))

obycode commented 2 years ago

Hi Friedger. Thanks for reporting. I added an analysis pass in the REPL which checks the number of arguments for user-defined functions, but it doesn't check the builtins. I'll add this check to that pass.

obycode commented 2 years ago

This should actually be fixed in the type checker in the clarity library.

--- a/src/clarity/analysis/type_checker/natives/maps.rs
+++ b/src/clarity/analysis/type_checker/natives/maps.rs
@@ -5,8 +5,8 @@ use crate::clarity::functions::tuples;

 use super::check_special_tuple_cons;
 use crate::clarity::analysis::type_checker::{
-    check_arguments_at_least, no_type, CheckError, CheckErrors, TypeChecker, TypeResult,
-    TypingContext,
+    check_argument_count, check_arguments_at_least, no_type, CheckError, CheckErrors, TypeChecker,
+    TypeResult, TypingContext,
 };

 use crate::clarity::costs::cost_functions::ClarityCostFunction;
@@ -57,7 +57,7 @@ pub fn check_special_delete_entry(
     args: &[SymbolicExpression],
     context: &TypingContext,
 ) -> TypeResult {
-    check_arguments_at_least(2, args)?;
+    check_argument_count(2, args)?;

     let map_name = args[0].match_atom().ok_or(CheckErrors::BadMapName)?;

Since we intend to soon switch to a clarity library extracted from the blockchain code (stacks-network/stacks-blockchain#2985), I think the right move is to open a PR there with this change.

obycode commented 2 years ago

@lgalabru Should we also fix this in the clarity-repl repo for now? And if so, should it be fixed inside the clarity code, or in a separate analysis pass, since we expect to remove that copy/pasted clarity library soon. I fixed it in hirosystems/clarity-repl#86.