feat: gno type check

[ltzmaxwell]

Pinned Update:

The original https://github.com/gnolang/gno/pull/1426 is now divided into 4 parts, with the dependency relationship being: https://github.com/gnolang/gno/pull/1426 < https://github.com/gnolang/gno/pull/1775, https://github.com/gnolang/gno/pull/1426 <- https://github.com/gnolang/gno/pull/1890<- https://github.com/gnolang/gno/pull/1891.

Among these, the main part, https://github.com/gnolang/gno/pull/1426, has been supplemented and optimized for the missing parts in the type checks of the original implementation, specifically as follows:

• A new layer for type check is added(type_check.go). during the preprocess stage, the compatibility of operators and operands in expressions is checked, such as 1 - "a". This part used to be implemented as a runtime error, but now it is checked in type_check.go;

• Modifications have been made to checkOrConvertType to add conversion checks for constants, such as int(1) + int64(1), which previously would not trigger a compile-time error;

• Refined and improved several aspects of the handling logic for BinaryExpr during the preprocessing stage.

• The existing checkType has been renamed to assertAssignableTo.

==========================update complete=======================

Problem Definition

Please proceed to #1424.

======update: fix #1462 , tests located in gnovm/tests/files/type2. this issue is fixed since they share the same contexts of type check and conversion.

briefly for #1462, type of shift expression (or any composed expression involved shift expression) will be determined in the context they are used if they are untyped, also can be mutated by explicitly conversion with a type call.

==========================================================================================

Overview of Solution

checkOperandWithOp function:

Purpose: Newly introduced to evaluate operand compatibility before deep type analysis. Functionality: Employs predefined rules to quickly identify incompatible patterns (e.g., "a" << 1 is flagged as incompatible). Advantage: Prevents unnecessary processing by checkOrConvertType for clear mismatches.

checkOrConvertType function:

Role: Engages after checkOperandWithOp's clearance. It's the hub for core type checking and conversion. Key Improvement: Enhanced handling of const conversions by limiting it within a certain range. Example: In cases like int(1) + int(8), the issue of unregulated const conversion is addressed. Constraints: Mandatory const conversion is now limited to specific scenarios (e.g., explicit conversion, operand in array/slice index, RHS of a shift expression).

Specific Problems Solved

1. assignable and sameType check: This code should output "something else". the root cause for this is Error(0) is assignable to errCmp since it satisfies the interface of error, and result in inequality since the have different concrete type in runtime. Thanks @jaekwon for pointing out my mistake and give an improved version of this.

package main

import (
    "errors"
    "strconv"
)

type Error int64

func (e Error) Error() string {
    return "error: " + strconv.Itoa(int(e))
}

var errCmp = errors.New("XXXX")

func main() {
    if Error(0) == errCmp {
        println("what the firetruck?")
    } else {
        println("something else")
    }
}

2. Early Incompatibility Detection: Conducted during preprocessing, not runtime. Example:

package main
func main() {
    println(1 / "a") // Detects incompatibility early.
}

func main() {
    println(int(1) == int8(1))  // this is checked before checkOrConvertType if LHS and RHS are both typed.
}

~~3. Implicit Conversion:~~(this is split out) ~~Focus: Ensuring accurate conversions, particularly unnamed to named types.~~ ~~Example:~~ ~~go~~ ~~package main~~ ~~type word uint~~ ~~type nat []word~~ ~~func (n nat) add() bool {~~ ~~ return true~~ ~~} ~~func Gen() nat {~~ ~~n := []word{0}~~ ~~return n~~ ~~}~~ ~~func main() {~~ ~~r := Gen()~~ ~~switch r.(type) {~~ ~~ case nat:~~ ~~println("nat")~~ ~~println(r.add())~~ ~~default:~~ ~~println("should not happen")~~ ~~ }~~ ~~}~~ ~~~~ ~~4. Type of Shift Expressions:~~ ~~Context: Determines the type based on usage context and explicit conversions.~~ ~~Implementation: Additional checks in assignStmt, callExpr for potential untyped shift expressions (or else expressions with untyped shift expression embedded)~~~~appear, e.g. uint64(1 << x). This will trigger a potentially recursive check&convert until the shift expr got its final type.~~

Conclusion:

This PR enhances the type check workflow and addresses previously overlooked aspects, resolving a variety of type-related issues.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 56.39464% with 358 lines in your changes missing coverage. Please review.

Project coverage is 54.61%. Comparing base (6284669) to head (0ced1b7). Report is 1 commits behind head on master.

Files	Patch %	Lines
gnovm/pkg/gnolang/type_check.go	63.58%	160 Missing and 41 partials :warning:
gnovm/pkg/gnolang/preprocess.go	62.41%	44 Missing and 12 partials :warning:
gnovm/pkg/gnolang/values.go	15.62%	50 Missing and 4 partials :warning:
gnovm/pkg/gnolang/op_binary.go	0.00%	17 Missing :warning:
gnovm/tests/file.go	26.66%	11 Missing :warning:
gnovm/pkg/gnolang/op_assign.go	0.00%	9 Missing :warning:
gnovm/pkg/gnolang/types.go	44.44%	4 Missing and 1 partial :warning:
gnovm/pkg/gnolang/values_conversions.go	20.00%	3 Missing and 1 partial :warning:
gnovm/pkg/gnolang/op_exec.go	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1426      +/-   ##
==========================================
- Coverage   54.64%   54.61%   -0.03%     
==========================================
  Files         581      582       +1     
  Lines       77967    78353     +386     
==========================================
+ Hits        42602    42794     +192     
- Misses      32187    32349     +162     
- Partials     3178     3210      +32     

Flag	Coverage Δ
gno.land	61.86% <ø> (ø)
gnovm	59.73% <56.39%> (-0.33%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[moul]

We will prioritize the review of this PR during the next review meeting. Thank you again, Maxwell, and I apologize for the delay in the review.

[jaekwon]

The implementation of isEql hasn't changed. Is this addressing the Error(0) == errCmp firetruck issue? If so, how? Also can this PR be broken down? Or before that, how about we sync by voice or txt? I'll be looking at this & #1501 concurrently. Also please check this comment: https://github.com/gnolang/gno/issues/1424#issuecomment-1887857271

I'll be back to check this tonight and tomorrow. I'm on the move and only available sporadically

[ltzmaxwell]

The implementation of isEql hasn't changed.

The reason why master branch is giving out unexpected result on this case is that assertEqualityTypes does not signal inequality to the caller-> doOpEql().

Is this addressing the Error(0) == errCmp firetruck issue?

The case of Error(0) == errCmp is one of the problems solved. And it is not dealing with correctness only, it also improves the type check flow. e.g. comparison like "errCmp" > Error(0), in the current setup, would fail on runtime(isGtr), but will fail on preprocess stage on the proposed PR.

Here is a simple example to show how it works:

for case Error(0) == errCmp:

1. first check of the dest type(type of errCmp) is compatible with ==, this is accomplished in checkOp, a new introduced func that works as a pre-check before checkOrConvertType, it works fast and will filter out most incompatible operation, like "a" << 1, "errCmp" > Error(0), etc;
2. if 1 pass, check the other side can be converted to this side, in this case, Error(0) is convertible to errCmp cuz it conforms the interface of error;
3. if 2 pass, preprocess stage check ends. It goes to equality evaluation in runtime，and finally get the result of this Expr, that should be false since they have different underlying type.

for case Error(0) > errCmp:

1. first check of the dest type is compatible with >, the check won't pass since the type of errCmp is interface, it can not be used as the operand of >.
2. 1 failed. process end.

[ltzmaxwell]

Also can this PR be broken down?

I actually hoped to do this. This PR proposed an improved flow for the type check, and solved several cases of problems based on it, so it makes it a bit harder to break it down into separate part, but I'd love to.

[ltzmaxwell]

Or before that, how about we sync by voice or txt? I'll be looking at this & https://github.com/gnolang/gno/pull/1501 concurrently. Also please check this comment: https://github.com/gnolang/gno/issues/1424#issuecomment-1887857271

Yes, sure!

[ltzmaxwell]

Hi @piux2 , Thank you so much for your time in reviewing this!!

It seems there are multiple intents for this PR, accompanied by a lot of code changes. Could we break the PR down into pieces, based on your definition of the problem to solve?

Yes, I think we can keep the main task in this PR (which might still be quite large), while moving the other branch tasks to separate PRs (though they will have a direct dependency on this main PR, which might be a bit troublesome). The current picture in my mind maybe to split #1462 into a separate one. @thehowl, What are your thoughts on this? I'd love to hear your feedback! :)

Below are my observations;

1. Refactoring the code related to type checking: ( what problem does this solve, easy to debug, easy to understand, better performance, etc)

The whole problem definition is in #1424. Please proceed to.

a. Introduced a new abstraction by using checkOperandWithOp before checkTypeAndConvert and within checkTypeAndConvert recursively. ( What is the end result that the early check brings us in the preprocessing?)

what checkOperandWithOp provides is to check the compatibility with operand and Op, to quickly find the incompatible situation before checkOrConvertType.(This kind of check in the current setup happens in runtime).

b. Replaced checkType with checkAssignable() and moved the code from preprocess.go to type.go.

yes, also with some other checks newly introduced like assertComparable.

2. Fixed a bug

several bugs that share similar context are fixed. please proceed to #1424.:)

3. Conducted some performance tuning in preprocessing. (Is this a side effect or a target problem to solve?) Is there a benchmark to compare this with? To conduct performance tuning, not only do we need to identify the bottleneck and see the real improvement, but we also need to assess whether the changes create other performance issues.

The work enhances the checking process by migrating some of compatibility check logic from runtime to preprocessing, such as what happens with checkOperandWithOp. It also integrates new logic to address previously unresolved issues. Consequently, these modifications are likely to impact performance. Given the significant changes introduced, it seems prudent to perform benchmarking to assess the effects thoroughly.

[ltzmaxwell]

WIP to break down into separate ones. block until done.

[zivkovicmilos]

@ltzmaxwell What is the status of breaking up this PR?

[ltzmaxwell]

@ltzmaxwell What is the status of breaking up this PR?

it’s almost done, making final confirmation.

[ltzmaxwell]

Update:

The original #1426 is now divided into 4 parts, with the dependency relationship being: #1426 < #1775, #1426 <- #1890<- #1891.

Among these, the main part, #1426, has been supplemented and optimized for the missing parts in the type checks of the original implementation, specifically as follows:

1. A new layer for type check is added(type_check.go). during the preprocess stage, the compatibility of operators and operands in expressions is checked, such as 1 - "a". This part used to be implemented as a runtime error, but now it is checked in type_check.go;

2. Modifications have been made to checkOrConvertType to add conversion checks for constants, such as int(1) + int64(1), which previously would not trigger a compile-time error;

3. Refined and improved several aspects of the handling logic for BinaryExpr during the preprocessing stage.

4. The existing checkType has been renamed to checkAssignableTo, fix bug with unnamed type conversion.

[thehowl]

2. Modifications have been made to checkOrConvertType to add conversion checks for constants, such as int(1) + int(8), which previously would not trigger a compile-time error;

Do you mean like const x = int(1) + int64(2)?

Thank you for the split. I'll make a review now

[ltzmaxwell]

2. Modifications have been made to checkOrConvertType to add conversion checks for constants, such as int(1) + int(8), which previously would not trigger a compile-time error;

Do you mean like const x = int(1) + int64(2)?

Thank you for the split. I'll make a review now

yes that's the intention. (int(1) + int(8) should be int(1) + int8(1), or int(1) + int64(1), etc).

[piux2]

@ltzmaxwell can you check, do we have some over lapping work with #1141 #1246 #1143? thanks!

[ltzmaxwell]

@ltzmaxwell can you check, do we have some over lapping work with #1141 #1246 #1143? thanks!

Hey @piux2 , I think they have different intentions, that the work in #1141, #1246, #1143 is mainly focus on named <-> unamed assignment(check&conversion), which is excluded from this PR.

[piux2]

@ltzmaxwell can you check, do we have some over lapping work with #1141 #1246 #1143? thanks!

Hey @piux2 , I think they have different intentions, that the work in #1141, #1246, #1143 is mainly focus on named <-> unamed assignment(check&conversion), which is excluded from this PR.

What about 3rd issue to solve: " Implicit Conversion: Focus: Ensuring accurate conversions, particularly unnamed to named types. "

[deelawn]

@ltzmaxwell can you resolve the conflicts with the main branch?

[ltzmaxwell]

@ltzmaxwell can you resolve the conflicts with the main branch?

this is resolved.

[ltzmaxwell]

Sorry it has taken so long to review; I had to carve our some dedicated time to focus on this. I primarily reviewed the code in type_check.go. I will begin the rest soon. Also have a look at the changes in the #1246 PR as there may be some conflicting approaches with how some of the unnamed types are converted.

I think we need to talk about this to smooth the merge process, even though I believe there are not much modifications about unnamed types in this PR.

[thehowl]

cc @deelawn, @jaekwon -- please review

[jaekwon]

This is next on my plate before the varloop review. Looking.