fix(inlayhints): unify method-call suppliers under a single methodCall setting#4234
Conversation
The methodCall parameter is documented in the configuration schema as a single switch for both source-defined and platform method-call hints, but the enable/disable check keyed off each supplier's own id (platformMethodCall / sourceDefinedMethodCall), so methodCall: false was ignored. Suppliers now expose getConfigurationKeys() (default: own id); the method-call suppliers return the unified [methodCall, sourceDefinedMethodCall] keys. supplierIsEnabled scans them in priority order, so methodCall: false disables both suppliers at once, with sourceDefinedMethodCall kept as a legacy fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR splits method-call inlay hint generation into two collectors and one aggregating supplier: ChangesMethod-call inlay hint supplier/collector refactor
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MethodCallInlayHintSupplier
participant SourceDefinedMethodCallInlayHintCollector
participant PlatformMethodCallInlayHintCollector
participant PlatformMethodCallHintRenderer
Client->>MethodCallInlayHintSupplier: getInlayHints(documentContext, params)
MethodCallInlayHintSupplier->>SourceDefinedMethodCallInlayHintCollector: getInlayHints(documentContext, params)
SourceDefinedMethodCallInlayHintCollector-->>MethodCallInlayHintSupplier: source-defined hints
MethodCallInlayHintSupplier->>PlatformMethodCallInlayHintCollector: getInlayHints(documentContext, params)
PlatformMethodCallInlayHintCollector->>PlatformMethodCallHintRenderer: appendHints(member, signature, args)
PlatformMethodCallHintRenderer-->>PlatformMethodCallInlayHintCollector: rendered hints
PlatformMethodCallInlayHintCollector-->>MethodCallInlayHintSupplier: platform hints
MethodCallInlayHintSupplier-->>Client: combined hints list
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/inlayhints/AbstractMethodCallInlayHintSupplier.java`:
- Around line 35-48: Keep backward compatibility for the platform inlay-hint
toggle: `supplierIsEnabled(...)` currently falls back only to `methodCall` and
`sourceDefinedMethodCall`, so legacy configs using `platformMethodCall` are
ignored and platform hints re-enable unexpectedly. Update the configuration
lookup in `AbstractMethodCallInlayHintSupplier` (or override it in the
platform-specific supplier) so `platformMethodCall` remains part of the fallback
chain, and add a regression test covering `platformMethodCall: false` to verify
the platform supplier stays disabled after upgrade.
🪄 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: a9b5dc5f-8469-41b2-bccb-187c107a9a2c
📒 Files selected for processing (6)
src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/AbstractMethodCallInlayHintSupplier.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/InlayHintSupplier.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/infrastructure/InlayHintsConfiguration.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/providers/InlayHintProvider.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintSupplierUnitTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/infrastructure/InlayHintsConfigurationTest.java
Address review feedback: - getConfigurationKeys() now returns [methodCall, <ownId>], so each method-call supplier keeps honoring its own legacy key (platformMethodCall / sourceDefinedMethodCall) while the unified methodCall key takes precedence. Prevents platform hints from silently re-enabling after upgrade. - supplierIsEnabled accepts Iterable<String> instead of List<String> (S3242). - Extend tests with per-supplier legacy-key regression cases. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Test Results 3 582 files 3 582 suites 1h 42m 18s ⏱️ Results for commit 897d0be. ♻️ This comment has been updated with latest results. |
Collapse PlatformMethodCallInlayHintSupplier and SourceDefinedMethodCallInlayHintSupplier into a single MethodCallInlayHintSupplier (id "methodCall") that delegates to two collectors: source-defined calls (resolved via the reference index, with a link to the parameter declaration) and platform/global/constructor calls (resolved via the type system). Their coverage is disjoint — the platform path skips members with a non-null sourceSymbol. A single supplier means a single configuration key, so the per-supplier getConfigurationKeys() machinery is dropped and supplierIsEnabled goes back to keying off the supplier id. methodCall: false now disables all method-call parameter hints. Nested flags (showDefaultValues, showParametersWithTheSameName) still read the legacy sourceDefinedMethodCall key for backward compatibility. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
- Declare getInlayHints() abstract on AbstractMethodCallInlayHintCollector so the base has an abstract member (java:S1694). - Fix modifier order and replace a lambda with Either::getLeft in tests (java:S1124, java:S1612). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Boy-scout pass over the merged method-call inlay hints: - Replace the shared abstract base with a MethodCallInlayHintFlags utility (composition over inheritance; removes the abstract-class and scoped-bean single-arg-ctor smells). - Split PlatformMethodCallInlayHintCollector into resolution (collector) and rendering (PlatformMethodCallHintRenderer): cuts class dependencies and method complexity below thresholds, and splits getInlayHints into focused steps (fewer returns, single loop exit). - Drop redundant non-null checks on ANTLR getText(), the unused DEFAULT_SHOW_ALL_PARAMETERS field and stale TODOs, name the identifier-center divisor constant, and bound the source-defined loop with Math.min instead of a break. - Parameterize four structurally identical "no hints" tests. Behavior is unchanged; the full inlay-hint test suite stays green. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollector.java (1)
53-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winJavadoc describes a calling scenario instead of the class contract.
"Коллектор используется единым
{@linkMethodCallInlayHintSupplier}" documents who invokes this class rather than what it does/contracts.As per path instructions for
src/main/java/**/*.java: "Javadoc should describe the contract of a class or method (what it does, inputs/outputs, invariants, side effects), not calling scenarios or who invokes it."📝 Suggested rewording
- * Коллектор используется единым {`@link` MethodCallInlayHintSupplier} и отвечает за - * резолв вызова; построение меток и tooltip делегируется + * Резолвит вызов метода/конструктора через систему типов и делегирует построение + * меток и tooltip * {`@link` PlatformMethodCallHintRenderer}. Резолв члена выполняется через * {`@link` TypeService#memberAt}, что покрывает три кейса:🤖 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/inlayhints/PlatformMethodCallInlayHintCollector.java` around lines 53 - 56, Update the Javadoc for PlatformMethodCallInlayHintCollector so it describes the class’s responsibility and contract, not who uses it or the calling scenario. Reword the comment to focus on what the collector does: resolving method-call targets, building the inlay hint data, and delegating rendering details to PlatformMethodCallHintRenderer, while keeping the TypeService#memberAt behavior description if relevant.Source: Coding guidelines
src/test/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollectorTest.java (1)
58-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winField name
supplieris stale after the Supplier→Collector rename.The field type was retyped to
PlatformMethodCallInlayHintCollector, but the variable is still namedsupplier, which is confusing given this PR's whole point is splitting the old supplier into collectors + a separate aggregatingMethodCallInlayHintSupplier.✏️ Suggested rename
- `@Autowired` - private PlatformMethodCallInlayHintCollector supplier; + `@Autowired` + private PlatformMethodCallInlayHintCollector collector;(and update all
supplier.getInlayHints(...)call sites in this file accordingly)🤖 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/test/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollectorTest.java` around lines 58 - 59, The test field name `supplier` in `PlatformMethodCallInlayHintCollectorTest` is stale after the Supplier→Collector rename. Rename the injected `PlatformMethodCallInlayHintCollector` field to a collector-appropriate name and update all `supplier.getInlayHints(...)` call sites in this test to use the new identifier so the test matches the refactored `PlatformMethodCallInlayHintCollector`/`MethodCallInlayHintSupplier` split.
🤖 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.
Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollector.java`:
- Around line 53-56: Update the Javadoc for PlatformMethodCallInlayHintCollector
so it describes the class’s responsibility and contract, not who uses it or the
calling scenario. Reword the comment to focus on what the collector does:
resolving method-call targets, building the inlay hint data, and delegating
rendering details to PlatformMethodCallHintRenderer, while keeping the
TypeService#memberAt behavior description if relevant.
In
`@src/test/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollectorTest.java`:
- Around line 58-59: The test field name `supplier` in
`PlatformMethodCallInlayHintCollectorTest` is stale after the Supplier→Collector
rename. Rename the injected `PlatformMethodCallInlayHintCollector` field to a
collector-appropriate name and update all `supplier.getInlayHints(...)` call
sites in this test to use the new identifier so the test matches the refactored
`PlatformMethodCallInlayHintCollector`/`MethodCallInlayHintSupplier` split.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91da190b-eed1-4333-aa06-e3bcd3c9b901
📒 Files selected for processing (8)
src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/MethodCallInlayHintFlags.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallHintRenderer.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollector.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/SourceDefinedMethodCallInlayHintCollector.javasrc/main/resources/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallHintRenderer_en.propertiessrc/main/resources/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallHintRenderer_ru.propertiessrc/test/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollectorTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollectorUnitTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollectorUnitTest.java
DocumentContext.getAst() is non-null (sibling suppliers call it without a guard), so the ast == null branch was unreachable dead code (sonar S2583). Empty-file behavior stays covered by the parameterized "no hints" test that goes through the real getAst(). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The method-call hint flags are now read only from the methodCall key; the deprecated sourceDefinedMethodCall key is no longer honored and is removed from the schema and docs. Old configuration files that still contain the key keep loading without error (it lands in the free-form parameters map and is simply ignored) — covered by LanguageServerConfigurationTest. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|



What
Merge the two method-call inlay hint suppliers (source-defined and platform) into a single
MethodCallInlayHintSupplier, controlled by oneinlayHint.parameters.methodCallsetting.Why
methodCallis documented in the configuration schema as the single switch for both source-defined and platform method-call hints ("Show parameters of called methods (source-defined and platform)"), withsourceDefinedMethodCalldeprecated in favor of it. In practice there were two separate suppliers, each keyed off its own id, so:methodCall: falsewas silently ignored — the documented single switch did nothing;Unifying them makes the documented setting actually work and removes the artificial split.
Behavior
methodCall: falsemethodCall: { showDefaultValues: false, showParametersWithTheSameName: true }The deprecated
sourceDefinedMethodCallkey is dropped: it is no longer read and is removed from the schema and docs. Old configuration files that still contain it keep loading without error (the key lands in the free-form parameters map and is ignored), so no hard break — only that legacy setting stops taking effect.Structure
MethodCallInlayHintSupplier(idmethodCall) — the single LSP supplier. Delegates to two collectors whose coverage is disjoint (the platform path skips members with a non-nullsourceSymbol):SourceDefinedMethodCallInlayHintCollector— calls resolved via the reference index (including bare local calls in the same module); hint labels carry a link to the parameter declaration.PlatformMethodCallInlayHintCollector— platform methods, global functions and constructors resolved via the type system.PlatformMethodCallHintRendererbuilds the platform hint labels/tooltips (rendering split from resolution).MethodCallInlayHintFlagsreads the shared rendering flags.Note on scope: the collectors keep separate resolution paths because
TypeService.memberAtdoes not resolve bare same-module calls (those come through the reference index), so the source-defined path cannot be folded into the type-system path without losing hints for the most common case.Tests
MethodCallInlayHintSupplierTest— supplier id, source-defined parameter link through the unified supplier, andmethodCall: falsedisabling all parameter hints.SourceDefinedMethodCallInlayHintCollectorTest,PlatformMethodCallInlayHintCollectorTest,PlatformMethodCallInlayHintCollectorUnitTest,InlayHintPatternsTest).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes