Validate define functions

[ameeratgithub]

This PR fixes #391

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 98.25871% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 87.28%. Comparing base (8878bf2) to head (6db0fb5). Report is 13 commits behind head on main.

Files	Patch %	Lines
clar2wasm/src/tools.rs	80.00%	4 Missing :warning:
clar2wasm/src/wasm_generator.rs	0.00%	3 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #404 +/- ## ========================================== + Coverage 86.95% 87.28% +0.33% ========================================== Files 43 43 Lines 18325 18788 +463 Branches 18325 18788 +463 ========================================== + Hits 15934 16400 +466 + Misses 1051 1050 -1 + Partials 1340 1338 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[cylewitruk]

I'm not entirely sure this is the fix we want here.

If you look at the top of the file, you'll see that tools.rs only contains functions used during testing. So, your fix only works in a testing context.

@Acaccia is correct. Also, for an implementation in the Clarity VM this kind of fix will need to be epoch- and/or clarity version-gated as existing contracts which break this rule must still function exactly the same.

[ameeratgithub]

I'm not entirely sure this is the fix we want here. If you look at the top of the file, you'll see that tools.rs only contains functions used during testing. So, your fix only works in a testing context.

@Acaccia is correct. Also, for an implementation in the Clarity VM this kind of fix will need to be epoch- and/or clarity version-gated as existing contracts which break this rule must still function exactly the same.

Yes. I've looked at clarity VM code related to that. Thanks for reminding me. I'll take a deeper look again.

[ameeratgithub]

The problem with generator.is_reserved_name() was contract_analysis.get_public_function_type(), which is also weird that it only checks public functions. During analysis, name get inserted in ContractAnalysis::public_function_types and in generator.is_reserved_name(), it always returns true.

It wasn't causing problem in bindings ("let" function) because "let" isn't a public function.

Analysis pass takes care of name conflict issues like this

(define-public (a) (ok true))
(define-public (a) (ok true))

That's why I've commented contract_analysis.get_public_function_type(). To make sure this works, I've added tests with conflicting names. If you want to add anything, please feel free to suggest @Acaccia @krl

[ameeratgithub]

@krl wrote this method and added contract_analysis.get_public_function_type(). The reason was to check if a custom function with the same name is already defined. If it's defined and there are two functions with the same name as below, it should throw error

(define-public (a) (ok true))
(define-public (a) (ok true))

If you take a look at struct ContractAnalysis in clarity/src/vm/analysis/types.rs, there's a field public_function_types, which is responsible for saving public function types during analysis pass. If I've following clarity function,

(define-public (a) (ok true))

during analysis pass, clarity function name, a in this case, gets inserted in public_function_types. When we call generator.is_reserved_name(), contract_analysis.get_public_function_type() will always return true because it's already there, and it's was causing the error that name is already defined (which is not the case). It also doesn't make sense because it just takes care of public functions. What about private, read-only and define-trait functions?

If the reason to call this method was to check if custom method is re-defined second time with same name, this doesn't seem the right approach. It occurred to me that analysis pass already takes care of that conflict problem and we don't need that check. If you take a look at tests, they contain conflicted names, and compiler's and interpreter's results are matching.

In bindings, it wasn't causing any problem because contract_analysis.get_public_function_type() would always return false, because it's not a public_function_type.

[ameeratgithub]

Regarding epoch comment, I want to ask something. There's a method called ClarityVersion::default_for_epoch(), is it all I need? I mean default for Epoch21 is Clarity2, is it possible to run Clarity1 in Epoch21? If It's possible, then are all combinations possible? Your help would be much appreciated @cylewitruk

[ameeratgithub]

I'm getting default Clarity version for a given epoch. I suppose Clarity2 should behave similar across different supported epochs. Correct me if I'm wrong.

[ameeratgithub]

requested changes made :)

[ameeratgithub]

@Acaccia could you give it a look again?