feat(gnovm): Support all valid types for function parameters and return values in code generation for native binding #2323

notJoon · 2024-06-11T07:29:57Z

Description

Enhances the code generation functionality to support all valid types for function parameters and return values that will be injected to gno.

Changes

Modifies the typesEqual function to handle the following additional types:

ast.MapType: Recursively compare the key and value types of maps.
ast.InterfaceType: Iterate over the methods of an interface and compare their names and type. The detailed method comparision will be automatically checked in the ast.FuncType handler below.
ast.StructType: Iterate over the fileds of a struct and compare their field names and types.
ast.FuncType: Compare the parameter and result lists of functions. In this case, parameter names and named return identifiers are ignored, and only the type is checked for a one-to-one match.
ast.Ellipsis: Recursively compare the element type of ellipsis. This type is only valid when using as a parameter and cannot be return type.
ast.SelectorExpr: Compare the package path (also, consider alias) and name of selectors.

part of #2317

codecov · 2024-06-11T07:31:32Z

Codecov Report

Attention: Patch coverage is 78.12500% with 14 lines in your changes missing coverage. Please review.

Project coverage is 54.66%. Comparing base (edb321f) to head (8cd96f3).
Report is 247 commits behind head on master.

Files with missing lines	Patch %	Lines
misc/genstd/mapping.go	78.12%	7 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2323      +/-   ##
==========================================
+ Coverage   54.64%   54.66%   +0.02%     
==========================================
  Files         578      578              
  Lines       77870    77932      +62     
==========================================
+ Hits        42551    42603      +52     
- Misses      32149    32154       +5     
- Partials     3170     3175       +5

Flag	Coverage Δ
misc/autocounterd	`0.00% <ø> (ø)`
misc/genproto	`0.00% <ø> (ø)`
misc/genstd	`75.43% <78.12%> (+1.53%)`	⬆️
misc/goscan	`0.00% <ø> (ø)`
misc/logos	`17.68% <ø> (ø)`
misc/loop	`0.00% <ø> (ø)`

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

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

zivkovicmilos · 2024-10-02T09:25:12Z

@notJoon is this PR still relevant?

Can you please update it with the latest master?

zivkovicmilos

Looks good 💯

Pinging @thehowl / @petar-dambovaliev for a second sanity check

zivkovicmilos · 2024-10-02T09:27:56Z

misc/genstd/mapping.go

+		if !ok {
+			return &mismatch
+		}
+		if err := m.typesEqual(gnoe.Key, goe.Key); err != nil {


Do these errors need to be returned raw, or can you use fmt.Errorf to give them a bit more context?

zivkovicmilos · 2024-10-02T09:36:37Z

misc/genstd/mapping.go

+		for i := 0; i < len(gnoe.Fields.List); i++ {
+			gnoField, goField := gnoe.Fields.List[i], goe.Fields.List[i]
+			if len(gnoField.Names) != len(goField.Names) {
+				return &mismatch
+			}
+			for j := 0; j < len(gnoField.Names); j++ {
+				if gnoField.Names[j].Name != goField.Names[j].Name {
+					return &mismatch
+				}
+			}
+			if err := m.typesEqual(gnoField.Type, goField.Type); err != nil {
+				return err
+			}
+		}
+		return nil


What do you think about this mini cleanup?

Suggested change

for i := 0; i < len(gnoe.Fields.List); i++ {

gnoField, goField := gnoe.Fields.List[i], goe.Fields.List[i]

if len(gnoField.Names) != len(goField.Names) {

return &mismatch

}

for j := 0; j < len(gnoField.Names); j++ {

if gnoField.Names[j].Name != goField.Names[j].Name {

return &mismatch

}

}

if err := m.typesEqual(gnoField.Type, goField.Type); err != nil {

return err

}

}

return nil

for i, gnoField := range gnoe.Fields.List {

goField := goe.Fields.List[i]

if len(gnoField.Names) != len(goField.Names) {

return &mismatch

}

for j, gnoName := range gnofield.Names {

if gnoName.Name != goField.Names[j].Name {

return &mismatch

}

}

if err := m.typesEqual(gnoField.Type, goField.Type); err != nil {

return err

}

}

return nil

I'm not sure if you can move this check up, before the name loop:

if err := m.typesEqual(gnoField.Type, goField.Type); err != nil { return err }

zivkovicmilos · 2024-10-02T09:40:17Z

misc/genstd/mapping.go

+		for i := 0; i < len(gnoe.Methods.List); i++ {
+			gnoField, goField := gnoe.Methods.List[i], goe.Methods.List[i]
+			if len(gnoField.Names) != len(goField.Names) {
+				return &mismatch
+			}
+			for j := 0; j < len(gnoField.Names); j++ {
+				if gnoField.Names[j].Name != goField.Names[j].Name {
+					return &mismatch
+				}
+			}
+			if err := m.typesEqual(gnoField.Type, goField.Type); err != nil {
+				return err
+			}
+		}
+		return nil


Same mini cleanup as before. It also uses the correct terminology (method instead of field), since we're iterating over a method list:

Suggested change

for i := 0; i < len(gnoe.Methods.List); i++ {

gnoField, goField := gnoe.Methods.List[i], goe.Methods.List[i]

if len(gnoField.Names) != len(goField.Names) {

return &mismatch

}

for j := 0; j < len(gnoField.Names); j++ {

if gnoField.Names[j].Name != goField.Names[j].Name {

return &mismatch

}

}

if err := m.typesEqual(gnoField.Type, goField.Type); err != nil {

return err

}

}

return nil

for i, gnoMethod := range gnoe.Methods.List {

goMethod := goe.Methods.List[i]

if len(gnoMethod.Names) != len(goMethod.Names) {

return &mismatch

}

for j, gnoName := range gnoMethod.Names {

if gnoName.Name != goMethod.Names[j].Name {

return &mismatch

}

}

if err := m.typesEqual(gnoMethod.Type, goMethod.Type); err != nil {

return err

}

}

return nil

I'm not sure if you can move this check up, before the name loop:

if err := m.typesEqual(gnoMethod.Type, goMethod.Type); err != nil { return err }

zivkovicmilos · 2024-10-02T09:42:24Z

misc/genstd/mapping.go

+		if ok := m.fieldListsMatch(gnoe.Params, goe.Params); !ok {
+			return &mismatch
+		}
+		if ok := m.fieldListsMatch(gnoe.Results, goe.Results); !ok {
+			return &mismatch
+		}


Suggested change

if ok := m.fieldListsMatch(gnoe.Params, goe.Params); !ok {

return &mismatch

}

if ok := m.fieldListsMatch(gnoe.Results, goe.Results); !ok {

return &mismatch

}

if !m.fieldListsMatch(gnoe.Params, goe.Params) || !m.fieldListsMatch(gnoe.Results, goe.Results) {

return &mismatch

}

zivkovicmilos · 2024-10-02T09:46:11Z

misc/genstd/mapping_test.go

@@ -217,6 +217,49 @@ func Test_typesEqual(t *testing.T) {
 		{"[]int", "[1]int", "does not match"},
 		{"[1]int", "[]int", "does not match"},
 		{"[2]int", "[2]string", "does not match"},
+


You can run this testing suite in parallel with t.Parallel()

zivkovicmilos · 2024-10-02T09:46:16Z

misc/genstd/mapping_test.go

@@ -243,15 +286,106 @@ func Test_typesEqual(t *testing.T) {
 	}
 }

+func Test_typesEqual_SelectorExpr(t *testing.T) {


You can run this testing suite in parallel with t.Parallel()

thehowl

Wait. Why do you need this?

The problem is the generated code will still not convert them properly, and it shouldn't convert them properly.

If you're passing interfaces from Go to Gno and viceversa, you're doing something wrong. My last PR specifically on genstd actually removed the support for any named type: 0f2e755

thehowl · 2024-10-02T10:10:15Z

Feel free to reopen this / comment with good use-case, but I don't think there's any.

I'll now comment on #2317 as well.

feat: support more types

8cd96f3

notJoon requested a review from thehowl as a code owner June 11, 2024 07:29

github-actions bot assigned notJoon Jun 11, 2024

zivkovicmilos requested review from ltzmaxwell and mvertes June 14, 2024 09:04

adr-sk mentioned this pull request Jun 19, 2024

Onbloc's Builder Journey gnolang/hackerspace#29

Open

zivkovicmilos requested a review from petar-dambovaliev October 2, 2024 09:25

zivkovicmilos approved these changes Oct 2, 2024

View reviewed changes

thehowl requested changes Oct 2, 2024

View reviewed changes

thehowl closed this Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gnovm): Support all valid types for function parameters and return values in code generation for native binding #2323

feat(gnovm): Support all valid types for function parameters and return values in code generation for native binding #2323

notJoon commented Jun 11, 2024 •

edited

Loading

codecov bot commented Jun 11, 2024 •

edited

Loading

zivkovicmilos commented Oct 2, 2024

zivkovicmilos left a comment

zivkovicmilos Oct 2, 2024

zivkovicmilos Oct 2, 2024

zivkovicmilos Oct 2, 2024

zivkovicmilos Oct 2, 2024

zivkovicmilos Oct 2, 2024

zivkovicmilos Oct 2, 2024

zivkovicmilos Oct 2, 2024

zivkovicmilos Oct 2, 2024

thehowl left a comment

thehowl commented Oct 2, 2024

feat(gnovm): Support all valid types for function parameters and return values in code generation for native binding #2323

feat(gnovm): Support all valid types for function parameters and return values in code generation for native binding #2323

Conversation

notJoon commented Jun 11, 2024 • edited Loading

Description

Changes

codecov bot commented Jun 11, 2024 • edited Loading

Codecov Report

zivkovicmilos commented Oct 2, 2024

zivkovicmilos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehowl left a comment

Choose a reason for hiding this comment

thehowl commented Oct 2, 2024

notJoon commented Jun 11, 2024 •

edited

Loading

codecov bot commented Jun 11, 2024 •

edited

Loading