munificent / craftinginterpreters

Repository for the book "Crafting Interpreters"
http://www.craftinginterpreters.com/
Other
8.42k stars 1.01k forks source link

Token definition is not suitable unique (in immutable language/contexts) #1154

Open jcdavis opened 5 months ago

jcdavis commented 5 months ago

Chapter 11 (Resolving and Binding) introduces a Hashmap from Expr to depth. As written in Java, this works fine, because Java's default "dumb" hashcode/equality is identity-based, and tokens/expressions are never created after scanning/parsing, so they are all considered distinct. However using a more immutable-based pattern (either explicitly, or implicitly in languages where that is the default behavior), the current class definition is not suitably unique, IE 2 different instances of the same tokens on the same line are considered identical. This breaks the given fib example

fun fib(n) {
  if (n <= 1) return n;
  return fib(n - 2) + fib(n - 1);
}

for (var i = 0; i < 20; i = i + 1) {
  print fib(i);
}

The way the for loop blocks are constructed, The is in the increment clause are in a different block from the initialization/condition, and the locals map needs to hold these separately with their respective different depths. Without this, these 2 lookups collide, one of the resolutions is thrown away, and the interpreter will fail.

As mentioned above, this isn't a problem in the reference java solution due to java's default equals/hashCode - in my case, I'm implementing Section II in python and using @dataclass(frozen=true) for the immutable value types (Token, Expr, Stmt etc). However with the introduction of record classes in Java 16, someone hoping to follow that best practice would hit the same issue, so it seems possibly worth adding to avoid future annoying debugging (speaking from experience :))

My quick, and maybe not necessarily correct/optimal fix for this was to include the start from the scanner in the token definition in addition line, which makes the different i variable definitions unique. I could throw up a quick PR with this change, but I'm not sure that its the best approach.

Just want to say thanks for the great book, and making it available to read/update online to go alongside the physical version