-
Notifications
You must be signed in to change notification settings - Fork 366
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): handle loop variables #2429
base: master
Are you sure you want to change the base?
Conversation
c4a72f3
to
6b440e0
Compare
// Error: | ||
// main/files/switch13.gno:0:0: i<VPBlock(2,0)> is not a type | ||
// main/files/switch13.gno:8:0: i<VPBlock(2,0)> is not a type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also with line & column issue, neither is correct. to be fixed later.
Thank you Maxwell for the main readme description and for completing this PR. |
gnovm/pkg/gnolang/preprocess.go
Outdated
nx := n.Key.(*NameExpr) | ||
if nx.Name != blankIdentifier { | ||
// XXX, temp method to NOT heapDefine loopvar for range, | ||
// to make it consistent with `for i:=0;i<3;i++;`, | ||
// this should be uncommented when supporting full Go1.22 loopvar. | ||
//nx.Type = NameExprTypeDefine | ||
last.Predefine(false, nx.Name) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temp method to NOT
support loopvar of range, to make it consistent with for i:=0;i<3;i++.
{in: "s\n\n", out: "=> 33: num := 5"}, | ||
{in: "s\n\n\n", out: "=> 33: num := 5"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also step on type decl.
I noticed this while reviewing #2429. The previous conditional could never evaluate to true: `if (1<<16 - 1) < sb.NumNames {`. `sb.NumNames` is a `uint16` and the value it is being compared to, `1<<16 - 1` is the max uint16 value, so it is impossible the the number of names to be greater than this value. Instead, we should check to see if the current number of names is the maximum value and panic if this is the case. <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [x] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicks for now; will need some more time to go through it all.
@@ -669,7 +669,7 @@ func (m *Machine) runFileDecls(fns ...*FileNode) []TypedValue { | |||
} | |||
} | |||
// if dep already in loopfindr, abort. | |||
if hasName(dep, loopfindr) { | |||
if hasName(loopfindr, dep) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if hasName(loopfindr, dep) { | |
if slices.Contains(loopfindr, dep) { |
and remove hasName
@@ -172,26 +187,36 @@ func (attr *Attributes) GetLabel() Name { | |||
return attr.Label | |||
} | |||
|
|||
// XXX I think should be AddLabel instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, as there may only be 1 label; I think SetLabel is good.
@@ -580,11 +618,15 @@ func (ftxz FieldTypeExprs) IsNamed() bool { | |||
named := false | |||
for i, ftx := range ftxz { | |||
if i == 0 { | |||
named = ftx.Name != "" | |||
if ftx.Name == "" || strings.HasPrefix(string(ftx.Name), ".res_") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make a hiddenResultVariable
and a isHiddenResultVariable
function; so we don't have "magic" ".res_"
strings in our code.
} | ||
attr.data[key] = value | ||
} | ||
|
||
func (attr *Attributes) DelAttribute(key GnoAttribute) { | ||
if attr.data == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if attr.data == nil { | |
if debug && attr.data == nil { |
} | ||
|
||
// NOTE: path.Depth == 1 means it's in bn. | ||
var bn BlockNode = sb.GetSource(store) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var bn BlockNode = sb.GetSource(store) | |
bn := sb.GetSource(store) |
@@ -1651,6 +1697,21 @@ func (sb *StaticBlock) GetPathForName(store Store, n Name) ValuePath { | |||
panic(fmt.Sprintf("name %s not declared", n)) | |||
} | |||
|
|||
// Get the containing block node for node with path relative to this containing block. | |||
func (sb *StaticBlock) GetBlockNodeForPath(store Store, path ValuePath) BlockNode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace the code in StaticBlock.GetStaticTypeOfAt with this method?
@@ -1582,6 +1625,7 @@ func (sb *StaticBlock) GetBlockNames() (ns []Name) { | |||
} | |||
|
|||
// Implements BlockNode. | |||
// NOTE: Extern names may also be local, if declared later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// NOTE: Extern names may also be local, if declared later. | |
// NOTE: Extern names may also be local, if declared after usage as an extern | |
// (thus shadowing the extern name). |
// NOTE: if a name is later declared in this block later, it is both an | ||
// extern name with depth > 1, as well as local name with depth == 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// NOTE: if a name is later declared in this block later, it is both an | |
// extern name with depth > 1, as well as local name with depth == 1. | |
// NOTE: externs may also be shadowed later in the block. Thus, usages | |
// before the declaration will have depth > 1; following it, depth == 1, | |
// matching the two different identifiers they refer to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution seems to work well. Please see the comments and questions I left.
// r, ok := 1, true | ||
func isLocallyDefined2(bn BlockNode, n Name) bool { | ||
_, ok := bn.GetLocalIndex(n) | ||
if !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to return ok
continue | ||
} | ||
last2.Predefine(n.Const, nx.Name) | ||
if !isLocallyDefined(last, nn) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what case would something already be locally defined?
defined = true | ||
if isLocallyDefined2(last, ln) { | ||
// already defined, do nothing | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just make this if !isLocallyDefined2
and remove the if isLocallyDefined2
condition.
} | ||
line := n.GetLine() | ||
if line == lastLine { | ||
} else if line == 0 { // XXX, should not happen? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this happen or not? If not, this conditional block can be replaced with lastLine = line
to make it easier to understand what it's doing.
n.Predefine(false, rte.Name) | ||
for i, rte := range n.Type.Results { | ||
if rte.Name == "" { | ||
rn := fmt.Sprintf(".res_%d", i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define .res_
as a constant?
// Find the block where name is defined | ||
dbn := last.GetBlockNodeForPath(nil, n.Path) | ||
// if the name is loop defined, | ||
lds, _ := dbn.GetAttribute(ATTR_LOOP_DEFINES).([]Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only do something when the name is in ATTR_LOOP_USES, can we get rid of the ATTR_LOOP_DEFINES lookup?
ptr := b.GetPointerTo(store, path) | ||
hiv := &HeapItemValue{} | ||
// point to a heapItem | ||
*ptr.TV = TypedValue{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this doing? ptr.TV
's value is being set but is not used to construct the value being returned.
@@ -845,6 +845,10 @@ func getChildObjects(val Value, more []Value) []Value { | |||
if bv, ok := cv.Closure.(*Block); ok { | |||
more = getSelfOrChildObjects(bv, more) | |||
} | |||
// XXX return Captures | |||
for _, c := range cv.Captures { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? These captures are for values defined within the scope of a loop, right? It seems like the only way this value would be saved is if a reference was made to it during another assignment. In that case would this case inside this function handle it?
case *HeapItemValue:
more = getSelfOrChildObjects(cv.Value.V, more)
Co-authored-by: Morgan <morgan@morganbaz.com>
Problem Definition
The problem originates from the issue described in #1135. While the full scope of the issue is broader, it fundamentally relates to the concept of loop variable escapes block where it's defined.
e.g. 1:
e.g. 2:
e.g. 3:
Solution ideas
identify escaped vars in preprocess:
Detect situations where a loop variable is defined within a loop block(including
for/range
loops or loops constructed usinggoto
statements), and escapes the block where it's defined.runtime allocation:
Allocate a new heap item for the loop variable in each iteration to ensure each iteration operates with its unique variable instance.
NOTE1: this is consistent with Go's Strategy:
"Each iteration has its own separate declared variable (or variables) [Go 1.22]. The variable used by the first iteration is declared by the init statement. The variable used by each subsequent iteration is declared implicitly before executing the post statement and initialized to the value of the previous iteration's variable at that moment."
NOTE2: the
loopvar
feature of Go 1.22 is not supported in this version, and will be supported in next version.not supporting capture
i
defined in for/range clause;Implementation Details
Preprocess Stage(Multi-Phase Preprocessor):
Phase 1:
initStaticBlocks
: Establish a cascading scope structure wherepredefine
is conducted. In this phase Name expressions are initially marked asNameExprTypeDefine
, which may later be upgraded toNameExprTypeHeapDefine
if it is determined that they escape the loop block. This phase also supports other processes as a prerequisite#2077.Phase 2:
preprocess1
: This represents the original preprocessing phase(not going into details).Phase 3:
findGotoLoopDefines
: By traversing the AST, any name expression defined in a loop block (for/range, goto) with the attributeNameExprTypeDefine
is promoted toNameExprTypeHeapDefine
. This is used in later phase.Phase 4:
findLoopUses1
: Identify the usage ofNameExprTypeHeapDefine
name expressions. If a name expression is used in a function literal or is referrnced(e.g. &a), and it was previously defined asNameExprTypeHeapDefine
, theused
name expression is then given the attributeNameExprTypeHeapUse
. This step finalizes whether a name expression will be allocated on the heap and used from heap.Closures
represent a particular scenario in this context. Each closure, defined by a funcLitExpr that captures variables, is associated with a HeapCaptures list. This list consists of NameExprs, which are utilized at runtime to obtain the actual variable values for each iteration. Correspondingly, within the funcLitExpr block, a list of placeholder values are defined. These placeholders are populated during the doOpFuncLit phase and subsequently utilized in thedoOpCall
to ensure that each iteration uses the correct data.Phase 5:
findLoopUses2
: Convert non-loop uses of loop-defined names toNameExprTypeHeapUse
. Also, demoteNameExprTypeHeapDefine
back toNameExprTypeDefine
if no actual usage is found. Also , as the last phase, attributes no longer needed will be cleaned up after this phase.Runtime Stage:
Variable Allocation:
NameExprTypeHeapDefine
triggers the allocation of a newheapItemValue
for it, which will be used by anyNameExprTypeHeapUse
.Function Literal Handling:
doOpFuncLit
, retrieve theHeapCapture
values (previously allocated heap item values) and fill in the placeholder values within thefuncLitExpr
block.doOpCall
), theplaceHolder
values(fv.Captures) are used to update the execution context, ensuring accurate and consistent results across iterations.