From b400c6b4ef2a83cfdd7032e78d039a81693f8f2b Mon Sep 17 00:00:00 2001 From: ctsk <9384305+ctsk@users.noreply.github.com> Date: Wed, 14 Sep 2022 20:45:54 +0200 Subject: [PATCH] [jlox] Add Resolution and Binding A very subtle bug: The resolution relies on the fact that Java has a concept of object identity: Even when the "Contents" of two objects are equal, those objects are not considered equal. This does not hold for records: ``` record Test(int val) {} var a = new Test(1); var b = new Test(1); var m = new HashMap<>(); m.put(a, "Hello"); m.get(b); // returns "Hello" ``` tl;dr - got bitten by trying to do things fancy --- .../main/java/xyz/ctsk/lox/Environment.java | 19 ++ .../main/java/xyz/ctsk/lox/FunctionType.java | 7 + .../main/java/xyz/ctsk/lox/Interpreter.java | 29 ++- jlox/lox/src/main/java/xyz/ctsk/lox/Lox.java | 7 +- .../main/java/xyz/ctsk/lox/LoxFunction.java | 6 +- .../src/main/java/xyz/ctsk/lox/Parser.java | 1 - .../src/main/java/xyz/ctsk/lox/Resolver.java | 197 ++++++++++++++++++ .../src/main/java/xyz/ctsk/lox/Scanner.java | 4 +- .../lox/src/main/java/xyz/ctsk/lox/Token.java | 6 +- .../test/java/xyz/ctsk/lox/ResolverTest.java | 13 ++ .../resources/xyz/ctsk/lox/ResolverBug.lox | 1 + 11 files changed, 278 insertions(+), 12 deletions(-) create mode 100644 jlox/lox/src/main/java/xyz/ctsk/lox/FunctionType.java create mode 100644 jlox/lox/src/main/java/xyz/ctsk/lox/Resolver.java create mode 100644 jlox/lox/src/test/java/xyz/ctsk/lox/ResolverTest.java create mode 100644 jlox/lox/src/test/resources/xyz/ctsk/lox/ResolverBug.lox diff --git a/jlox/lox/src/main/java/xyz/ctsk/lox/Environment.java b/jlox/lox/src/main/java/xyz/ctsk/lox/Environment.java index 18e2791..ed55b46 100644 --- a/jlox/lox/src/main/java/xyz/ctsk/lox/Environment.java +++ b/jlox/lox/src/main/java/xyz/ctsk/lox/Environment.java @@ -20,6 +20,16 @@ public class Environment { values.put(name, value); } + private Environment ancestor(int distance) { + Environment environment = this; + + for (int i = 0; i < distance; i++) { + environment = environment.enclosing; + } + + return environment; + } + void assign(Token name, Object value) { if (values.containsKey(name.lexeme())) { values.put(name.lexeme(), value); @@ -30,6 +40,10 @@ public class Environment { } } + void assignAt(int distance, Token name, Object value) { + ancestor(distance).values.put(name.lexeme(), value); + } + Object get(Token name) { if (values.containsKey(name.lexeme())) { return values.get(name.lexeme()); @@ -40,4 +54,9 @@ public class Environment { throw new RuntimeError(name, message); } } + + Object getAt(int distance, String name) { + return ancestor(distance).values.get(name); + } + } diff --git a/jlox/lox/src/main/java/xyz/ctsk/lox/FunctionType.java b/jlox/lox/src/main/java/xyz/ctsk/lox/FunctionType.java new file mode 100644 index 0000000..9218a77 --- /dev/null +++ b/jlox/lox/src/main/java/xyz/ctsk/lox/FunctionType.java @@ -0,0 +1,7 @@ +package xyz.ctsk.lox; + +public enum FunctionType { + NONE, + FUNCTION, + METHOD +} diff --git a/jlox/lox/src/main/java/xyz/ctsk/lox/Interpreter.java b/jlox/lox/src/main/java/xyz/ctsk/lox/Interpreter.java index 3fb0233..fea0580 100644 --- a/jlox/lox/src/main/java/xyz/ctsk/lox/Interpreter.java +++ b/jlox/lox/src/main/java/xyz/ctsk/lox/Interpreter.java @@ -1,10 +1,13 @@ package xyz.ctsk.lox; +import java.util.HashMap; import java.util.List; +import java.util.Map; public class Interpreter implements Expr.Visitor, Stmt.Visitor { final Environment globals = new Environment(); private Environment environment = globals; + private final Map locals = new HashMap<>(); Interpreter() { @@ -54,6 +57,10 @@ public class Interpreter implements Expr.Visitor, Stmt.Visitor { return expr.accept(this); } + void resolve(Expr expr, int depth) { + locals.put(expr, depth); + } + static String stringify(Object object) { if (object == null) return "nil"; @@ -79,7 +86,7 @@ public class Interpreter implements Expr.Visitor, Stmt.Visitor { @Override public Void visitFunctionStmt(Stmt.Function stmt) { - environment.define(stmt.name().lexeme(), new LoxFunction(stmt)); + environment.define(stmt.name().lexeme(), new LoxFunction(stmt, environment)); return null; } @@ -124,7 +131,14 @@ public class Interpreter implements Expr.Visitor, Stmt.Visitor { @Override public Object visitAssignExpr(Expr.Assign expr) { var value = evaluate(expr.value()); - environment.assign(expr.name(), value); + + Integer distance = locals.get(expr); + if (distance != null) { + environment.assignAt(distance, expr.name(), value); + } else { + globals.assign(expr.name(), value); + } + return value; } @@ -218,7 +232,16 @@ public class Interpreter implements Expr.Visitor, Stmt.Visitor { @Override public Object visitVariableExpr(Expr.Variable expr) { - return environment.get(expr.name()); + return lookupVariable(expr.name(), expr); + } + + private Object lookupVariable(Token name, Expr.Variable expr) { + var distance = locals.get(expr); + if (distance != null) { + return environment.getAt(distance, name.lexeme()); + } else { + return globals.get(name); + } } diff --git a/jlox/lox/src/main/java/xyz/ctsk/lox/Lox.java b/jlox/lox/src/main/java/xyz/ctsk/lox/Lox.java index 63711e8..52911ba 100644 --- a/jlox/lox/src/main/java/xyz/ctsk/lox/Lox.java +++ b/jlox/lox/src/main/java/xyz/ctsk/lox/Lox.java @@ -45,6 +45,11 @@ public class Lox { if (hadError) return; + var resolver = new Resolver(interpreter); + resolver.resolve(statements); + + if (hadError) return; + interpreter.interpret(statements); } @@ -61,7 +66,7 @@ public class Lox { } } - private static void runFile(String path) throws IOException { + protected static void runFile(String path) throws IOException { byte[] bytes = Files.readAllBytes(Paths.get(path)); run(new String(bytes, Charset.defaultCharset())); diff --git a/jlox/lox/src/main/java/xyz/ctsk/lox/LoxFunction.java b/jlox/lox/src/main/java/xyz/ctsk/lox/LoxFunction.java index 9c2c29f..cb5114f 100644 --- a/jlox/lox/src/main/java/xyz/ctsk/lox/LoxFunction.java +++ b/jlox/lox/src/main/java/xyz/ctsk/lox/LoxFunction.java @@ -4,9 +4,11 @@ import java.util.List; public class LoxFunction implements LoxCallable { private final Stmt.Function declaration; + private final Environment closure; - public LoxFunction(Stmt.Function declaration) { + public LoxFunction(Stmt.Function declaration, Environment closure) { this.declaration = declaration; + this.closure = closure; } @Override @@ -16,7 +18,7 @@ public class LoxFunction implements LoxCallable { @Override public Object call(Interpreter interpreter, List arguments) { - Environment environment = new Environment(interpreter.globals); + Environment environment = new Environment(closure); for (int i = 0; i < declaration.params().size(); i++) { environment.define(declaration.params().get(i).lexeme(), arguments.get(i)); diff --git a/jlox/lox/src/main/java/xyz/ctsk/lox/Parser.java b/jlox/lox/src/main/java/xyz/ctsk/lox/Parser.java index 3e381c8..8236bc0 100644 --- a/jlox/lox/src/main/java/xyz/ctsk/lox/Parser.java +++ b/jlox/lox/src/main/java/xyz/ctsk/lox/Parser.java @@ -430,5 +430,4 @@ public class Parser { } private static class ParseError extends RuntimeException {} - private enum FunctionType { FUNCTION } } diff --git a/jlox/lox/src/main/java/xyz/ctsk/lox/Resolver.java b/jlox/lox/src/main/java/xyz/ctsk/lox/Resolver.java new file mode 100644 index 0000000..0bb747e --- /dev/null +++ b/jlox/lox/src/main/java/xyz/ctsk/lox/Resolver.java @@ -0,0 +1,197 @@ +package xyz.ctsk.lox; + + +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Stack; + +public class Resolver implements Expr.Visitor, Stmt.Visitor { + private final Interpreter interpreter; + private final Stack> scopes = new Stack<>(); + private FunctionType currentFuntion = FunctionType.NONE; + + + Resolver(Interpreter interpreter) { + this.interpreter = interpreter; + } + + private void resolve(Stmt stmt) { + stmt.accept(this); + } + + void resolve(List statements) { + statements.forEach(this::resolve); + } + + private void resolve(Expr expr) { + expr.accept(this); + } + + + private void beginScope() { + scopes.push(new HashMap<>()); + } + + private void endScope() { + scopes.pop(); + } + + private void declare(Token name) { + if (scopes.isEmpty()) return; + var scope = scopes.peek(); + if (scope.containsKey(name.lexeme())) { + Lox.error(name, "Already a variable with this name in this scope."); + } + scope.put(name.lexeme(), false); + } + + private void define(Token name) { + if (scopes.isEmpty()) return; + scopes.peek().put(name.lexeme(), true); + } + + private void resolveFunction(Stmt.Function function, FunctionType functionType) { + var enclosingFunction = currentFuntion; + currentFuntion = functionType; + beginScope(); + for (var param : function.params()) { + declare(param); + define(param); + } + resolve(function.body()); + endScope(); + currentFuntion = enclosingFunction; + } + + + private void resolveLocal(Expr expr, Token name) { + for (int i = scopes.size() - 1; i >= 0; i--) { + if (scopes.get(i).containsKey(name.lexeme())) { + interpreter.resolve(expr, scopes.size() - 1 - i); + return; + } + } + } + + @Override + public Void visitBlockStmt(Stmt.Block stmt) { + beginScope(); + resolve(stmt.statements()); + endScope(); + return null; + } + + + @Override + public Void visitExpressionStmt(Stmt.Expression stmt) { + resolve(stmt.expression()); + return null; + } + + @Override + public Void visitFunctionStmt(Stmt.Function stmt) { + declare(stmt.name()); + define(stmt.name()); + resolveFunction(stmt, FunctionType.FUNCTION); + return null; + } + + @Override + public Void visitIfStmt(Stmt.If stmt) { + resolve(stmt.condition()); + resolve(stmt.thenBranch()); + if (stmt.elseBranch() != null) resolve(stmt.elseBranch()); + return null; + } + + @Override + public Void visitPrintStmt(Stmt.Print stmt) { + resolve(stmt.expression()); + return null; + } + + @Override + public Void visitReturnStmt(Stmt.Return stmt) { + if (currentFuntion == FunctionType.NONE) { + Lox.error(stmt.keyword(), "Can't return fom top-level code."); + } + + if (stmt.value() != null) { + resolve(stmt.value()); + } + return null; + } + + @Override + public Void visitVarStmt(Stmt.Var stmt) { + declare(stmt.name()); + if (stmt.initializer() != null) { + resolve(stmt.initializer()); + } + define(stmt.name()); + return null; + } + + @Override + public Void visitWhileStmt(Stmt.While stmt) { + resolve(stmt.condition()); + resolve(stmt.body()); + return null; + } + + + @Override + public Void visitAssignExpr(Expr.Assign expr) { + resolve(expr.value()); + resolveLocal(expr, expr.name()); + return null; + } + + @Override + public Void visitBinaryExpr(Expr.Binary expr) { + resolve(expr.left()); + resolve(expr.right()); + return null; + } + + @Override + public Void visitCallExpr(Expr.Call expr) { + resolve(expr.callee()); + expr.arguments().forEach(this::resolve); + return null; + } + + @Override + public Void visitGroupingExpr(Expr.Grouping expr) { + resolve(expr.expression()); + return null; + } + + @Override + public Void visitLiteralExpr(Expr.Literal expr) { + return null; + } + + @Override + public Void visitLogicalExpr(Expr.Logical expr) { + resolve(expr.left()); + resolve(expr.right()); + return null; + } + + @Override + public Void visitUnaryExpr(Expr.Unary expr) { + resolve(expr.right()); + return null; + } + + @Override + public Void visitVariableExpr(Expr.Variable expr) { + if (!scopes.isEmpty() && scopes.peek().get(expr.name().lexeme()) == Boolean.FALSE) { + Lox.error(expr.name(), "Can't read local variable in its own initializer."); + } + resolveLocal(expr, expr.name()); + return null; + } +} diff --git a/jlox/lox/src/main/java/xyz/ctsk/lox/Scanner.java b/jlox/lox/src/main/java/xyz/ctsk/lox/Scanner.java index 05ecf21..3ee2bfc 100644 --- a/jlox/lox/src/main/java/xyz/ctsk/lox/Scanner.java +++ b/jlox/lox/src/main/java/xyz/ctsk/lox/Scanner.java @@ -44,7 +44,7 @@ public class Scanner { } private void addToken(TokenType type, Object literal) { - tokens.add(new Token(type, currentMatch(), literal, line)); + tokens.add(new Token(type, currentMatch(), literal, line, current)); } private void addToken(TokenType type) { @@ -191,7 +191,7 @@ public class Scanner { scanToken(); } - tokens.add(new Token(EOF, "", line)); + tokens.add(new Token(EOF, "", line, current)); return tokens; } } diff --git a/jlox/lox/src/main/java/xyz/ctsk/lox/Token.java b/jlox/lox/src/main/java/xyz/ctsk/lox/Token.java index a644dfc..f0ade27 100644 --- a/jlox/lox/src/main/java/xyz/ctsk/lox/Token.java +++ b/jlox/lox/src/main/java/xyz/ctsk/lox/Token.java @@ -1,8 +1,8 @@ package xyz.ctsk.lox; -public record Token(TokenType type, String lexeme, Object literal, int line) { - public Token(TokenType type, String lexeme, int line) { - this(type, lexeme, null, line); +public record Token(TokenType type, String lexeme, Object literal, int line, int position) { + public Token(TokenType type, String lexeme, int line, int position) { + this(type, lexeme, null, line, position); } @Override diff --git a/jlox/lox/src/test/java/xyz/ctsk/lox/ResolverTest.java b/jlox/lox/src/test/java/xyz/ctsk/lox/ResolverTest.java new file mode 100644 index 0000000..4c04dc1 --- /dev/null +++ b/jlox/lox/src/test/java/xyz/ctsk/lox/ResolverTest.java @@ -0,0 +1,13 @@ +package xyz.ctsk.lox; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; + +public class ResolverTest { + @Test + void forLoopBug() throws IOException { + var canary = this.getClass().getResource("ResolverBug.lox").getPath(); + Lox.runFile(canary); + } +} diff --git a/jlox/lox/src/test/resources/xyz/ctsk/lox/ResolverBug.lox b/jlox/lox/src/test/resources/xyz/ctsk/lox/ResolverBug.lox new file mode 100644 index 0000000..34cfc7e --- /dev/null +++ b/jlox/lox/src/test/resources/xyz/ctsk/lox/ResolverBug.lox @@ -0,0 +1 @@ +for (var i = 0; i < 2; i = i + 1) print i; \ No newline at end of file