Added Checks for the Length of Arguments List

[BowTiedWoo]

Added checks in traverse functions throughout the code in order to verify that the number of arguments provided at compile-time is the expected one for the given function.

Reference issue: #159

[CLAassistant]

All committers have signed the CLA.

[Acaccia]

I would go for Generator::TypeError for all these. Giving one too many or too few arguments to a function doesn't respect the signature of the function, which I would qualify as a typing error.
What do you think?

InternalError should be reserved for the cases where something unexpected happens with the compilation process.

[csgui]

I would go for Generator::TypeError for all these...

@Acaccia I would expect a Generator::TypeError for an argument case if the function expects an int and I am passing a uint, for instance.

What you think in adding a new GeneratorError::ArgumentLenghtError for the cases in this PR? That should be simple. And it is more close to the thrown error messages.

[csgui]

Added a comment to discussion.

[BowTiedWoo]

Thank you for the feedback @Acaccia, @csgui !
I also think they are a bit different and GeneratorError::ArgumentLenghtError is more specific. I will create a new error type for that, if it's alright, as it doesn't add to much complexity on my end.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.38%. Comparing base (92dafc2) to head (341ec46).
Report is 24 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #533      +/-   ##
==========================================
+ Coverage   85.91%   86.38%   +0.47%     
==========================================
  Files          45       45              
  Lines       21257    22686    +1429     
  Branches    21257    22686    +1429     
==========================================
+ Hits        18263    19598    +1335     
- Misses       1422     1443      +21     
- Partials     1572     1645      +73     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Acaccia]

@chris We can always add more types of errors, our current error handling is left to be desired.

For example, in #516, I had to use a disgusting regex to match the string representation of the error instead of dealing directly with an error type. That was subpar.

I'm not entirely sure what this type would add compared to a type error here (whatever way I look at this, for me passing the wrong number of arguments to a function is a type error). But here you are the one who dealt with adapting our errors to the interpreter's, so I will defer to your judgement here.

[csgui]

Just adding more context: not sure if the ArgumentLengthError will be caught since a compile error is thrown before the traverse step.

@Acaccia what is your take on that? It seems that this PR should be handled after the work on compile-time error handling, issue #421.

[Acaccia]

Not sure I understand your question @csgui . Why should a compile-time error be thrown before traversal?

Should it be a runtime-error instead of compile-time?

UPDATE: we clarified it on slack. The issue is that some errors due to the number of arguments are detected during the typechecking phase, some aren't.

[Acaccia]

@hugocaillard did a few tests, and it seems some contracts can be deployed if the typechecking doesn't detect the ArgumentLengthError. For example, this can deploy:

(define-map squares { x: int } { square: int })

(define-public (insert)
  (begin
    (map-insert squares { x: 255 } { square: 2147483648 } { square: 1 })
    (ok true)
  )
)

@BowTiedWoo I would recommend you change all the

return Err(GeneratorError::ArgumentLengthError(format!(
                "... expected at least 2 arguments, got {}",
                args.len()
            )));

by the generation of a wasm runtime error.

@csgui has a lot of experience with runtime errors if you need any help.

[BowTiedWoo]

Just adding more context: not sure if the ArgumentLengthError will be caught since a compile error is thrown before the traverse step.

@Acaccia what is your take on that? It seems that this PR should be handled after the work on compile-time error handling, issue #421.

Hi, I've updated the tests and saw that they indeed do not always return the error I've added in traverse. I followed where the error was coming from and went into the type_checker in stacks-core to understand what was happening.

From here I've made some modifications for the type-checker to throw a compile time error for the functions that type_checker was not working as expected and tested them.

I opened a draft PR for this.

I wasn’t sure if it would create any backwards incompatibility, but from your message, @Acaccia, I understand that I shouldn’t modify on the stacks-core typechecker and only check runtime on our side. I will sync with @csgui when he has some time, I think that would be helpful for me in having the broader picture here.

[Acaccia]

@BowTiedWoo exactly, we cannot change the typechecker. The contracts that were already deployed "exploiting" this bug should still deploy with the compiled version.

We'll rewrite a new version later of the typechecker later, but we still need to be compatible with the 2 currently existing versions.

Here you will have to throw runtime-errors like this:

clarity-wasm/clar2wasm/src/words/arithmetic.rs

Lines 98 to 102 in db50363

	if arg_types.len() == 1 {
	// unary 'uint' subtraction:
	// throws an underflow runtime error.
	builder.i32_const(ErrorMap::ArithmeticUnderflow as i32);
	generator.func_by_name("stdlib.runtime-error")

It will require a new type of ErrorMap, and passing the numbers as arguments.

[BowTiedWoo]

Makes sense, thank you. Will start looking into it.

[Acaccia]

Nice job, now to replicate it to all functions.........

[csgui]

@BowTiedWoo, it might be better to update your branch with the latest main. There are some structural changes to error_mapping.rs that will affect (and simplify) your work.

[csgui]

@BowTiedWoo Great work so far! 👍 Now it's necessary to validate (and tag) all the tests you've written where the typechecker is failing and the runtime error is being thrown. You can check the example below and use it to guide your work. Let me know if you have any question. Thanks for the patience on all work on this issue.

fn get_block_info_more_than_two_args() {
    // TODO: see issue #488
    // The inconsistency in function arguments should have been caught by the typechecker.
    // The runtime error below is being used as a workaround for a typechecker issue
    // where certain errors are not properly handled.
    // This test should be re-worked once the typechecker is fixed
    // and can correctly detect all argument inconsistencies
    crosscheck(
        "(get-block-info? burnchain-header-hash u0 u0)",
        Err(Error::Unchecked(CheckErrors::IncorrectArgumentCount(2, 3))),
    )
}

[BowTiedWoo]

Thank you @csgui!

I added comments for the tests where typechecker fails.

[csgui]

Code LGTM! Thanks @BowTiedWoo for the great work on this PR!

[Acaccia]

Great job! Two remarks:

• there is an optimization that could be done by short-cutting the traversal of an expression.
• we should also handle the number of arguments of user-defined functions. Look for WasmGenerator::traverse_call_user_defined

[Acaccia]

Looks great, just two questions...

[Acaccia]

LGTM!

[csgui]

LGTM! Thanks.