Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shift operator where first operand is an untyped bigint always results in a bigint #1462

Closed
thehowl opened this issue Dec 19, 2023 · 6 comments · Fixed by #1426
Closed

shift operator where first operand is an untyped bigint always results in a bigint #1462

thehowl opened this issue Dec 19, 2023 · 6 comments · Fixed by #1426
Assignees
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related

Comments

@thehowl
Copy link
Member

thehowl commented Dec 19, 2023

package main

func main() {
        x := 11
        y := uint64(1 << x)
        println(y)
}

// panic running expression main(): cannot convert BigintKind to Uint64Kind 

The solution is to explicitly convert the first operand to a uint64...

package main

func main() {
	x := 11
	y := uint64(uint64(1) << x)
	println(y)
}

// 2048
@thehowl thehowl added 🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related labels Dec 19, 2023
@thehowl thehowl added this to the 🚀 main.gno.land (required) milestone Dec 19, 2023
@thehowl thehowl self-assigned this Dec 19, 2023
@thehowl thehowl changed the title shift operator where LHS is an untyped bigint always results in a bigint shift operator where first operand is an untyped bigint always results in a bigint Dec 19, 2023
@deelawn
Copy link
Contributor

deelawn commented Dec 20, 2023

I'm not sure but on the surface the solution to this may be related to the solution in #1423

@ltzmaxwell
Copy link
Contributor

ltzmaxwell commented Dec 22, 2023

Hi,
I've submitted a fix at #1426. It's about the type check/conversion for some special case, e.g. shift expr or any expr involve a shift expr.

This was referenced Dec 22, 2023
@thehowl
Copy link
Member Author

thehowl commented Jan 8, 2024

Let me add an issue related to this. I'll avoid making this a separate issue, mostly because I believe Maxwell's #1426 likely tackles this as well, but I want to publicly document another situation where this creates issues.

Consider the following code:

package main

func main() {
	x := 1024
	p := 10
	if x != 1<<p {
		println("WHAT?")
	}
}

This will print WHAT?, unexpectedly (Go prints nothing).

Because x and 1 << p are different types (per the issue title, 1 << p is a Bigint), they are considered different.

Naturally, changing 1 to int(1) makes the code work as it should. ;_;

@ltzmaxwell, rest assured I will try to review your PR as soon as possible because tracing down bugs like these destroys my mental sanity.

@thehowl thehowl removed their assignment Jan 8, 2024
thehowl added a commit that referenced this issue Jan 8, 2024
@ltzmaxwell
Copy link
Contributor

Hi Morgan @thehowl,
this works:

package main

func main() {
	x := 1024
	p := 10
	if x != 1<<p {
		println("WHAT?")
	} else {
		println("nothing")
	}
}

// Output:
// nothing

@thehowl
Copy link
Member Author

thehowl commented Jan 9, 2024

@ltzmaxwell I'm getting WHAT? on master :)

Or do you mean on your branch?

@ltzmaxwell
Copy link
Contributor

@ltzmaxwell I'm getting WHAT? on master :)

Or do you mean on your branch?

Oh yes, I mean on my branch. Sorry for the ambiguity.

@thehowl thehowl moved this from Backlog to In Progress in 🧙‍♂️gno.land core team Apr 11, 2024
jaekwon added a commit that referenced this issue Jun 19, 2024
**Pinned Update:**

The original #1426 is now divided
into 4 parts, with the dependency relationship being:
#1426 <
#1775,
#1426 <-
https://github.com/gnolang/gno/pull/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:

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

```go
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**:
```go
package main
func main() {
    println(1 / "a") // Detects incompatibility early.
}
```
```go
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.

---------

Co-authored-by: Morgan <[email protected]>
Co-authored-by: jaekwon <[email protected]>
@github-project-automation github-project-automation bot moved this from In Progress to Done in 🧙‍♂️gno.land core team Jun 19, 2024
mvertes added a commit that referenced this issue Oct 15, 2024
[shift operator where first operand is an untyped bigint always results
in a bigint](#1462) is not resolved
by #1426, it's fixed by this one.

=================================================================
1. This is a fix to /issues/1462;
3. **NOTE**: This PR should be reviewed following the potential merger
of #1426, from which it is both decoupled and dependent. #1426 serves as
base branch of this one.
4. **NOTE**: Currently, this PR displays all code including that from
#1426, because it is being compared to the master branch instead of
differing against #1426 directly.

---------

Co-authored-by: Morgan <[email protected]>
Co-authored-by: Marc Vertes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Development

Successfully merging a pull request may close this issue.

4 participants