Оптимизация хранения + некоторые фиксы сонара #3555
Conversation
|
Warning Rate limit exceeded@theshadowco has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 1 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)
WalkthroughBumped mdclasses dependency; migrated many Lombok-backed DTOs to Java records and renamed getter-style methods to fluent accessors; added @serial to serialVersionUID fields; tightened nullability and visibility in a few places; assorted cleanup and corresponding test updates across the codebase. Changes
Sequence Diagram(s)(Skipped — changes are API/type renames and refactors without primary control-flow feature additions that benefit from a sequence diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to inspect closely:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 7
🧹 Nitpick comments (11)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ScheduledJobHandlerDiagnostic.java (1)
89-89: Avoid usingpeek()for side effects; prefermap()or explicit iteration.Using
.peek()to mutate stream elements (sorting in-place) is discouraged becausepeek()is primarily intended for debugging. This makes the side effect less obvious and could be fragile.Apply this diff to use
map()instead:- .peek(mdScheduledJobs -> mdScheduledJobs.sort(Comparator.comparing(ScheduledJob::getName))) + .map(mdScheduledJobs -> { + mdScheduledJobs.sort(Comparator.comparing(ScheduledJob::getName)); + return mdScheduledJobs; + })Alternatively, if clarity is preferred over streaming style, consider:
- scheduledJobHandlers.values().stream() - .filter(mdScheduledJobs -> mdScheduledJobs.size() > 1) - .peek(mdScheduledJobs -> mdScheduledJobs.sort(Comparator.comparing(ScheduledJob::getName))) - .forEach(this::fireIssueForDoubles); + for (var mdScheduledJobs : scheduledJobHandlers.values()) { + if (mdScheduledJobs.size() > 1) { + mdScheduledJobs.sort(Comparator.comparing(ScheduledJob::getName)); + fireIssueForDoubles(mdScheduledJobs); + } + }src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java (2)
144-152: Consider enhancing test coverage for rename functionality.The test validates that
rename()returns a non-null result but doesn't verify the actual rename behavior or result structure. Additionally, testing with an unknown file (without callingdidOpen()first) may not provide meaningful validation.Consider following the pattern from
testDiagnosticsKnownFileby opening a document first and asserting on the result's content or structure.Example enhancement:
@Test -void testRename() { +void testRename() throws IOException { + // given + var textDocumentItem = getTextDocumentItem(); + var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); + textDocumentService.didOpen(didOpenParams); + + // when var params = new RenameParams(); params.setTextDocument(getTextDocumentIdentifier()); params.setPosition(new Position(0, 16)); + params.setNewName("newName"); var result = textDocumentService.rename(params); + // then assertThat(result).isNotNull(); + assertThat(result.getChanges()).isNotEmpty(); }
155-163: Consider enhancing test coverage for prepareRename functionality.Similar to
testRename(), this test only validates non-null results without verifying the actual functionality or result structure. Testing with an unknown file may not exercise the intended behavior.Consider opening a document first and asserting on the result's range or placeholder content.
Example enhancement:
@Test -void testRenamePrepare() { +void testRenamePrepare() throws IOException { + // given + var textDocumentItem = getTextDocumentItem(); + var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); + textDocumentService.didOpen(didOpenParams); + + // when var params = new PrepareRenameParams(); params.setTextDocument(getTextDocumentIdentifier()); params.setPosition(new Position(0, 16)); var result = textDocumentService.prepareRename(params); + // then assertThat(result).isNotNull(); + // Verify the result contains a valid range or placeholder + assertThat(result.getLeft()).isNotNull(); // or getRight() depending on return type }src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/CompilerDirectiveKind.java (1)
25-25: Question the necessity of adding Lombok for a single getter.The overall PR appears to be converting Lombok-annotated POJOs to Java records, but this file adds Lombok's
@Getterinstead. For an enum with a single field, a manual getter is typically simpler and more explicit than introducing Lombok.Consider keeping the manual
getTokenType()method for clarity, or verify if there's a specific reason for adding Lombok here when the PR removes it elsewhere.Also applies to: 41-42
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (1)
141-143: Consider using.get()for clarity.Since
refContextInsideDefAssignis guaranteed to be non-empty at this point (due to the guard on line 138), using.orElseThrow()is redundant. The.get()method would more clearly express that we know the value is present.Apply this diff for improved clarity:
-var isSelfAssign = refContextInsideDefAssign - .map(RewriteMethodParameterDiagnostic::isVarNameOnlyIntoExpression) - .orElseThrow(); +var isSelfAssign = isVarNameOnlyIntoExpression(refContextInsideDefAssign.get());Alternatively, if you prefer to keep the mapping style:
-var isSelfAssign = refContextInsideDefAssign - .map(RewriteMethodParameterDiagnostic::isVarNameOnlyIntoExpression) - .orElseThrow(); +var isSelfAssign = refContextInsideDefAssign + .map(RewriteMethodParameterDiagnostic::isVarNameOnlyIntoExpression) + .get();src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/variable/VariableDescription.java (1)
106-107: Уточните null‑контракт для param.link()Если link() может быть null, проверьте, что downstream‑код готов к null в поле String link. Иначе нормализуйте (напр., пустая строка) или используйте Optional.
- String link; + // Может быть null, если ссылка отсутствует (уточнить контракт). + String link;src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/AbstractComplexityInlayHintSupplier.java (1)
98-102: Переход на range()/message(): корректноМаппинг в InlayHint не меняет семантику.
Можно заменить Collectors.toList() на Stream.toList() в методе getInlayHints при целевом JDK ≥ 16 для лаконичности.
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.java (1)
59-60: Guard against null and reuse local URIDefensively null-check location and avoid double dereference; keeps Sonar happy and improves readability.
- locations.computeIfAbsent(symbolOccurrence.location().getUri(), uri -> ConcurrentHashMap.newKeySet()) + var uri = java.util.Objects.requireNonNull(symbolOccurrence.location(), "location").getUri(); + locations.computeIfAbsent(uri, u -> ConcurrentHashMap.newKeySet()) .add(symbolOccurrence);src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java (1)
44-64: Refactor compareTo to a clear, accessor-based Comparator chainCleaner, consistent with equals, and avoids direct field access of the peer instance.
- public int compareTo(@Nullable SymbolOccurrence symbolOccurrence) { - if (symbolOccurrence == null) { - return 1; - } - if (this.equals(symbolOccurrence)) { - return 0; - } - final var uriCompare = location.getUri().compareTo(symbolOccurrence.location.getUri()); - if (uriCompare != 0) { - return uriCompare; - } - final var rangesCompare = Ranges.compare(location.getRange(), symbolOccurrence.location.getRange()); - if (rangesCompare != 0) { - return rangesCompare; - } - final var occurrenceCompare = occurrenceType.compareTo(symbolOccurrence.occurrenceType); - if (occurrenceCompare != 0) { - return occurrenceCompare; - } - return symbol.compareTo(symbolOccurrence.symbol); - } + public int compareTo(@Nullable SymbolOccurrence other) { + if (other == null) { + return 1; + } + var cmp = java.util.Comparator + .comparing(SymbolOccurrence::location, java.util.Comparator + .comparing(Location::getUri) + .thenComparing((l1, l2) -> Ranges.compare(l1.getRange(), l2.getRange()))) + .thenComparing(SymbolOccurrence::occurrenceType) + .thenComparing(SymbolOccurrence::symbol) + .compare(this, other); + return cmp; + }Please confirm that Location.equals/hashCode are based on (URI, Range). If Location has additional fields affecting equals, compareTo must incorporate them to stay consistent with equals for use in ConcurrentSkipListSet.
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java (2)
51-51: Make interner field finalImmutable static improves thread-safety semantics.
- private static GenericInterner<Symbol> interner = new GenericInterner<>(); + private static final GenericInterner<Symbol> interner = new GenericInterner<>();
58-89: Simplify compareTo with Comparator chainRemoves boilerplate, keeps ordering over all record components explicit.
- public int compareTo(@Nullable Symbol symbol) { - if (symbol == null) { - return 1; - } - - if (this.equals(symbol)) { - return 0; - } - - int compareResult = mdoRef.compareTo(symbol.mdoRef); - if (compareResult != 0) { - return compareResult; - } - - compareResult = moduleType.compareTo(symbol.moduleType); - if (compareResult != 0) { - return compareResult; - } - - compareResult = scopeName.compareTo(symbol.scopeName); - if (compareResult != 0) { - return compareResult; - } - - compareResult = symbolKind.compareTo(symbol.symbolKind); - if (compareResult != 0) { - return compareResult; - } - - compareResult = symbolName.compareTo(symbol.symbolName); - return compareResult; - } + public int compareTo(@Nullable Symbol other) { + if (other == null) { + return 1; + } + return java.util.Comparator + .comparing(Symbol::mdoRef) + .thenComparing(Symbol::moduleType) + .thenComparing(Symbol::scopeName) + .thenComparing(Symbol::symbolKind) + .thenComparing(Symbol::symbolName) + .compare(this, other); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (74)
build.gradle.kts(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncher.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/PermissionFilterBeforeSendCallback.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/SentryScopeConfigurer.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/databind/ParametersDeserializer.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/events/LanguageServerConfigurationChangedEvent.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexityData.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexitySecondaryLocation.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputer.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/DocumentContextContentChangedEvent.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/ServerContextPopulatedEvent.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/AnnotationParameterDefinition.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/CompilerDirectiveKind.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/DescriptionReader.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/ParameterDescription.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/TypeDescription.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/variable/VariableDescription.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CognitiveComplexityDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CyclomaticComplexityDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DuplicatedInsertionIntoCollectionDiagnostic.java(6 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/LineLengthDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingCommonModuleMethodDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingParameterDescriptionDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingReturnedValueDescriptionDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ScheduledJobHandlerDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SetPermissionsForNewObjectsDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TimeoutsInExternalResourcesDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TransferringParametersBetweenClientAndServerDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UnreachableCodeDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/WrongDataPathForFormElementsDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/events/LanguageServerInitializeRequestReceivedEvent.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatter.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/AbstractComplexityInlayHintSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CognitiveComplexityInlayHintSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CyclomaticComplexityInlayHintSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/SourceDefinedMethodCallInlayHintSupplier.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/AnnotationReferenceFinder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndex.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Location.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/reporters/data/FileInfo.java(1 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(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/TernaryOperatorNode.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncherTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/aop/measures/MeasureCollectorTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplierTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplierTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContextTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CognitiveComplexityComputerTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CyclomaticComplexityComputerTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputerTest.java(3 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/VariableSymbolTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/MethodDescriptionTest.java(6 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/EmptyStatementDiagnosticTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SmokyTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ThisObjectAssignDiagnosticTest.java(0 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatterTest.java(0 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorInformationsAssert.java(3 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorPresentationsAssert.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/FoldingRangesAssert.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitorTest.java(1 hunks)
💤 Files with no reviewable changes (8)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/SentryScopeConfigurer.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/SourceDefinedMethodCallInlayHintSupplier.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Location.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatterTest.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ThisObjectAssignDiagnosticTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/AbstractVariableSymbol.java (1)
Builder(121-207)
⏰ 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: Benchmark
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build
🔇 Additional comments (68)
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/EmptyStatementDiagnosticTest.java (1)
72-72: Good refactoring to useisEmpty().Using
isEmpty()instead ofequals("")is more idiomatic and clearly expresses the intent. This aligns with Sonar best practices (rule S1155).src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncher.java (1)
127-129: LGTM! Clean modernization to Java 16+ Stream API.The migration from
Collectors.toList()toStream.toList()is appropriate here. The resulting unmodifiable list is safe sinceunmatchedArgsis only used for read operations (isEmpty check and iteration).src/test/java/com/github/_1c_syntax/bsl/languageserver/aop/measures/MeasureCollectorTest.java (1)
25-25: LGTM! Modern AssertJ API usage.The migration from
.asList()to.asInstanceOf(InstanceOfAssertFactories.LIST)follows current AssertJ best practices and improves type safety. This modernization aligns well with the PR's SonarQube fixes objective.Also applies to: 74-74
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncherTest.java (1)
69-69: LGTM! Clean removal of unnecessary exception declaration.The
throws Exceptionclause was correctly removed since no checked exceptions are actually thrown by this test method. TheRuntimeExceptionis caught byassertThatThrownBy, making this a proper cleanup that improves code clarity.src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java (1)
178-182: LGTM—cleaner dispatch logic.The refactoring from switch-default to if-else is appropriate for a single-case check and maintains identical behavior.
src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitorTest.java (1)
103-103: LGTM—cosmetic whitespace change.The whitespace modification in the string literal has no impact on test behavior.
src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/FoldingRangesAssert.java (1)
49-49: LGTM - Minor documentation cleanup.The removal of parentheses around "см." in the Javadoc is a harmless stylistic improvement that maintains correct Javadoc syntax.
src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/databind/ParametersDeserializer.java (1)
79-80: LGTM!The reformatted TypeReference instantiation is a stylistic improvement with no behavioral change. This aligns with the PR's Sonar fix objectives.
src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorInformationsAssert.java (1)
43-72: LGTM! Documentation formatting improved.The Javadoc formatting has been cleaned up by removing parentheses around the "см." references, making the parameter documentation more concise and consistent across all three methods.
src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Trees.java (1)
361-373: LGTM! Clean refactor using pattern matching.The change properly leverages the pattern variable
parserRuleContextintroduced by theinstanceofcheck on line 364, eliminating the redundant cast. This is idiomatic Java 16+ code that improves readability without changing behavior.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/VariableSymbolTest.java (1)
97-97: Good refactoring to more idiomatic Java.The change from a string equality check to
isEmpty()is clearer and follows Java best practices for empty string validation.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContextTest.java (1)
114-114: Good cleanup removing unnecessary exception declaration.The method body doesn't throw
IOException, so removing it from the signature is appropriate and improves code clarity.src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/TernaryOperatorNode.java (1)
40-40: Constructor visibility change is correct and safe.Verification confirms no subclasses of
TernaryOperatorNodeexist in the codebase. All instantiation already uses the Lombok-generated factory methodTernaryOperatorNode.create()(ExpressionTreeBuildingVisitor.java:411). The private constructor properly enforces the factory pattern and aligns with immutability best practices for@Valueclasses.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/LineLengthDiagnostic.java (1)
136-140: LGTM! Logic simplification is correct.The collapsed boolean expression correctly implements the filtering logic described in the comment. The short-circuit evaluation efficiently handles both cases: including all comments when trailing comments aren't excluded, or filtering based on whether the comment shares a line with code.
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/CompilerDirectiveKind.java (1)
48-52: LGTM: Theof()method will work correctly with the Lombok-generated getter.The call to
getTokenType()at line 50 will use the Lombok-generated method, maintaining the same functionality as before.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (1)
164-164: LGTM! Unnecessary cast removed.The cast to
RuleNodewas redundant sinceTerminalNode.getParent()already returns aRuleNode. Good cleanup.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DuplicatedInsertionIntoCollectionDiagnostic.java (4)
93-96: Clean migration to Java record.The conversion from Lombok
@Valueto a Java record is well-executed. Records provide the same immutability guarantees while using modern Java syntax, and all seven components are appropriately preserved.
180-186: Record accessor method references correctly updated.All
groupingBykey extractors properly reference the record component accessors (collectionName,methodName,firstParamName) instead of the old Lombok-style getters.
305-305: Record accessor correctly applied.The mapping to extract
callStatementproperly uses the record accessor syntax.
319-337: Cleaner collection creation with.toList().The replacement of
Collectors.toUnmodifiableList()with.toList()is appropriate. Both return unmodifiable lists, and the newer syntax (Java 16+) is more concise. The cached lists are used read-only throughout, so immutability is maintained.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SetPermissionsForNewObjectsDiagnostic.java (1)
71-71: Change verified and approved.The method rename from
isSetForNewObjects()tosetForNewObjects()correctly adapts to the mdclasses library upgrade. Integration tests inSetPermissionsForNewObjectsDiagnosticTest.java(three test methods covering different configurations) exercise this code path by loading metadata and verifying diagnostic output. The tests verify the diagnostic correctly identifies roles, which indirectly confirms the filter logic works as expected. No test changes were needed since the tests operate at the diagnostic output level rather than mocking the underlying API.src/main/java/com/github/_1c_syntax/bsl/languageserver/references/AnnotationReferenceFinder.java (1)
220-222: Accessor method change verified and correct.The conversion from
getValue()tovalue()is confirmed complete and consistent.AnnotationParameterDefinitionis a Java record with avaluefield that auto-generates thevalue()accessor (not a getter-style method). Existing tests already validate this accessor pattern, and no incomplete conversions remain in the codebase.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingCommonModuleMethodDiagnostic.java (4)
65-67: LGTM!The configuration source check is correctly implemented and serves as an appropriate guard clause.
69-71: LGTM!The conversion from getter-style to record-style accessors is correctly implemented with proper method call syntax.
78-104: LGTM!The accessor method updates in this method are correctly implemented. All record-style accessor calls properly include parentheses, maintaining type safety and compilation correctness.
117-119: LGTM!The record definition is well-structured with appropriate component types and access modifier. The conversion from Lombok to a Java record maintains immutability and reduces boilerplate.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/WrongDataPathForFormElementsDiagnostic.java (1)
65-67: Semantic change fromsegments().startsWith()to directstartsWith()is verified and correct.The modification has been confirmed via git history: the change removes the
.segments()call as part of an external dependency API refactoring (records migration). The existing test suite—WrongDataPathForFormElementsDiagnosticTest.javawith three test cases—validates that the new implementation correctly identifies form elements with data paths starting with "~" in both FormModule and non-FormModule scenarios. No behavioral regression detected.src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java (1)
78-79: LGTM! Cleaner assertion syntax.The refactor from a multiline lambda to
allSatisfywith key-value parameters is more concise and idiomatic.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UnreachableCodeDiagnostic.java (1)
145-145: Good use of Stream.toList().Using the built-in
.toList()(Java 16+) is cleaner thanCollectors.toList().src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorPresentationsAssert.java (1)
56-56: LGTM! Minor Javadoc formatting improvement.The Javadoc updates improve consistency by removing unnecessary parentheses.
Also applies to: 71-71
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SmokyTest.java (1)
75-75: LGTM! Simplified test signature.Removing the
throws Exceptiondeclaration improves readability and indicates the test no longer throws checked exceptions.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/AnnotationParameterDefinition.java (1)
28-28: LGTM! Modern Java record migration.Converting from Lombok
@Valueto a Java record is a good modernization. Records provide immutability, conciseness, and built-in equals/hashCode/toString implementations. This aligns with the broader refactoring across the codebase.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexitySecondaryLocation.java (1)
26-26: LGTM! Clean record migration.The conversion from Lombok
@Valueto a Java record improves code clarity and leverages modern Java features. The immutable data carrier pattern is well-suited for records.build.gradle.kts (1)
91-91: I need to gather more information about the mdclasses library and the 0.16.1-rc.1 release status.Prefer stable 0.16.0 over release candidate 0.16.1-rc.1.
A stable version 0.16.0 is available in Maven Central. Using the 0.16.1-rc.1 release candidate introduces unnecessary risk when a stable release exists. Consider updating to 0.16.0 to avoid potential regressions in a non-final version.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TimeoutsInExternalResourcesDiagnostic.java (1)
239-240: No null-safety issues found.The removal of the null-check is safe. The
diagnosticCompatibilityvariable is assigned fromgetCompatibilityMode()(line 209-213), which usesOptional.map().orElseGet()to guarantee a non-nullDiagnosticCompatibilityModereturn value. Both arguments passed toCompatibilityMode.compareTo()at lines 239-240 are provably non-null: the first from the Optional chain and the second from the enum method callDiagnosticCompatibilityMode.UNDEFINED.getCompatibilityMode(). The existing test suite confirms this code path is correct.src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CognitiveComplexityInlayHintSupplier.java (1)
47-50: Accessor rename to methodsComplexitySecondaryLocations() verified and completeAll six call sites across the codebase consistently use the new record-style accessor method. No legacy
getMethodsComplexitySecondaryLocations()references remain.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/ServerContextPopulatedEvent.java (1)
27-32: @serial annotation is fully compatible with Java 17 target.The project is configured to use Java 17 (
sourceCompatibilityandtargetCompatibilityare set toJavaVersion.VERSION_17in build.gradle.kts, lines 153-154). The@Serialannotation, introduced in Java 14 and finalized in Java 16, compiles without issues on Java 17. No verification needed.src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplier.java (1)
47-48: methodsComplexity() refactoring verifiedTests already cover the new method: CognitiveComplexityCodeLensSupplierTest and CyclomaticComplexityCodeLensSupplierTest both validate the new
methodsComplexity()delegation pattern. The old wrapper methods remain functional as safe delegates. No incomplete transitions or stale code detected.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java (1)
422-423: LGTM: Accessor rename aligns with record refactoring.The migration from
getFileComplexity()tofileComplexity()is consistent with the broader conversion of ComplexityData from Lombok POJOs to Java records. The change is purely mechanical and maintains identical behavior.src/main/java/com/github/_1c_syntax/bsl/languageserver/events/LanguageServerInitializeRequestReceivedEvent.java (1)
29-30: LGTM: Modern Java serialization best practice.Adding the
@Serialannotation toserialVersionUIDfollows modern Java conventions and improves compile-time validation of serialization-related fields. This is a good housekeeping improvement.Also applies to: 38-39
src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CyclomaticComplexityInlayHintSupplier.java (1)
49-49: LGTM: Consistent accessor refactoring.The update to
methodsComplexitySecondaryLocations()aligns with the record-based API surface. No logic changes, purely a mechanical rename.src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplier.java (1)
47-47: LGTM: Record accessor update.The change to
methodsComplexity()is consistent with the broader refactoring to Java records. Behavior remains unchanged.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/DescriptionReader.java (1)
129-129: LGTM: Record accessor migration.The update from
getTypes()totypes()reflects the conversion of ParameterDescription to a Java record. This is a straightforward mechanical change with no behavioral impact.src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplierTest.java (1)
70-70: LGTM: Test updated for new API.The test correctly uses
methodsComplexity()to match the refactored API surface. Proper maintenance of test consistency with production code changes.src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplierTest.java (1)
70-70: LGTM: Test API alignment.The test properly uses
methodsComplexity()consistent with the record-based refactoring. Good parallel update to the cognitive complexity test.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputer.java (1)
321-321: LGTM: Record accessor in filter predicate.The change from
getName()toname()is consistent with ParameterDescription's conversion to a record. The filtering logic remains identical.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CognitiveComplexityComputerTest.java (1)
51-51: LGTM! Test correctly updated for record accessor.The test properly uses the new record accessor
methodsComplexity()instead of the previous Lombok-generated getter. The change maintains test correctness while aligning with the ComplexityData record conversion.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingReturnedValueDescriptionDiagnostic.java (1)
104-105: LGTM! Diagnostic correctly updated for record accessors.The code properly uses the new TypeDescription record accessors (
description(),parameters(),name()) instead of the previous Lombok-generated getters. The diagnostic logic remains unchanged and continues to correctly identify types without descriptions.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CyclomaticComplexityComputerTest.java (1)
52-52: LGTM! Test correctly updated for record accessor.The test properly uses the new record accessor
methodsComplexity()instead of the previous Lombok-generated getter, consistent with the ComplexityData record conversion.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/DocumentContextContentChangedEvent.java (1)
27-32: LGTM! Proper serialization annotation added.The
@Serialannotation correctly marks theserialVersionUIDfield, following Java best practices and addressing Sonar recommendations. This improves code clarity and enables compile-time validation of serialization-related members.src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/events/LanguageServerConfigurationChangedEvent.java (1)
27-37: LGTM! Proper serialization annotation added.The
@Serialannotation correctly marks theserialVersionUIDfield, consistent with Java best practices and Sonar recommendations. This change aligns with other serialization improvements across the codebase.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexityData.java (1)
29-36: LGTM! Excellent conversion to Java record.The conversion from Lombok POJO to a Java record is well-executed. Records are ideal for this immutable data carrier, providing:
- Automatic canonical constructor, accessors,
equals(),hashCode(), andtoString()- More compact memory representation
- Clearer intent as an immutable data structure
- Reduced dependency on Lombok
The five components comprehensively capture file-level and method-level complexity metrics with associated secondary locations.
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/TypeDescription.java (1)
29-46: LGTM! Well-documented record conversion.The conversion to a Java record is excellent, with comprehensive Javadoc documentation for each component. The record appropriately models immutable type description metadata. The preserved TODO comment at lines 32-36 indicates awareness of the temporary
linkdesign, which is good for future maintainability.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/ParameterDescription.java (1)
28-37: LGTM! Clean record conversion with documentation.The conversion to a Java record is well-executed with proper Javadoc documentation. The compact single-line record declaration is clean and readable while modeling immutable parameter description metadata. The preserved TODO comment maintains visibility of the temporary
linkdesign decision.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TransferringParametersBetweenClientAndServerDiagnostic.java (2)
171-173: LGTM! Clean conversion to Java record.The conversion from Lombok to a Java record is appropriate for this immutable data carrier. Records provide better compile-time guarantees and are more idiomatic in modern Java.
73-76: LGTM! Accessor usage updated correctly.The method calls have been properly updated to use record-style accessors (
parameterDefinitions(),methodSymbol(),references()) instead of Lombok-generated getters. The logic remains unchanged.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputerTest.java (2)
274-274: LGTM! Exception declaration removed.The
throws IOExceptionclause has been removed from the test method signature. This suggests the underlying implementation no longer throws this checked exception, likely due to the dependency upgrade mentioned in the PR.
162-172: LGTM! Test assertions updated to new API.All test assertions have been consistently updated to use record-style accessors (
name(),optional(),value()) instead of getter methods. The test logic and expectations remain unchanged.Also applies to: 179-179, 186-186, 193-193, 245-246
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CyclomaticComplexityDiagnostic.java (1)
74-74: LGTM! Complexity data accessor calls updated consistently.All accessor calls for complexity data structures have been properly updated to record-style naming (
methodsComplexity(),methodsComplexitySecondaryLocations(),fileCodeBlockComplexity(),fileBlockComplexitySecondaryLocations(),range(),message()). The diagnostic logic remains unchanged.Also applies to: 80-81, 93-93, 126-126, 139-139, 145-146
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/MethodDescriptionTest.java (1)
87-143: LGTM! Comprehensive test updates to new API.All test assertions throughout this file have been systematically updated to use record-style accessors (
name(),types(),description(),parameters(),link()). The test coverage remains thorough, including complex nested type structures. The test expectations are unchanged, confirming the API migration is purely structural.Also applies to: 158-162, 266-427
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CognitiveComplexityDiagnostic.java (1)
81-81: LGTM! Cognitive complexity accessor calls updated consistently.All accessor calls for cognitive complexity data have been updated to record-style naming, mirroring the changes in
CyclomaticComplexityDiagnostic. The diagnostic logic remains unchanged.Also applies to: 87-88, 100-100, 133-133, 146-146, 152-153
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingParameterDescriptionDiagnostic.java (1)
108-108: LGTM! ParameterDescription accessor calls updated correctly.All accessor calls on
ParameterDescriptionhave been updated to use record-style naming (name(),types()). The diagnostic logic for validating parameter descriptions remains unchanged.Also applies to: 123-123, 142-142, 158-158
src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatter.java (1)
242-242: LGTM! Description formatter accessor calls updated comprehensively.All accessor calls for
ParameterDescriptionandTypeDescriptionthroughout the formatter have been updated to record-style naming. The complex formatting logic, including nested structures and hyperlink handling, has been preserved correctly.Also applies to: 283-283, 291-291, 296-296, 299-299, 326-328, 351-351, 356-356, 361-361
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java (1)
49-50: LGTM: accessor migrationUsing symbolOccurrence.symbol() aligns with record accessors.
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndex.java (4)
106-106: LGTM: accessor updateUses location() accessor correctly in filter.
134-137: LGTM: kind filter via record accessorRecords migration applied consistently.
241-251: Rely on non-null invariants for SymbolOccurrence componentsbuildReference assumes non-null location, occurrenceType, and symbol. Ensure the record enforces this (canonical constructor with requireNonNull), else this will NPE under malformed inputs.
Would you like me to add a compact constructor to SymbolOccurrence to enforce non-null?
273-275: LGTM: localizing URI/positionLocal vars reduce repeated derefs and improve readability.
.../com/github/_1c_syntax/bsl/languageserver/aop/sentry/PermissionFilterBeforeSendCallback.java
Show resolved
Hide resolved
...om/github/_1c_syntax/bsl/languageserver/diagnostics/MissingCommonModuleMethodDiagnostic.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java
Show resolved
Hide resolved
...va/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/reporters/data/FileInfo.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java (1)
36-40: Enforce non-null invariants for record componentsAdd a compact constructor with requireNonNull to fail fast and avoid latent NPEs downstream. This also tightens Comparable logic guarantees.
public record SymbolOccurrence( OccurrenceType occurrenceType, Symbol symbol, Location location ) implements Comparable<SymbolOccurrence> { + public SymbolOccurrence { + java.util.Objects.requireNonNull(occurrenceType, "occurrenceType"); + java.util.Objects.requireNonNull(symbol, "symbol"); + java.util.Objects.requireNonNull(location, "location"); + }src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java (1)
42-48: Add compact constructor to enforce non-null componentsGuarantees immutability invariants and avoids NPEs in compareTo/callers. Uses clear parameter names.
public record Symbol( String mdoRef, ModuleType moduleType, String scopeName, SymbolKind symbolKind, String symbolName ) implements Comparable<Symbol> { + public Symbol { + java.util.Objects.requireNonNull(mdoRef, "mdoRef"); + java.util.Objects.requireNonNull(moduleType, "moduleType"); + java.util.Objects.requireNonNull(scopeName, "scopeName"); + java.util.Objects.requireNonNull(symbolKind, "symbolKind"); + java.util.Objects.requireNonNull(symbolName, "symbolName"); + }
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/TypeDescription.java (2)
28-36: Tidy Javadoc: replace TODO with tracked issue and clarify link/hyperlink.Move the TODO to a tracked issue reference to satisfy Sonar and keep Javadoc user‑facing. Minor wording tweak below.
Proposed edit:
- * @param link Если описание параметров содержит только ссылку, то здесь будет ее значение - * <p> - * TODO Временное решение, надо будет продумать в следующем релизе + * @param link Значение гиперссылки, если описание параметров содержит только ссылку. + * <p> + * См. задачу #<ISSUE_ID> для пересмотра подхода в следующем релизе. - * @param isHyperlink Признак того, что параметр является гиперссылкой + * @param isHyperlink Признак того, что описание представлено гиперссылкойPlease replace <ISSUE_ID> with the actual tracker ID.
37-43: Apply defensive copy pattern toparametersfield for true immutability.The verification found no existing mutations of
parameters()in the codebase. However, sinceListis mutable, callers could mutate the returned list (e.g.,.parameters().add(...)), creating a defensive coding concern. Add a compact constructor withList.copyOf()to guarantee immutability and null-safety:public record TypeDescription( String name, String description, List<ParameterDescription> parameters, String link, boolean isHyperlink ) { + public TypeDescription { + parameters = (parameters == null) ? List.of() : List.copyOf(parameters); + } }No legacy getter pattern usage or active mutations detected during verification.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/TypeDescription.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingCommonModuleMethodDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ReservedParameterNamesDiagnostic.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/reporters/data/FileInfo.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputerTest.java(3 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ReservedParameterNamesDiagnostic.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingCommonModuleMethodDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputerTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/AbstractVariableSymbol.java (1)
Builder(121-207)
⏰ 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). (21)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: Analyse
- GitHub Check: build
- GitHub Check: Benchmark
- GitHub Check: Analyze the repo with CodeSee
🔇 Additional comments (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/reporters/data/FileInfo.java (1)
51-51: Refactoring is correct; centralizes MDO reference extraction with proper null-safety.The
MdoRefBuilder.getMdoRef()method properly handles null cases through Optional chaining and an explicit fallback todocumentContext.getUri().toString(), ensuring the method always returns a non-null String. This centralization eliminates duplicate inline logic and is consistent with the PR's optimization goals.src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java (2)
25-25: Do not remove the Nullable import—@nullable is still in useThe
@Nullableannotation is still present on thecompareTomethod parameter at line 43, making the import necessary. The suggested removal is not applicable.Likely an incorrect or invalid review comment.
35-40: No action required — Lombok version is compatibleThe project uses the FreeFair Gradle plugin v9.0.0, which defaults to Lombok 1.18.38. Lombok supports @builder for records since v1.18.20, so the codebase already meets the compatibility requirement for
@Builderon records.src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java (2)
27-27: Let me verify if there are any other uses of@Nullablein the entire Symbol.java file to confirm the import is actually needed:Based on the verification results, the review comment's assumption is incorrect. The execution of the shell script confirmed that@Nullableis still actively used on line 57 of the Symbol.java file in thecompareTomethod parameter. Since the annotation is still in use, the import at line 27 is not unused and should NOT be removed.Keep the Nullable import—@nullable is still used in the compareTo method
The suggestion to drop this import is based on an incorrect premise. The verification shows
@Nullableremains on line 57 in thecompareTomethod signature, so the import is still required.Likely an incorrect or invalid review comment.
50-54: Verify GenericInterner characteristics before adding JavadocThe
GenericInternerclass is imported fromcom.github._1c_syntax.utilsand is not present in this repository. Unable to inspect its implementation to confirm reference type (weak/strong), thread-safety model, or locking strategy. Verify that:
- GenericInterner documentation/source is accessible in the external
_1c_syntax.utilsmodule- Its characteristics (weak refs, thread-safety) should be documented based on actual implementation
- Adding Javadoc to the
INTERNERfield is appropriate once implementation details are confirmed
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (1)
140-146: Consider more idiomatic Optional handling.While calling
.get()is safe here (we're in theelseblock after checkingisEmpty()), a more idiomatic approach would avoid explicit.get()calls:} else { - boolean isSelfAssign = refContextInsideDefAssign - .map(RewriteMethodParameterDiagnostic::isVarNameOnlyIntoExpression) - .get(); + boolean isSelfAssign = refContextInsideDefAssign + .map(RewriteMethodParameterDiagnostic::isVarNameOnlyIntoExpression) + .orElse(false); if (isSelfAssign && references.size() > COUNT_OF_PAIR_FOR_SELF_ASSIGN) { return isOverwrited(references.subList(COUNT_OF_PAIR_FOR_SELF_ASSIGN, references.size())); } }Note: Since we're in the else block,
.orElse(false)would never actually returnfalse, but this pattern is more consistent with Optional best practices and avoids the code smell of calling.get().
📜 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/diagnostics/RewriteMethodParameterDiagnostic.java(2 hunks)
⏰ 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). (21)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: Analyse
- GitHub Check: build
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build
- GitHub Check: Benchmark
- GitHub Check: Analyze the repo with CodeSee
🔇 Additional comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (1)
153-166: ---No action required—review comment is based on incorrect assumptions.
The compilation verification confirms the code is type-safe. The gradle build completed with
--warning-mode=alland found no errors, warnings, or unchecked cast warnings. This contradicts the assumption that removing the explicitRuleNode.class::castwould cause a type mismatch. The ANTLR4 API ensures thatTerminalNode.getParent()returns a type compatible withOptional<RuleNode>, making the cast unnecessary and the current implementation correct.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java (1)
29-29: Avoid @nullable on Comparable#compareTo; drop null acceptance.Comparable’s contract assumes non‑null argument. Remove @nullable and the null branch; callers should not pass null.
-import edu.umd.cs.findbugs.annotations.Nullable; @@ - public int compareTo(@Nullable MethodSymbol other) { - if (other == null) { - return 1; - } + public int compareTo(MethodSymbol other) {Also applies to: 126-130
📜 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/context/symbol/MethodSymbol.java(4 hunks)
⏰ 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). (20)
- GitHub Check: build
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: Analyse
- GitHub Check: build
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: Benchmark
- GitHub Check: Analyze the repo with CodeSee
🔇 Additional comments (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java (3)
50-50: Good call making MethodSymbol Comparable. Ensure natural order matches equals.Comparable is appropriate here; just align compareTo with equals (see next comment).
60-67: No breaking changes detected — private getter restriction is safe.Verification found no external usages of MethodSymbol's getters. All calls to
getStartLine(),getStartCharacter(),getEndLine(),getEndCharacter(), and sub-name accessors occur within MethodSymbol.java itself (lines 134–140 in the compareTo method), where private access is appropriate. Other files use identically-named getters on different classes (FoldingRange, PhysicalLocation), not MethodSymbol.
125-142: Based on verification, the review comment contains both valid and invalid concerns:Valid:
- compareTo includes fields (startLine, startCharacter, endLine, endCharacter) not included in equals/hashCode, which violates Comparable contract consistency
- symbolKind comparison is redundant (always SymbolKind.Method per @Builder.Default)
Invalid/Problematic:
- The concern about DocumentContext requiring Comparable is incorrect—DocumentContext implements Comparable (verified line 86), so thenComparing(MethodSymbol::getOwner) compiles without issue
- The suggested fix removes null safety by dropping the null check, but the parameter is annotated @nullable—this would introduce a potential NullPointerException
- The concern about TreeSet/TreeMap undefined behavior is theoretical—no TreeSet or TreeMap with MethodSymbol as key exists in the codebase; all such usages found are for String types
- The suggested diff contradicts the @nullable annotation on the parameter
Critical issue with suggested fix: Removing the null check while keeping @nullable parameter violates Nullable contract and contradicts DocumentContext's own null-handling pattern (it uses @nonnull and no null check).
Ensure compareTo/equals consistency by comparing only declared @EqualsAndHashCode.Include fields.
The inconsistency is valid: compareTo includes startLine, startCharacter, endLine, endCharacter which are not in equals/hashCode. Also, symbolKind comparison is redundant (always Method).
Fix:
@Override public int compareTo(@Nullable MethodSymbol other) { if (other == null) { return 1; } return java.util.Comparator.comparing(MethodSymbol::getName) .thenComparing(MethodSymbol::getOwner) .thenComparingInt(MethodSymbol::getSubNameLine) .thenComparingInt(MethodSymbol::getSubNameStartCharacter) .thenComparingInt(MethodSymbol::getSubNameEndCharacter) .compare(this, other); }Remove symbolKind and full range (startLine/Character/endLine/endCharacter) comparisons. Keep null check since parameter is @nullable. Use thenComparingInt for primitive int comparisons (subName fields).
theshadowco
force-pushed
the
feature/optimizedMem
branch
from
989f08d to
1950f9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/PermissionFilterBeforeSendCallback.java (1)
34-35: Reconsider @nonnull on Hint
hintisn’t used; enforcing non-null adds risk if SDK passes null. Keep@NonNullonevent, drop it onhintunless the SDK guarantees non-null hints across all call paths.If keeping it, please confirm the exact Sentry Java SDK version and its
BeforeSendCallbackcontract.Quick tweak:
- public SentryEvent execute(@NonNull SentryEvent event, @NonNull Hint hint) { + public SentryEvent execute(@NonNull SentryEvent event, Hint hint) {Also applies to: 74-80
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java (1)
69-71: Fix NPE and remove empty sets when deleting occurrencesoccurrencesToSymbols.get(...).remove(...) will NPE if no set exists for the key, and empty sets remain in the map after removals. Use computeIfPresent and drop empty sets.
Apply minimal safe fix:
- public void deleteAll(Set<SymbolOccurrence> symbolOccurrences) { - symbolOccurrences.forEach(symbolOccurrence -> - occurrencesToSymbols.get(symbolOccurrence.symbol()).remove(symbolOccurrence) - ); - } + public void deleteAll(Set<SymbolOccurrence> symbolOccurrences) { + symbolOccurrences.forEach(symbolOccurrence -> + occurrencesToSymbols.computeIfPresent(symbolOccurrence.symbol(), (s, set) -> { + set.remove(symbolOccurrence); + return set.isEmpty() ? null : set; // null => remove mapping + }) + ); + }Optional optimization: reduce map lookups by grouping by symbol.
+ public void deleteAll(Set<SymbolOccurrence> symbolOccurrences) { + var bySymbol = new java.util.HashMap<Symbol, java.util.Set<SymbolOccurrence>>(); + for (var occ : symbolOccurrences) { + bySymbol.computeIfAbsent(occ.symbol(), k -> new java.util.HashSet<>()).add(occ); + } + bySymbol.forEach((symbol, toRemove) -> + occurrencesToSymbols.computeIfPresent(symbol, (s, set) -> { + set.removeAll(toRemove); + return set.isEmpty() ? null : set; + }) + ); + }src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java (2)
36-40: Add compact constructor to enforce non-null invariants.The record components can be null, which may cause NPEs in
compareToand other downstream code. Add validation in a compact constructor.Apply this diff to add non-null validation:
) implements Comparable<SymbolOccurrence> { + public SymbolOccurrence { + java.util.Objects.requireNonNull(occurrenceType, "occurrenceType"); + java.util.Objects.requireNonNull(symbol, "symbol"); + java.util.Objects.requireNonNull(location, "location"); + }Based on past review comments.
43-54: Comparable contract: reject null arguments.The
compareTomethod should not accept null per theComparablecontract. Remove@Nullableand let it throwNullPointerExceptionfor null arguments.Apply this diff:
- public int compareTo(@Nullable SymbolOccurrence other) { - if (other == null) { - return 1; - } + public int compareTo(SymbolOccurrence other) { + java.util.Objects.requireNonNull(other, "other");Based on past review comments.
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java (2)
42-48: Add compact constructor to enforce non-null invariants.The record components may be null, which can cause NPEs in
compareToand other code. Add validation in a compact constructor.Apply this diff to add non-null validation:
) implements Comparable<Symbol> { + public Symbol { + java.util.Objects.requireNonNull(mdoRef, "mdoRef"); + java.util.Objects.requireNonNull(moduleType, "moduleType"); + java.util.Objects.requireNonNull(scopeName, "scopeName"); + java.util.Objects.requireNonNull(symbolKind, "symbolKind"); + java.util.Objects.requireNonNull(symbolName, "symbolName"); + }Based on past review comments.
57-68: Comparable contract: reject null arguments.The
compareTomethod should not accept null per theComparablecontract. Remove@Nullableand let it throwNullPointerExceptionfor null arguments.Apply this diff:
- public int compareTo(@Nullable Symbol other) { - if (other == null) { - return 1; - } + public int compareTo(Symbol other) { + java.util.Objects.requireNonNull(other, "other");Based on past review comments.
🧹 Nitpick comments (8)
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java (2)
144-152: Consider adding document setup and verifying rename functionality.The test operates on a potentially unopened document and only asserts non-null, unlike
testDiagnosticsKnownFile(line 117) which callsdidOpen()first. Consider:
- Opening the document with
doOpen()before testing rename- Verifying the actual rename result (e.g., checking
WorkspaceEditcontains expected changes)Apply this diff to improve the test:
@Test -void testRename() { +void testRename() throws IOException { + doOpen(); + var params = new RenameParams(); params.setTextDocument(getTextDocumentIdentifier()); params.setPosition(new Position(0, 16)); + params.setNewName("newName"); var result = textDocumentService.rename(params); assertThat(result).isNotNull(); + assertThat(result.getChanges()).isNotEmpty(); }
155-163: Consider adding document setup and verifying prepareRename functionality.Similar to
testRename, this test operates without opening the document first and only checks for non-null. Consider:
- Opening the document with
doOpen()before testing prepareRename- Verifying the result contains the expected range or placeholder
Apply this diff to improve the test:
@Test -void testRenamePrepare() { +void testRenamePrepare() throws IOException { + doOpen(); + var params = new PrepareRenameParams(); params.setTextDocument(getTextDocumentIdentifier()); params.setPosition(new Position(0, 16)); var result = textDocumentService.prepareRename(params); assertThat(result).isNotNull(); + // Verify the result is an Either with left (Range) or right (PrepareRenameResult) + assertThat(result.isLeft() || result.isRight()).isTrue(); }src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/databind/ParametersDeserializer.java (1)
79-82: Simplify deserialization; avoid extra TypeFactory danceUse convertValue with TypeReference (or use the parser’s codec) for clarity and to honor mapper modules.
- JavaType type = mapper.getTypeFactory().constructType(new TypeReference<Map<String, Object>>() { - }); - parameterConfiguration = mapper.readValue(mapper.treeAsTokens(parameterConfig), type); + parameterConfiguration = mapper.convertValue( + parameterConfig, + new TypeReference<Map<String, Object>>() {} + );Or, prefer the parser’s codec to keep configuration consistent:
- var mapper = new ObjectMapper(); + var mapper = (ObjectMapper) p.getCodec();src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/TypeDescription.java (1)
27-44: Make record components defensively safeConsider validating non-null components and copying the list to enforce immutability.
-public record TypeDescription( - String name, - String description, - List<ParameterDescription> parameters, - String link, - boolean isHyperlink -) { +public record TypeDescription( + String name, + String description, + List<ParameterDescription> parameters, + String link, + boolean isHyperlink +) { + public TypeDescription { + // null-safety + if (name == null) throw new NullPointerException("name"); + if (parameters == null) parameters = List.of(); + // defensive copy + parameters = List.copyOf(parameters); + } }src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/ParameterDescription.java (2)
36-36: Harden record invariants: null-safety + defensive copiesEnsure name is non-null and types is immutable to callers.
Apply:
@@ package com.github._1c_syntax.bsl.languageserver.context.symbol.description; import java.util.List; +import java.util.Objects; @@ -public record ParameterDescription(String name, List<TypeDescription> types, String link, boolean isHyperlink) { +public record ParameterDescription(String name, List<TypeDescription> types, String link, boolean isHyperlink) { + public ParameterDescription { + Objects.requireNonNull(name, "name"); + types = (types == null) ? List.of() : List.copyOf(types); + } }
26-35: Javadoc formatting nitThe TODO line alignment is off; consider reflowing or moving to an implNote to keep API docs tidy.
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexitySecondaryLocation.java (1)
35-35: Guard against nulls in value objectRange/message should never be null; make it explicit.
Apply:
@@ package com.github._1c_syntax.bsl.languageserver.context.computer; import org.eclipse.lsp4j.Range; +import java.util.Objects; @@ -public record ComplexitySecondaryLocation(Range range, String message) { +public record ComplexitySecondaryLocation(Range range, String message) { + public ComplexitySecondaryLocation { + Objects.requireNonNull(range, "range"); + Objects.requireNonNull(message, "message"); + } }src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexityData.java (1)
41-47: Make ComplexityData truly immutable and null-safeCopy incoming collections and default nulls to empty; wrap nested lists.
Apply:
@@ package com.github._1c_syntax.bsl.languageserver.context.computer; import com.github._1c_syntax.bsl.languageserver.context.symbol.MethodSymbol; import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; @@ public record ComplexityData( int fileComplexity, int fileCodeBlockComplexity, List<ComplexitySecondaryLocation> fileBlockComplexitySecondaryLocations, Map<MethodSymbol, Integer> methodsComplexity, Map<MethodSymbol, List<ComplexitySecondaryLocation>> methodsComplexitySecondaryLocations ) { + public ComplexityData { + fileBlockComplexitySecondaryLocations = + fileBlockComplexitySecondaryLocations == null ? List.of() : List.copyOf(fileBlockComplexitySecondaryLocations); + + methodsComplexity = methodsComplexity == null ? Map.of() : Map.copyOf(methodsComplexity); + + methodsComplexitySecondaryLocations = + methodsComplexitySecondaryLocations == null + ? Map.of() + : methodsComplexitySecondaryLocations.entrySet().stream() + .collect(Collectors.toUnmodifiableMap( + Map.Entry::getKey, + e -> { + var v = e.getValue(); + return v == null ? List.of() : List.copyOf(v); + } + )); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
build.gradle.kts(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncher.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/PermissionFilterBeforeSendCallback.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/SentryScopeConfigurer.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/databind/ParametersDeserializer.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/events/LanguageServerConfigurationChangedEvent.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexityData.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexitySecondaryLocation.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputer.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/DocumentContextContentChangedEvent.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/ServerContextPopulatedEvent.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/AnnotationParameterDefinition.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/CompilerDirectiveKind.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/DescriptionReader.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/ParameterDescription.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/TypeDescription.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/variable/VariableDescription.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CognitiveComplexityDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CyclomaticComplexityDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DuplicatedInsertionIntoCollectionDiagnostic.java(6 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/LineLengthDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingCommonModuleMethodDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingParameterDescriptionDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingReturnedValueDescriptionDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ReservedParameterNamesDiagnostic.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SetPermissionsForNewObjectsDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TimeoutsInExternalResourcesDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TransferringParametersBetweenClientAndServerDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UnreachableCodeDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/WrongDataPathForFormElementsDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/events/LanguageServerInitializeRequestReceivedEvent.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatter.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/AbstractComplexityInlayHintSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CognitiveComplexityInlayHintSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CyclomaticComplexityInlayHintSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/SourceDefinedMethodCallInlayHintSupplier.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/AnnotationReferenceFinder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndex.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Location.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/reporters/data/FileInfo.java(2 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(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/TernaryOperatorNode.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncherTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/aop/measures/MeasureCollectorTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplierTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplierTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContextTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CognitiveComplexityComputerTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CyclomaticComplexityComputerTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputerTest.java(3 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/VariableSymbolTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/MethodDescriptionTest.java(6 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/EmptyStatementDiagnosticTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SmokyTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ThisObjectAssignDiagnosticTest.java(0 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatterTest.java(0 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorInformationsAssert.java(3 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorPresentationsAssert.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/FoldingRangesAssert.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitorTest.java(1 hunks)
💤 Files with no reviewable changes (9)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ReservedParameterNamesDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Location.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatterTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/SentryScopeConfigurer.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ThisObjectAssignDiagnosticTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/SourceDefinedMethodCallInlayHintSupplier.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java
🚧 Files skipped from review as they are similar to previous changes (32)
- src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/EmptyStatementDiagnosticTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TransferringParametersBetweenClientAndServerDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Trees.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/events/LanguageServerConfigurationChangedEvent.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorPresentationsAssert.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/reporters/data/FileInfo.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CyclomaticComplexityInlayHintSupplier.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitorTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CognitiveComplexityDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingReturnedValueDescriptionDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/AnnotationReferenceFinder.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SmokyTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/variable/VariableDescription.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CognitiveComplexityComputerTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputer.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/WrongDataPathForFormElementsDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplier.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplierTest.java
- build.gradle.kts
- src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/AnnotationParameterDefinition.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/DocumentContextContentChangedEvent.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UnreachableCodeDiagnostic.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplierTest.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/FoldingRangesAssert.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/events/LanguageServerInitializeRequestReceivedEvent.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/AbstractComplexityInlayHintSupplier.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncher.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingCommonModuleMethodDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TimeoutsInExternalResourcesDiagnostic.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/AbstractVariableSymbol.java (1)
Builder(121-207)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CyclomaticComplexityDiagnostic.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/RelatedInformation.java (1)
RelatedInformation(36-54)
⏰ 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). (20)
- GitHub Check: build
- GitHub Check: Analyse
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: Benchmark
🔇 Additional comments (26)
src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncherTest.java (1)
69-69: Good cleanup removing unnecessary exception declaration.The
throws Exceptionclause was unnecessary since no checked exceptions are thrown outside theassertThatThrownBylambda. This aligns with the PR's Sonar fixes objective.src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorInformationsAssert.java (1)
43-79: LGTM! Documentation formatting improvements.The Javadoc changes consistently remove parentheses around "см." across all three methods, resulting in cleaner parameter documentation. The changes align well with the PR's cleanup objectives.
src/test/java/com/github/_1c_syntax/bsl/languageserver/aop/measures/MeasureCollectorTest.java (1)
25-25: LGTM! Modern AssertJ assertion pattern.The migration from
.asList()to.asInstanceOf(InstanceOfAssertFactories.LIST)improves type safety and follows current AssertJ best practices. The necessary import has been correctly added.Also applies to: 74-74
src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/TernaryOperatorNode.java (1)
42-47: Good practice: enforcing factory method usage.Making the constructor private when using Lombok's
staticConstructoris a best practice that ensures all instantiation goes through thecreate()factory method. Verification confirms no breaking changes—the codebase already uses the factory pattern correctly, with no direct constructor calls or subclasses.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/CompilerDirectiveKind.java (2)
25-25: Clean refactoring to Lombok-generated getter.The addition of the
@Getterannotation and corresponding import correctly replaces the manual getter method, reducing boilerplate while maintaining the same functionality.Also applies to: 41-42
48-52: Correct usage of Lombok-generated getter.The
of()method correctly callsgetTokenType(), which will be generated by Lombok at compile time. No changes needed here.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/VariableSymbolTest.java (1)
97-97: Good improvement using.isEmpty()instead of.equals("").This aligns with SonarQube best practices (rule S1155) and is more idiomatic Java.
The
VariableDescriptionclass uses Lombok's@Valueannotation, which generates traditional getters (getDescription(), etc.), not fluent accessors. The test file correctly usesgetDescription()throughout and requires no changes.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/ServerContextPopulatedEvent.java (1)
27-38: Good modernization: @serial on serialVersionUIDCorrect usage; aligns with JDK 14+ best practices and other event classes. No behavior change.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/LineLengthDiagnostic.java (1)
136-140: Predicate simplification looks correctLogic matches the comment and prior behavior; cleaner and equivalent.
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContextTest.java (2)
114-121: LGTM: no longer declaring IOExceptionMethod doesn’t throw; signature cleanup is appropriate.
152-158: LGTM: style-only tweakIndent/format change inside lambda; no behavior change.
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SetPermissionsForNewObjectsDiagnostic.java (1)
70-76: No critical issues found—accessor is confirmed working correctlyThe code at line 71 uses
setForNewObjects()in a filter context, which requires a boolean return value. The fact that this code compiles and executes proves the method is correctly functioning as a boolean accessor, despite the unconventionalset*naming convention from mdclasses.The optional refactor to trim/normalize role names in
getSetFromString()is a nice-to-have for robustness but not required.src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplier.java (1)
45-48: LGTM: uses record accessor methodsComplexity()Matches ComplexityData record API; no functional change.
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/DescriptionReader.java (1)
129-130: LGTM: switch to types() accessorConsistent with ParameterDescription as a record; result type unchanged.
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java (1)
429-431: Verification complete: accessor switch to record API is correct and completeNo legacy
getFileComplexity()calls remain in the codebase. ThegetMethodsComplexity()usages in code lens suppliers are legitimate accessor calls for the record component—not leftover legacy methods. The migration from traditional getters to record accessors is consistent and properly applied.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CyclomaticComplexityComputerTest.java (1)
52-53: No issues found—test correctly uses methodsComplexity() accessorThe test has been properly updated to call the accessor on the
ComplexityDatarecord, which is automatically generated for themethodsComplexityfield. This matches the production API, which already uses this accessor in four locations across the codebase. Assertions remain intact.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputerTest.java (2)
179-179: Consistent accessor updates across annotation parameter checks — OKvalue() usage is consistent with the refactor.
Also applies to: 186-186, 193-193
272-275: Removing checked exception from testOwner is appropriateNo IO here; cleaner signature.
As PR checklist items for tests/precommit aren’t checked, please run them locally.
src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CognitiveComplexityInlayHintSupplier.java (1)
47-50: Accessor rename verified—no stale usages foundGrep confirmed the old
getMethodsComplexitySecondaryLocations()method has no remaining references in the codebase. The newmethodsComplexitySecondaryLocations()accessor is properly defined and consistently used across all four call sites (InlayHintSupplier and Diagnostic classes).src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndex.java (1)
112-114: LGTM! Accessor migration is consistent.The migration from Lombok-generated getters to record component accessors (
location(),symbol(),occurrenceType(),mdoRef(),moduleType(),symbolName(), etc.) is correctly applied throughout the file. The logic remains unchanged.Also applies to: 140-143, 247-256, 260-275, 279-293
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DuplicatedInsertionIntoCollectionDiagnostic.java (2)
93-96: LGTM! Lombok @value class correctly converted to record.The GroupingData nested class has been successfully migrated from a Lombok-annotated class to a private record with all components properly declared.
319-319: LGTM! Simplified collector usage.The change from
Collectors.toUnmodifiableList()to.toList()is appropriate, as.toList()(Java 16+) returns an unmodifiable list by default.Also applies to: 328-328, 337-337
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CyclomaticComplexityDiagnostic.java (1)
74-87: LGTM! Accessor migration is consistent.All method calls have been correctly updated to use the new fluent accessors from the ComplexityData and ComplexitySecondaryLocation records (
methodsComplexity(),methodsComplexitySecondaryLocations(),fileCodeBlockComplexity(),range(),message()).Also applies to: 93-104, 126-157
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/MethodDescriptionTest.java (1)
87-143: LGTM! Test assertions updated consistently.All test assertions have been correctly updated to use the new fluent accessors (
name(),types(),description(),parameters(),link()) instead of the previous getter-style methods. The test logic and coverage remain unchanged.Also applies to: 157-162, 265-427
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingParameterDescriptionDiagnostic.java (1)
108-112: LGTM! Accessor migration is consistent.All usages of
ParameterDescriptionhave been correctly updated to use the new fluent accessors (name(),types()) instead of the previous getter methods.Also applies to: 123-125, 142-142, 158-158
src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatter.java (1)
242-244: LGTM! Accessor migration is consistent.All method calls have been correctly updated to use the new fluent accessors from
ParameterDescriptionandTypeDescriptionrecords (types(),name(),description(),parameters(),link()).Also applies to: 283-287, 289-303, 319-334, 349-364
...a/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java
Show resolved
Hide resolved
...java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputerTest.java
Show resolved
Hide resolved
theshadowco
force-pushed
the
feature/optimizedMem
branch
from
7473b1f to
d4eade6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/PermissionFilterBeforeSendCallback.java (1)
74-74: Remove@NonNullfrom thehintparameter to avoid unnecessary overhead and potential NPE.This concern was already raised in a previous review: the
hintparameter is marked@NonNullbut is never used in the method body (lines 75-79), creating unnecessary null-check overhead. Additionally, depending on the Sentry Java SDK version and call scenarios, the hint may be passed as null, which would cause a Lombok-enforcedNullPointerException.Consider removing
@NonNullfrom thehintparameter while keeping it onevent(which is actually used):- public SentryEvent execute(@NonNull SentryEvent event, @NonNull Hint hint) { + public SentryEvent execute(@NonNull SentryEvent event, Hint hint) {src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingCommonModuleMethodDiagnostic.java (1)
107-115: Critical: Fix record accessor syntax on line 109.Line 109 uses direct field access
callData.existsinstead of the record accessor methodcallData.exists(). This will cause a compilation error since Java records only expose accessor methods, not public fields.Apply this diff:
private void fireIssue(CallData callData) { final String message; - if (!callData.exists) { + if (!callData.exists()) { message = info.getMessage(callData.methodName(), callData.moduleName()); } else { message = info.getResourceString(PRIVATE_METHOD_MESSAGE, callData.methodName(), callData.moduleName()); } diagnosticStorage.addDiagnostic(callData.moduleMethodRange(), message); }
🧹 Nitpick comments (8)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/CompilerDirectiveKind.java (1)
41-42: LGTM! Lombok-generated getter replaces manual implementation.The
@Getterannotation will generate the samepublic int getTokenType()method that was previously implemented manually, maintaining the existing API. The usage at line 50 remains compatible.Optional consideration: While functionally correct, using Lombok on enum fields is less common since enums are typically simple and self-contained. However, this is a valid choice that reduces boilerplate and is consistent if Lombok is already used throughout the codebase.
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java (2)
78-79: Consider applying this pattern consistently across all similar tests.The refactored
allSatisfyapproach is cleaner and more idiomatic than the previousassertThatCode().doesNotThrowAnyException()pattern. However, other similar tests in this file (testAddDiagnosticsHaveDiagnosticNameat lines 64-66,testAllDiagnosticsHaveDiagnosticMessageat lines 71-73, andtestAllDiagnosticsHaveTagsat lines 84-89) still use the older pattern, creating inconsistency.Consider either:
- Reverting this change to match the existing pattern, OR
- Applying the
allSatisfypattern to the other similar tests for consistencyIf you'd like to standardize on
allSatisfy, here's an example fortestAddDiagnosticsHaveDiagnosticName:- assertThatCode(() -> diagnosticInfos.values().forEach(diagnosticInfo - -> assertThat(diagnosticInfo.getName()).isNotEmpty())) - .doesNotThrowAnyException(); + assertThat(diagnosticInfos) + .allSatisfy((key, diagnosticInfo) -> assertThat(diagnosticInfo.getName()).isNotEmpty());
79-79: Unused parameterkey.The
keyparameter in the lambda is declared but never used. Consider using an underscore to indicate it's intentionally unused, if your Java version supports it (Java 22+), or simply acknowledge that it's required by theBiConsumersignature.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (2)
139-141: Simplify Optional flow; avoidmap(...).get()on Optional.You already checked presence above. Call the predicate directly on the unwrapped
RuleNodefor clarity and to keep static analyzers calm.- boolean isSelfAssign = refContextInsideDefAssign - .map(RewriteMethodParameterDiagnostic::isVarNameOnlyIntoExpression) - .get(); + boolean isSelfAssign = isVarNameOnlyIntoExpression(refContextInsideDefAssign.get());
130-149: Minor naming nit: “isOverwrited” → “isOverwritten”.Private method, so safe to rename for readability when convenient.
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java (2)
66-80: Avoid generating private getters; use field lambdas in comparatorThese getters exist only for the comparator method refs. Switching to field lambdas (thenComparingInt(m -> m.subNameLine), etc.) removes the need to generate extra methods.
Apply this diff to suppress getter generation for internal fields:
- @Getter(AccessLevel.PRIVATE) + @Getter(AccessLevel.NONE) int startLine; - @Getter(AccessLevel.PRIVATE) + @Getter(AccessLevel.NONE) int startCharacter; - @Getter(AccessLevel.PRIVATE) + @Getter(AccessLevel.NONE) int endLine; - @Getter(AccessLevel.PRIVATE) + @Getter(AccessLevel.NONE) int endCharacter; - @Getter(AccessLevel.PRIVATE) + @Getter(AccessLevel.NONE) int subNameLine; - @Getter(AccessLevel.PRIVATE) + @Getter(AccessLevel.NONE) int subNameStartCharacter; - @Getter(AccessLevel.PRIVATE) + @Getter(AccessLevel.NONE) int subNameEndCharacter;
29-29: Consider NonNull for compareTo parameterAccepting null in compareTo is unconventional; typical practice is to disallow and let callers use null-friendly Comparators when needed.
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/variable/VariableDescription.java (1)
99-109: Add a null guard for ParameterDescription in ctorDefensive check avoids surprising NPEs at call sites and matches style used elsewhere (requireNonNull).
Apply:
- public VariableDescription(ParameterDescription param) { + public VariableDescription(ParameterDescription param) { - description = ""; + requireNonNull(param, "param"); + description = ""; deprecationInfo = ""; deprecated = false; purposeDescription = ""; range = Ranges.create(); - link = param.link(); + link = param.link(); trailingDescription = Optional.empty(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
build.gradle.kts(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncher.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/PermissionFilterBeforeSendCallback.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/SentryScopeConfigurer.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/databind/ParametersDeserializer.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/events/LanguageServerConfigurationChangedEvent.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexityData.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexitySecondaryLocation.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputer.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/DocumentContextContentChangedEvent.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/ServerContextPopulatedEvent.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/AnnotationParameterDefinition.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/CompilerDirectiveKind.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/DescriptionReader.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/ParameterDescription.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/TypeDescription.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/variable/VariableDescription.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CognitiveComplexityDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CyclomaticComplexityDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DuplicatedInsertionIntoCollectionDiagnostic.java(6 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/LineLengthDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingCommonModuleMethodDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingParameterDescriptionDiagnostic.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingReturnedValueDescriptionDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ReservedParameterNamesDiagnostic.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SetPermissionsForNewObjectsDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TimeoutsInExternalResourcesDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TransferringParametersBetweenClientAndServerDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UnreachableCodeDiagnostic.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/WrongDataPathForFormElementsDiagnostic.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/events/LanguageServerInitializeRequestReceivedEvent.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatter.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/AbstractComplexityInlayHintSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CognitiveComplexityInlayHintSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CyclomaticComplexityInlayHintSupplier.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/SourceDefinedMethodCallInlayHintSupplier.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/AnnotationReferenceFinder.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndex.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Location.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/reporters/data/FileInfo.java(2 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(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/TernaryOperatorNode.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncherTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/aop/measures/MeasureCollectorTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplierTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplierTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContextTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CognitiveComplexityComputerTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CyclomaticComplexityComputerTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputerTest.java(3 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/VariableSymbolTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/MethodDescriptionTest.java(6 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/EmptyStatementDiagnosticTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SmokyTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ThisObjectAssignDiagnosticTest.java(0 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatterTest.java(0 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorInformationsAssert.java(3 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorPresentationsAssert.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/FoldingRangesAssert.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitorTest.java(1 hunks)
💤 Files with no reviewable changes (9)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Location.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/AbstractRunTestsCodeLensSupplier.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ThisObjectAssignDiagnosticTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/SourceDefinedMethodCallInlayHintSupplier.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RefOveruseDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/SentryScopeConfigurer.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ReservedParameterNamesDiagnostic.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatterTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/AnnotationParamSymbolMarkupContentBuilder.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorPresentationsAssert.java
🚧 Files skipped from review as they are similar to previous changes (27)
- src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplierTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingReturnedValueDescriptionDiagnostic.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/EmptyStatementDiagnosticTest.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java
- build.gradle.kts
- src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncherTest.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/aop/measures/MeasureCollectorTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CyclomaticComplexityInlayHintSupplier.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/FoldingRangesAssert.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputerTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/reporters/data/FileInfo.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/LineLengthDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/AnnotationReferenceFinder.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitorTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/databind/ParametersDeserializer.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CognitiveComplexityComputerTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/AbstractComplexityInlayHintSupplier.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/UnreachableCodeDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CognitiveComplexityDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TimeoutsInExternalResourcesDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndex.java
- src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContextTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/DocumentContextContentChangedEvent.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/CyclomaticComplexityDiagnostic.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-27T08:05:10.053Z
Learnt from: theshadowco
PR: 1c-syntax/bsl-language-server#3555
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java:163-166
Timestamp: 2025-10-27T08:05:10.053Z
Learning: В проекте bsl-language-server используется собственная версия ANTLR4, в которой интерфейс TerminalNode переопределяет метод getParent() с возвращаемым типом RuleNode вместо ParseTree (как в стандартной версии ANTLR4).
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java
📚 Learning: 2025-04-18T22:42:30.042Z
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:88-97
Timestamp: 2025-04-18T22:42:30.042Z
Learning: В реализации ANTLR, если метод `getChildCount()` возвращает значение больше 0, то поле `children` гарантированно не равно null.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/AbstractVariableSymbol.java (1)
Builder(121-207)
🔇 Additional comments (41)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLLSPLauncher.java (1)
134-136: LGTM! Clean migration to modern Java Stream API.The replacement of
.collect(Collectors.toList())with.toList()is a good simplification. SinceunmatchedArgsis only read after creation (checked for emptiness and iterated), the unmodifiable nature of the returned list is appropriate and enhances safety.src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SmokyTest.java (1)
75-75: LGTM! Unnecessary exception declaration removed.The
throws Exceptionclause was unnecessary since the method body doesn't throw any checked exceptions directly—assertThatThrownBy()handles exception catching internally. This cleanup improves code quality and aligns with best practices.src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/Trees.java (1)
393-405: Good refactor using pattern matching.Using the
parserRuleContextpattern variable directly eliminates the redundant cast and makes the code cleaner. This is idiomatic modern Java.src/main/java/com/github/_1c_syntax/bsl/languageserver/configuration/events/LanguageServerConfigurationChangedEvent.java (1)
27-28: LGTM! Proper use of @serial annotation.The addition of the
@Serialannotation onserialVersionUIDfollows Java serialization best practices and likely addresses SonarQube recommendations mentioned in the PR title.Also applies to: 36-37
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/ServerContextPopulatedEvent.java (1)
27-28: LGTM! Consistent with serialization best practices.The
@Serialannotation is correctly applied, maintaining consistency with other event classes in this PR.Also applies to: 37-38
src/main/java/com/github/_1c_syntax/bsl/languageserver/events/LanguageServerInitializeRequestReceivedEvent.java (1)
29-30: LGTM! Proper serialization metadata annotation.The
@Serialannotation is correctly applied toserialVersionUID, consistent with the other event classes reviewed in this PR.Also applies to: 38-39
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/annotations/CompilerDirectiveKind.java (1)
25-25: LGTM!The Lombok import is correctly added to support the
@Getterannotation.src/test/java/com/github/_1c_syntax/bsl/languageserver/util/assertions/ColorInformationsAssert.java (1)
43-43: LGTM! Minor documentation formatting cleanup.The removal of parentheses around "см." in the Javadoc @param tags improves readability and consistency. This is a minor but appropriate cleanup that aligns with the PR's objective of addressing sonar fixes.
Also applies to: 57-57, 71-72
src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/sentry/PermissionFilterBeforeSendCallback.java (1)
34-34: Import addition is appropriate.The
lombok.NonNullimport is necessary for the parameter annotations added on line 74.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DuplicatedInsertionIntoCollectionDiagnostic.java (2)
93-96: Excellent modernization from Lombok to Java records!The migration from Lombok's
@Valueannotation to a Java record is well-executed. Records provide the same immutability guarantees while offering more concise syntax. All accessor method references have been correctly updated from getter-style (getCollectionName(),getMethodName(),getFirstParamName(),getCallStatement()) to fluent record accessors (collectionName(),methodName(),firstParamName(),callStatement()).Also applies to: 180-180, 183-183, 186-186, 305-305
319-319: Clean simplification with Stream.toList()!Replacing
Collectors.toUnmodifiableList()with.toList()is a good modernization. Both return unmodifiable lists, but.toList()is more concise and idiomatic in Java 16+. The usage patterns confirm these collections are never modified after creation.Also applies to: 328-328, 337-337
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/VariableSymbolTest.java (1)
97-97: LGTM! Improved idiomatic check.Using
isEmpty()instead of comparing to an empty string is more idiomatic and clearer in intent.src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/TernaryOperatorNode.java (1)
42-42: Good encapsulation improvement aligning with Lombok best practices.Changing the constructor from
protectedtoprivatecorrectly enforces the use of thecreate()static factory method and prevents unintended subclassing, which aligns with the immutable value object pattern of@Value. Verification confirms no code depends on the protected constructor—no subclasses exist, no direct constructor calls exist, and the factory method is the intended access pattern.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (2)
131-134: Good early-exit guard.Prevents false positives when the first reference isn’t a definition. LGTM.
161-164:TerminalNode::getParentreturningRuleNodeis correct here.With your custom ANTLR, this yields
Optional<RuleNode>without extra casts. All good. Based on learnings.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingCommonModuleMethodDiagnostic.java (3)
65-75: LGTM! Configuration check and fluent accessor usage are correct.The early return for EMPTY configuration source is appropriate, and the stream pipeline correctly uses the new fluent accessors (
occurrenceType(),symbol(),symbolKind(),moduleType()).
77-105: Accessor migrations look good overall.The migration to fluent accessors is correctly applied throughout:
symbol(),mdoRef(),moduleType(),symbolName(),location(), andgetRange(). The logic for checking method existence and export status is preserved.However, verify line 109 in the next code segment for proper record accessor syntax.
117-119: LGTM! CallData record definition is correct.The record components are well-named and appropriately typed. The boolean flags (
nonExport,exists) clearly distinguish between missing methods and private method access violations.src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/Symbol.java (2)
42-54: Record migration looks good; consider past review suggestions for null-safety.The migration from Lombok to a Java record is clean, and the interning mechanism is correctly updated to use the dedicated
INTERNERinstance.Past reviews suggested adding a compact constructor to enforce non-null invariants on all components. While not critical for immediate merge, this would improve robustness and align with defensive programming practices.
57-68: compareTo implementation functional; stricter contract suggested in past reviews.The current null-handling in
compareTo(returning 1 for null) works but diverges from the typicalComparablecontract, which expects non-null arguments. Past reviews recommended rejecting null withObjects.requireNonNull(other)and removing@Nullable.This refactor can be deferred but would improve consistency with Java conventions.
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrence.java (3)
25-25: LGTM! Nullable annotation updated to project standard.The migration from
javax.annotation.Nullabletoedu.umd.cs.findbugs.annotations.Nullablealigns with the explicitspotbugs-annotationsdependency and addresses the past review concern.
36-40: Record migration is correct; compact constructor suggested in past reviews.The record definition is clean and well-documented. Past reviews recommended adding a compact constructor to validate non-null components, which would prevent latent NPEs. This is a recommended improvement but not blocking.
43-54: compareTo works; stricter contract recommended in past reviews.The comparator chain correctly orders by location, occurrence type, and symbol. As with
Symbol.java, past reviews suggested removing@Nullableand rejecting null arguments to align with theComparablecontract. This refactor can be deferred.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/SetPermissionsForNewObjectsDiagnostic.java (1)
71-71: Method change verified as part of mdclasses API modernization.The change from
isSetForNewObjects()tosetForNewObjects()is an intentional API modernization in the mdclasses dependency (version 0.16.1-rc.1). The method correctly returns a boolean and is used appropriately in the filter predicate at line 71. No other files require updates, and the behavior remains unchanged.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/WrongDataPathForFormElementsDiagnostic.java (1)
65-67: The review comment is based on false premises and should be disregarded.The AI summary incorrectly claimed the code changed from
getDataPath().segments().startsWith("~")togetDataPath().startsWith("~"). Verification shows:
- No
segments()method exists – Zero results in the entire codebase; this method never existed.- DataPath is a String – Confirmed by external API documentation and web sources; it returns
Stringtype.- No uncommitted changes – Git diff HEAD is empty; the file contains no pending modifications.
- Current code is correct –
formItem.getDataPath().startsWith("~")is the proper and only valid implementation for checking if a String path begins with "~".- Test coverage confirms correctness – Existing tests validate that paths with "~" prefix are correctly flagged.
There is no behavioral change to verify; the current implementation is correct as-is.
Likely an incorrect or invalid review comment.
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java (1)
429-431: Accessor rename LGTMSwitch to fileComplexity() aligns with ComplexityData record API; no behavior change.
src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/CognitiveComplexityInlayHintSupplier.java (1)
47-50: API rename LGTMUpdated to methodsComplexitySecondaryLocations(); consistent with ComplexityData changes.
src/test/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplierTest.java (1)
69-71: Test update LGTMmethodsComplexity() accessor matches new public API.
src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CognitiveComplexityCodeLensSupplier.java (1)
47-47: LGTM! Accessor renamed to match record convention.The change from
getMethodsComplexity()tomethodsComplexity()correctly aligns with Java record accessor naming.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/computer/CyclomaticComplexityComputerTest.java (1)
52-52: LGTM! Test updated for new record accessor.The test correctly uses
methodsComplexity()instead of the oldgetMethodsComplexity().src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/MethodSymbolComputer.java (1)
327-327: LGTM! Updated to use record accessor.The change from
getName()toname()correctly reflects the ParameterDescription migration to a record.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/DescriptionReader.java (1)
129-129: LGTM! Record accessor update.The change from
getTypes()totypes()aligns with the ParameterDescription record conversion.src/main/java/com/github/_1c_syntax/bsl/languageserver/codelenses/CyclomaticComplexityCodeLensSupplier.java (1)
47-47: LGTM! Accessor renamed to match record convention.The change from
getMethodsComplexity()tomethodsComplexity()correctly aligns with Java record accessor naming, consistent with the CognitiveComplexityCodeLensSupplier.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/ParameterDescription.java (1)
27-36: LGTM! Clean migration to Java record.The conversion from a Lombok-backed class to a Java record is correct. The Javadoc properly documents all record components, and the immutable semantics are appropriate for this value object.
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexitySecondaryLocation.java (1)
26-35: LGTM! Proper record conversion.The migration to a Java record is correctly implemented with appropriate Javadoc. The two-component record with
RangeandStringis well-suited for this immutable value object.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/TypeDescription.java (1)
27-44: LGTM! Well-structured record conversion.The migration to a Java record is correctly implemented. All five components are properly documented in the Javadoc, and the circular reference between
TypeDescriptionandParameterDescriptionis valid and appropriate for modeling nested type structures.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/computer/ComplexityData.java (1)
29-47: LGTM! Clean migration to Java record.The conversion from Lombok-based POJO to Java record is well-executed. The record components are properly typed, and the Javadoc correctly documents all components using
@paramtags, which is the standard convention for records. This change should improve memory efficiency and reduce boilerplate.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/description/MethodDescriptionTest.java (1)
86-427: LGTM! Comprehensive test updates aligned with new fluent accessors.All test assertions have been systematically updated to use the new fluent accessor pattern (e.g.,
name(),types(),description(),parameters(),link()instead of their getter equivalents). The nested accessor chains are properly updated throughout, and the test logic remains unchanged—only the accessor names have been refactored to match the new record-based API.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/TransferringParametersBetweenClientAndServerDiagnostic.java (1)
171-173: LGTM! Well-structured internal record.The migration of the
ParamReferenceinner class from Lombok to a Java record is clean and appropriate. The record serves as a simple data carrier with three well-typed components, and all usage sites (lines 73-76) have been correctly updated to use the fluent component accessors.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissingParameterDescriptionDiagnostic.java (1)
108-158: LGTM! Consistent accessor updates throughout the diagnostic.All references to
ParameterDescriptionaccessors have been correctly updated to use the new fluent API (name()andtypes()instead ofgetName()andgetTypes()). The changes include both direct method calls and method references, maintaining consistency across the diagnostic logic.src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/DescriptionFormatter.java (1)
242-361: LGTM! Thorough migration to fluent accessors across hover formatting.The file has been comprehensively updated to use the new record-based fluent accessors for both
ParameterDescription(name(),types()) andTypeDescription(name(),description(),parameters(),link()). All usage patterns—including method references, direct calls, and nested accessor chains—have been consistently updated while preserving the original formatting logic.
.../_1c_syntax/bsl/languageserver/context/symbol/annotations/AnnotationParameterDefinition.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java (1)
131-144: RemovegetSymbolKind()from compareTo to be consistent with equals.The
compareTomethod includesgetSymbolKind()(line 138), which is not part ofequals/hashCode. Onlyname,owner, andgetSubNameRange()are included in equality (see@EqualsAndHashCode.Includeannotations). This violates the Comparable contract and can break sorted collections likeTreeSet.Apply this diff to remove the inconsistent field:
@Override public int compareTo(@Nullable MethodSymbol other) { if (other == null) { return 1; } return java.util.Comparator.comparing(MethodSymbol::getName) - .thenComparing(MethodSymbol::getSymbolKind) .thenComparing(MethodSymbol::getOwner) .thenComparingInt(m -> m.subNameLine) .thenComparingInt(m -> m.subNameStartCharacter) .thenComparingInt(m -> m.subNameEndCharacter) .compare(this, other); }
📜 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/context/symbol/MethodSymbol.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/variable/VariableDescription.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-27T08:05:10.053Z
Learnt from: theshadowco
PR: 1c-syntax/bsl-language-server#3555
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java:163-166
Timestamp: 2025-10-27T08:05:10.053Z
Learning: В проекте bsl-language-server используется собственная версия ANTLR4, в которой интерфейс TerminalNode переопределяет метод getParent() с возвращаемым типом RuleNode вместо ParseTree (как в стандартной версии ANTLR4).
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java
📚 Learning: 2025-04-18T22:42:30.042Z
Learnt from: nixel2007
PR: 1c-syntax/bsl-language-server#3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:88-97
Timestamp: 2025-04-18T22:42:30.042Z
Learning: В реализации ANTLR, если метод `getChildCount()` возвращает значение больше 0, то поле `children` гарантированно не равно null.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.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). (21)
- GitHub Check: Analyse
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: Benchmark
- GitHub Check: Analyze the repo with CodeSee
🔇 Additional comments (9)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java (2)
29-29: LGTM!The
@Nullableimport is appropriate for documenting that thecompareToparameter can be null.
56-56: LGTM!Adding
Comparable<MethodSymbol>enables natural ordering for method symbols, which is useful for sorted collections and ordering operations.src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/RewriteMethodParameterDiagnostic.java (2)
129-146: Well-structured refactoring with clear early exits.The refactored logic improves readability by handling edge cases upfront and clearly separating the single-reference and multi-reference scenarios. The early exit pattern for empty references and non-DEFINITION types is a solid defensive programming practice.
160-160: Correct removal of unnecessary cast.The cast to
RuleNodeis no longer needed since the project's custom ANTLR4 implementation hasTerminalNode.getParent()returningRuleNodedirectly.Based on learnings
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java (3)
64-65: Good refactoring to AssertJ's allSatisfy pattern.The migration from
forEachiteration toallSatisfyimproves readability and follows AssertJ best practices for map assertions.
70-71: LGTM - consistent refactoring.
76-77: LGTM - consistent refactoring.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/variable/VariableDescription.java (2)
29-29: Good addition for null-safety.The
@NonNullimport is properly used in the constructor below, enhancing compile-time null-safety.
100-108: Constructor changes are correct and consistent with codebase migration.The
@NonNullannotation appropriately prevents null-safety issues, and the transition fromparam.getLink()toparam.link()correctly follows Java record accessor conventions. Verification confirms no old-style getters remain in the codebase forParameterDescription, and the record accessor pattern is applied consistently across all usages.
src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/ShortBasedVariableSymbol.java (1)
56-63: Same equality semantics concern as IntBasedVariableSymbol.The same position-based equality concern raised for
IntBasedVariableSymbolapplies here. The changes are consistent across both implementations, which is good, but the verification requested in the previous file should cover both classes.
📜 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/context/symbol/AbstractVariableSymbol.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/IntBasedVariableSymbol.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/ShortBasedVariableSymbol.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/VariableSymbol.java(0 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/VariableSymbol.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/AbstractVariableSymbol.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DiagnosticInfosTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/ShortBasedVariableSymbol.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/ConditionalVertex.java (1)
EqualsAndHashCode(30-66)
🔇 Additional comments (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java (4)
29-29: LGTM!The new imports support the @nullable annotation for null-safe compareTo parameter handling and the @default annotation that replaces @Builder.Default throughout the file.
Also applies to: 32-32
61-61: LGTM!The migration from
@Builder.Defaultto@Defaultaligns with Lombok best practices and the broader refactoring theme across the codebase.Also applies to: 88-88, 92-92, 101-101, 104-106
77-77: LGTM!Including the sub-name range fields in equals/hashCode correctly distinguishes methods with identical names and owners but different source positions.
Also applies to: 80-80, 83-83
57-57: Comparable interface addition enables natural ordering.The class declaration correctly extends the API to support Comparable, though the implementation requires correction (see separate comment on lines 134-147).
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/IntBasedVariableSymbol.java (1)
56-63: Position-based equality is intentional and supported by test design.The test suite explicitly uses position ranges to differentiate and verify variable symbols. The change to include
variableNameLine,variableNameStartCharacter, andvariableNameEndCharacterin equals/hashCode is semantically correct—it distinguishes declarations of identically-named variables at different source locations within the same scope.No issues identified:
- Position fields are immutable (set in constructor only, no setters exist)
VariableSymbolis never used as a Map key, only as a valueHashSet<VariableSymbol>operations work correctly with immutable position-based equality- Test fixture confirms multiple distinct variables can coexist with position as the key differentiator
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/MethodSymbol.java
Show resolved
Hide resolved
|
Описание
Оптимизация хранения + некоторые фиксы сонара
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Summary by CodeRabbit
Dependencies
Refactor
Diagnostics
Tests