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

fix: solve problem that variable (of declaredType) is not assigned with correct type #674

Closed
wants to merge 15 commits into from

Conversation

ltzmaxwell
Copy link
Contributor

@ltzmaxwell ltzmaxwell commented Mar 29, 2023

code like this won't work. when abs is assigned like this: abs = []Word{0}, abs is assigned with a type of []Word, thus abs.add3() does not work.

type nat3 []Word
type Word uint


func (n nat3) add3() bool {
	return true
}

func main() {
	var abs nat3
	abs = []Word{0}
	abs.add3()
}

another example, abs is a nested field:

type nat3 []Word
type Word uint

type Int3 struct {
	neg bool
	abs nat3
}

func (n nat3) add3() bool {
	return true
}

func main() {
	z := &Int3{
		neg: true,
		abs: []Word{0},
	}
	z.abs.add3()
}

The problem come from : #651 .

The solution is to give proper type to variable with declaredType.

@ltzmaxwell ltzmaxwell requested a review from a team as a code owner March 29, 2023 11:25
@ltzmaxwell ltzmaxwell changed the title (WIP)fix(gnolang): fix problem that variable is not correctly assigned with declaredType (WIP)fix: solve problem that variable is not correctly assigned with declaredType Mar 29, 2023
@ltzmaxwell ltzmaxwell marked this pull request as draft March 29, 2023 11:30
@ltzmaxwell ltzmaxwell changed the title (WIP)fix: solve problem that variable is not correctly assigned with declaredType fix: solve problem that variable is not correctly assigned with declaredType Mar 29, 2023
@ltzmaxwell ltzmaxwell changed the title fix: solve problem that variable is not correctly assigned with declaredType fix: solve problem that variable (of declaredType) is not correctly assigned with type. Mar 29, 2023
@ltzmaxwell
Copy link
Contributor Author

Hi, @r3v4s , would you check this.

@ltzmaxwell ltzmaxwell changed the title fix: solve problem that variable (of declaredType) is not correctly assigned with type. fix: solve problem that variable (of declaredType) is not assigned with correct type Mar 29, 2023
@moul moul added the 🐞 bug Something isn't working label Mar 29, 2023
tests/files/method38gno Outdated Show resolved Hide resolved
@piux2
Copy link
Contributor

piux2 commented Mar 31, 2023

@ltzmaxwell good findings! We are definitely in the right direction.

However, there are a lot of changes. Can you help describe the root cause and why the fix solved the problem?

I noticed a side effect in this test case.
examples/gno.land/r/demo/nft/z_0_filetest.gno

https://github.com/gnolang/gno/pull/674/files#diff-5a26a017ddff81d0ef1a36c2190cab1fedc6eac9cee7e404c62bc7a3bb73c303L54

//             "T": {
//             -    "@type": "/gno.PrimitiveType",
//              -  "value": "16"
//              +   "@type": "/gno.RefType",
//              +   "ID": "gno.land/p/demo/grc/grc721.TokenID"
//             },
//             "V": {
//                 "@type": "/gno.StringValue",
//                 "value": "NFT#1"
//             }
//         }

 type NFToken struct {
        owner    std.Address
        approved std.Address
        tokenID  grc721.TokenID
        data     string
    }

Above is the "data" field metadata in an NFTToken instance. The value is "NFT#1"
Its Type should be

"@type": "/gno.PrimitiveType", "value": "16"

instead of

@type": "/gno.RefType", "ID": "gno.land/p/demo/grc/grc721.TokenID"

@peter7891 peter7891 self-requested a review March 31, 2023 01:48
@ltzmaxwell
Copy link
Contributor Author

Thank you @piux2 , it's actually a bug, I'm giving fix.

@ltzmaxwell ltzmaxwell marked this pull request as ready for review April 7, 2023 08:06
@ltzmaxwell
Copy link
Contributor Author

@peter7891 is ok for review

@jaekwon
Copy link
Contributor

jaekwon commented May 4, 2023

Do not merge, please see #651 (comment)

Also please add me as a reviewer for all gnolang vm changes!

@ltzmaxwell ltzmaxwell requested a review from jaekwon as a code owner May 9, 2023 09:51
@ltzmaxwell ltzmaxwell changed the title fix: solve problem that variable (of declaredType) is not assigned with correct type [WIP]Gfix: solve problem that variable (of declaredType) is not assigned with correct type May 9, 2023
@ltzmaxwell ltzmaxwell changed the title [WIP]Gfix: solve problem that variable (of declaredType) is not assigned with correct type [WIP]fix: solve problem that variable (of declaredType) is not assigned with correct type May 9, 2023
@ltzmaxwell
Copy link
Contributor Author

WIP, block.

@zivkovicmilos zivkovicmilos added the don't merge Please don't merge this functionality temporarily label May 15, 2023
@ajnavarro ajnavarro added the 📦 🤖 gnovm Issues or PRs gnovm related label May 18, 2023
@ltzmaxwell ltzmaxwell changed the title [WIP]fix: solve problem that variable (of declaredType) is not assigned with correct type chore(fix): solve problem that variable (of declaredType) is not assigned with correct type May 30, 2023
@ltzmaxwell ltzmaxwell changed the title chore(fix): solve problem that variable (of declaredType) is not assigned with correct type fix: solve problem that variable (of declaredType) is not assigned with correct type May 30, 2023
@ltzmaxwell
Copy link
Contributor Author

ltzmaxwell commented May 30, 2023

Hi @jaekwon , Can you take a look at this? Thank you!

update:

1.enhanced preprocessing. An explicit conversion from unnamed-composite to named declared types has now been implemented. This solution addresses the majority of the issues encountered previously.

2.The copy function has also been fixed. According to the Go specification, "The core types of both arguments must be slices with identical element types". Our previous implementation lacked this necessary check, so we have now added it for improved robustness.

@ltzmaxwell
Copy link
Contributor Author

ltzmaxwell commented Jun 2, 2023

Hi, @jaekwon , I have another question, currently the code below would not work since n2 is not assigned with correct type from the assignment. It works in go playground: https://go.dev/play/p/pA0cqF-nuBV

the question is whether we can fix it in preprocess stage?
In case like, a, b = x, y., when traverse into AssignStmt node in preprocess, the Rhs can be easily converted to the target type , but in case like n1, n2 = x() , seems we can not do the same thing without a list of result Exprs of the CallExpr?

Thank you!

package main

type nat []int

func (n nat) add() bool {
	return true
}

func x() (nat, []int) {
	a := nat{0}
	b := []int{1}
	return a, b
}

func main() {
	var n1, n2 nat

	n1, n2 = x()

        n1.add()
	n2.add()

}

@jaekwon
Copy link
Contributor

jaekwon commented Jun 2, 2023

this is the solution we want: https://gist.github.com/piux2/49bada7be72186371e6684d007d3ed75?permalink_comment_id=4579166#gistcomment-4579166 links to https://github.com/gnolang/gno/compare/master...piux2:gno:fix_declaredtype_preprocess?expand=1 .

The thread on piux2's thread (first link) has comments on improvements and what is necessary to make complete.

To answer your exact question, yes we can and we will, but the simplest solution for now is to do the following:

// pseudocode
// on LEAVE AssignStmt around 1574 for `case *CallExpr:`
    ...
    if dest type is interface type { 
        ...
    } else if leftType is exactly rightType {
        // perfect match, continue
    } else if checkType(leftType, rightType) {
        panic("a, b := x() implicit conversion not supported yet")
    } else {
        panic("type check error")
    }

Ray has the context to complete this. Then, you can work with Ray to complete the next PR which is to do the expansion only when necessary.

@@ -2364,7 +2379,7 @@ func convertConst(store Store, last BlockNode, cx *ConstExpr, t Type) {
// Assert that xt can be assigned as dt (dest type).
// If autoNative is true, a broad range of xt can match against
// a target native dt type, if and only if dt is a native type.
func checkType(xt Type, dt Type, autoNative bool) {
func checkType(xt Type, dt Type, autoNative bool, conversionNeeded *bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, right. this would make sense (or it could just return the value as a bool, i think is simpler) to get the information out of the function. we will need this later for the full unroll solution (e.g. the second pr after ray's), but I suggest a return bool value instead.

args1T := evalStaticTypeOf(store, last, n.Args[1])
if args0T.Elem().TypeID() != args1T.Elem().TypeID() {
panic(fmt.Sprintf(
"arguments to copy have different element types, want %s, got %s", args0T.Elem().TypeID(), args1T.Elem().TypeID()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what prompted this? seems like this could/should be a separate PR for complete discussion. seems correct though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a fix of copy function. According to the Go specification, "The core types of both arguments must be slices with identical element types". Our previous implementation lacked this necessary check.

I'll put it into another PR. Thank you~

Copy link
Contributor

@jaekwon jaekwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can close this or @piux2 can merge here.

@ltzmaxwell
Copy link
Contributor Author

house keeping.

@ltzmaxwell ltzmaxwell closed this Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working don't merge Please don't merge this functionality temporarily 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: 🚀 Needed for Launch
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants