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

[Bug ??] panic: interface conversion: gnolang.Type is *gnolang.SliceType, not *gnolang.DeclaredType #651

Closed
r3v4s opened this issue Mar 25, 2023 · 5 comments
Assignees
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.

Comments

@r3v4s
Copy link
Contributor

r3v4s commented Mar 25, 2023

This is related to #650.
I'm porting math/big from go to gno for gno to support what we call arbitrary-precision

Since math/big has bunch of implmentaion I'm not going to post full, bug only small part that I'm having issue.

image

this screenshot is part of Add()'s definitation in big/math

variable z is type of Int struct

type Int struct {
  neg bool // sign
  abs nat // absoulte value of the integer
}

// type nat []Word
// type Word uint

As you can see Int struct has abs member with type nat which is type []Word that actually uses uint
I'm not sure exact terms for that kind of define, but let's just call as a nested define.

67 line is origin code from go that panics on gno,
69 ~ 72 lines does fix panic by manually defining type, however I'm not sure this is right way to fix them.

If this is a bug, I thinkg pre-process stage is missing some jobs for declard type

If this is not a bug and meant to be intentional, I would like to hear some awesome ways to fix it.

@ltzmaxwell
Copy link
Contributor

ltzmaxwell commented Mar 28, 2023

I changed some code of yours, it works now, but I need further work into it:

func NewInt(x int64) *Int {
	// This code is arranged to be inlineable and produce
	// zero allocations when inlined. See issue 29951.
	u := uint64(x)
	
	if x < 0 {
		u = -u
	}

	r := &Int{
		neg: x < 0,
	}
	
	var abs nat

	if x == 0 {
		// r3v4
		// abs = []Word{0}
	} else if _W == 32 && u>>32 != 0 {
		abs = nat{Word(u), Word(u >> 32)}
	} else {
		abs = nat{Word(u), Word(u)}
	}
	r.abs = abs
	return r
}

@ltzmaxwell
Copy link
Contributor

as a contrast, the code doesn't work in gno but ok in go:

func NewInt(x int64) *Int {
	// This code is arranged to be inlineable and produce
	// zero allocations when inlined. See issue 29951.
	u := uint64(x)
	
	if x < 0 {
		u = -u
	}
	var abs []Word

	
	if x == 0 {
		// r3v4
		// abs = []Word{0}
	} else if _W == 32 && u>>32 != 0 {
		abs = []Word{Word(u), Word(u >> 32)}
	} else {
		abs = []Word{Word(u), Word(u)}
	}
	return &Int{neg: x < 0, abs: abs}
}

@ltzmaxwell
Copy link
Contributor

#674 , a fix, still checking side effects.

@jaekwon
Copy link
Contributor

jaekwon commented May 4, 2023

This should be solved in the preprocessor such that these implicit conversions are turned into explicit conversions.
This way, additional logic is kept out of primitive operations like OpAssign.

There is already something similar but it only applies to conversions for untyped values.
I believe the right solution is to extend checkOrConvertType to also mutate the AST for the conversion of unnamed-composite -> (named)DeclaredTypes. (from Go spec "V and T have identical underlying types but are not type parameters and at least one of V or T is not a named type."; notice that unnamed type implies composite type, such as []byte, while string is a named type, so this rule explains the automatic conversion from []byte to string).

cx := Expr(Call(constType(nil, t), *x))
cx = Preprocess(store, last, cx).(Expr)
*x = cx

(NOTE we should keep in mind in our mission to optimize/benchmark, to make certain assumptions, such as we want OpAssign to be no slower than necessary, regardless of external factors).

@ltzmaxwell
Copy link
Contributor

@jaekwon thank you for pointing out the right path, I did have a hard time patching all around. I'll give the fix.

@ajnavarro ajnavarro added 🌱 feature New update to Gno 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related 🐞 bug Something isn't working and removed 🌱 feature New update to Gno labels May 15, 2023
@moul moul moved this to 🏆 Needed for Launch in 🚀 The Launch [DEPRECATED] Sep 5, 2023
@moul moul added this to the 🚀 main.gno.land milestone Sep 6, 2023
@dongwon8247 dongwon8247 moved this from Todo to In Progress in 🤝🏻 Partner: Onbloc Nov 13, 2023
@Kouteki Kouteki closed this as completed Nov 15, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in 🤝🏻 Partner: Onbloc Nov 15, 2024
@Kouteki Kouteki moved this from Onbloc (confirmed) to Core in 🍜 Seoul triage Nov 15, 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 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Core
Status: Done
Development

No branches or pull requests

7 participants