Validate define functions

[ameeratgithub]

This PR fixes #391

[codecov]

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 ⚠️
clar2wasm/src/wasm_generator.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            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     

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

[Acaccia]

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.

[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

[Acaccia]

I don't really know what is happening here. Why was contract_analysis.get_public_function_type() called there? Why is it safe to remove it? This function was already used for checking bindings, can it be used with the same rules for other-thingies definitions?

Also, you did not address @cylewitruk remark about Clarity version or epoch.
For example, (define-private (index-of?) (ok u0)) would work in Clarity 1 but not in Clarity 2.

[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.

[Acaccia]

The fix looks good, but I'm not entirely sure I perfectly understood the issue with get_public_function_type. So maybe @krl should actually validate this?

However, I have an issue with the usage of crosscheck_compare_only. When it is easy to determine the output of a snippet, we should check that it corresponds to our expectation. It's also easier for someone who will read your tests later to know which tests should work and which are expected to fail.

[ameeratgithub]

requested changes made :)

[krl]

lgtm

[Acaccia]

A few remarks about the naming.

Also I remark that there are tests with different Epochs only for the function definition functions. Shouldn't there be some for the other kind of defines?

[ameeratgithub]

@Acaccia could you give it a look again?

[Acaccia]

Lgtm