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

automatic loop variables in for loops ("loop var"), consistent with go 1.22 behavior #1645

Merged
merged 10 commits into from
Jul 20, 2024

Conversation

rcoreilly
Copy link
Contributor

This builds on #1644 and adds automatic per-loop variables that are consistent with go 1.22 behavior. See #1643 for discussion.

This is still a draft because the for7 version ends up capturing the per-loop var values when they are +1 relative to what they should be. Maybe somehow the incrementing and conditional code is somehow capturing the within loop variables and incrementing them? not sure how that would work. anyway, need to investigate further before this is ready to go, but pushing it here in case there are other issues or someone might figure out this bug before I do..

@rcoreilly rcoreilly marked this pull request as draft July 9, 2024 09:37
…t was going infinite. must apply to all for7 cases. also updated tests to skip: now doing everything in 1.22 and skipping closures for 1.21
@rcoreilly rcoreilly marked this pull request as ready for review July 11, 2024 08:18
@rcoreilly rcoreilly mentioned this pull request Jul 11, 2024
@mvertes mvertes added enhancement New feature or request area/core labels Jul 17, 2024
@rcoreilly
Copy link
Contributor Author

note that this one is ready to go now. it has the mysterious error on the "main / checks code and generated code" test which doesn't replicate locally, and the test doesn't report any specific issues to fix, so I'm not sure how to proceed on that.

@mvertes
Copy link
Member

mvertes commented Jul 19, 2024

In the logs from CI, the tests fail with the following:

=== RUN   TestExportClosureArg
    interp_issue_1634_test.go:57: 
        Got: "0\n0\n0\n",
         want: "0\n1\n2\n"
--- FAIL: TestExportClosureArg (0.00s)

I have not investigated further.

@rcoreilly
Copy link
Contributor Author

it appears that the closure PR interacts with this one, wrt the definition of shadow local variables:

	for i := 0; i < 3; i++ {
		i := i // eliminate this or use a variable with a different name here and it works
		tmp.Func(&fs, func() { println(i) })
	}

will investiage.

@rcoreilly
Copy link
Contributor Author

actually it looks like all of the for loop closure tests used a new variable name (e.g., a := i) and so this issue was not caught by the prior tests, and still occurs if you undo the closure PR. Needs an additional tweak to this PR and some tests!

…re 1.22 fix for loopvar) with the same name as the original. all the other tests were using a new variable name, so we missed this case.
@mvertes
Copy link
Member

mvertes commented Jul 19, 2024

The test case TestInterpConsistencyBuild/closure20.go fails in CI in oldstable which means go1.21, where range on numbers is not yet supported for the go compiler version of the test.

It's possible to blacklist the test (see interp/interp_consistency_test.go), or hold this PR until release of go1.23, which will force the removal of go1.21. Or get rid of TestInterpConsistencyBuild which is mostly redundant and a bit of a PITA.

Copy link
Member

@mvertes mvertes left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution. This is an amazing piece of work to keep the interpreter up-to-date with the latest evolutions of the Go language.

Well done!

@traefiker
Copy link
Contributor

🚫 the milestone is missing

@mvertes mvertes added this to the v0.16.x milestone Jul 20, 2024
@traefiker traefiker merged commit 9c4dcfc into traefik:master Jul 20, 2024
12 checks passed
@rcoreilly
Copy link
Contributor Author

Thanks and thank you so much for writing this amazing system! Incidentally I had written something similar to this in overall structure, for C++ (which I called "C Super Script" or CSS, just before the internet started...). It is so much nicer to be working in Go, and the ability to bidirectionally interoperate with compiled code is so powerful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants