[WIP] Приоритет операторов вместо рекурсивного спуска и другие оптимизации #1651
+180
−232
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughRefactors expression parsing to a precedence-climbing algorithm, adds LanguageDef accessors for binary/unary priorities, introduces direct AST constructors for unary and binary operations, and streamlines various parser node creation paths (terminals, calls, variable/section handling). Changes
Sequence Diagram(s)sequenceDiagram
participant TS as TokenStream
participant P as DefaultBslParser
participant LD as LanguageDef
participant AST as AST Nodes
TS->>P: start BuildExpression(prio)
P->>LD: GetUnaryPriority(currentToken)
alt token is unary
P->>P: BuildPrimaryExpression() -> parse operand
P->>AST: create UnaryOperationNode(operand, op)
AST-->>P: unary node
else primary (literal / identifier / paren)
P->>AST: create TerminalNode or parse parenthesized expr
AST-->>P: terminal node
end
loop while peek token is binary and LD.GetBinaryPriority(peek) > currentPrio
P->>LD: GetBinaryPriority(peekToken)
LD-->>P: priority
P->>P: BuildExpression(newPrio) -> parse RHS
P->>AST: create BinaryOperationNode(left, right, op)
AST-->>P: binary node
end
P-->>TS: return constructed node
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs`:
- Around line 1347-1357: BuildParenthesis currently calls NextLexem()
unconditionally after AddError, which can double-advance because AddError may
fast-forward; change the logic to only call NextLexem() when the captured token
is actually Token.ClosePar (i.e., check _lastExtractedLexem.Token ==
Token.ClosePar before advancing), and keep the rest intact so you still call
BuildDereference(expr) afterwards; update BuildParenthesis to rely on the token
check rather than always NextLexem() to avoid skipping the recovery token.
Comment on lines
1347
to
+1357
| private BslSyntaxNode BuildParenthesis() | ||
| { | ||
| if (_lastExtractedLexem.Token == Token.OpenPar) | ||
| { | ||
| NextLexem(); | ||
| var expr = BuildOrExpression(); | ||
| if (_lastExtractedLexem.Token != Token.ClosePar) | ||
| { | ||
| AddError(LocalizedErrors.TokenExpected(Token.ClosePar)); | ||
| } | ||
| NextLexem(); | ||
|
|
||
| return BuildDereference(expr); | ||
| } | ||
|
|
||
| return TerminalNode(); | ||
| { | ||
| NextLexem(); | ||
| var expr = BuildExpression(0); | ||
| if (_lastExtractedLexem.Token != Token.ClosePar) | ||
| { | ||
| AddError(LocalizedErrors.TokenExpected(Token.ClosePar)); | ||
| } | ||
| NextLexem(); | ||
|
|
||
| return BuildDereference(expr); |
Contributor
There was a problem hiding this comment.
Avoid skipping a token after missing ) recovery.
AddError(...) fast‑forwards by default; the unconditional NextLexem() risks skipping the recovery token. Consider advancing only when ) is actually present.
🐛 Proposed fix to avoid double-advancing on recovery
if (_lastExtractedLexem.Token != Token.ClosePar)
{
AddError(LocalizedErrors.TokenExpected(Token.ClosePar));
+ return BuildDereference(expr);
}
- NextLexem();
-
- return BuildDereference(expr);
+ NextLexem();
+ return BuildDereference(expr);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private BslSyntaxNode BuildParenthesis() | |
| { | |
| if (_lastExtractedLexem.Token == Token.OpenPar) | |
| { | |
| NextLexem(); | |
| var expr = BuildOrExpression(); | |
| if (_lastExtractedLexem.Token != Token.ClosePar) | |
| { | |
| AddError(LocalizedErrors.TokenExpected(Token.ClosePar)); | |
| } | |
| NextLexem(); | |
| return BuildDereference(expr); | |
| } | |
| return TerminalNode(); | |
| { | |
| NextLexem(); | |
| var expr = BuildExpression(0); | |
| if (_lastExtractedLexem.Token != Token.ClosePar) | |
| { | |
| AddError(LocalizedErrors.TokenExpected(Token.ClosePar)); | |
| } | |
| NextLexem(); | |
| return BuildDereference(expr); | |
| private BslSyntaxNode BuildParenthesis() | |
| { | |
| NextLexem(); | |
| var expr = BuildExpression(0); | |
| if (_lastExtractedLexem.Token != Token.ClosePar) | |
| { | |
| AddError(LocalizedErrors.TokenExpected(Token.ClosePar)); | |
| return BuildDereference(expr); | |
| } | |
| NextLexem(); | |
| return BuildDereference(expr); | |
| } |
🤖 Prompt for AI Agents
In `@src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs` around lines 1347
- 1357, BuildParenthesis currently calls NextLexem() unconditionally after
AddError, which can double-advance because AddError may fast-forward; change the
logic to only call NextLexem() when the captured token is actually
Token.ClosePar (i.e., check _lastExtractedLexem.Token == Token.ClosePar before
advancing), and keep the rest intact so you still call BuildDereference(expr)
afterwards; update BuildParenthesis to rely on the token check rather than
always NextLexem() to avoid skipping the recovery token.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/OneScript.Language/SyntaxAnalysis/AstNodes/BinaryOperationNode.cs`:
- Around line 21-27: The BinaryOperationNode constructor currently calls
AddChild(firstArg) and AddChild(secondArg) without null checks, which can add
null children when BuildExpression returns default; mirror the defensive pattern
used in UnaryOperationNode by only calling AddChild for firstArg and secondArg
when they are not null (and ensure Operation assignment handles a possibly null
operation safely if needed), so update the BinaryOperationNode constructor to
guard AddChild calls against null children.
In `@src/OneScript.Language/SyntaxAnalysis/AstNodes/UnaryOperationNode.cs`:
- Around line 21-25: The UnaryOperationNode constructor can receive a null arg
which then gets passed to AddChild causing downstream failures; update the
UnaryOperationNode(BslSyntaxNode arg, Lexem operation) constructor to
defensively handle a null arg (either by validating and throwing a clear
ArgumentNullException mentioning UnaryOperationNode/arg, or by skipping AddChild
when arg is null and setting a safe placeholder), and ensure Operation is still
set from operation.Token; reference the UnaryOperationNode constructor, the arg
parameter, the Operation property, and the AddChild method when making the
change.
In `@src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs`:
- Around line 1246-1253: The loop in DefaultBslParser where you call
BuildExpression and then create a BinaryOperationNode can construct a node with
a null right-hand operand; modify the loop in the method using
_lastExtractedLexem, NextLexem() and BuildExpression(...) so that after
obtaining secondArg you check for null/default and break or handle the syntax
error (e.g., return firstArg or produce an error node) before instantiating new
BinaryOperationNode(firstArg, secondArg, operationLexem); ensure
LanguageDef.GetBinaryPriority(...) logic stays the same but do not construct
BinaryOperationNode when secondArg is null.
- Around line 1287-1295: The code constructs a UnaryOperationNode with a
potentially null argument because BuildExpression(prio) can return default;
modify the UnaryOperationNode creation path in the method containing
LanguageDef.GetUnaryPriority(_lastExtractedLexem.Token) so that after calling
var arg = BuildExpression(prio) you check if arg is null/default, call
AddError(LocalizedErrors.ExpressionSyntax()) (or propagate the existing error)
and return default instead of new UnaryOperationNode(arg, operation); this
prevents passing a null arg into the UnaryOperationNode constructor and
preserves existing error handling semantics.
♻️ Duplicate comments (1)
src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs (1)
1335-1346: Double-advance risk on missing)recovery.The unconditional
NextLexem()on line 1343 can skip past the recovery token whenAddError(...)has already fast-forwarded. This was flagged in a previous review.
Comment on lines
+21
to
+27
| public BinaryOperationNode(BslSyntaxNode firstArg, BslSyntaxNode secondArg, Lexem operation) | ||
| : base(NodeKind.BinaryOperation, operation) | ||
| { | ||
| Operation = operation.Token; | ||
| AddChild(firstArg); | ||
| AddChild(secondArg); | ||
| } |
Contributor
There was a problem hiding this comment.
Same null child concern applies here.
Both firstArg and secondArg can be null when expression parsing fails. The BuildExpression method returns default on syntax errors (line 1290 in DefaultBslParser.cs), which would result in null children being added to the node.
Consider defensive null checks consistent with UnaryOperationNode.
🛡️ Proposed defensive check
public BinaryOperationNode(BslSyntaxNode firstArg, BslSyntaxNode secondArg, Lexem operation)
: base(NodeKind.BinaryOperation, operation)
{
Operation = operation.Token;
- AddChild(firstArg);
- AddChild(secondArg);
+ if (firstArg != null)
+ AddChild(firstArg);
+ if (secondArg != null)
+ AddChild(secondArg);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public BinaryOperationNode(BslSyntaxNode firstArg, BslSyntaxNode secondArg, Lexem operation) | |
| : base(NodeKind.BinaryOperation, operation) | |
| { | |
| Operation = operation.Token; | |
| AddChild(firstArg); | |
| AddChild(secondArg); | |
| } | |
| public BinaryOperationNode(BslSyntaxNode firstArg, BslSyntaxNode secondArg, Lexem operation) | |
| : base(NodeKind.BinaryOperation, operation) | |
| { | |
| Operation = operation.Token; | |
| if (firstArg != null) | |
| AddChild(firstArg); | |
| if (secondArg != null) | |
| AddChild(secondArg); | |
| } |
🤖 Prompt for AI Agents
In `@src/OneScript.Language/SyntaxAnalysis/AstNodes/BinaryOperationNode.cs` around
lines 21 - 27, The BinaryOperationNode constructor currently calls
AddChild(firstArg) and AddChild(secondArg) without null checks, which can add
null children when BuildExpression returns default; mirror the defensive pattern
used in UnaryOperationNode by only calling AddChild for firstArg and secondArg
when they are not null (and ensure Operation assignment handles a possibly null
operation safely if needed), so update the BinaryOperationNode constructor to
guard AddChild calls against null children.
Comment on lines
+21
to
+25
| public UnaryOperationNode(BslSyntaxNode arg, Lexem operation) : base(NodeKind.UnaryOperation, operation) | ||
| { | ||
| Operation = operation.Token; | ||
| AddChild(arg); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential null child propagation.
The arg parameter could be null when the caller's expression parsing fails (e.g., BuildExpression returns default on line 1290 of DefaultBslParser.cs). Adding a null child may cause issues downstream during AST traversal or code generation.
Consider adding a null guard or ensuring callers handle the error case before constructing the node.
🛡️ Proposed defensive check
public UnaryOperationNode(BslSyntaxNode arg, Lexem operation) : base(NodeKind.UnaryOperation, operation)
{
Operation = operation.Token;
- AddChild(arg);
+ if (arg != null)
+ AddChild(arg);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public UnaryOperationNode(BslSyntaxNode arg, Lexem operation) : base(NodeKind.UnaryOperation, operation) | |
| { | |
| Operation = operation.Token; | |
| AddChild(arg); | |
| } | |
| public UnaryOperationNode(BslSyntaxNode arg, Lexem operation) : base(NodeKind.UnaryOperation, operation) | |
| { | |
| Operation = operation.Token; | |
| if (arg != null) | |
| AddChild(arg); | |
| } |
🤖 Prompt for AI Agents
In `@src/OneScript.Language/SyntaxAnalysis/AstNodes/UnaryOperationNode.cs` around
lines 21 - 25, The UnaryOperationNode constructor can receive a null arg which
then gets passed to AddChild causing downstream failures; update the
UnaryOperationNode(BslSyntaxNode arg, Lexem operation) constructor to
defensively handle a null arg (either by validating and throwing a clear
ArgumentNullException mentioning UnaryOperationNode/arg, or by skipping AddChild
when arg is null and setting a safe placeholder), and ensure Operation is still
set from operation.Token; reference the UnaryOperationNode constructor, the arg
parameter, the Operation property, and the AddChild method when making the
change.
Comment on lines
1246
to
1253
| while (LanguageDef.GetBinaryPriority(_lastExtractedLexem.Token) > prio) | ||
| { | ||
| var operationLexem = _lastExtractedLexem; | ||
| NextLexem(); | ||
| var secondArg = BuildNotExpression(); | ||
| firstArg = MakeBinaryOperationNode(firstArg, secondArg, operationLexem); | ||
| } | ||
|
|
||
| return firstArg; | ||
| } | ||
|
|
||
| private BslSyntaxNode BuildNotExpression() | ||
| { | ||
| if (_lastExtractedLexem.Token == Token.Not) | ||
| { | ||
| var operation = _lastExtractedLexem; | ||
| NextLexem(); | ||
| var op = new UnaryOperationNode(operation); | ||
| var argument = BuildLogicalComparison(); | ||
| op.AddChild(argument); | ||
| return op; | ||
| } | ||
| var secondArg = BuildExpression(LanguageDef.GetBinaryPriority(operationLexem.Token)); | ||
|
|
||
| return BuildLogicalComparison(); | ||
| } | ||
|
|
||
| private BslSyntaxNode BuildLogicalComparison() | ||
| { | ||
| var firstArg = BuildAdditionExpression(); | ||
| while (_lastExtractedLexem.Token == Token.Equal || | ||
| _lastExtractedLexem.Token == Token.MoreThan || | ||
| _lastExtractedLexem.Token == Token.LessThan || | ||
| _lastExtractedLexem.Token == Token.MoreOrEqual || | ||
| _lastExtractedLexem.Token == Token.LessOrEqual || | ||
| _lastExtractedLexem.Token == Token.NotEqual) | ||
| { | ||
| var operationLexem = _lastExtractedLexem; | ||
| NextLexem(); | ||
| var secondArg = BuildAdditionExpression(); | ||
| firstArg = MakeBinaryOperationNode(firstArg, secondArg, operationLexem); | ||
| firstArg = new BinaryOperationNode(firstArg, secondArg, operationLexem); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against null secondArg before constructing BinaryOperationNode.
If BuildExpression returns default on a syntax error, the loop continues constructing a BinaryOperationNode with a null operand, which can cause downstream failures.
🛡️ Proposed fix
var operationLexem = _lastExtractedLexem;
NextLexem();
var secondArg = BuildExpression(LanguageDef.GetBinaryPriority(operationLexem.Token));
+ if (secondArg == default)
+ return firstArg; // or return default;
firstArg = new BinaryOperationNode(firstArg, secondArg, operationLexem);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (LanguageDef.GetBinaryPriority(_lastExtractedLexem.Token) > prio) | |
| { | |
| var operationLexem = _lastExtractedLexem; | |
| NextLexem(); | |
| var secondArg = BuildNotExpression(); | |
| firstArg = MakeBinaryOperationNode(firstArg, secondArg, operationLexem); | |
| } | |
| return firstArg; | |
| } | |
| private BslSyntaxNode BuildNotExpression() | |
| { | |
| if (_lastExtractedLexem.Token == Token.Not) | |
| { | |
| var operation = _lastExtractedLexem; | |
| NextLexem(); | |
| var op = new UnaryOperationNode(operation); | |
| var argument = BuildLogicalComparison(); | |
| op.AddChild(argument); | |
| return op; | |
| } | |
| var secondArg = BuildExpression(LanguageDef.GetBinaryPriority(operationLexem.Token)); | |
| return BuildLogicalComparison(); | |
| } | |
| private BslSyntaxNode BuildLogicalComparison() | |
| { | |
| var firstArg = BuildAdditionExpression(); | |
| while (_lastExtractedLexem.Token == Token.Equal || | |
| _lastExtractedLexem.Token == Token.MoreThan || | |
| _lastExtractedLexem.Token == Token.LessThan || | |
| _lastExtractedLexem.Token == Token.MoreOrEqual || | |
| _lastExtractedLexem.Token == Token.LessOrEqual || | |
| _lastExtractedLexem.Token == Token.NotEqual) | |
| { | |
| var operationLexem = _lastExtractedLexem; | |
| NextLexem(); | |
| var secondArg = BuildAdditionExpression(); | |
| firstArg = MakeBinaryOperationNode(firstArg, secondArg, operationLexem); | |
| firstArg = new BinaryOperationNode(firstArg, secondArg, operationLexem); | |
| } | |
| while (LanguageDef.GetBinaryPriority(_lastExtractedLexem.Token) > prio) | |
| { | |
| var operationLexem = _lastExtractedLexem; | |
| NextLexem(); | |
| var secondArg = BuildExpression(LanguageDef.GetBinaryPriority(operationLexem.Token)); | |
| if (secondArg == default) | |
| return firstArg; // or return default; | |
| firstArg = new BinaryOperationNode(firstArg, secondArg, operationLexem); | |
| } |
🤖 Prompt for AI Agents
In `@src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs` around lines 1246
- 1253, The loop in DefaultBslParser where you call BuildExpression and then
create a BinaryOperationNode can construct a node with a null right-hand
operand; modify the loop in the method using _lastExtractedLexem, NextLexem()
and BuildExpression(...) so that after obtaining secondArg you check for
null/default and break or handle the syntax error (e.g., return firstArg or
produce an error node) before instantiating new BinaryOperationNode(firstArg,
secondArg, operationLexem); ensure LanguageDef.GetBinaryPriority(...) logic
stays the same but do not construct BinaryOperationNode when secondArg is null.
Comment on lines
+1287
to
+1295
| if (LanguageDef.GetUnaryPriority(_lastExtractedLexem.Token) <= prio) | ||
| { | ||
| AddError(LocalizedErrors.ExpressionSyntax()); | ||
| return default; | ||
| } | ||
|
|
||
| var arg = BuildExpression(prio); | ||
| return new UnaryOperationNode(arg, operation); | ||
| } |
Contributor
There was a problem hiding this comment.
Null result from BuildExpression propagates into UnaryOperationNode.
When line 1293's BuildExpression(prio) fails and returns default, the code still constructs UnaryOperationNode(arg, operation) with a null arg. Consider guarding against this:
🛡️ Proposed fix
var arg = BuildExpression(prio);
+ if (arg == default)
+ return default;
return new UnaryOperationNode(arg, operation);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (LanguageDef.GetUnaryPriority(_lastExtractedLexem.Token) <= prio) | |
| { | |
| AddError(LocalizedErrors.ExpressionSyntax()); | |
| return default; | |
| } | |
| var arg = BuildExpression(prio); | |
| return new UnaryOperationNode(arg, operation); | |
| } | |
| if (LanguageDef.GetUnaryPriority(_lastExtractedLexem.Token) <= prio) | |
| { | |
| AddError(LocalizedErrors.ExpressionSyntax()); | |
| return default; | |
| } | |
| var arg = BuildExpression(prio); | |
| if (arg == default) | |
| return default; | |
| return new UnaryOperationNode(arg, operation); |
🤖 Prompt for AI Agents
In `@src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs` around lines 1287
- 1295, The code constructs a UnaryOperationNode with a potentially null
argument because BuildExpression(prio) can return default; modify the
UnaryOperationNode creation path in the method containing
LanguageDef.GetUnaryPriority(_lastExtractedLexem.Token) so that after calling
var arg = BuildExpression(prio) you check if arg is null/default, call
AddError(LocalizedErrors.ExpressionSyntax()) (or propagate the existing error)
and return default instead of new UnaryOperationNode(arg, operation); this
prevents passing a null arg into the UnaryOperationNode constructor and
preserves existing error handling semantics.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Bug Fixes
Performance
Compatibility
✏️ Tip: You can customize this high-level summary in your review settings.