fix: устранить S2583 reliability-issue и почистить nullability/scope-аннотации#4003
Merged
Merged
Conversation
…Diagnostic ANTLR doCall() can be null — both legitimately for the optional `doCall?` branch of newExpression (e.g. `Новый Массив` without parentheses) and during parser error recovery for mandatory rule references. Annotating the parameters @nullable documents this and resolves the S2583 reliability false positive (condition "always false") without dropping the required defensive null guard. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Replace jakarta.annotation.Nullable and javax.annotation.Nullable with the project's standard org.jspecify.annotations.Nullable across TypeService, DereferenceMemberMatcher, WorkspaceScope, PlatformMemberSymbol and WorkspaceContextHolder. JSpecify @nullable is TYPE_USE, so annotations are moved into type-use position (e.g. `private @nullable URI`). Co-Authored-By: Claude Opus 4.8 <[email protected]>
Replace the repeated `@Scope(value = WorkspaceScope.SCOPE_NAME, proxyMode = ScopedProxyMode.TARGET_CLASS)` boilerplate (used on ~30 beans) with a composed `@WorkspaceScope` meta-annotation. The proxy mode is exposed via @aliasfor (default TARGET_CLASS), so the few interface-proxied @bean executors use `@WorkspaceScope(proxyMode = ScopedProxyMode.INTERFACES)`. The Spring Scope SPI implementation is renamed WorkspaceScope -> WorkspaceBeanScope to free the `WorkspaceScope` name for the annotation; SCOPE_NAME stays on the implementation and is referenced by the meta-annotation. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Contributor
📝 WalkthroughWalkthroughThis PR refactors Spring workspace-scoped bean management by extracting the scope implementation into a dedicated ChangesWorkspace Scope Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/WorkspaceBeanScope.java`:
- Around line 77-80: The remove(String name) method removes the bean from store
but leaves its associated entry in destructionCallbacks, causing stale
callbacks; update remove(...) (use resolveKey(), store, destructionCallbacks) to
also remove the callback for the same key/name pair (e.g., fetch
destructionCallbacks.get(key) and remove(name) when present) so that no stale
callback remains and removeWorkspace(...) won't run a left-over callback later.
- Around line 103-108: The removeWorkspace method currently runs all destruction
callbacks via callbacks.values().forEach(Runnable::run) which lets any thrown
exception abort the loop and skip store.remove; change removeWorkspace in
WorkspaceBeanScope so each Runnable from destructionCallbacks is invoked inside
its own try/catch (catch Throwable) to log the error and continue, and ensure
store.remove(workspaceUri) always executes (e.g., run callbacks in a try block
and call store.remove in a finally or simply perform callbacks with per-callback
error handling before removing from store); reference destructionCallbacks,
Runnable callbacks, removeWorkspace, and store to locate the code to modify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5022159-9ce7-4688-8d8e-6e0f48b0d56e
📒 Files selected for processing (37)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContextProvider.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/MissedRequiredParameterDiagnostic.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/infrastructure/DiagnosticInfos.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/ExecutorConfiguration.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/WorkspaceBeanScope.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/WorkspaceContextHolder.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/WorkspaceScope.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/WorkspaceScopeConfiguration.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/AnnotationRepository.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/DereferenceMemberMatcher.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/TypeService.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/CallStatementByReceiverIndex.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/InferredVariableTypeIndex.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/SymbolTypeIndex.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/inferencer/ExpressionTypeInferencer.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/inferencer/autumn/AutumnBeanIndex.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/inferencer/autumn/AutumnCollectionIndex.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/inferencer/autumn/AutumnComponentInferencer.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/inferencer/autumn/AutumnMetaAnnotationResolver.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/oscript/OScriptLibraryIndex.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/oscript/OScriptModuleMembersProvider.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/oscript/OScriptModuleTypeResolver.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/registry/BslContextHolder.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/registry/BslContextPlatformTypesProvider.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/registry/BuiltinOScriptPlatformTypesProvider.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/registry/BuiltinPlatformTypesProvider.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/registry/ConfigurationModuleMembersProvider.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/registry/ConfigurationTypesProvider.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/registry/GlobalScopeProvider.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/registry/MemberMetadataIndex.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/registry/PlatformContextProviderFactory.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/registry/TypeRegistry.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/scope/GlobalSymbolScope.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/symbol/PlatformMemberSymbol.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/util/AbstractDirtyContextTestExecutionListener.java
Comment on lines
+77
to
+80
| public @Nullable Object remove(String name) { | ||
| var key = resolveKey(); | ||
| var beans = store.get(key); | ||
| return beans != null ? beans.remove(name) : null; |
Contributor
There was a problem hiding this comment.
remove(...) leaves destruction callbacks behind.
On Line 77, the bean is removed from store, but its callback remains in destructionCallbacks. That stale callback can still run during removeWorkspace(...), causing double-destroy side effects and callback-map growth over time.
Suggested fix
`@Override`
public `@Nullable` Object remove(String name) {
var key = resolveKey();
var beans = store.get(key);
- return beans != null ? beans.remove(name) : null;
+ var removed = beans != null ? beans.remove(name) : null;
+ var callbacks = destructionCallbacks.get(key);
+ if (callbacks != null) {
+ callbacks.remove(name);
+ }
+ return removed;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/WorkspaceBeanScope.java`
around lines 77 - 80, The remove(String name) method removes the bean from store
but leaves its associated entry in destructionCallbacks, causing stale
callbacks; update remove(...) (use resolveKey(), store, destructionCallbacks) to
also remove the callback for the same key/name pair (e.g., fetch
destructionCallbacks.get(key) and remove(name) when present) so that no stale
callback remains and removeWorkspace(...) won't run a left-over callback later.
Comment on lines
+103
to
+108
| public void removeWorkspace(URI workspaceUri) { | ||
| var callbacks = destructionCallbacks.remove(workspaceUri); | ||
| if (callbacks != null) { | ||
| callbacks.values().forEach(Runnable::run); | ||
| } | ||
| store.remove(workspaceUri); |
Contributor
There was a problem hiding this comment.
removeWorkspace(...) should be exception-safe for callback execution.
On Line 106, any callback exception aborts the loop and prevents full cleanup on Line 108. This can leave workspace-scoped beans alive and pollute subsequent runs/tests.
Suggested fix
public void removeWorkspace(URI workspaceUri) {
var callbacks = destructionCallbacks.remove(workspaceUri);
- if (callbacks != null) {
- callbacks.values().forEach(Runnable::run);
- }
- store.remove(workspaceUri);
+ RuntimeException failure = null;
+ if (callbacks != null) {
+ for (var callback : callbacks.values()) {
+ try {
+ callback.run();
+ } catch (RuntimeException ex) {
+ if (failure == null) {
+ failure = ex;
+ } else {
+ failure.addSuppressed(ex);
+ }
+ }
+ }
+ }
+ store.remove(workspaceUri);
+ if (failure != null) {
+ throw failure;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void removeWorkspace(URI workspaceUri) { | |
| var callbacks = destructionCallbacks.remove(workspaceUri); | |
| if (callbacks != null) { | |
| callbacks.values().forEach(Runnable::run); | |
| } | |
| store.remove(workspaceUri); | |
| public void removeWorkspace(URI workspaceUri) { | |
| var callbacks = destructionCallbacks.remove(workspaceUri); | |
| RuntimeException failure = null; | |
| if (callbacks != null) { | |
| for (var callback : callbacks.values()) { | |
| try { | |
| callback.run(); | |
| } catch (RuntimeException ex) { | |
| if (failure == null) { | |
| failure = ex; | |
| } else { | |
| failure.addSuppressed(ex); | |
| } | |
| } | |
| } | |
| } | |
| store.remove(workspaceUri); | |
| if (failure != null) { | |
| throw failure; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/WorkspaceBeanScope.java`
around lines 103 - 108, The removeWorkspace method currently runs all
destruction callbacks via callbacks.values().forEach(Runnable::run) which lets
any thrown exception abort the loop and skip store.remove; change
removeWorkspace in WorkspaceBeanScope so each Runnable from destructionCallbacks
is invoked inside its own try/catch (catch Throwable) to log the error and
continue, and ensure store.remove(workspaceUri) always executes (e.g., run
callbacks in a try block and call store.remove in a finally or simply perform
callbacks with per-callback error handling before removing from store);
reference destructionCallbacks, Runnable callbacks, removeWorkspace, and store
to locate the code to modify.
nixel2007
enabled auto-merge
June 4, 2026 06:47
|
nixel2007
disabled auto-merge
June 4, 2026 07:00
Member
Author
|
Учёл оба замечания CodeRabbit по
Добавлен |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Зачем
Quality gate на
developпадал из-заnew_reliability_rating = C— единственная reliability-issue в новом коде: S2583 вMissedRequiredParameterDiagnostic(«condition always evaluates to false»). По ходу разбора заодно убраны связанные шероховатости с nullability- и scope-аннотациями.Что сделано
1. Фикс S2583 (reliability) —
fix(diagnostics)@Nullableна параметрахarguments(...)/appendMethodCall(...).doCall()у ANTLR реально может бытьnull— и легитимно (опциональныйdoCall?вnewExpression, напримерНовый Массивбез скобок), и при восстановлении парсера после ошибок даже для обязательных правил. Null-guard нужен; аннотация документирует это и снимает ложное срабатывание, не удаляя защитную проверку.2. Миграция
@Nullableна JSpecify —refactorjakarta.annotation.Nullable/javax.annotation.Nullable→org.jspecify.annotations.Nullable(стандарт проекта, 110+ использований). Аннотации переведены в TYPE_USE-позицию.3. Мета-аннотация
@WorkspaceScope—refactor(infrastructure)Композитная замена повторяющегося
@Scope(value = WorkspaceScope.SCOPE_NAME, proxyMode = ScopedProxyMode.TARGET_CLASS)(~30 бинов).proxyModeчерез@AliasFor(по умолчаниюTARGET_CLASS); interface-proxied@Bean-executors используют@WorkspaceScope(proxyMode = ScopedProxyMode.INTERFACES). Класс-реализация Spring Scope переименованWorkspaceScope→WorkspaceBeanScope, чтобы освободить имя под аннотацию.Проверки
compileJava/compileTestJava— зелёныеspotlessJavaCheck— зелёныйTypeServiceTest(TARGET_CLASS-бины) иAnalyzeProjectOnStartTest(per-workspace INTERFACES-executors) — проходят, подтверждая, что Spring корректно понимает мета-аннотацию в рантайме🤖 Generated with Claude Code
Summary by CodeRabbit