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

feat: add support for type declarations on pointer types #1733

Merged
merged 13 commits into from
Apr 10, 2024

Conversation

ltzmaxwell
Copy link
Contributor

@ltzmaxwell ltzmaxwell commented Mar 5, 2024

support type decl upon pointer type:

package main

import "fmt"

// Define a base type
type Base int

// Declare a new type that is a pointer to the base type
type PtrToBase *Base

func main() {
	var b Base = 42      // Initialize a variable of the base type
	var p PtrToBase = &b // Initialize a variable of the new pointer type with the address of b

	fmt.Printf("The value of b is: %d\n", b)

	// Using the new pointer type
	fmt.Printf("The value pointed to by p is: %d\n", *p)

	// Modifying the value pointed to by p
	*p = 100
	fmt.Printf("The new value of b after modification through p is: %d\n", b)
}

// Output:
// The value of b is: 42
// The value pointed to by p is: 42
// The new value of b after modification through p is: 100

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Mar 5, 2024
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.79%. Comparing base (629c438) to head (a2521a5).
Report is 75 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1733      +/-   ##
==========================================
+ Coverage   47.43%   47.79%   +0.35%     
==========================================
  Files         384      393       +9     
  Lines       61205    61891     +686     
==========================================
+ Hits        29032    29579     +547     
- Misses      29744    29829      +85     
- Partials     2429     2483      +54     

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

@ltzmaxwell ltzmaxwell changed the title feat: Adde support for type declarations on pointer types feat: add support for type declarations on pointer types Mar 5, 2024
Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

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

Nice one @ltzmaxwell. We just need to make sure that support for this feature is added in all places. I imagine there could be quite a few places that are currently casting to *PointerType without bothering to ensure it is using the pointer's base type now that the pointer type can be a declared type.

Take this code as an example:

package main

type BytePtr *byte

func main() {
	bs := []byte("hello")
	var p BytePtr = &bs[0]
	println(*p)
}

Running this will cause a panic. There is a line here that is incorrect: https://github.com/gnolang/gno/blob/master/gnovm/pkg/gnolang/op_expressions.go#L150

It should probably be changed to:

tv := TypedValue{T: bt.Elt}

to ensure it is using the base pointer type.

I haven't searched extensively but there may be other situations like this. This was one of the first hits that came up when I searched for .(*PointerType).

@zivkovicmilos
Copy link
Member

@ltzmaxwell
Have you had a chance to address @deelawn's comment?

@ltzmaxwell
Copy link
Contributor Author

@ltzmaxwell Have you had a chance to address @deelawn's comment?

yes I'm on it, sorry for the delay.

@ltzmaxwell ltzmaxwell requested a review from a team as a code owner April 8, 2024 05:30
@ltzmaxwell
Copy link
Contributor Author

Nice one @ltzmaxwell. We just need to make sure that support for this feature is added in all places. I imagine there could be quite a few places that are currently casting to *PointerType without bothering to ensure it is using the pointer's base type now that the pointer type can be a declared type.

Take this code as an example:

package main

type BytePtr *byte

func main() {
	bs := []byte("hello")
	var p BytePtr = &bs[0]
	println(*p)
}

Running this will cause a panic. There is a line here that is incorrect: https://github.com/gnolang/gno/blob/master/gnovm/pkg/gnolang/op_expressions.go#L150

It should probably be changed to:

tv := TypedValue{T: bt.Elt}

to ensure it is using the base pointer type.

I haven't searched extensively but there may be other situations like this. This was one of the first hits that came up when I searched for .(*PointerType).

this is added, also some other tests, thank you @deelawn !

@deelawn
Copy link
Contributor

deelawn commented Apr 9, 2024

Thanks for fixing the type assertion issues @ltzmaxwell. I can see one more thing that should be done to make this solution complete. Now that we support aliasing pointer types, we should also make the change to ensure that method receivers cannot have an underlying pointer type, as per the go spec https://arc.net/l/quote/mihfuukq, and also to avoid ambiguity for cases where this would currently be allowed. For example, this should fail during realm creation in the preprocessor with a message like invalid receiver type IntPtr (pointer or interface type):

package main

type IntPtr *int

var ip IntPtr = new(int)

func (p IntPtr) Int() int {
	return *p
}

func main() {
	println(ip.Int())
}

This could also be a separate PR. What do you think @petar-dambovaliev

@ltzmaxwell
Copy link
Contributor Author

Thanks for fixing the type assertion issues @ltzmaxwell. I can see one more thing that should be done to make this solution complete. Now that we support aliasing pointer types, we should also make the change to ensure that method receivers cannot have an underlying pointer type, as per the go spec https://arc.net/l/quote/mihfuukq, and also to avoid ambiguity for cases where this would currently be allowed. For example, this should fail during realm creation in the preprocessor with a message like invalid receiver type IntPtr (pointer or interface type):

package main

type IntPtr *int

var ip IntPtr = new(int)

func (p IntPtr) Int() int {
	return *p
}

func main() {
	println(ip.Int())
}

This could also be a separate PR. What do you think @petar-dambovaliev

Nice! as always.
I have applied a patch to address this issue. Please refer to the test files for further details and validation.

Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

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

Nice job 👍

@deelawn deelawn merged commit ca6566f into gnolang:master Apr 10, 2024
189 of 190 checks passed
@ltzmaxwell ltzmaxwell deleted the feat/typeDecl_PointerType branch April 10, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants