Raise for already used arg name in function definition

[matteojug]

Address #473.

To add the context to the error I've added two globals, please let me know if some other approach works better.
Also fixes crosscheck_expect_failure that wasn't really expecting failures (it was used for an earlier test in this PR).

If the approach sounds good I'll port the standard.wat edits to stacks-core and bump it here before merging this PR.

A similar approach for the error (adding also the stringified type) could be also used for #385.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.11%. Comparing base (9d5dd14) to head (085736d).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #510      +/-   ##
==========================================
+ Coverage   86.10%   86.11%   +0.01%     
==========================================
  Files          44       44              
  Lines       20708    20799      +91     
  Branches    20708    20799      +91     
==========================================
+ Hits        17830    17912      +82     
- Misses       1346     1347       +1     
- Partials     1532     1540       +8     

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

[Acaccia]

(Continued from stacks-network/stacks-core#5168 (review))

For now, we try to not change the typechecker implementation, since we don't have the capabilities to check for the repercussions any change will have.

For this issue, my idea of a fix would be to check if any argument name is already used when we traverse a function:

• if no, we keep going as usual
• if yes, the body of the function is directly a runtime error.

[matteojug]

Moving the convo here:

The interpreter will let the contract deploy, but then fail when invoked with an Unchecked error (#473 (comment)). Addressing this at typechecker level will give an early feedback about it.
Do you think we should keep the interpreter current behaviour (and address it only at compiler level)?

[Acaccia]

We cannot change the current interpreter behavior. The compiler should be a perfect replacement for it. Otherwise, believe me, we wouldn't have all those awful typechecker workarounds in the codebase :(

[Acaccia]

If the approach sounds good I'll port the standard.wat edits to stacks-core and bump it here before merging this PR.

Do we have any changes to make when adding a global? Not sure.

A similar approach for the error (adding also the stringified type) could be also used for #385.

Excellent idea :)

[matteojug]

Do we have any changes to make when adding a global? Not sure.

I was thinking about what would happen for contract calls (given the code duplication situation), but you're right: the global itself will not cause issues, as the code path involving it can only be triggered by the changed wasm (raising the new error type), so given the wasm change will not be present it should be fine as is.

[Acaccia]

No you are right, I just noticed that we have more code duplication here: https://github.com/stacks-network/stacks-core/blob/c3c3af4ba8602fb1ed3bc870376ef1f96237e24c/clarity/src/vm/clarity_wasm.rs#L7647

Yeah I guess we will need to add this to stacks-core too :(

[Acaccia]

LGTM

[csgui]

LGTM!