Приоритет операторов вместо рекурсивного спуска, другие исправления и оптимизации #1651
+377
−416
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 approach, exposes unary/binary operator precedences via LanguageDef helpers, adds direct AST constructors for unary/binary operations, replaces several ad-hoc empty-array allocations with Changes
Sequence Diagram(s)sequenceDiagram
participant TS as TokenStream
participant P as DefaultBslParser
participant LD as LanguageDef
participant AST as ASTBuilder
TS->>P: parse expression request
P->>TS: peek currentToken
P->>LD: GetUnaryPriority(currentToken)
alt token is unary
P->>P: BuildExpression(unaryPriority) -> parse operand
P->>AST: create UnaryOperationNode(operand, op)
else primary
P->>P: BuildPrimaryExpression()
P->>AST: create TerminalNode or parenthesized expression
end
loop while peek is binary and LD.GetBinaryPriority(peek) > currentPrio
P->>LD: GetBinaryPriority(peekToken)
P->>P: BuildExpression(newPrio) -> parse RHS
P->>AST: create BinaryOperationNode(left, right, op)
end
P-->>TS: return expression node
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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
🧪 Generate unit tests (beta)
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.
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.
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 1377-1386: The code calls AddError when
NextExpected(Token.OpenPar) fails but then continues executing NextLexem() and
TryParseNode, causing cascading parsing errors; update the logic in the block
containing NextExpected/ AddError so that after AddError(...) you immediately
return (or otherwise short-circuit) to avoid calling NextLexem(), TryParseNode,
and subsequent BuildExpressionUpTo calls when the open parenthesis is missing —
locate the sequence using NextExpected, AddError, NextLexem, TryParseNode,
BuildExpressionUpTo and Token.OpenPar and ensure control exits early on the
missing-token path.
♻️ Duplicate comments (3)
src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs (3)
1224-1237: Guard against nullsecondArgbefore constructingBinaryOperationNode.If
BuildExpressionreturnsdefaulton a syntax error, the loop continues constructing aBinaryOperationNodewith 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; firstArg = new BinaryOperationNode(firstArg, secondArg, operationLexem);
1239-1276: Null result fromBuildExpressionpropagates intoUnaryOperationNode.When
BuildExpression(prio)at line 1274 fails and returnsdefault, the code still constructsUnaryOperationNode(arg, operation)with a nullarg.🛡️ Proposed fix
var arg = BuildExpression(prio); + if (arg == default) + return default; return new UnaryOperationNode(arg, operation);
1316-1327: Avoid skipping a token after missing)recovery.
AddError(...)fast-forwards by default; the unconditionalNextLexem()at line 1324 risks skipping the recovery token when)is missing.🐛 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);
Collaborator
Author
|
Был баг в условном операторе. В качестве разделителей вместо запятых допускались и другие символы: Ф = ?(Истина? 1 ; 2);В v1 этого бага нет |
EvilBeaver
reviewed
Jan 26, 2026
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 1174-1207: The loop in BuildCallArguments continues after
AddError(...) which can create an extra empty CallArgument node; change the
error branch so that after calling
AddError(LocalizedErrors.TokenExpected(Token.ClosePar)) the parser stops parsing
arguments (break or return) instead of continuing — update BuildCallArguments to
break out of the while when that error is reported (optionally call NextLexem()
first to synchronize) so BuildOptionalCallArgument and argsList.AddNode(...) are
not invoked for a missing delimiter.
Mr-Rm
changed the title
Owner
|
@cursor review |
Collaborator
Author
|
Пока Cursor молчит Асинх Функция Асинх(Асинх)
Сообщить("Асинх=" + Асинх);
Асинх = Асинх + 1;
Если Асинх <= 1 Тогда
Возврат Асинх(Асинх + 1);
КонецЕсли;
Возврат Асинх;
КонецФункции
; // обязательно оператор до вызова
Асинх(0)И если функцию назвать |
Owner
Он что-то перестал ревью делать, ага
Асинх и его поведения вообще мало изучены и в 1скрипт сделано прям по наитию скорее, чем по принципу "как в 1С" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.