Исправление замечаний к коду. Продолжение 25.12.12. Часть 2 #3659
Conversation
|
Warning Rate limit exceeded@nixel2007 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 24 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughAdds nullability annotations and runtime null-safety checks, refactors some helpers/constants and local typing, reorganizes semantic token emission into helper methods, and updates tests to use fluent AssertJ and adds new unit tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMetadataDiagnostic.java (1)
60-124: Replaceassertwith an always-on precondition (asserts may be disabled).
assert diagnosticRange != null;(Line 122) won’t protect prod runs without-ea, and can still lead to NPE indiagnosticStorage.addDiagnostic(...).@@ protected void addDiagnostic(String message) { - assert diagnosticRange != null; - diagnosticStorage.addDiagnostic(diagnosticRange, message); + var range = java.util.Objects.requireNonNull( + diagnosticRange, + "diagnosticRange must be computed before addDiagnostic()" + ); + diagnosticStorage.addDiagnostic(range, message); }src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java (2)
194-214: Fix likely NPE:new ErrorExpressionNode(expression)whenexpression == null.
Line 198-200 allowsexpressionto be null and then passes it intoErrorExpressionNode.@@ - if (token == BSLLexer.LPAREN) { - visitParenthesis(ctx.expression(), ctx.modifier()); + if (token == BSLLexer.LPAREN) { + visitParenthesis(ctx.expression(), ctx.modifier(), ctx); } else { operands.push(new ErrorExpressionNode(dispatchChild)); } @@ - private void visitParenthesis(BSLParser.@Nullable ExpressionContext expression, - List<? extends BSLParser.ModifierContext> modifiers) { + private void visitParenthesis(BSLParser.@Nullable ExpressionContext expression, + List<? extends BSLParser.ModifierContext> modifiers, + ParseTree representingAst) { @@ - if (expression == null || expression.getTokens().isEmpty()) { - operands.push(new ErrorExpressionNode(expression)); + if (expression == null || expression.getTokens().isEmpty()) { + operands.push(new ErrorExpressionNode(representingAst)); return; }
399-407: ReplaceassertandObjects.requireNonNull()with explicit null checks that push error recovery nodes.
makeSubexpression()returns@Nullable BslExpression, but the current code treats null returns unsafely:
assert expressionArg != null;(line 403) is a no-op without-eaflag and silently ignores parse errors in production.Objects.requireNonNull(...)(lines 423-425) crashes the parser with NPE on recoverable parse issues instead of graceful error handling.The codebase already establishes the recovery pattern elsewhere: check for null and push
new ErrorExpressionNode(ctx)to the operands stack. Apply this consistently to both methods:public ParseTree visitAccessIndex(BSLParser.AccessIndexContext ctx) { var target = operands.pop(); var expressionArg = makeSubexpression(ctx.expression()); - assert expressionArg != null; + if (expressionArg == null) { + operands.push(new ErrorExpressionNode(ctx)); + return ctx; + } var indexOperation = BinaryOperationNode.create(BslOperator.INDEX_ACCESS, target, expressionArg, ctx); operands.push(indexOperation); return ctx; } public ParseTree visitTernaryOperator(BSLParser.TernaryOperatorContext ctx) { - var ternary = TernaryOperatorNode.create( - Objects.requireNonNull(makeSubexpression(ctx.expression(0))), - Objects.requireNonNull(makeSubexpression(ctx.expression(1))), - Objects.requireNonNull(makeSubexpression(ctx.expression(2))) - ); - - ternary.setRepresentingAst(ctx); - operands.push(ternary); - - return ctx; + var e0 = makeSubexpression(ctx.expression(0)); + var e1 = makeSubexpression(ctx.expression(1)); + var e2 = makeSubexpression(ctx.expression(2)); + if (e0 == null || e1 == null || e2 == null) { + operands.push(new ErrorExpressionNode(ctx)); + return ctx; + } + var ternary = TernaryOperatorNode.create(e0, e1, e2); + ternary.setRepresentingAst(ctx); + operands.push(ternary); + return ctx; }
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java (1)
318-548: GuardcurrentScopewith fallback to module scope infindVariableSymbol().The
currentScopefield is@Nullableand uninitialized untilvisitSub()runs. Since module-level visitors likevisitModuleVarDeclaration()may execute beforevisitSub(),findVariableSymbol()can pass null togetVariableSymbol(variableName, currentScope). WhileSymbolTree.getVariableSymbol()handles null keys defensively viaMap.getOrDefault(), this is implicit and fragile. UseObjects.requireNonNullElse()to normalize null scope to module scope, making the intent explicit.@@ private Optional<VariableSymbol> findVariableSymbol(String variableName) { - var variableSymbol = documentContext.getSymbolTree() - .getVariableSymbol(variableName, currentScope); + var symbolTree = documentContext.getSymbolTree(); + var scope = java.util.Objects.requireNonNullElse(currentScope, symbolTree.getModule()); + var variableSymbol = symbolTree.getVariableSymbol(variableName, scope); if (variableSymbol.isPresent()) { return variableSymbol; } - return documentContext.getSymbolTree() - .getVariableSymbol(variableName, documentContext.getSymbolTree().getModule()); + return symbolTree.getVariableSymbol(variableName, symbolTree.getModule()); }src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/MultilingualStringAnalyser.java (1)
156-162: Consider simplifying by removing the else block.The null check correctly provides null safety. However, the
elseblock is unnecessary since theifblock returns early.Apply this diff to simplify the code:
private static String getMultilingualString(BSLParser.@Nullable GlobalMethodCallContext globalMethodCallContext) { if (globalMethodCallContext == null) { return ""; - } else { - return globalMethodCallContext.doCall().callParamList().callParam(0).getText(); } + return globalMethodCallContext.doCall().callParamList().callParam(0).getText(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMetadataDiagnostic.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/MultilingualStringAnalyser.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Trees.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMetadataDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Trees.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/utils/MultilingualStringAnalyser.java
**/diagnostics/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentation
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMetadataDiagnostic.java
🧠 Learnings (6)
📚 Learning: 2025-01-19T21:44:32.675Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java:47-47
Timestamp: 2025-01-19T21:44:32.675Z
Learning: В классах-наследниках AbstractRunTestsCodeLensSupplier метод getSelf() должен возвращать this вместо использования Spring-based self-reference, чтобы избежать проблем с циклическими зависимостями и timing issues при тестировании.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java
📚 Learning: 2025-01-19T21:45:52.703Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java:47-47
Timestamp: 2025-01-19T21:45:52.703Z
Learning: В классах-наследниках AbstractRunTestsCodeLensSupplier метод getCodeLenses должен проверять isApplicable перед выполнением своей логики, чтобы учитывать поддержку клиента и другие условия применимости линз.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java
📚 Learning: 2025-01-19T21:47:05.209Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunAllTestsCodeLensSupplier.java:47-47
Timestamp: 2025-01-19T21:47:05.209Z
Learning: В классе AbstractRunTestsCodeLensSupplier проверка поддержки клиента должна выполняться до вызова getTestIds, чтобы предотвратить выполнение лишних операций и обеспечить корректное поведение при неподдерживаемом клиенте.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java
📚 Learning: 2025-02-10T17:13:41.452Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:54-58
Timestamp: 2025-02-10T17:13:41.452Z
Learning: In ProtectedModuleDiagnostic.java, the usage of `Range` field as a diagnostic target is accepted as is, despite being marked as a temporary solution in the comments.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMetadataDiagnostic.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java
📚 Learning: 2025-02-10T17:12:56.150Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:63-66
Timestamp: 2025-02-10T17:12:56.150Z
Learning: In BSL Language Server, `documentContext.getServerContext().getConfiguration()` is guaranteed to return a non-null value, making null checks unnecessary.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/MultilingualStringAnalyser.java
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/SkippedCallArgumentNode.java (1)
SkippedCallArgumentNode(29-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build
- GitHub Check: Analyse
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
🔇 Additional comments (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Trees.java (1)
130-136: Null-safegetAncestorByRuleIndexlooks good.
Returningnullwhenelementisnullis a sensible contract and reduces accidental NPEs.src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java (1)
62-97: The self-injection pattern with@Autowired @Lazyis necessary and correct. The base classAbstractRunTestsCodeLensSupplierusesgetSelf()to call the proxiedgetTestSources()method decorated with@Cacheable, which requires Spring AOP proxy-based invocation. The@SuppressWarnings("NullAway.Init")suppression appropriately handles the initialization-order analysis warning for this pattern. The framework'sCodeLensProvidercallsisApplicable()before invokinggetCodeLenses()(line 81 filters suppliers), so no additional applicability check is needed withingetCodeLenses()itself. The current implementation is correct.src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/MultilingualStringAnalyser.java (2)
66-66: Good addition of @nullable annotation for static analysis.The annotation correctly indicates that this field may be null, improving type safety documentation.
215-215: Good defensive assertion before accessing the field.The assertion appropriately guards against potential null access if
isParentTemplate()is called beforeparse()completes successfully. This provides runtime safety given the class's stateful API design.
...github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java
Show resolved
Hide resolved
theshadowco
force-pushed
the
feature/sqfixes251212
branch
from
8efb955 to
2c2eb6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/infrastructure/CacheConfiguration.java (1)
193-204: Guard againstgetCache(...)returning null.
If the cache wasn’t created/registered as expected,nativeCachecan be null and you’ll fail later (possibly with a less clear error). ConsiderObjects.requireNonNullwith a clear message.public CacheManager typoCacheManager(org.ehcache.CacheManager ehcacheManager) { - var nativeCache = ehcacheManager.getCache(TYPO_CACHE_NAME, String.class, WordStatus.class); + var nativeCache = java.util.Objects.requireNonNull( + ehcacheManager.getCache(TYPO_CACHE_NAME, String.class, WordStatus.class), + "Ehcache cache '" + TYPO_CACHE_NAME + "' was not created" + );
🧹 Nitpick comments (6)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.java (1)
130-146:varfordelimiteris OK, but consider keepingfinal char delimiter = ','for intent.
Purely a readability nit:varinferscharhere; explicitfinalcan make the delimiter role more obvious.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java (1)
54-81: Nice improvement: extracted expression count/index constants; also replace remainingexp.get(0)usages.
Right now the diagnostic payload still usesexp.get(0)(Line 72, Line 75); switching toINDEX_CONDITIONkeeps the refactor consistent.- diagnosticStorage.addDiagnostic(ctx, DiagnosticStorage.createAdditionalData(exp.get(0).getText())); + diagnosticStorage.addDiagnostic(ctx, DiagnosticStorage.createAdditionalData(exp.get(INDEX_CONDITION).getText())); } else if (trueBranch == BSLParser.FALSE && falseBranch == BSLParser.TRUE) { diagnosticStorage.addDiagnostic(ctx, - DiagnosticStorage.createAdditionalData(getAdaptedText(exp.get(0).getText()))); + DiagnosticStorage.createAdditionalData(getAdaptedText(exp.get(INDEX_CONDITION).getText()))); } else if (trueBranch != SKIPPED_RULE_INDEX || falseBranch != SKIPPED_RULE_INDEX) {src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/MethodSymbolMarkupContentBuilder.java (1)
89-95: Avoid mixingStringandvarin the same method unless the style guide explicitly allows it.
This is functionally safe, but the method currently uses explicitStringfor most sections andvaronly for the last two locals; consider making it consistent one way or the other. As per coding guidelines, follow docs/en/contributing/StyleGuide.md.src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java (3)
53-54: Prefer unit-specific constant names (avoid ambiguity forDISK_SIZE).
DISK_SIZEis used as MB (MemoryUnit.MB), so naming itDISK_SIZE_MB(or adding a short comment) prevents future misuse.- private static final long DISK_SIZE = 50; + private static final long DISK_SIZE_MB = 50;- .disk(DISK_SIZE, MemoryUnit.MB, true); + .disk(DISK_SIZE_MB, MemoryUnit.MB, true);
113-130: Don’t silently swallowStateTransitionException—add at least debug logging.
Right now a lock (or any state transition failure) is ignored, which makes diagnosing “why typo cache became in-memory” hard. Consider logging the instanceNumber + resolved path; and optionally log once when falling back to in-memory.private static org.ehcache.CacheManager createEhcacheManagerWithRetry( CachePathProvider cachePathProvider, String basePath, String fullPath ) { for (var instanceNumber = 0; instanceNumber < MAX_CACHE_INSTANCES; instanceNumber++) { try { var cacheDir = cachePathProvider.getCachePath(basePath, fullPath, instanceNumber); return createEhcacheManager(cacheDir); } catch (org.ehcache.StateTransitionException e) { + // TODO log at debug/warn: cacheDir + instanceNumber + e.getMessage() // This exception indicates the directory is locked by another process // Continue to try next instance number } } // If we exhausted all attempts, fall back to in-memory cache + // TODO log at warn: falling back to in-memory cache after MAX_CACHE_INSTANCES attempts return createInMemoryEhcacheManager(); }
138-151: Make cache entry count and disk size configurable via Spring properties with sane defaults.The constants
ENTRIES_COUNTandDISK_SIZEare hard-coded, limiting flexibility across environments. Following the established pattern in the codebase (e.g.,app.cache.basePath,app.cache.fullPath), consider externalizing these toapplication.propertieswith@Valueinjection. For example:app.cache.entriesCount=125000andapp.cache.diskSizeMb=50.Also applies to: 161-172, 180-190
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/EventPublisherAspect.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationSymbolMarkupContentBuilder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/MethodSymbolMarkupContentBuilder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/ModuleSymbolMarkupContentBuilder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/VariableSymbolMarkupContentBuilder.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java(5 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CachePathProviderTest.java(9 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFillerTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/ModuleReferenceTest.java(4 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/EventPublisherAspect.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/ModuleSymbolMarkupContentBuilder.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/ModuleReferenceTest.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationSymbolMarkupContentBuilder.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/hover/MethodSymbolMarkupContentBuilder.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CachePathProviderTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/hover/VariableSymbolMarkupContentBuilder.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFillerTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java
**/test/java/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use appropriate test frameworks (JUnit, AssertJ, Mockito) for testing
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CachePathProviderTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFillerTest.java
**/diagnostics/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentation
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.java
🧠 Learnings (3)
📚 Learning: 2025-11-27T07:17:33.726Z
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to **/test/java/**/*.java : Use appropriate test frameworks (JUnit, AssertJ, Mockito) for testing
Applied to files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CachePathProviderTest.java
📚 Learning: 2025-01-19T20:47:40.061Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T20:47:40.061Z
Learning: Configuration classes in the BSL Language Server project use mutable collections (HashMap, ArrayList) and Data annotation from Lombok, allowing for modification of configuration properties after initialization.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
📚 Learning: 2025-02-10T17:12:56.150Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:63-66
Timestamp: 2025-02-10T17:12:56.150Z
Learning: In BSL Language Server, `documentContext.getServerContext().getConfiguration()` is guaranteed to return a non-null value, making null checks unnecessary.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build
- GitHub Check: Analyse
- GitHub Check: Analyze the repo with CodeSee
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
🔇 Additional comments (25)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java (2)
137-137: Good extraction of the close-timeout constant (avoid magic number).
493-493: Timeout unification indidCloselooks correct.awaitTermination(AWAIT_CLOSE, TimeUnit.SECONDS)matches the constant’s units and keeps behavior unchanged.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java (1)
201-217: Lambda parameter typing inversionWaiters.computeis fine (behavior-neutral) and improves clarity.
No concerns with this change; the explicit types match theConcurrentSkipListMap<Integer, CopyOnWriteArrayList<CompletableFuture<Void>>>value contract.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.java (1)
180-226:varconversions incheck()/fireDiagnosticOnCheckedWordsWithErrors()look safe and keep the code cleaner.
No behavior changes observed.Also applies to: 228-241
src/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFillerTest.java (1)
373-373: LGTM! Improved assertion style.Replacing
hasSize(0)withisEmpty()makes the assertions more readable and idiomatic. This follows AssertJ best practices for checking empty collections.Also applies to: 455-455
src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.java (1)
64-70:varhere is fine, but please keep style consistent with the project’s Java Style Guide.
No behavior change; both locals infer toString. If the style guide discouragesvarfor simple types (or mixedString/varin the same method), prefer reverting these toStringfor consistency. As per coding guidelines, follow docs/en/contributing/StyleGuide.md.src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationSymbolMarkupContentBuilder.java (1)
81-87: LGTM; just ensurevarusage matches the project conventions.
If the style guide prefers explicit types forString, revert; otherwise this is a clean, low-risk refactor. As per coding guidelines, follow docs/en/contributing/StyleGuide.md.src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/VariableSymbolMarkupContentBuilder.java (1)
58-74:varimproves brevity here; verify it won’t conflict with style/checkstyle rules.
No functional concern—just ensure the repo’s style guide permitsvarfor simpleStringlocals. As per coding guidelines, follow docs/en/contributing/StyleGuide.md.src/test/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CachePathProviderTest.java (9)
93-97: LGTM! Good fluent assertion refactor.The chained assertion style improves readability while maintaining the same verification logic for the MD5 hash format.
126-127: LGTM! Idiomatic AssertJ path assertion.Using
hasToString()instead of explicittoString()calls is the preferred AssertJ pattern for path component verification.
144-144: LGTM! Consistent path assertion style.The refactor to
hasToString()maintains consistency with other path assertions in the test class.
159-159: LGTM! Consistent refactoring pattern.Applies the same
hasToString()refactoring consistently across similar test scenarios.
189-192: LGTM! Clear fluent assertion chain.Chaining the string assertions provides a clear, readable verification of the complete path structure.
221-223: LGTM! Logical assertion grouping.The chained assertions effectively verify that instance number 0 produces a plain MD5 hash without suffix.
238-240: LGTM! Correct inequality chaining.The chained assertions correctly verify that
cachePath0differs from bothcachePath1andcachePath2. Line 241 appropriately completes the verification by testingcachePath1vscachePath2.
257-259: LGTM! Effective transitive equality chain.The chained
isEqualToassertions elegantly verify that all instance-numbered paths share the same parent directory through transitive equality.
275-277: LGTM! Clear equality verification.The chained assertions effectively verify that instance number is ignored when
fullPathis provided, establishing that all paths equal the custom path.src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java (8)
139-145: LGTM!The
SPEC_LITERALSconstant follows the established pattern of other token type sets in this file and correctly groups the special literal tokens for keyword classification.
342-346: Clean decomposition of preprocessor handling.The extraction into three focused helper methods improves readability and maintainability.
348-367: LGTM!The method correctly filters out region-related preprocessor nodes and emits Macro tokens for the remaining HASH and PREPROC_* tokens. The null check on
symbolicNameat line 362 is properly handled.
369-386: LGTM!The method correctly processes use and native directives with proper null guards. The inline comments clearly document the purpose of each section.
388-408: LGTM with minor observation.The method correctly handles both region start and end nodes. The fallback to
addNamespaceForPreprocessorNodeat line 398 may re-fetch the preprocessor ancestor when it's already available butPREPROC_REGION()is null. This is acceptable as defensive coding sinceaddNamespaceForPreprocessorNodehandles the null case gracefully.
423-438: LGTM!The variable renaming to
hashNodeanduseNodeimproves clarity by indicating these are AST nodes.
526-534: LGTM!The simplification delegates token type selection to the new
selectAndAddSemanticTokenmethod, keeping this loop clean and focused on iteration.
536-555: Well-structured consolidation of token type dispatch.The method clearly organizes the token classification logic with appropriate comments. The skip for
AMPERSANDandANNOTATION_TOKENSat line 545-546 with the explanatory comment is good practice to avoid duplicate emissions.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java (1)
72-75: Complete the refactoring with INDEX_CONDITION.Lines 72 and 75 still use hard-coded
exp.get(0)instead ofexp.get(INDEX_CONDITION), creating inconsistency with the constants introduced in this same change.Apply this diff to complete the refactoring:
if (condition != SKIPPED_RULE_INDEX) { diagnosticStorage.addDiagnostic(ctx); } else if (trueBranch == BSLParser.TRUE && falseBranch == BSLParser.FALSE) { - diagnosticStorage.addDiagnostic(ctx, DiagnosticStorage.createAdditionalData(exp.get(0).getText())); + diagnosticStorage.addDiagnostic(ctx, DiagnosticStorage.createAdditionalData(exp.get(INDEX_CONDITION).getText())); } else if (trueBranch == BSLParser.FALSE && falseBranch == BSLParser.TRUE) { diagnosticStorage.addDiagnostic(ctx, - DiagnosticStorage.createAdditionalData(getAdaptedText(exp.get(0).getText()))); + DiagnosticStorage.createAdditionalData(getAdaptedText(exp.get(INDEX_CONDITION).getText()))); } else if (trueBranch != SKIPPED_RULE_INDEX || falseBranch != SKIPPED_RULE_INDEX) {src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.java (1)
148-154: Guardlang→ pool lookup + make acquire/release NPE-safe (avoid leaks and hard crashes).
getLanguageToolPoolMap().get(lang)can be null (bundle misconfig / new language), and ifacquireLanguageTool(lang)throws,finallywon’t run and a checked-out instance may leak. Consider validatinglangonce and releasing only when acquired.+import java.util.Objects; +import org.jspecify.annotations.Nullable; ... - private static JLanguageTool acquireLanguageTool(String lang) { - return getLanguageToolPoolMap().get(lang).checkOut(); - } + private static JLanguageTool acquireLanguageTool(String lang) { + var pool = getLanguageToolPoolMap().get(lang); + if (pool == null) { + throw new IllegalArgumentException("Unsupported diagnosticLanguage: " + lang); + } + return pool.checkOut(); + } - private static void releaseLanguageTool(String lang, JLanguageTool languageTool) { - getLanguageToolPoolMap().get(lang).checkIn(languageTool); - } + private static void releaseLanguageTool(String lang, @Nullable JLanguageTool languageTool) { + if (languageTool == null) { + return; + } + var pool = getLanguageToolPoolMap().get(lang); + if (pool != null) { + pool.checkIn(languageTool); + } + } ... - var languageTool = acquireLanguageTool(lang); - - List<RuleMatch> matches = Collections.emptyList(); - try { + List<RuleMatch> matches = Collections.emptyList(); + JLanguageTool languageTool = null; + try { + languageTool = acquireLanguageTool(lang); matches = languageTool.check( uncheckedWordsString, true, JLanguageTool.ParagraphHandling.ONLYNONPARA ); } catch (IOException e) { LOGGER.error(e.getMessage(), e); } finally { releaseLanguageTool(lang, languageTool); }Also applies to: 183-213
🧹 Nitpick comments (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java (2)
53-55: Make cache-size constants self-describing (units in the name).
DISK_SIZEis ambiguous since it’s “50 MB” only by implication at usage sites.- private static final long ENTRIES_COUNT = 125_000; - private static final long DISK_SIZE = 50; + private static final long HEAP_ENTRIES_COUNT = 125_000; + private static final long DISK_SIZE_MB = 50;(Then update usages accordingly.)
113-130: Add logging for locked dirs and for the “fallback to in-memory” path.
Right nowStateTransitionExceptionis swallowed and the fallback is silent, which can lead to “why did my typo cache stop persisting?” debugging later.public class CacheConfiguration { + private static final org.slf4j.Logger log = + org.slf4j.LoggerFactory.getLogger(CacheConfiguration.class); ... private static org.ehcache.CacheManager createEhcacheManagerWithRetry( CachePathProvider cachePathProvider, String basePath, String fullPath ) { for (var instanceNumber = 0; instanceNumber < MAX_CACHE_INSTANCES; instanceNumber++) { try { var cacheDir = cachePathProvider.getCachePath(basePath, fullPath, instanceNumber); return createEhcacheManager(cacheDir); } catch (org.ehcache.StateTransitionException e) { + log.debug("Ehcache directory is locked, trying next instanceNumber={}", instanceNumber, e); } } - // If we exhausted all attempts, fall back to in-memory cache + log.warn("All ehcache directories are locked (attempts={}); falling back to in-memory cache", MAX_CACHE_INSTANCES); return createInMemoryEhcacheManager(); }src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.java (1)
130-146: Optional: avoid producing[""]when exceptions string is empty.Today it’s mostly harmless (later filters drop short/blank tokens), but returning an empty set here is cleaner and avoids surprising config states.
- return Arrays.stream(exceptions.split(String.valueOf(delimiter))) - .collect(Collectors.toSet()); + if (exceptions.isEmpty()) { + return Collections.emptySet(); + } + return Arrays.stream(exceptions.split(String.valueOf(delimiter))) + .filter(Predicate.not(String::isBlank)) + .collect(Collectors.toSet());src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java (2)
388-408: Region token split (Namespace for#регион, Variable for region name) is a solid overlap-avoidance strategy.Minor: the fallback
addNamespaceForPreprocessorNode(entries, regionStart)can reintroduce overlap withregionNameif it ever triggers for a valid regionStart; consider ensuring the “keyword-only” range is always used whenregionStart.regionName() != null.
526-555: Lexical token dispatch is cleaner and reduces branching duplication.Optional: consider a
switchontokenTypefor the singleton cases (DATETIME,AMPERSAND) to reduce chainedelse ifdepth.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/EventPublisherAspect.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMetadataDiagnostic.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationSymbolMarkupContentBuilder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/MethodSymbolMarkupContentBuilder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/ModuleSymbolMarkupContentBuilder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/VariableSymbolMarkupContentBuilder.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/MultilingualStringAnalyser.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Trees.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java(6 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CachePathProviderTest.java(9 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFillerTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/ModuleReferenceTest.java(4 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/EventPublisherAspect.java
🚧 Files skipped from review as they are similar to previous changes (15)
- src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/ModuleReferenceTest.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFillerTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMetadataDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/MultilingualStringAnalyser.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/MethodSymbolMarkupContentBuilder.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CachePathProviderTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationSymbolMarkupContentBuilder.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/VariableSymbolMarkupContentBuilder.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/ModuleSymbolMarkupContentBuilder.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/RunTestCodeLensSupplier.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Trees.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java
**/diagnostics/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentation
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.java
🧠 Learnings (2)
📚 Learning: 2025-01-19T20:47:40.061Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T20:47:40.061Z
Learning: Configuration classes in the BSL Language Server project use mutable collections (HashMap, ArrayList) and Data annotation from Lombok, allowing for modification of configuration properties after initialization.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java
📚 Learning: 2025-11-27T07:17:33.726Z
Learnt from: CR
Repo: 1c-syntax/bsl-language-server PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T07:17:33.726Z
Learning: Applies to docs/*/diagnostics/**/*.md : Update diagnostic documentation in both Russian and English with examples of problematic code and fixes
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TypoDiagnostic.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gatekeeper
🔇 Additional comments (7)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java (2)
55-58: LGTM! Clear semantic constants.The named constants improve code readability by replacing magic numbers with meaningful identifiers.
64-67: LGTM! Consistent use of named constants.The refactoring correctly replaces hard-coded indices with the semantic constants defined above.
src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java (2)
138-143: Nice cleanup: centralized sizing constants; consider renaming + keep disk sizing consistent with comments.
Using constants here improves readability and makes tuning easier.Also applies to: 161-165
180-190: Static helper looks good.
No behavior change; makes intent clearer (no instance state).src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Trees.java (1)
125-136: All call sites properly handle the nullable contract—no action needed.A survey of 26+ call sites across the codebase confirms that callers already handle nullable inputs and returns correctly: most use explicit null checks before accessing the result, some wrap the result in
Optional.ofNullable(), and others pass the result to@Nullable-annotated parameters that safely check for null. Even the cast atTimeoutsInExternalResourcesDiagnostic.java:213is safe because the result is passed togetVariableName(), which accepts@Nullable StatementContextand checks for null before use. The API contract is sound and consistently respected.Likely an incorrect or invalid review comment.
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java (2)
139-145: Good: explicit “special literals” classification keeps keyword handling predictable.
423-438:addNamespaceForUse: clearer node naming; token-range behavior still consistent.
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/TreesTest.java (1)
38-56: Good test coverage for valid parse tree scenario.The test correctly validates token extraction from a valid BSL procedure. The expected token count of 6 matches the grammar output, and the use of given-when-then structure with AssertJ assertions follows best practices.
Optional: Consider using
varfor consistency.Line 47 uses explicit type
ParseTree treewhile other test methods usevar tokens. For consistency, consider:- ParseTree tree = parser.file(); + var tree = parser.file();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/MethodSymbolMarkupContentBuilder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java(5 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/TreesTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/utils/TreesTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/hover/MethodSymbolMarkupContentBuilder.java
**/diagnostics/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Each diagnostic should have a Java implementation class, resource bundle for localized messages, unit tests, and documentation
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java
**/test/java/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use appropriate test frameworks (JUnit, AssertJ, Mockito) for testing
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/TreesTest.java
🧠 Learnings (2)
📚 Learning: 2025-01-19T20:47:40.061Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3388
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/codelens/TestRunnerAdapterOptions.java:46-46
Timestamp: 2025-01-19T20:47:40.061Z
Learning: Configuration classes in the BSL Language Server project use mutable collections (HashMap, ArrayList) and Data annotation from Lombok, allowing for modification of configuration properties after initialization.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java
📚 Learning: 2025-11-19T09:02:03.154Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3610
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/folding/QueryPackageFoldingRangeSupplier.java:26-26
Timestamp: 2025-11-19T09:02:03.154Z
Learning: В проекте bsl-language-server используется кастомная версия ANTLR4 от 1c-syntax (io.github.1c-syntax:antlr4), которая включает базовый класс org.antlr.v4.runtime.Tokenizer с методами getAst() и getTokens(). SDBLTokenizer и BSLTokenizer наследуются от этого базового класса.
Applied to files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/TreesTest.java
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticStorage.java (1)
DiagnosticStorage(50-332)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: Analyse
🔇 Additional comments (9)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java (1)
55-58: Nice readability win by replacing magic numbers with named constants.src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/CacheConfiguration.java (4)
53-54: Excellent refactoring to eliminate magic numbers.The introduction of these constants improves maintainability and makes the cache sizing configuration explicit and centralized.
113-113: LGTM: Appropriate conversion to static methods.All four helper methods are correctly made static since they don't access instance state. This follows Java best practices for utility-style methods within a class.
Also applies to: 138-138, 161-161, 180-180
118-118: Good use of local type inference.The type is clearly inferred from the context, and this aligns with the modern Java style used elsewhere in the file.
141-142: Consistent application of constants.The hard-coded values have been properly replaced with the newly defined constants throughout the file, ensuring consistent cache sizing configuration.
Also applies to: 164-164
src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/TreesTest.java (2)
58-65: Excellent null-safety test.This test correctly validates that
Trees.getTokens()handles null input gracefully by returning an empty list. This aligns with the PR's focus on null-safety improvements across the codebase.
67-103: Comprehensive edge case and token type verification.Both test methods provide valuable coverage:
testGetTokensWithEmptyTreecorrectly validates that empty input produces a single EOF token.testGetTokensReturnsCorrectTokenTypesverifies not just token count but also token type sequence, ensuring the parser correctly identifies BSL keywords.The tests follow consistent structure and use AssertJ fluent assertions effectively.
src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/MethodSymbolMarkupContentBuilder.java (2)
54-54: LGTM: Good use of local variable type inference.The conversion from explicit
Stringdeclarations tovaris appropriate here. The types are immediately clear from the method names (getSignature,getLocation, etc.), and this reduces verbosity while maintaining readability. This follows modern Java practices for Java 17.Also applies to: 58-58, 62-62, 66-66, 70-70, 74-74, 78-78, 81-81
42-84: Removed constants have been properly relocated toDescriptionFormatter.All private static final String constants (PROCEDURE_KEY, FUNCTION_KEY, EXPORT_KEY, VAL_KEY, PARAMETERS_KEY, RETURNED_VALUE_KEY, EXAMPLES_KEY, CALL_OPTIONS_KEY, PARAMETER_TEMPLATE) have been successfully moved to
DescriptionFormatter(lines 50-60) and are actively used throughout that class. The formatting logic has been properly delegated throughdescriptionFormattermethod calls with no orphaned references in the codebase. Separation of concerns is maintained with formatting logic consolidated in the formatter utility class.
| if (exp != null && exp.size() >= COUNT_EXPRESSIONS) { | ||
| var condition = getBooleanToken(exp.get(INDEX_CONDITION)); | ||
| var trueBranch = getBooleanToken(exp.get(INDEX_TRUE_BRANCH)); | ||
| var falseBranch = getBooleanToken(exp.get(INDEX_FALSE_BRANCH)); |
There was a problem hiding this comment.
Harden null-safety: getBooleanToken(Optional.of(expCtx)) will NPE if any exp.get(i) is null.
If ctx.expression() can’t contain null items, it’s fine today, but given the PR’s general null-safety direction, it’d be safer to make getBooleanToken tolerate null inputs (or explicitly assert non-null at call sites).
- private static int getBooleanToken(BSLParser.ExpressionContext expCtx) {
+ private static int getBooleanToken(@org.jetbrains.annotations.Nullable BSLParser.ExpressionContext expCtx) {
var tmpCtx = Optional.of(expCtx)
+ // or: Optional.ofNullable(expCtx)
.filter(ctx -> ctx.children.size() == 1)
.map(ctx -> ctx.member(0))
.map(ctx -> ctx.getChild(0))
.filter(BSLParser.ConstValueContext.class::isInstance)
.map(BSLParser.ConstValueContext.class::cast);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UselessTernaryOperatorDiagnostic.java
around lines 64 to 67, calling getBooleanToken(exp.get(...)) can NPE if any
exp.get(i) returns null; either guard each exp.get(INDEX_...) with a null check
and only call getBooleanToken when non-null (e.g., use
Optional.ofNullable(exp.get(i)) or skip/handle missing entries), or change
getBooleanToken to accept a possibly-null parameter (or Optional) and handle
null internally; update the callsite to pass a nullable/Optional value and
adjust downstream logic accordingly so no NPE occurs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilderTest.java (2)
41-56: Consider using more fluent AssertJ assertions.The assertion at line 55 creates a new
MarkupContentobject for comparison. For better readability and to align with the fluent AssertJ style used inAnnotationSymbolMarkupContentBuilderTest.java, consider checking the fields separately.Apply this diff to use more fluent assertions:
// then - assertThat(result).isEqualTo(new MarkupContent(MarkupKind.MARKDOWN, "")); + assertThat(result.getValue()).isEmpty(); + assertThat(result.getKind()).isEqualTo(MarkupKind.MARKDOWN); }
58-76: Consider using more fluent AssertJ assertions.Similar to the previous test, the assertion at line 75 creates a new
MarkupContentobject for comparison. Use the same fluent style for consistency.Apply this diff to use more fluent assertions:
// then - assertThat(result).isEqualTo(new MarkupContent(MarkupKind.MARKDOWN, "")); + assertThat(result.getValue()).isEmpty(); + assertThat(result.getKind()).isEqualTo(MarkupKind.MARKDOWN); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilderTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationSymbolMarkupContentBuilderTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationSymbolMarkupContentBuilderTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilderTest.java
**/test/java/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use appropriate test frameworks (JUnit, AssertJ, Mockito) for testing
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationSymbolMarkupContentBuilderTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilderTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Analyse
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build
🔇 Additional comments (4)
src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilderTest.java (1)
78-89: LGTM!The test correctly verifies that
getSymbolKind()returnsSymbolKind.TypeParameter. The test structure follows best practices with the given-when-then pattern.src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationSymbolMarkupContentBuilderTest.java (3)
39-54: LGTM!The test correctly uses fluent AssertJ assertions by checking
content.getValue()andcontent.getKind()separately. This approach is more readable and follows AssertJ best practices. The given-when-then structure is clear and the mocking setup is appropriate.
56-70: LGTM!Good edge case coverage for when the parent is absent. The test uses fluent AssertJ style consistently and follows the given-when-then pattern properly.
72-83: LGTM!The test correctly verifies that
getSymbolKind()returnsSymbolKind.Interface. The test structure is clean and follows best practices.
| } | ||
| var subExpression = makeSubexpression(typeNameArg.expression()); | ||
| if (subExpression == null) { | ||
| operands.push(new ErrorExpressionNode(ctx)); |
There was a problem hiding this comment.
А почему ExpressionTree стал Nullable?
| public static <T extends ParserRuleContext> T getAncestorByRuleIndex(ParserRuleContext element, int type) { | ||
| public static <T extends ParserRuleContext> T getAncestorByRuleIndex(@Nullable ParserRuleContext element, int type) { | ||
| if (element == null) { | ||
| return null; |
There was a problem hiding this comment.
Может на Trees из antlr навесить jspecify аннотации и эти проверки? Тогда тут их можно будет убрать.
There was a problem hiding this comment.
Не, не нравится мне Nullable в параметрах...
| var result = builder.getContent(symbol); | ||
|
|
||
| // then | ||
| assertThat(result).isEqualTo(new MarkupContent(MarkupKind.MARKDOWN, "")); |
There was a problem hiding this comment.
Прям таки пустая строка? Там разве хотя бы сигнатуры и списка параметров не должно быть для зарегистрированных аннотаций?
|
|
||
| // then | ||
| assertThat(content.getValue()).isEmpty(); | ||
| assertThat(content.getKind()).isEqualTo(MarkupKind.MARKDOWN); |
There was a problem hiding this comment.
Было бы хорошо что-нибудь в плане содержимого тут проверить, как в соседних тестах
There was a problem hiding this comment.
Вообще странно, что ты создаёшь эти тесты, я был уверен, что они есть...
| String input = """ | ||
| Процедура Тест() | ||
| КонецПроцедуры"""; | ||
| var lexer = new BSLLexer(CharStreams.fromString(input)); |
There was a problem hiding this comment.
Может BSLTokenizer использовать тут? Для простоты
...in/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/AbstractMetadataDiagnostic.java
Show resolved
Hide resolved
nixel2007
force-pushed
the
feature/sqfixes251212
branch
from
9561ebd to
4be9da8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java (2)
389-408: Comment mismatch: this loop doesn’t actually “exclude native/use”; it only excludes regions.
Not a functional bug (given grammar:use/preproc_nativearen’tPreprocessorContext), but the comment currently implies logic that isn’t present, which can mislead future edits.@@ - // Other preprocessor directives: Macro for each HASH and PREPROC_* token, - // excluding region start/end, native, use (handled as Namespace) + // Other preprocessor directives (PreprocessorContext): Macro for HASH and PREPROC_* tokens. + // Regions are excluded here (handled in addRegionsNamespaces). + // Module annotations (use/preproc_native) are handled in addDirectives and are not part of PreprocessorContext.
526-555: Add tests to cover NULL literal and PREPROC_ keyword exclusion.*The refactored logic correctly forces
UNDEFINED/TRUE/FALSE/NULLtoKeywordtype via the newSPEC_LITERALSconstant, and the keyword detection now excludesPREPROC_*tokens (handled via AST). However, test coverage is incomplete:
datetimeAndUndefinedTrueFalse_areHighlighted()testsUNDEFINED/TRUE/FALSEbut notNULL- The
PREPROC_*exclusion in the lexical path is not directly verified—only the AST emission path is testedAdd unit tests covering both the
NULLliteral classification and the explicit exclusion ofPREPROC_*_KEYWORDtokens from semantic token emission to prevent regressions in token classification.
🧹 Nitpick comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java (3)
342-368: Potential overlap fallback:addNamespaceForPreprocessorNode(entries, regionStart)can include the region name.
In the “else” branch (when ancestor/PREPROC_REGIONisn’t available), you may emit aNamespacerange that spans intoregionStart.regionName()while also emittingVariablefor the region name (Line 361-363). Consider a conservative fallback to avoid overlapping types.@@ - } else { - addNamespaceForPreprocessorNode(entries, regionStart); - } + } else { + // Conservative fallback: avoid spanning into regionName (which is emitted as Variable below) + var preprocessor = Trees.<PreprocessorContext>getAncestorByRuleIndex(regionStart, BSLParser.RULE_preprocessor); + if (preprocessor != null && preprocessor.getStart() != null) { + addRange(entries, Ranges.create(preprocessor.getStart()), SemanticTokenTypes.Namespace); + } else { + addNamespaceForPreprocessorNode(entries, regionStart); + } + }
370-387: Consider emitting#nativeas a single Macro range when both tokens exist (fewer tokens, consistent style).
Current behavior is correct, but you can reduce output noise by joiningHASH..PREPROC_NATIVE(like you do for#Использовать).@@ - if (hash != null) { - addRange(entries, Ranges.create(hash), SemanticTokenTypes.Macro); - } - if (nativeKw != null) { - addRange(entries, Ranges.create(nativeKw), SemanticTokenTypes.Macro); - } + if (hash != null && nativeKw != null) { + addRange(entries, Ranges.create(hash, nativeKw), SemanticTokenTypes.Macro); + } else if (hash != null) { + addRange(entries, Ranges.create(hash), SemanticTokenTypes.Macro); + } else if (nativeKw != null) { + addRange(entries, Ranges.create(nativeKw), SemanticTokenTypes.Macro); + }
423-438:addNamespaceForUse: logic is fine; trailingelse { // no-op }can be dropped.
Optional cleanup to reduce dead branches.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java
🧠 Learnings (2)
📚 Learning: 2025-11-19T09:02:03.154Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3610
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/folding/QueryPackageFoldingRangeSupplier.java:26-26
Timestamp: 2025-11-19T09:02:03.154Z
Learning: В проекте bsl-language-server используется кастомная версия ANTLR4 от 1c-syntax (io.github.1c-syntax:antlr4), которая включает базовый класс org.antlr.v4.runtime.Tokenizer с методами getAst() и getTokens(). SDBLTokenizer и BSLTokenizer наследуются от этого базового класса.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build
- GitHub Check: Analyze the repo with CodeSee
🔇 Additional comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/SemanticTokensProvider.java (1)
139-145: SPEC_LITERALS → Keyword classification looks correct (undefined/true/false/null).
This makes these literals consistent with other “language-defined” atoms and avoids them falling through to “unknown”.
|
| } | ||
| var subExpression = makeSubexpression(typeNameArg.expression()); | ||
| if (subExpression == null) { | ||
| operands.push(new ErrorExpressionNode(ctx)); |
There was a problem hiding this comment.
А почему ExpressionTree стал Nullable?
| makeSubexpression(ctx.expression(0)), | ||
| makeSubexpression(ctx.expression(1)), | ||
| makeSubexpression(ctx.expression(2)) | ||
| Objects.requireNonNull(makeSubexpression(ctx.expression(0))), |
There was a problem hiding this comment.
Давай лучше makeSubexpression не будет возвращать null? Или в этом есть тайный смысл, который я не понял?
Пусть бы resultExpression если null, возврат ErrorNode и результат всегда не нулл, не?
Описание
Исправление замечаний к коду
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Summary by CodeRabbit
Bug Fixes & Improvements
UI / Editor Experience
Maintenance
Tests
✏️ Tip: You can customize this high-level summary in your review settings.