Skip to content

Commit

Permalink
Fix for parsing prefixes, closes #409
Browse files Browse the repository at this point in the history
The way ABS used to parse prefixes (eg. -5) was kind of wonky:
we would dump in a token the whole prefix + number (-5) vs
parsing them separately and build the AST according to where
the minus token is found. This change makes it so -5 is actually
`(-)(5)`.

Problems with the previous approach:

* inconsistent behavior between `x = 5; -x` and `-5`
* `-5` would be treated differently than `- 5`
* code such as `1 -1` would be parsed as `(1); (-1)`, producing 2
statements. The return value would be -1 rather than 0

In general, I looked around and the common behavior is to read
tokens separately and, while parsing, figure out, based on their
position, what precedence they should have. This is what this PR
fundamentally does.

For example:

* when `-` is a prefix (`-2`), it should have the highest precedence over
any other operator. For example, `-2.clamp(0, 1)` should interpret as
`(-2).clamp(0, 1)`
* when `-` is not a prefix, it can fallback to its usual precedence (eg.
less than `*` or `/`)

Thie behavior is also consistent with other, languages, take ruby as an
example:

```ruby
irb(main):011:0> -1.clamp(0, 10)
=> 0
```

One way in which ABS now deviates with other implementations
is that, since we do not bother reading whitespaces etc in our lexer
`-1.clamp(0, 10)` and `- 1.clamp(0, 10)` are exactly the same (while
in ruby, for example, the latter is intepreted as `(-)(1.clamp(0,
10))`). I think for now this is a reasonable compromise, but I'm open
to change this behavior in subsequent versions of ABS.

This PR also fixes a couple additional issues:

* `+2` is now a valid number, earlier we only considered `-` as the only
valid prefix for numbers
* there could be instances where an infinite loop would be triggered
because of ABS parsing `x +y` as `x; +y;` where the return value is just
`+y`. A for loop written like this would have triggered the bug: `decr = f(x) {x -1}; for x = 0; x > -10; x = decr(x)`. It would only happen on for loops where our looper is decrementing
  • Loading branch information
odino committed Mar 30, 2021
1 parent e8575e8 commit 97d8d0c
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 14 deletions.
10 changes: 10 additions & 0 deletions evaluator/evaluator.go
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,8 @@ func evalPrefixExpression(tok token.Token, operator string, right object.Object)
return evalBangOperatorExpression(right)
case "-":
return evalMinusPrefixOperatorExpression(tok, right)
case "+":
return evalPlusPrefixOperatorExpression(tok, right)
case "~":
return evalTildePrefixOperatorExpression(tok, right)
default:
Expand Down Expand Up @@ -601,6 +603,14 @@ func evalMinusPrefixOperatorExpression(tok token.Token, right object.Object) obj
return &object.Number{Value: -value}
}

func evalPlusPrefixOperatorExpression(tok token.Token, right object.Object) object.Object {
if right.Type() != object.NUMBER_OBJ {
return newError(tok, "unknown operator: +%s", right.Type())
}

return right
}

func evalNumberInfixExpression(
tok token.Token, operator string,
left, right object.Object,
Expand Down
21 changes: 20 additions & 1 deletion evaluator/evaluator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,24 @@ func TestEvalNumberExpression(t *testing.T) {
{"a = 5; a **= 2; a", 25},
{"a = 5; a %= 3; a", 2},
{"a = 0; a += 1 + 1; a", 2},
{"2", 2},
{"-2", -2},
{"+2", 2},
{"+-2", -2},
{"+-+-+-2", -2},
{"2 + 1", 3},
{"2 +1", 3},
{"2 - 1", 1},
{"2 -1", 1},
{"+2", 2},
{"+2 +2", 4},
{" +2 + 2", 4},
{"+2 + 2", 4},
{"+ 2 + 2", 4},
{"-2 + 2", 0},
{"- 2 + 2", 0},
{"-2 +-2", -4},
{"x = 1; x+1", 2},
}

for _, tt := range tests {
Expand Down Expand Up @@ -296,7 +314,8 @@ func TestForExpressions(t *testing.T) {
{`x = 0; for k = 0; k < 11; k = k + 1 { if k < 10 { continue; }; x += k }; x`, 10},
{"a = 0; for x = 0; x < 10; x = x + 1 { a = a + 1}; a", 10},
{"a = 0; for x = 0; x < y; x = x + 1 { a = a + 1}; a", "identifier not found: y"},
{"a = 0; increment = f(x) {x+1}; for x = 0; x < 10; x = increment(x) { a = a + 1}; a", 10},
{"a = 0; increment = f(x) { x+1 }; for x = 0; x < 10; x = increment(x) { a = a + 1}; a", 10},
{"a = 0; decrement = f(x) { x-1 }; for x = 0; x > -10; x = decrement(x) { a = a + 1}; a", 10},
{`a = 0; for k = 0; k < 10; k = k + 1 { a = a + 1}; k`, "identifier not found: k"},
{`k = 100; for k = 0; k < 10; k = k { k = k + 1}; k`, 100},
{`k = 100; for k = y; k < 10; k = k { k = 9 }; k`, "identifier not found: y"},
Expand Down
7 changes: 0 additions & 7 deletions lexer/lexer.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,6 @@ func (l *Lexer) NextToken() token.Token {
tok.Position = l.position
tok.Literal = "-="
l.readChar()
} else if isDigit(l.peekChar()) {
tok.Position = l.position
l.readChar()
literal, kind := l.readNumber()
tok.Type = kind
tok.Literal = "-" + literal
return tok
} else {
tok = l.newToken(token.MINUS)
}
Expand Down
3 changes: 2 additions & 1 deletion lexer/lexer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ f hello(x, y) {
{token.ASSIGN, "="},
{token.NUMBER, "10"},
{token.SEMICOLON, ";"},
{token.NUMBER, "-10"},
{token.MINUS, "-"},
{token.NUMBER, "10"},
{token.SEMICOLON, ";"},
{token.MINUS, "-"},
{token.NUMBER, "10"},
Expand Down
20 changes: 16 additions & 4 deletions parser/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const (
INDEX // array[index]
QUESTION // some?.function() or some?.property
DOT // some.function() or some.property
HIGHEST // special preference for -x or +y
)

var precedences = map[token.TokenType]int{
Expand Down Expand Up @@ -97,6 +98,7 @@ func New(l *lexer.Lexer) *Parser {
p.registerPrefix(token.STRING, p.ParseStringLiteral)
p.registerPrefix(token.NULL, p.ParseNullLiteral)
p.registerPrefix(token.BANG, p.parsePrefixExpression)
p.registerPrefix(token.PLUS, p.parsePrefixExpression)
p.registerPrefix(token.MINUS, p.parsePrefixExpression)
p.registerPrefix(token.TILDE, p.parsePrefixExpression)
p.registerPrefix(token.TRUE, p.ParseBoolean)
Expand All @@ -115,10 +117,10 @@ func New(l *lexer.Lexer) *Parser {
p.registerPrefix(token.AT, p.parseDecorator)

p.infixParseFns = make(map[token.TokenType]infixParseFn)
p.registerInfix(token.QUESTION, p.parseQuestionExpression)
p.registerInfix(token.DOT, p.parseDottedExpression)
p.registerInfix(token.PLUS, p.parseInfixExpression)
p.registerInfix(token.MINUS, p.parseInfixExpression)
p.registerInfix(token.QUESTION, p.parseQuestionExpression)
p.registerInfix(token.DOT, p.parseDottedExpression)
p.registerInfix(token.SLASH, p.parseInfixExpression)
p.registerInfix(token.EXPONENT, p.parseInfixExpression)
p.registerInfix(token.MODULO, p.parseInfixExpression)
Expand Down Expand Up @@ -380,6 +382,7 @@ func (p *Parser) parseExpressionStatement() *ast.ExpressionStatement {

func (p *Parser) parseExpression(precedence int) ast.Expression {
prefix := p.prefixParseFns[p.curToken.Type]

if prefix == nil {
p.noPrefixParseFnError(p.curToken)
return nil
Expand Down Expand Up @@ -465,9 +468,18 @@ func (p *Parser) parsePrefixExpression() ast.Expression {
Token: p.curToken,
Operator: p.curToken.Literal,
}

precedence := PREFIX

// When +- are used as prefixes, we want them to have
// the highest priority, so that -5.clamp(4, 5) is read
// as (-5).clamp(4, 5) = 4 instead of
// -(5.clamp(4,5)) = -5
if p.curTokenIs(token.PLUS) || p.curTokenIs(token.MINUS) {
precedence = HIGHEST
}
p.nextToken()
expression.Right = p.parseExpression(PREFIX)

expression.Right = p.parseExpression(precedence)

return expression
}
Expand Down
2 changes: 1 addition & 1 deletion parser/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ func TestOperatorPrecedenceParsing(t *testing.T) {
},
{
"3 + 4; -5 * 5",
"(3 + 4)(-5 * 5)",
"(3 + 4)((-5) * 5)",
},
{
"3 + 4; - 5 * 5",
Expand Down

0 comments on commit 97d8d0c

Please sign in to comment.