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

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

Open
jcdavis opened this issue Jan 17, 2024 · 1 comment
Open

Comments

@jcdavis
Copy link

jcdavis commented Jan 17, 2024

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

@WJWH
Copy link

WJWH commented Jul 31, 2024

I have just run into this bug in my Haskell version as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants