Support infix rules - #134
Conversation
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 6c2d06c | Previous: d803417 | Ratio |
|---|---|---|---|
BenchmarkWorkspaceCycle (typefox.dev/fastbelt/examples/statemachine) - MB/s |
22 MB/s |
5.21 MB/s |
4.22 |
BenchmarkParser (typefox.dev/fastbelt/examples/statemachine) - MB/s |
90.81 MB/s |
37.68 MB/s |
2.41 |
BenchmarkLexer (typefox.dev/fastbelt/examples/statemachine) - MB/s |
119.62 MB/s |
64.67 MB/s |
1.85 |
BenchmarkLexerAndParser (typefox.dev/fastbelt/examples/statemachine) - MB/s |
50.43 MB/s |
23.82 MB/s |
2.12 |
BenchmarkLocalLinking (typefox.dev/fastbelt/examples/statemachine) - MB/s |
25.57 MB/s |
16.69 MB/s |
1.53 |
This comment was automatically generated by workflow using github-action-benchmark.
ssmifi
left a comment
There was a problem hiding this comment.
Looks well-designed and -tested. There are one design and some minor questions left.
| iin.AppendLine("operators = append(operators, token)") | ||
| }) | ||
| in.AppendLine("}") | ||
| emitOperand(in, secondCall) |
There was a problem hiding this comment.
Question: Is it safe to emit the operand, even if the operator wasn't appended before?
There was a problem hiding this comment.
Emitting the operand is definitely safe. The question should be: Is it safe to append the operand if the operator didn't exist ;)
In that case, it's still safe, since we cannot enter the for loop that is emitted before that. The lookahead done there asserts that the next token is the expected operator token. And even then, error recovery should ensure that this works as expected.
fe3f042 to
b6735dc
Compare
6c2d06c to
efe4301
Compare
|
|
||
| // Mixed with tighter ("%") and looser ("*") levels. | ||
| expr = parseExpression(t, "1 * 2 ^ 3 % 4 ^ 5") | ||
| assert.Equal(t, "(1 * (2 ^ ((3 % 4) ^ 5)))", printExpression(expr)) |
There was a problem hiding this comment.
This line reads weird, until you mind the comment and the special grammar with % as strongest operator.
| // and "+" | "-" bind loosest, so "1 + 2 * 3" parses as "1 + (2 * 3)". | ||
| // | ||
| // Groups are left-associative by default; prefix a group with "right assoc" | ||
| // (or explicitly "left assoc") to control associativity: |
There was a problem hiding this comment.
We could also think of a "non assoc" for operators like less and greater. But I would prefer a extra validation I think
There was a problem hiding this comment.
How would an AST for a non-associative operation look like? It wouldn't be binary (i.e. just left vs right) anymore.
| // becomes the current object. The operator += is also valid for tree-rewriting | ||
| // actions on slice properties. | ||
| // | ||
| // # Infix Rules |
There was a problem hiding this comment.
We could also explain
- what the difference between associativity and precedence is
- how it is done without infix notation, and that infix is faster than manual
- that this is useful to reduce ambiguities in expressions
- and that reducing ambiguities, is increasing parsing performance
There was a problem hiding this comment.
Thanks, I've adjusted the documentation a bit.
what the difference between associativity and precedence is
They are unrelated. I think this is clearer with the adjusted documentation.
that this is useful to reduce ambiguities in expressions, and that reducing ambiguities, is increasing parsing performance.
This is not true? It's increasing performance by flattening the parse tree into one single loop, instead of a list of recursive parsing calls. Ambiguity isn't relevant here.
| return fmt.Errorf("could not resolve the operand rule of infix rule '%s'", rule.Name()) | ||
| } | ||
|
|
||
| tokenGroup, err := synthesizeOperatorGroup(g, rule, groupName, ctx) |
There was a problem hiding this comment.
What if an operator is not a terminal? Looking at the grammar, this should be an option.
There was a problem hiding this comment.
We have a validation that prevents this.
|
|
||
| // synthesizeInfixBody attaches the flat "operand (operator operand)*" body to | ||
| // the infix rule. | ||
| func synthesizeInfixBody(rule InfixRule, operand ParserRule, operatorGroup TokenGroup) { |
There was a problem hiding this comment.
I feel a bit lost here.
operand (operator operand)*only handles left association. What about right association? Or what do I read wrong?- Is this about creating a AST for the infix operators?
There was a problem hiding this comment.
Both associativity directions aren't handled in the parsing phase. They are only relevant for the AST. I.e. whether an expression like A + B + C is (A + B) + C or A + (B + C) is a pure AST mutation. We can apply this as a post-processing on the parsing. Therefore, we first parse all operators and operands into an array, and then apply a transformation that produces the final AST, based on the predetermined precedence.
| // Mirrors the arithmetics grammar: "%" > "^" > "*" | "/" > "+" | "-", | ||
| // plus a loosest right-associative "=" level. | ||
| precedence := map[int]InfixPrecedence{ | ||
| typeIds["%"]: {Level: 0}, |
There was a problem hiding this comment.
Just because it reads weird:
Instead of the modulo operator can we instead use the tetration operator?
Either "!!" or "↑↑" (decimal: 8593, hex: 2191, entity: &uarr).
There was a problem hiding this comment.
What is problematic about the modulo operator? We use the same in the arithmetics language in Langium.
| func TestInfixRuleOperatorGroupNameTaken(t *testing.T) { | ||
| f := test.New(t, CreateServices()) | ||
| doc := f.Parse(` | ||
| grammar Test; | ||
| ` + infixInterfaces + ` | ||
| token BinaryExpressionOperator: /@+/; | ||
| infix <|1:BinaryExpression|> on PrimaryExpression: "+" | ||
| ` + commonTokens) | ||
| doc.ExpectDiagnostic("1").WithSeverity(core.SeverityError).WithCode(ValidateInfixOperatorGroupName) | ||
| } |
There was a problem hiding this comment.
What is the problem here? Is BinaryExpressionOperator a reserved name when using infix?
There was a problem hiding this comment.
Yes, the <infix-rule-name>Operator is a token group that is automatically generated as an optimization (so lookahead just needs to do a BinaryExpressionOperator.Matches(p.LA(1)) to figure out whether to continue parsing the binary expression).
Closes #115 (as a side effect).
Adds infix rule support, similar to how Langium did it (but a bit simpler, since we don't need CST support).
Implements the support by rewriting infix rules into parser rules of the shape
Expr (op Expr)*.