feat(gnovm): sync code AssignStmt - ValueDecl #3017

hthieu1110 · 2024-10-24T15:43:36Z

This PR aims at fixing this issue 1958

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests

codecov · 2024-10-24T15:45:58Z

Codecov Report

Attention: Patch coverage is 95.27559% with 6 lines in your changes missing coverage. Please review.

Project coverage is 63.32%. Comparing base (367408a) to head (daf9766).

Files with missing lines	Patch %	Lines
gnovm/pkg/gnolang/preprocess.go	95.27%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3017      +/-   ##
==========================================
- Coverage   63.33%   63.32%   -0.01%     
==========================================
  Files         548      548              
  Lines       78601    78559      -42     
==========================================
- Hits        49780    49751      -29     
+ Misses      25466    25454      -12     
+ Partials     3355     3354       -1

Flag	Coverage Δ
contribs/gnodev	`61.11% <ø> (ø)`
contribs/gnofaucet	`15.77% <ø> (+0.94%)`	⬆️
gno.land	`67.37% <ø> (ø)`
misc/genstd	`79.72% <ø> (ø)`
tm2	`62.39% <ø> (+0.08%)`	⬆️

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.

…t-valuedecl-1958

MikaelVallenet

Looks good to me 🔥

gnovm/pkg/gnolang/preprocess.go

Co-authored-by: Mikael VALLENET <mikael.vallenetpro@gmail.com>

…t-valuedecl-1958

omarsy · 2024-10-29T19:42:40Z

gnovm/pkg/gnolang/preprocess.go

-							nx.Path = last.GetPathForName(nil, nx.Name)
-						}
-					}
+				if len(n.Values) != 1 && len(n.Values) != 0 && len(n.NameExprs) != len(n.Values) {


why this len(n.Values) != 1 && len(n.Values) != 0 ?
I think this will not handle this case:

package main var a, b = 1 func main() { }

Originally, there were 3 cases:

if numNames > 1 && len(n.Values) == 1 { ... } else if len(n.Values) != 0 && numNames != len(n.Values) { ... } else { ... }

To be able to refactor the code, I've moved this check first then 2 others. So I've added the condition but it looks strange like that, I've pushed the change.

@omarsy I've checked, actually the errors for var a, b = 1 vs a, b := 1 are identical with Golang

Yes don't know why but I cannot resolve the comment :/

…t-valuedecl-1958

mvertes

Overall, this refactoring is on the good track, and I agree with the general direction. Nice work.

In addition to the remarks in the code, could you please add a few more tests to cover some corner cases such as:

Mixing single and multiple return expression in the same assignment:

func f() (a, b int) {return 1, 2}
var x, y, z = 1, f()

Or covering slices, arrays, type conversions, etc.

Thanks

mvertes · 2024-11-05T14:07:57Z

gnovm/pkg/gnolang/preprocess.go

+// - `a, b, c T := f()`
+// - `a, b := n.(T)`
+// - `a, b := n[i], where n is a map`
+func specialParseTypeVals(


Suggested change

func specialParseTypeVals(

func parseMultipleAssignFromOneExp(

specialParseTypeVals is unclear. Please use a descriptive function name as suggested.

Also, for the function comment, please follow Go conventions to describe the function:

// parseMultimpleAssignFromOneExp parses assignment to multiple variables from a single expression, etc...

I've updated the function name + comment

mvertes · 2024-11-05T14:13:06Z

gnovm/pkg/gnolang/preprocess.go

+// - `var a, b = n.(T)`
+// - `var a, b = n[i], where n is a map`
+// Assign
+// - `a, b, c T := f()`


Suggested change

// - `a, b, c T := f()`

// - `a, b, c := f()`

Type name makes no sense in short declarations.

I've updated :)

mvertes · 2024-11-05T14:24:44Z

gnovm/pkg/gnolang/preprocess.go

+		// var a, b, c T = f()
+		// a, b, c := f()
+		valueType := evalStaticTypeOfRaw(store, bn, valueExpr)
+		tuple = valueType.(*tupleType)


Should not you check this type assertion? Are you sure that valueType is always a tuple?

Also keep in mind that CallExpr may also refer to a type conversion, not only a function call: var a = int8(b), and that should be covered (maybe not here).

According to the doc of evalStaticTypeOfRaw: like evalStaticTypeOf() but returns the raw *tupleType for *CallExpr., so I think we can expect a raw tuple here.

The case var a = int8(b) is not handled here because we process only multi assignments from 1 expression

mvertes · 2024-11-05T14:37:13Z

gnovm/pkg/gnolang/preprocess.go

+		tuple = &tupleType{Elts: []Type{tt, BoolType}}
+		expr.HasOK = true
+	case *IndexExpr:
+		// Map index case:


IndexExpr may refer either to a slice, an array or a map. Please make sure you handle also slice and array types here. I understand it was not covered prior to your change (so it's not your fault :), but I see no reason not to do the job here also for slices/arrays.

The scope of this function handles only multi assignments from 1 expression, so arrays/slides index is not consider as this case, there is already a check to ensure that we process only map

mt, ok := baseOf(dt).(*MapType) if !ok { panic(fmt.Sprintf("invalid index expression on %T", dt)) }

mvertes · 2024-11-05T14:41:58Z

gnovm/pkg/gnolang/preprocess.go

+
+	for i := 0; i < numNames; i++ {
+		if st != nil {
+			// TODO check tt and nt compat.


Please, perform the required type check, as it is important to catch types mismatch as early as possible.

I've added the compat check :)

mvertes · 2024-11-05T14:46:28Z

gnovm/pkg/gnolang/preprocess.go

+
+// This func aims at syncing logic between op define (:=) and declare(var/const)
+// for general cases (not handled by specialParseTypeVals)
+func generalParseTypeVals(


Suggested change

func generalParseTypeVals(

func parseAssignFromExprList(

generalParseTypeVals is unclear. Please name function with more descriptive names. Also please fix function comment to better follow Go conventions.

I've updated the name + comment :)

mvertes · 2024-11-05T14:48:02Z

gnovm/pkg/gnolang/preprocess.go

+) {
+	numNames := len(nameExprs)
+
+	// ensure that function only return 1 value


Suggested change

// ensure that function only return 1 value

// Ensure that function only return 1 value.

Comment lines should follow normal sentence rules (1st letter capital, punctuation, ...).

mvertes · 2024-11-05T14:48:33Z

gnovm/pkg/gnolang/preprocess.go

+		}
+	}
+
+	// evaluate types and convert consts.


Suggested change

// evaluate types and convert consts.

// Evaluate types and convert consts.

mvertes · 2024-11-05T14:48:47Z

gnovm/pkg/gnolang/preprocess.go

+		for i := 0; i < numNames; i++ {
+			sts[i] = nt
+		}
+		// convert if const to nt.


Suggested change

// convert if const to nt.

// Convert if const to nt.

mvertes · 2024-11-05T14:49:09Z

gnovm/pkg/gnolang/preprocess.go

+			checkOrConvertType(store, bn, &valueExprs[i], nt, false)
+		}
+	} else if isConst {
+		// derive static type from values.


Suggested change

// derive static type from values.

// Derive static type from values.

hthieu1110 · 2024-11-06T09:16:20Z

thanks @mvertes for your time and your reviews , I've addressed all your feedbacks. Please re-check the PR, please. Thanksssss.

hthieu1110 and others added 3 commits October 23, 2024 21:15

chore: refactor DRY code

297dd6d

wip

6e6bcae

wip: sync AssignStmt DEFINE with ValueDecl

f196044

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Oct 24, 2024

github-actions bot assigned hthieu1110 Oct 24, 2024

hthieu1110 changed the title ~~Feat/sync assignstmt valuedecl 1958~~ feat(gnovm) sync assignstmt - valuedecl Oct 25, 2024

hthieu1110 changed the title ~~feat(gnovm) sync assignstmt - valuedecl~~ feat(gnovm): sync assignstmt - valuedecl Oct 25, 2024

Merge remote-tracking branch 'remote/master' into feat/sync-assignstm…

efc7467

…t-valuedecl-1958

hthieu1110 changed the title ~~feat(gnovm): sync assignstmt - valuedecl~~ feat(gnovm): sync AssignStmt - ValueDecl Oct 25, 2024

hthieu1110 changed the title ~~feat(gnovm): sync AssignStmt - ValueDecl~~ feat(gnovm): sync code AssignStmt - ValueDecl Oct 25, 2024

hieu.ha and others added 6 commits October 25, 2024 14:53

fix: use tupletype for evalStaticTypeOfRaw for CallExpr

26792b5

wip: sync the general case

551ab25

feat: refactor general case code for AssignStmt vs ValueDecl

d7401c0

chore: remove unused codes + file

96f279c

Merge branch 'master' into feat/sync-assignstmt-valuedecl-1958

e54abf8

Merge remote-tracking branch 'remote/master' into feat/sync-assignstm…

b050dc6

…t-valuedecl-1958

MikaelVallenet reviewed Oct 26, 2024

View reviewed changes

gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved

omarsy reviewed Oct 26, 2024

View reviewed changes

gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved

omarsy reviewed Oct 26, 2024

View reviewed changes

gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved

hthieu1110 and others added 2 commits October 26, 2024 08:10

chore: optimize numNames check

81b6be5

Co-authored-by: Mikael VALLENET <mikael.vallenetpro@gmail.com>

chore: add more tests

1d95d48

MikaelVallenet approved these changes Oct 26, 2024

View reviewed changes

hthieu1110 added 4 commits October 28, 2024 21:21

Merge remote-tracking branch 'remote/master' into feat/sync-assignstm…

0cf7ca1

…t-valuedecl-1958

chore: refactor tests

edd2287

feat: refactor defineOrDecl to raise the same error + add more tests

5ecc039

fix: fix tests

0e80d12

omarsy reviewed Oct 29, 2024

View reviewed changes

hthieu1110 added 2 commits October 30, 2024 07:49

chore: optimize code

f3e021f

Merge remote-tracking branch 'remote/master' into feat/sync-assignstm…

f57aafa

…t-valuedecl-1958

hthieu1110 mentioned this pull request Oct 30, 2024

feat(gnovm): returns error as Go when assignment with a function which does not return any value #3049

Draft

6 tasks

hthieu1110 added 2 commits October 30, 2024 21:38

Merge remote-tracking branch 'remote/master' into feat/sync-assignstm…

937f0b2

…t-valuedecl-1958

Merge branch 'master' into feat/sync-assignstmt-valuedecl-1958

c4398a2

hthieu1110 marked this pull request as ready for review November 1, 2024 01:34

zxxma mentioned this pull request Nov 1, 2024

Teritori's Builder Journey gnolang/hackerspace#7

Open

jefft0 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Nov 5, 2024

Merge branch 'master' into feat/sync-assignstmt-valuedecl-1958

67c1dce

mvertes requested changes Nov 5, 2024

View reviewed changes

ltzmaxwell requested review from ltzmaxwell and thehowl November 5, 2024 15:05

hieu.ha added 2 commits November 6, 2024 15:42

fix: addresses all comments + add tests

234e028

fix: check compat with given type vs tuple type

4145083

fix: fix type compare using kind to cover custom type

daf9766

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gnovm): sync code AssignStmt - ValueDecl #3017

feat(gnovm): sync code AssignStmt - ValueDecl #3017

hthieu1110 commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

MikaelVallenet left a comment

omarsy Oct 29, 2024 •

edited

Loading

hthieu1110 Oct 30, 2024

hthieu1110 Oct 31, 2024

omarsy Nov 5, 2024

mvertes left a comment •

edited

Loading

mvertes Nov 5, 2024

hthieu1110 Nov 6, 2024 •

edited

Loading

mvertes Nov 5, 2024

hthieu1110 Nov 6, 2024

mvertes Nov 5, 2024

hthieu1110 Nov 6, 2024 •

edited

Loading

hthieu1110 Nov 6, 2024

mvertes Nov 5, 2024

hthieu1110 Nov 6, 2024

mvertes Nov 5, 2024

hthieu1110 Nov 6, 2024

mvertes Nov 5, 2024

hthieu1110 Nov 6, 2024

mvertes Nov 5, 2024

mvertes Nov 5, 2024

mvertes Nov 5, 2024

mvertes Nov 5, 2024

hthieu1110 commented Nov 6, 2024

	func specialParseTypeVals(
	func parseMultipleAssignFromOneExp(

	// ensure that function only return 1 value
	// Ensure that function only return 1 value.

	// evaluate types and convert consts.
	// Evaluate types and convert consts.

	// derive static type from values.
	// Derive static type from values.

feat(gnovm): sync code AssignStmt - ValueDecl #3017

Are you sure you want to change the base?

feat(gnovm): sync code AssignStmt - ValueDecl #3017

Conversation

hthieu1110 commented Oct 24, 2024 • edited Loading

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

MikaelVallenet left a comment

Choose a reason for hiding this comment

omarsy Oct 29, 2024 • edited Loading

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

mvertes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hthieu1110 Nov 6, 2024 • edited Loading

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

hthieu1110 Nov 6, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hthieu1110 commented Nov 6, 2024

hthieu1110 commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

omarsy Oct 29, 2024 •

edited

Loading

mvertes left a comment •

edited

Loading

hthieu1110 Nov 6, 2024 •

edited

Loading

hthieu1110 Nov 6, 2024 •

edited

Loading