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.91837% with 6 lines in your changes missing coverage. Please review.

Project coverage is 63.33%. Comparing base (81a88a2) to head (d60ec6c).

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

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3017   +/-   ##
=======================================
  Coverage   63.33%   63.33%           
=======================================
  Files         548      548           
  Lines       78646    78624   -22     
=======================================
- Hits        49807    49794   -13     
+ Misses      25478    25469    -9     
  Partials     3361     3361

Flag	Coverage Δ
contribs/gnodev	`61.11% <ø> (ø)`
contribs/gnofaucet	`14.82% <ø> (ø)`
gno.land	`67.18% <ø> (ø)`
gnovm	`67.87% <95.91%> (-0.01%)`	⬇️
misc/genstd	`79.72% <ø> (ø)`
tm2	`62.41% <ø> (+0.09%)`	⬆️

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 <[email protected]>

…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 :)

gnovm/pkg/gnolang/preprocess.go

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 :)

gnovm/pkg/gnolang/preprocess.go

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.

mvertes · 2024-11-06T17:46:41Z

gnovm/tests/files/assign33.gno

+}
+
+// Error:
+// main/files/assign33.gno:8:6: cannot use foo<VPBlock(3,0)>() (value of type bool) as int value in assignment


Suggested change

// main/files/assign33.gno:8:6: cannot use foo<VPBlock(3,0)>() (value of type bool) as int value in assignment

// main/files/assign33.gno:8:6: cannot use foo<VPBlock(3,0)>() (value of type bool) as int value in assignment:

Please apply this to fix the failing test (see https://github.com/gnolang/gno/actions/runs/11701117443/job/32586531655?pr=3017)

mvertes · 2024-11-06T18:06:45Z

gnovm/pkg/gnolang/preprocess.go

+		if st != nil {
+			tt := tuple.Elts[i]
+
+			if tt.Kind() != st.Kind() {


Testing for identical type kinds is too weak!
For assignement expressions, you must verify that type st is assignable to type tt. Hint: look for function checkAssignableTo in types.go, same package.

omarsy · 2024-11-06T21:49:28Z

gnovm/pkg/gnolang/preprocess.go

+				if len(n.Values) > 1 && len(n.NameExprs) != len(n.Values) {
+					panic(fmt.Sprintf("assignment mismatch: %d variable(s) but %d value(s)", len(n.NameExprs), len(n.Values)))


Sorry my last request. What do you think about moving this check into the defineOrDecl function?

…t-valuedecl-1958

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 <[email protected]>

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

mvertes requested changes Nov 6, 2024

View reviewed changes

mvertes reviewed Nov 6, 2024

View reviewed changes

omarsy reviewed Nov 6, 2024

View reviewed changes

hieu.ha added 5 commits November 7, 2024 15:06

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

272187f

…t-valuedecl-1958

chore: fix typos

2e199b4

chore: remove unneeded comments

89646d8

test: test CI

f552c29

fix: try to fix CI

d60ec6c

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

mvertes Nov 5, 2024

mvertes Nov 5, 2024

mvertes Nov 5, 2024

mvertes Nov 5, 2024

hthieu1110 commented Nov 6, 2024

mvertes Nov 6, 2024

mvertes Nov 6, 2024

omarsy 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.

	// main/files/assign33.gno:8:6: cannot use foo<VPBlock(3,0)>() (value of type bool) as int value in assignment
	// main/files/assign33.gno:8:6: cannot use foo<VPBlock(3,0)>() (value of type bool) as int value in assignment:

		if len(n.Values) > 1 && len(n.NameExprs) != len(n.Values) {
		panic(fmt.Sprintf("assignment mismatch: %d variable(s) but %d value(s)", len(n.NameExprs), len(n.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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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