munificent / craftinginterpreters

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

Invalid section of code? #927

Closed jbduncan closed 3 years ago

jbduncan commented 3 years ago

@munificent Section https://www.craftinginterpreters.com/statements-and-state.html#assignment-syntax shows this block of code:

  private Expr assignment() {
    Expr expr = equality();

    if (match(EQUAL)) {
      Token equals = previous();
      Expr value = assignment();

      if (expr instanceof Expr.Variable) {
        Token name = ((Expr.Variable)expr).name;
        return new Expr.Assign(name, value);
      }

      error(equals, "Invalid assignment target."); 
    }

    return expr;
  }

However, my IDE warns that the "Result of 'error()' is not thrown", so I think it should say:

      . . .
      throw error(equals, "Invalid assignment target."); 
      . . .

What do you think?

P.S. This book is excellent! I'm enjoying reading it so far and it's shed a lot of light for me on how compilers and tools like ANTLR work. I wish you all the best with writing the rest.

jbduncan commented 3 years ago

D'oh! I should've read on ahead first. It says on the side:

We report an error if the left-hand side isn’t a valid assignment target, but we don’t throw it because the parser isn’t in a confused state where we need to go into panic mode and synchronize.

jbduncan commented 3 years ago

Am I right to think that we don't need to synchronize because the lvalue is syntactically correct, but it will be found to be invalid at runtime by the Interpreter and its Environment?

jbduncan commented 3 years ago

Ah, I looked at the code and remembered that calling Lox::error causes Lox.hadError to be set as true, so the Interpreter won't even run in that case...

jbduncan commented 3 years ago

So it's a parser error rather than an interpreter error! 🧠

Although, interestingly, given the following Lox example:

print "one";
print true;
print 2 + 1;

p = 2;
print "Success";

It is able to print "one", "true" and "3" before failing with "Undefined variable 'p'". I suppose I expected it to error out and not print anything, but I understand now that (my) jlox checks then runs each line separately, rather than checking all the lines at once as a compiler might.

Anyway, sorry for the rambling! If any of my understanding is incorrect, I'm very open to constructive criticism.

jbduncan commented 3 years ago

So it's a parser error rather than an interpreter error! 🧠

Ah, my understanding must still be incorrect because, playing with another example:

print "one";
print true;
print 2 + 1;

(p) = 2;
print "Success";

...that one doesn't print any of "one", "true" or "3".

So when the l-value isn't valid, like (p), then it's a parser error, but if it is syntactically correct but doesn't point to a real variable at runtime, like p, then it's an interpreter error.

munificent commented 3 years ago

So when the l-value isn't valid, like (p), then it's a parser error, but if it is syntactically correct but doesn't point to a real variable at runtime, like p, then it's an interpreter error.

Right. In particular, variable accesses are treated as runtime errors to play nicer with the REPL. See the mention of "REPL" here. Treating undefined variables as a runtime error lets REPL sessions like this work:

> fun f() { print(v); }
> var v = "defined after f()";
> f();
jbduncan commented 3 years ago

That makes sense! Thank you for your response @munificent. 👍