Merge pull request #3746 from onflow/bastian/improve-set-instructions

[Compiler] Fix operand order for SetField and SetIndex instructions, check resource invalidation

bbq/compiler/compiler.go

@@ -623,39 +623,55 @@ func (c *Compiler[_]) VisitVariableDeclaration(declaration *ast.VariableDeclarat
 	c.compileExpression(declaration.Value)
 
 	varDeclTypes := c.ExtendedElaboration.VariableDeclarationTypes(declaration)
-	c.emitCheckType(varDeclTypes.TargetType)
+	c.emitTransfer(varDeclTypes.TargetType)
 
 	local := c.currentFunction.declareLocal(declaration.Identifier.Identifier)
 	c.codeGen.Emit(opcode.InstructionSetLocal{LocalIndex: local.index})
 	return
 }
 
 func (c *Compiler[_]) VisitAssignmentStatement(statement *ast.AssignmentStatement) (_ struct{}) {
-	c.compileExpression(statement.Value)
-
-	assignmentTypes := c.ExtendedElaboration.AssignmentStatementTypes(statement)
-	c.emitCheckType(assignmentTypes.TargetType)
 
 	switch target := statement.Target.(type) {
 	case *ast.IdentifierExpression:
+		c.compileExpression(statement.Value)
+		assignmentTypes := c.ExtendedElaboration.AssignmentStatementTypes(statement)
+		c.emitTransfer(assignmentTypes.TargetType)
+
 		varName := target.Identifier.Identifier
 		local := c.currentFunction.findLocal(varName)
 		if local != nil {
-			c.codeGen.Emit(opcode.InstructionSetLocal{LocalIndex: local.index})
+			c.codeGen.Emit(opcode.InstructionSetLocal{
+				LocalIndex: local.index,
+			})
 			return
 		}
 
 		global := c.findGlobal(varName)
-		c.codeGen.Emit(opcode.InstructionSetGlobal{GlobalIndex: global.index})
+		c.codeGen.Emit(opcode.InstructionSetGlobal{
+			GlobalIndex: global.index,
+		})
 
 	case *ast.MemberExpression:
 		c.compileExpression(target.Expression)
+
+		c.compileExpression(statement.Value)
+		assignmentTypes := c.ExtendedElaboration.AssignmentStatementTypes(statement)
+		c.emitTransfer(assignmentTypes.TargetType)
+
 		constant := c.addStringConst(target.Identifier.Identifier)
-		c.codeGen.Emit(opcode.InstructionSetField{FieldNameIndex: constant.index})
+		c.codeGen.Emit(opcode.InstructionSetField{
+			FieldNameIndex: constant.index,
+		})
 
 	case *ast.IndexExpression:
 		c.compileExpression(target.TargetExpression)
 		c.compileExpression(target.IndexingExpression)
+
+		c.compileExpression(statement.Value)
+		assignmentTypes := c.ExtendedElaboration.AssignmentStatementTypes(statement)
+		c.emitTransfer(assignmentTypes.TargetType)
+
 		c.codeGen.Emit(opcode.InstructionSetIndex{})
 
 	default:
@@ -894,7 +910,7 @@ func (c *Compiler[_]) loadArguments(expression *ast.InvocationExpression) {
 	invocationTypes := c.ExtendedElaboration.InvocationExpressionTypes(expression)
 	for index, argument := range expression.Arguments {
 		c.compileExpression(argument.Expression)
-		c.emitCheckType(invocationTypes.ArgumentTypes[index])
+		c.emitTransfer(invocationTypes.ArgumentTypes[index])
 	}
 
 	// TODO: Is this needed?
@@ -928,7 +944,9 @@ func (c *Compiler[_]) loadTypeArguments(expression *ast.InvocationExpression) []
 func (c *Compiler[_]) VisitMemberExpression(expression *ast.MemberExpression) (_ struct{}) {
 	c.compileExpression(expression.Expression)
 	constant := c.addStringConst(expression.Identifier.Identifier)
-	c.codeGen.Emit(opcode.InstructionGetField{FieldNameIndex: constant.index})
+	c.codeGen.Emit(opcode.InstructionGetField{
+		FieldNameIndex: constant.index,
+	})
 	return
 }
 
@@ -1355,7 +1373,7 @@ func (c *Compiler[_]) patchLoop(l *loop) {
 	}
 }
 
-func (c *Compiler[_]) emitCheckType(targetType sema.Type) {
+func (c *Compiler[_]) emitTransfer(targetType sema.Type) {
 	index := c.getOrAddType(targetType)
 
 	c.codeGen.Emit(opcode.InstructionTransfer{TypeIndex: index})

bbq/opcode/instructions.yml

@@ -68,7 +68,7 @@
       type: "index"
   valueEffects:
     pop:
-      - name: "container"
+      - name: "target"
         type: "value"
       - name: "value"
         type: "value"

bbq/vm/vm.go

@@ -107,25 +107,34 @@ func (vm *VM) pop() Value {
 	return value
 }
 
-// pop2NonResource removes and returns the top two non-resource values from the stack:
+// pop2 removes and returns the top two values from the stack:
 // N-2, and N-1, where N is the number of elements on the stack.
-// It is efficient than calling `pop` twice, in the case of non-resource values.
-func (vm *VM) pop2NonResource() (Value, Value) {
+// It is efficient than calling `pop` twice.
+func (vm *VM) pop2() (Value, Value) {
 	lastIndex := len(vm.stack) - 1
 	value1, value2 := vm.stack[lastIndex-1], vm.stack[lastIndex]
 	vm.stack[lastIndex-1], vm.stack[lastIndex] = nil, nil
 	vm.stack = vm.stack[:lastIndex-1]
+
+	checkInvalidatedResourceOrResourceReference(value1)
+	checkInvalidatedResourceOrResourceReference(value2)
+
 	return value1, value2
 }
 
-// pop3NonResource removes and returns the top three non-resource values from the stack:
+// pop3 removes and returns the top three values from the stack:
 // N-3, N-2, and N-1, where N is the number of elements on the stack.
-// It is efficient than calling `pop` thrice, in the case of non-resource values.
-func (vm *VM) pop3NonResource() (Value, Value, Value) {
+// It is efficient than calling `pop` thrice.
+func (vm *VM) pop3() (Value, Value, Value) {
 	lastIndex := len(vm.stack) - 1
 	value1, value2, value3 := vm.stack[lastIndex-2], vm.stack[lastIndex-1], vm.stack[lastIndex]
 	vm.stack[lastIndex-2], vm.stack[lastIndex-1], vm.stack[lastIndex] = nil, nil, nil
 	vm.stack = vm.stack[:lastIndex-2]
+
+	checkInvalidatedResourceOrResourceReference(value1)
+	checkInvalidatedResourceOrResourceReference(value2)
+	checkInvalidatedResourceOrResourceReference(value3)
+
 	return value1, value2, value3
 }
 
@@ -143,7 +152,9 @@ func (vm *VM) peek() Value {
 func (vm *VM) dropN(count int) {
 	stackHeight := len(vm.stack)
 	startIndex := stackHeight - count
-	// TODO: checkInvalidatedResourceOrResourceReference for all dropped values?
+	for _, value := range vm.stack[startIndex:] {
+		checkInvalidatedResourceOrResourceReference(value)
+	}
 	clear(vm.stack[startIndex:])
 	vm.stack = vm.stack[:startIndex]
 }
@@ -412,16 +423,16 @@ func opSetGlobal(vm *VM, ins opcode.InstructionSetGlobal) {
 }
 
 func opSetIndex(vm *VM) {
-	element, array, index := vm.pop3NonResource()
-	indexValue := index.(IntValue)
+	array, index, element := vm.pop3()
 	arrayValue := array.(*ArrayValue)
+	indexValue := index.(IntValue)
 	arrayValue.Set(vm.config, int(indexValue.SmallInt), element)
 }
 
 func opGetIndex(vm *VM) {
-	array, index := vm.pop2NonResource()
-	indexValue := index.(IntValue)
+	array, index := vm.pop2()
 	arrayValue := array.(*ArrayValue)
+	indexValue := index.(IntValue)
 	element := arrayValue.Get(vm.config, int(indexValue.SmallInt))
 	vm.push(element)
 }
@@ -520,15 +531,13 @@ func opNew(vm *VM, ins opcode.InstructionNew) {
 }
 
 func opSetField(vm *VM, ins opcode.InstructionSetField) {
-	// TODO: support all container types
-	structValue := vm.pop().(MemberAccessibleValue)
-
-	fieldValue := vm.pop()
+	target, fieldValue := vm.pop2()
 
 	// VM assumes the field name is always a string.
 	fieldName := getStringConstant(vm, ins.FieldNameIndex)
 
-	structValue.SetMember(vm.config, fieldName, fieldValue)
+	target.(MemberAccessibleValue).
+		SetMember(vm.config, fieldName, fieldValue)
 }
 
 func opGetField(vm *VM, ins opcode.InstructionGetField) {
@@ -567,7 +576,11 @@ func opTransfer(vm *VM, ins opcode.InstructionTransfer) {
 
 	valueType := transferredValue.StaticType(config)
 	if !IsSubType(config, valueType, targetType) {
-		panic(errors.NewUnexpectedError("invalid transfer: expected '%s', found '%s'", targetType, valueType))
+		panic(errors.NewUnexpectedError(
+			"invalid transfer: expected '%s', found '%s'",
+			targetType,
+			valueType,
+		))
 	}
 
 	vm.replaceTop(transferredValue)

bbq/vm/vm_test.go

@@ -118,7 +118,7 @@ func TestVM_replaceTop(t *testing.T) {
 	)
 }
 
-func TestVM_pop2NonResource(t *testing.T) {
+func TestVM_pop2(t *testing.T) {
 	t.Parallel()
 
 	program := &bbq.Program[opcode.Instruction]{}
@@ -128,7 +128,7 @@ func TestVM_pop2NonResource(t *testing.T) {
 	vm.push(NewIntValue(2))
 	vm.push(NewIntValue(3))
 
-	a, b := vm.pop2NonResource()
+	a, b := vm.pop2()
 
 	assert.Equal(t, NewIntValue(2), a)
 	assert.Equal(t, NewIntValue(3), b)
@@ -141,7 +141,7 @@ func TestVM_pop2NonResource(t *testing.T) {
 	)
 }
 
-func TestVM_pop3NonResource(t *testing.T) {
+func TestVM_pop3(t *testing.T) {
 	t.Parallel()
 
 	program := &bbq.Program[opcode.Instruction]{}
@@ -162,7 +162,7 @@ func TestVM_pop3NonResource(t *testing.T) {
 		},
 	)
 
-	a, b, c := vm.pop3NonResource()
+	a, b, c := vm.pop3()
 
 	assert.Equal(t, NewIntValue(2), a)
 	assert.Equal(t, NewIntValue(3), b)