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(gnovm): solution for correctly capturing loop externs #1818

Closed
wants to merge 57 commits into from

Conversation

ltzmaxwell
Copy link
Contributor

@ltzmaxwell ltzmaxwell commented Mar 25, 2024

Address #1135.

This is the latest effort in a series of attempts: #1585, #1768, #1780.

The main idea is following the talk with Jae that:

  • Handling this process during the preprocessing stage.
  • Utilizing a code injection method to encapsulate the targeted loop body within a new BlockStmt.

The process can be break down into 3 steps:

  • Identify the pattern where a variable escapes a loop. This usually occurs when a function literal is embedded within a loop block (such as a for or range loop, or an implicit loop formed by goto statements), and there are names defined within the loop block are referenced by this function literal . For example:
func main() {
	var fns []func() int
	for i := 0; i < 3; i++ {
		x := i
		f := func() int {
			return x
		}
		fns = append(fns, f)
	}
	for _, fn := range fns {
		println(fn())
	}
}
  • Encapsulate the loop body within a BlockStmt, and inject additional code as necessary. This modification transforms it into:
for i := 0; i < 3; i++ {
    { // wrapper block stmt
		x := i
		f := func() int {
			return x
		}
		fns = append(fns, f)
	}
    }
}

if the loopVar i is references by the funcLit, we'll need to inject another stmt of i := i , to be:

for i := 0; i < 3; i++ {
    { // injected block stmt
		i := i // injected definition
		f := func() int {
			return i
		}
		fns = append(fns, f)
	}
    }
}
  • Reprocess the modified body, re-evaluate all the value paths.
    clear all the preprocessed memos, value-paths, staticBlocks, and REDO the preprocess work.(this is a cumbersome work, maybe we need a multi-phase preprocess).

cc: @jaekwon @deelawn @thehowl .

@ltzmaxwell ltzmaxwell requested review from jaekwon, piux2, thehowl, moul and a team as code owners March 25, 2024 03:04
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Mar 25, 2024
@ltzmaxwell ltzmaxwell changed the title WIP feat: yet another solution for loop externs capture WIP: feat: yet another solution for loop externs capture Mar 25, 2024
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 9.54064% with 256 lines in your changes are missing coverage. Please review.

Project coverage is 48.67%. Comparing base (b907e44) to head (c57498d).
Report is 5 commits behind head on master.

Files Patch % Lines
gnovm/pkg/gnolang/preprocess.go 10.50% 210 Missing and 3 partials ⚠️
gnovm/pkg/gnolang/nodes.go 0.00% 32 Missing ⚠️
gnovm/pkg/gnolang/helpers.go 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1818      +/-   ##
==========================================
- Coverage   48.82%   48.67%   -0.16%     
==========================================
  Files         579      579              
  Lines       78045    78310     +265     
==========================================
+ Hits        38107    38116       +9     
- Misses      36852    37103     +251     
- Partials     3086     3091       +5     
Flag Coverage Δ
gnovm 41.66% <9.54%> (-0.39%) ⬇️

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 zivkovicmilos marked this pull request as draft March 27, 2024 12:31
@ltzmaxwell ltzmaxwell marked this pull request as ready for review May 15, 2024 09:02
@ltzmaxwell ltzmaxwell changed the title WIP: feat: yet another solution for loop externs capture feat(gnovm): yet another solution for loop externs capture May 15, 2024
@ltzmaxwell ltzmaxwell changed the title feat(gnovm): yet another solution for loop externs capture feat(gnovm): solution for correctly capturing loop externs May 15, 2024
for _, loop := range loops {
if loop.isGotoLoop { // for/range has done the work
// find label stmt
lblstmt, idx := body.GetLabeledStmt(loop.label)
Copy link
Contributor

Choose a reason for hiding this comment

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

i think label conflicts are possible; same label used in different places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This search is conducted within specific block nodes such as funcDecl, forStmt, etc., and not within a fileNode. Thus, each label should be unique within the scope of the search. Did I misunderstand your question?

also, if jump from different places to same label stmt, it's also working, see this case:

package main

import (
	"fmt"
)

func main() {
	i := 0
	var fs []func()

	defer func() {
		for _, ff := range fs {
			ff()
		}
	}()

	if i == 0 {
		goto Start
	}

Start:
	if i <= 5 {
		x := i
		fmt.Println("i: ", i)
		i++
		fs = append(fs, func() { println(x) })

		goto Start // Jumps back to the label "Start"
	}

	if i <= 10 {
		x := i
		fmt.Println("Counting past 5:", i)
		i++
		fs = append(fs, func() { println(x) })
		goto Start // Another jump back to the label "Start"
	}

	fmt.Println("Finished counting")
}

// Output:
// i:  0
// i:  1
// i:  2
// i:  3
// i:  4
// i:  5
// Counting past 5: 6
// Counting past 5: 7
// Counting past 5: 8
// Counting past 5: 9
// Counting past 5: 10
// Finished counting
// 0
// 1
// 2
// 3
// 4
// 5
// 6
// 7
// 8
// 9
// 10

@@ -364,6 +416,8 @@ func Preprocess(store Store, ctx BlockNode, n Node) Node {
last.Define(Name(rn), anyValue(rf.Type))
}
}
// record, mostly used by goto... that with funcLit embedded within.
closureStack = append(closureStack, last)
Copy link
Contributor

@jaekwon jaekwon Jun 25, 2024

Choose a reason for hiding this comment

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

doesn't unwind, weird.
it's not a stack if it doesn't get unstacked, right?


// for goto...
// wrap goto loop into block stmt.
func rebuildBody(b Body, gloop *LoopInfo, loc Location) Body {
Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately, i don't think this works for the following kind of goto-loop.

LABEL1:
...
LABEL2:
...
if cond1 {
    goto LABEL1
}
...
if cond2 {
    goto LABEL2
}

notice how goto LABEL1 and goto LABEL2 make two loops that intersect, but one loop is not nested within the other.

@jaekwon
Copy link
Contributor

jaekwon commented Jun 25, 2024

Here's another example where Go and Gno currently differs, without any closures:

func main() {
        az := []*int{}
        for i := 0; i < 10; i++ {
                it := i
                az = append(az, &it)
        }
        println(az)
        for idx, v := range az {
                println(idx, *v)
        }
}

Even referencing a loop-declared differs in behavior; in Go, &it is a new value location, for every loop iteration.

@ltzmaxwell
Copy link
Contributor Author

close due to theoretical incompleteness.

@ltzmaxwell ltzmaxwell closed this Jul 1, 2024
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: No status
Development

Successfully merging this pull request may close these issues.

for loops maintain the same block on iteration, which is referenced in any closures generated within
4 participants