fix scope for function expressions and inner functions by capturing a clone

Author: turbolent

activations/activations.go

@@ -107,6 +107,19 @@ func (a *Activation[T]) Set(name string, value T) {
 	a.entries[name] = value
 }
 
+func (a *Activation[T]) Clone() *Activation[T] {
+	clone := NewActivation[T](a.MemoryGauge, a.Parent)
+
+	if a.entries != nil {
+		clone.entries = make(map[string]T, len(a.entries))
+		for name, value := range a.entries { //nolint:maprange
+			clone.entries[name] = value
+		}
+	}
+
+	return clone
+}
+
 // Activations is a stack of activation records.
 // Each entry represents a new activation record.
 //

ast/visitor.go

@@ -30,7 +30,7 @@ type Element interface {
 
 type StatementDeclarationVisitor[T any] interface {
 	VisitVariableDeclaration(*VariableDeclaration) T
-	VisitFunctionDeclaration(*FunctionDeclaration) T
+	VisitFunctionDeclaration(declaration *FunctionDeclaration, isStatement bool) T
 	VisitSpecialFunctionDeclaration(*SpecialFunctionDeclaration) T
 	VisitCompositeDeclaration(*CompositeDeclaration) T
 	VisitAttachmentDeclaration(*AttachmentDeclaration) T
@@ -68,7 +68,7 @@ func AcceptDeclaration[T any](declaration Declaration, visitor DeclarationVisito
 		return visitor.VisitVariableDeclaration(declaration.(*VariableDeclaration))
 
 	case ElementTypeFunctionDeclaration:
-		return visitor.VisitFunctionDeclaration(declaration.(*FunctionDeclaration))
+		return visitor.VisitFunctionDeclaration(declaration.(*FunctionDeclaration), false)
 
 	case ElementTypeSpecialFunctionDeclaration:
 		return visitor.VisitSpecialFunctionDeclaration(declaration.(*SpecialFunctionDeclaration))
@@ -151,7 +151,7 @@ func AcceptStatement[T any](statement Statement, visitor StatementVisitor[T]) (_
 		return visitor.VisitVariableDeclaration(statement.(*VariableDeclaration))
 
 	case ElementTypeFunctionDeclaration:
-		return visitor.VisitFunctionDeclaration(statement.(*FunctionDeclaration))
+		return visitor.VisitFunctionDeclaration(statement.(*FunctionDeclaration), true)
 
 	case ElementTypeSpecialFunctionDeclaration:
 		return visitor.VisitSpecialFunctionDeclaration(statement.(*SpecialFunctionDeclaration))

interpreter/interpreter.go

@@ -703,10 +703,10 @@ func (interpreter *Interpreter) VisitProgram(program *ast.Program) {
 }
 
 func (interpreter *Interpreter) VisitSpecialFunctionDeclaration(declaration *ast.SpecialFunctionDeclaration) StatementResult {
-	return interpreter.VisitFunctionDeclaration(declaration.FunctionDeclaration)
+	return interpreter.VisitFunctionDeclaration(declaration.FunctionDeclaration, false)
 }
 
-func (interpreter *Interpreter) VisitFunctionDeclaration(declaration *ast.FunctionDeclaration) StatementResult {
+func (interpreter *Interpreter) VisitFunctionDeclaration(declaration *ast.FunctionDeclaration, isStatement bool) StatementResult {
 
 	identifier := declaration.Identifier.Identifier
 
@@ -717,6 +717,25 @@ func (interpreter *Interpreter) VisitFunctionDeclaration(declaration *ast.Functi
 
 	// lexical scope: variables in functions are bound to what is visible at declaration time
 	lexicalScope := interpreter.activations.CurrentOrNew()
+	if isStatement {
+		// Cloning the current scope ensures that the function can access variables that are visible,
+		// but not variables which are declared after the function
+		// (variable declarations mutate the current activation in place).
+		//
+		// For example:
+		//
+		//     fun foo(a: Int): Int {
+		//         fun bar(): Int {
+		//             return a
+		//             //     ^ should refer to the `a` parameter of `foo`,
+		//             //     not to the `a` variable declared after `bar`
+		//         }
+		//         let a = 2
+		//         return bar()
+		//     }
+		//
+		lexicalScope = lexicalScope.Clone()
+	}
 
 	// make the function itself available inside the function
 	lexicalScope.Set(identifier, variable)

interpreter/interpreter_expression.go

@@ -1315,8 +1315,24 @@ func (interpreter *Interpreter) visitEntries(entries []ast.DictionaryEntry) []Di
 
 func (interpreter *Interpreter) VisitFunctionExpression(expression *ast.FunctionExpression) Value {
 
-	// lexical scope: variables in functions are bound to what is visible at declaration time
-	lexicalScope := interpreter.activations.CurrentOrNew()
+	// lexical scope: variables in functions are bound to what is visible at declaration time.
+	// Cloning the current scope ensures that the function can access variables that are visible,
+	// but not variables which are declared after the function
+	// (variable declarations mutate the current activation in place).
+	//
+	// For example:
+	//
+	//     fun foo(a: Int): Int {
+	//         let bar = fun(): Int {
+	//             return a
+	//             //     ^ should refer to the `a` parameter of `foo`,
+	//             //     not to the `a` variable declared after `bar`
+	//         }
+	//         let a = 2
+	//         return bar()
+	//     }
+	//
+	lexicalScope := interpreter.activations.CurrentOrNew().Clone()
 
 	functionType := interpreter.Program.Elaboration.FunctionExpressionFunctionType(expression)
 

interpreter/misc_test.go

@@ -6864,6 +6864,58 @@ func TestInterpretClosure(t *testing.T) {
 	)
 }
 
+func TestInterpretClosureScopingFunctionExpression(t *testing.T) {
+	t.Parallel()
+
+	inter := parseCheckAndInterpret(t, `
+        fun test(a: Int): Int {
+            let bar = fun(): Int {
+                return a
+            }
+            let a = 2
+            return bar()
+        }
+    `)
+
+	value, err := inter.Invoke("test",
+		interpreter.NewUnmeteredIntValueFromInt64(1),
+	)
+	require.NoError(t, err)
+
+	AssertValuesEqual(
+		t,
+		inter,
+		interpreter.NewUnmeteredIntValueFromInt64(1),
+		value,
+	)
+}
+
+func TestInterpretClosureScopingInnerFunction(t *testing.T) {
+	t.Parallel()
+
+	inter := parseCheckAndInterpret(t, `
+        fun test(a: Int): Int {
+            fun bar(): Int {
+                return a
+            }
+            let a = 2
+            return bar()
+        }
+    `)
+
+	value, err := inter.Invoke("test",
+		interpreter.NewUnmeteredIntValueFromInt64(1),
+	)
+	require.NoError(t, err)
+
+	AssertValuesEqual(
+		t,
+		inter,
+		interpreter.NewUnmeteredIntValueFromInt64(1),
+		value,
+	)
+}
+
 // TestInterpretCompositeFunctionInvocationFromImportingProgram checks
 // that member functions of imported composites can be invoked from an importing program.
 // See https://github.com/dapperlabs/flow-go/issues/838

sema/check_function.go

@@ -31,7 +31,7 @@ func PurityFromAnnotation(purity ast.FunctionPurity) FunctionPurity {
 
 }
 
-func (checker *Checker) VisitFunctionDeclaration(declaration *ast.FunctionDeclaration) (_ struct{}) {
+func (checker *Checker) VisitFunctionDeclaration(declaration *ast.FunctionDeclaration, _ bool) (_ struct{}) {
 	checker.visitFunctionDeclaration(
 		declaration,
 		functionDeclarationOptions{
@@ -46,7 +46,7 @@ func (checker *Checker) VisitFunctionDeclaration(declaration *ast.FunctionDeclar
 }
 
 func (checker *Checker) VisitSpecialFunctionDeclaration(declaration *ast.SpecialFunctionDeclaration) struct{} {
-	return checker.VisitFunctionDeclaration(declaration.FunctionDeclaration)
+	return checker.VisitFunctionDeclaration(declaration.FunctionDeclaration, false)
 }
 
 type functionDeclarationOptions struct {

sema/gen/main.go

@@ -185,7 +185,7 @@ func (*generator) VisitVariableDeclaration(_ *ast.VariableDeclaration) struct{}
 	panic("variable declarations are not supported")
 }
 
-func (g *generator) VisitFunctionDeclaration(decl *ast.FunctionDeclaration) (_ struct{}) {
+func (g *generator) VisitFunctionDeclaration(decl *ast.FunctionDeclaration, _ bool) (_ struct{}) {
 	if len(g.typeStack) == 0 {
 		panic("global function declarations are not supported")
 	}