Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR upgrades the BSL parser and adds supportconf, and applies widespread null-safety and parser-node access changes (adding Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ParseErrorDiagnostic.java (1)
91-98:⚠️ Potential issue | 🟡 Minor
@Nullablereturn may produce"null"text in diagnostic messages.
StringJoiner.add(null)appends the literal string"null". If bothgetLiteralNameandgetSymbolicNamereturn null for a token type, the error message will contain"null"as an expected token name. Consider filtering or providing a fallback.💡 Suggested: filter nulls in the caller
expectedTokens.getIntervals().stream() .flatMapToInt(interval -> IntStream.rangeClosed(interval.a, interval.b)) .mapToObj(ParseErrorDiagnostic::getTokenName) + .filter(Objects::nonNull) .forEachOrdered(sj::add);
🤖 Fix all issues with AI agents
In `@build.gradle.kts`:
- Line 85: The build declares a pre-release dependency
api("io.github.1c-syntax", "bsl-parser", "0.31.0-rc.1"); replace the RC with the
latest stable release by changing the version to "0.30.0" in that api(...)
declaration unless you explicitly require RC features—if RC is required, add a
clear TODO comment next to the api(...) line noting it's intentional for the
develop branch and must be upgraded to a stable version before any
production/release build.
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UsingFindElementByStringDiagnostic.java`:
- Around line 61-70: The code in UsingFindElementByStringDiagnostic accesses the
first parameter with callParamList.callParam().get(0) without checking for an
empty list which can throw IndexOutOfBoundsException; modify the lambda to guard
against an empty callParam() (e.g., check !callParamList.callParam().isEmpty()
or use callParamList.callParam().stream().findFirst()) before accessing the
first element, then proceed to inspect the param and call
diagnosticStorage.addDiagnostic(ctx, info.getMessage(matcher.group(0))) only
when a param is present.
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/documenthighlight/LoopStatementDocumentHighlightSupplier.java`:
- Around line 231-234: The method addBreakAndContinueFromTryStatement can NPE if
tryStatement.tryCodeBlock() or tryStatement.exceptCodeBlock() is null; add null
guards before calling .codeBlock(): check tryStatement.tryCodeBlock() != null
and call addBreakAndContinueHighlights(highlights,
tryStatement.tryCodeBlock().codeBlock()) only if present, and likewise check
tryStatement.exceptCodeBlock() != null before calling .codeBlock() and
addBreakAndContinueHighlights for the except branch; keep using the existing
addBreakAndContinueHighlights method and ensure both null-checked branches are
executed independently.
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/documenthighlight/SDBLJoinDocumentHighlightSupplier.java`:
- Around line 117-118: The code handles JOIN variants incorrectly causing an
NPE: in the branch checking joinCtx.innerJoin() != null it calls
joinCtx.fullJoin().keyword which can be null; change the call to use
joinCtx.innerJoin().keyword instead so addTokenHighlight(highlights, ...)
references the innerJoin token; update the conditional branch that contains
addTokenHighlight to use innerJoin() rather than fullJoin() and run tests for
addTokenHighlight and SDBLJoinDocumentHighlightSupplier to confirm no
regressions.
🧹 Nitpick comments (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Ranges.java (1)
107-109: Consider adding@Nullableto the two-argParserRuleContextoverload as well.With the parser upgrade introducing more potential nulls, callers of
create(startCtx, endCtx)could also receive null contexts. Unlike the single-arg variant (line 93), this overload will NPE if either argument is null. If null inputs are plausible here, the same defensive pattern should be applied.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UnsafeSafeModeMethodCallDiagnostic.java (1)
110-120: Unsafe cast ofgetChildren()elements toParserRuleContext.
getChildren()returnsList<ParseTree>, which can includeTerminalNodeinstances (e.g., operator tokens between members in an expression). Casting the prev/next sibling directly toParserRuleContexton Lines 112 and 120 risks aClassCastExceptionif the sibling is a terminal node.This appears to be a pre-existing risk carried over from the old
childrenfield access, but worth noting since you're already touching these lines.Safer alternative
- var prev = (ParserRuleContext) rootExpressionNode.getChildren().get(indexOfCurrentMemberNode - 1); + var prevTree = rootExpressionNode.getChildren().get(indexOfCurrentMemberNode - 1); + if (!(prevTree instanceof ParserRuleContext prev)) { + return false; + }Apply the same pattern on Line 120 for
next.src/main/java/com/github/_1c_syntax/bsl/languageserver/documenthighlight/SDBLJoinDocumentHighlightSupplier.java (1)
111-116: Nit: missing space afterif/else ifkeywords.Minor style inconsistency:
if(should beif (per Java conventions.🪧 Style fix
- if(joinCtx.leftJoin() != null) { + if (joinCtx.leftJoin() != null) { addTokenHighlight(highlights, joinCtx.leftJoin().keyword); - } else if(joinCtx.rightJoin() != null) { + } else if (joinCtx.rightJoin() != null) { addTokenHighlight(highlights, joinCtx.rightJoin().keyword); - } else if(joinCtx.fullJoin() != null) { + } else if (joinCtx.fullJoin() != null) { addTokenHighlight(highlights, joinCtx.fullJoin().keyword); - } else if(joinCtx.innerJoin() != null) { + } else if (joinCtx.innerJoin() != null) {src/test/java/com/github/_1c_syntax/bsl/languageserver/documenthighlight/AllKnownTokenTest.java (2)
39-40:@SpringBootTestis unnecessary here — no Spring beans are injected.This test only uses static methods (
SdblTokenTypes.getTokenTypeAndModifiers,SDBLLexer.VOCABULARY) and doesn't require the Spring application context. Loading the full context slows test execution for no benefit. Also, the class ispublicwhile neighboring test classes (e.g.,SDBLJoinDocumentHighlightSupplierTest) use package-private visibility.♻️ Suggested fix
-@SpringBootTest -public class AllKnownTokenTest { +class AllKnownTokenTest {
84-92:bslTokens()is a no-op placeholder test that always passes.The test creates an empty list and asserts it's empty — it can never fail. The TODO on Line 89 indicates this is intentional work-in-progress. Consider either implementing it or annotating with
@Disabled("TODO: implement BSL token coverage check")so it's visible as pending work rather than silently passing.♻️ Suggested change
+ `@org.junit.jupiter.api.Disabled`("TODO: implement analog for BSL lexer") `@Test` void bslTokens() {Would you like me to help implement the BSL token coverage check following the same pattern as
sdblTokens()?
...com/github/_1c_syntax/bsl/languageserver/diagnostics/UsingFindElementByStringDiagnostic.java
Show resolved
Hide resolved
.../_1c_syntax/bsl/languageserver/documenthighlight/LoopStatementDocumentHighlightSupplier.java
Outdated
Show resolved
Hide resolved
...ithub/_1c_syntax/bsl/languageserver/documenthighlight/SDBLJoinDocumentHighlightSupplier.java
Outdated
Show resolved
Hide resolved
Test Results 2 916 files 2 916 suites 1h 7m 50s ⏱️ Results for commit 5a11395. ♻️ This comment has been updated with latest results. |
.../com/github/_1c_syntax/bsl/languageserver/aop/sentry/PermissionFilterBeforeSendCallback.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private void addDiagnostic(BinaryOperationNode node) { | ||
| var startToken = Trees.getTokens(node.getParent().getRepresentingAst()) |
There was a problem hiding this comment.
@theshadowco тут node - это кусок cfg, а не parse tree. Оно точно @nullable?
There was a problem hiding this comment.
ну parent может быть null... это например рут
|
|
||
| @Override | ||
| public ParseTree visitIfStatement(BSLParser.IfStatementContext ctx) { | ||
| public @Nullable ParseTree visitIfStatement(BSLParser.IfStatementContext ctx) { |
There was a problem hiding this comment.
@theshadowco почему visit-методы стали @nullable для возвращаемого значения? Они же вызываются только, когда ctx не равен null, значит, должны вызвать либо super, либо вернуть себя же, то есть не-null.
There was a problem hiding this comment.
забываешь про рут.
Там схема вызова построена как раз на наличии null в результате - если он есть, то значит дальше нет никто.
There was a problem hiding this comment.
ну и да, прервать дальнейшее движение по дереву можно именно с null (кейсов не придумаю, но так есть)
There was a problem hiding this comment.
попробую поковырять antlr
theshadowco
force-pushed
the
feature/updParser260212
branch
from
58b9de5 to
5a11395
Compare
|
2 participants
Описание
Доработки, связанные с обновлением bsl parser
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests