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

double assignment + closure, in a for loop, reuses the same variable, instead of assigning to a new one. #1497

Closed
mpl opened this issue Dec 23, 2022 · 1 comment · Fixed by #1529
Labels
area/core bug Something isn't working

Comments

@mpl
Copy link
Collaborator

mpl commented Dec 23, 2022

The following program sample.go triggers an unexpected result

package main

import (
	"bufio"
	"flag"
	"fmt"
	"strconv"
	"strings"
)

var debug = flag.Bool("debug", false, `debug mode`)

type monkey struct {
	items        []int
	operation    func(int) int
	test         func(int) bool
	monkeyTrue   int
	monkeyFalse  int
	inspectCount int
}

func parseOperation(operation string) func(int) int {
	parts := strings.Split(operation, " ")
	self := false
	if parts[1] == "old" {
		self = true
	}
	operand, _ := strconv.Atoi(parts[1])
	sign := string(operation[0])
	if sign == "+" {
		return func(i int) int {
			if self {
				return i + i
			}
			return i + operand
		}
	}
	if sign == "*" {
		return func(i int) int {
			if self {
				return i * i
			}
			return i * operand
		}
	}
	panic(fmt.Sprintf("unsupported operation: %s", operation))
}

var input = `Monkey 0:
  Starting items: 79, 98
  Operation: new = old * 19
  Test: divisible by 23
    If true: throw to monkey 2
    If false: throw to monkey 3

Monkey 1:
  Starting items: 54, 65, 75, 74
  Operation: new = old + 6
  Test: divisible by 19
    If true: throw to monkey 2
    If false: throw to monkey 0

Monkey 2:
  Starting items: 79, 60, 97
  Operation: new = old * old
  Test: divisible by 13
    If true: throw to monkey 1
    If false: throw to monkey 3

Monkey 3:
  Starting items: 74
  Operation: new = old + 3
  Test: divisible by 17
    If true: throw to monkey 0
    If false: throw to monkey 1
`

func main() {
	flag.Parse()
	/*
		f, err := os.Open("./input.txt")
		if err != nil {
			log.Fatal(err)
		}
		defer f.Close()
	*/

	f := strings.NewReader(input)
	sc := bufio.NewScanner(f)

	var monkeys []*monkey

	debugCount := 0
	for sc.Scan() {
		if *debug {
			if debugCount > 34 {
				break
			}
		}
		debugCount++

		l := sc.Text()
		if !strings.HasPrefix(l, "Monkey") {
			panic("BIM")
		}
		kong := monkey{}

		sc.Scan()
		l = sc.Text()
		items := strings.Split(strings.TrimPrefix(l, "  Starting items: "), ", ")
		for _, v := range items {
			item, _ := strconv.Atoi(v)
			kong.items = append(kong.items, item)
		}

		sc.Scan()
		l = sc.Text()
		operation := strings.TrimPrefix(l, "  Operation: new = old ")
		kong.operation = parseOperation(operation)

		sc.Scan()
		l = sc.Text()
		if !strings.HasPrefix(l, "  Test: divisible by ") {
			panic(fmt.Sprintf("unsupported test: %s", l))
		}
		divisor, _ := strconv.Atoi(strings.TrimPrefix(l, "  Test: divisible by "))
		if *debug {
			println(len(monkeys), "DIVISOR: ", divisor)
		}
		// TODO: we should defer, and make it directly return the monkey destination
		kong.test = func(i int) bool {
			return i%divisor == 0
		}

		sc.Scan()
		l = sc.Text()
		kong.monkeyTrue, _ = strconv.Atoi(strings.TrimPrefix(l, "    If true: throw to monkey "))
		if *debug {
			println(len(monkeys), "MONKEYTRUE: ", kong.monkeyTrue)
		}

		sc.Scan()
		l = sc.Text()
		kong.monkeyFalse, _ = strconv.Atoi(strings.TrimPrefix(l, "    If false: throw to monkey "))
		if *debug {
			println(len(monkeys), "MONKEYFALSE: ", kong.monkeyFalse)
		}

		monkeys = append(monkeys, &kong)

		sc.Scan()
	}

	for i := 0; i < 20; i++ {
		for idx, mk := range monkeys {
			if *debug {
				println("MONKEY ", idx)
			}
			for k, item := range mk.items {
				if *debug {
					println(k, "OLD WORRY: ", item)
				}
				worry := mk.operation(item)
				worry = worry / 3
				// mk.items[k] = worry
				if *debug {
					println(k, "NEW WORRY: ", worry)
				}

				monkeyDest := 0
				if mk.test(worry) {
					monkeyDest = mk.monkeyTrue
				} else {
					monkeyDest = mk.monkeyFalse
				}
				if *debug {
					println(k, worry, "THROWING TO MK ", monkeyDest)
				}
				monkeys[monkeyDest].items = append(monkeys[monkeyDest].items, worry)
				mk.inspectCount++
			}
			mk.items = []int{}
		}
	}

	for i, mk := range monkeys {
		println("Monkey", i, "inspected items", mk.inspectCount, "times.")
	}

	// Answer: 54752
}

Expected result

% go run ./main.go
Monkey 0 inspected items 101 times.
Monkey 1 inspected items 95 times.
Monkey 2 inspected items 7 times.
Monkey 3 inspected items 105 times.

Got

% yaegi run ./main.go 
Monkey 0 inspected items 99 times.
Monkey 1 inspected items 97 times.
Monkey 2 inspected items 13 times.
Monkey 3 inspected items 108 times.

Yaegi Version

eee72d1

Additional Notes

I was solving the 11th advent of code: https://adventofcode.com/2022/day/11
and I noticed the result given by yaegi is different from the one with the go compiler.

I haven't had time to narrow it down yet, and to write a small repro. However, I strongly suspect the bug is connected to:
monkeys[monkeyDest].items = append(monkeys[monkeyDest].items, worry)
as the rest of the code is completely trivial.

I will update this issue with a better repro and title later.

@mpl
Copy link
Collaborator Author

mpl commented Dec 26, 2022

Alright, here's the actual bug, which actually has nothing to do with what I was suspecting:

package main

import "strconv"

type monkey struct {
	test func() int
}

func main() {
	input := []string{"1", "2", "3"}

	var monkeys []*monkey

	//	for k, v := range input { // <-- no problem if we assign k to divisor, for example.
	for _, v := range input {
		kong := monkey{}

		// divisor := k <-- no problem
		divisor, err := strconv.Atoi(v)
		if err != nil {
			panic(err)
		}
		// divisor, _ := := strconv.Atoi(v) <-- also buggy
		println(divisor)
		kong.test = func() int {
			return divisor
		}
		monkeys = append(monkeys, &kong)
	}

	for _, mk := range monkeys {
		println(mk.test())
	}

}
% go run ./issues/1497/main.go
1
2
3
1
2
3
% 
% 
% 
% yaegi run ./issues/1497/main.go
1
2
3
3
3
3

So I believe it's the combination of the double return/assignment (from strconv.Atoi in that case), and the use in a closure, that leads to the bug. If the assignment to divisor does not come from a func with a double return, then there's no problem.

@mpl mpl changed the title temporary title: objects in slice not properly updated when iterating on them double assignment + closure, in a for loop, reuses the same variable, instead of assigning to a new one. Dec 26, 2022
@mvertes mvertes added bug Something isn't working area/core labels Mar 16, 2023
mvertes added a commit that referenced this issue Mar 22, 2023
In the case of a Go short definition (i.e. `a, b := f()`), the
new defined variables must be (re-)created in order to preserve the
previous value (if in a loop) which can be still in use in the
context of a closure. This must not apply to redeclared variables
which simply see their value reassigned.

The problem was both occuring in callBin() for runtime functions
and assignFromCall() for functions created in the interpreter.

Fixes #1497.
traefiker pushed a commit that referenced this issue Mar 24, 2023
In the case of a Go short definition (i.e. `a, b := f()`), the new defined variables must be (re-)created in order to preserve the previous value (if in a loop) which can be still in use in the context of a closure. This must not apply to redeclared variables which simply see their value reassigned.

The problem was both occuring in callBin() for runtime functions and assignFromCall() for functions created in the interpreter.

Fixes #1497.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants