Skip to content

fix(inlayhints): unify method-call suppliers under a single methodCall setting#4234

Merged
nixel2007 merged 7 commits into
developfrom
fix/inlayhint-methodcall-toggle
Jul 1, 2026
Merged

fix(inlayhints): unify method-call suppliers under a single methodCall setting#4234
nixel2007 merged 7 commits into
developfrom
fix/inlayhint-methodcall-toggle

Conversation

@sfaqer

@sfaqer sfaqer commented Jun 30, 2026

Copy link
Copy Markdown
Member

What

Merge the two method-call inlay hint suppliers (source-defined and platform) into a single MethodCallInlayHintSupplier, controlled by one inlayHint.parameters.methodCall setting.

Why

methodCall is 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)"), with sourceDefinedMethodCall deprecated in favor of it. In practice there were two separate suppliers, each keyed off its own id, so:

  • methodCall: false was silently ignored — the documented single switch did nothing;
  • the two visually-identical kinds of hints could only be toggled with two different keys.

Unifying them makes the documented setting actually work and removes the artificial split.

Behavior

Config Result
methodCall: false all method-call parameter hints off (source-defined + platform/global/constructor)
methodCall: { showDefaultValues: false, showParametersWithTheSameName: true } hints on, nested rendering flags applied

The deprecated sourceDefinedMethodCall key 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 (id methodCall) — the single LSP supplier. Delegates to two collectors whose coverage is disjoint (the platform path skips members with a non-null sourceSymbol):
    • 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.
  • PlatformMethodCallHintRenderer builds the platform hint labels/tooltips (rendering split from resolution).
  • MethodCallInlayHintFlags reads the shared rendering flags.

Note on scope: the collectors keep separate resolution paths because TypeService.memberAt does 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, and methodCall: false disabling all parameter hints.
  • Collector-level suites retained (SourceDefinedMethodCallInlayHintCollectorTest, PlatformMethodCallInlayHintCollectorTest, PlatformMethodCallInlayHintCollectorUnitTest, InlayHintPatternsTest).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added unified method-call inlay hints for both source-defined and platform/global members, including constructor calls.
    • Improved parameter hints with better handling of defaults, repeated arguments, and variadic parameters.
    • Existing and legacy hint settings are now recognized under a single method-call option.
  • Bug Fixes

    • Preserved hint links back to original parameter declarations.
    • Reduced extra or missing hints in cases with skipped or incomplete arguments.

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]>
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR splits method-call inlay hint generation into two collectors and one aggregating supplier: SourceDefinedMethodCallInlayHintCollector handles source-defined calls, PlatformMethodCallInlayHintCollector (with new PlatformMethodCallHintRenderer) handles platform/constructor calls, and MethodCallInlayHintSupplier combines both under a unified "methodCall" ID. A new MethodCallInlayHintFlags utility centralizes configuration flag reads. Tests are updated accordingly.

Changes

Method-call inlay hint supplier/collector refactor

Layer / File(s) Summary
Shared config flag helper
src/main/.../inlayhints/MethodCallInlayHintFlags.java
New utility resolves showParametersWithTheSameName and showDefaultValues from configuration, checking current and legacy keys with fallback defaults.
Source-defined collector rename and flag usage
src/main/.../SourceDefinedMethodCallInlayHintCollector.java, src/test/.../SourceDefinedMethodCallInlayHintCollectorTest.java
Class renamed from supplier to collector, drops the getInlayHintDataClass() override, caps hintable parameters via min(parameters, callParams), and switches to MethodCallInlayHintFlags; tests updated to the collector type and config key "methodCall".
Platform hint renderer
src/main/.../PlatformMethodCallHintRenderer.java
New component renders parameter-name/default-value hints and tooltips from a resolved SignatureDescriptor, handling variadic parameters, skipped/blank arguments, and name-shadowing suppression.
Platform/constructor call collector
src/main/.../PlatformMethodCallInlayHintCollector.java, src/test/.../PlatformMethodCallInlayHintCollectorTest.java, src/test/.../PlatformMethodCallInlayHintCollectorUnitTest.java, src/test/.../InlayHintPatternsTest.java, src/test/.../ComprehensiveInferenceTest.java
New collector resolves platform method calls via TypeService#memberAt (excluding source-defined symbols) and constructor calls via TypeService#resolve/getConstructors, infers argument types for overload selection, and delegates rendering; tests/docs updated to the collector type.
Unified supplier aggregator
src/main/.../MethodCallInlayHintSupplier.java, src/test/.../MethodCallInlayHintSupplierTest.java
New InlayHintSupplier combines results from both collectors under a single "methodCall" id; new test verifies the ID, parameter-label location links, and disabling hints via configuration.

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
Loading

Possibly related PRs

  • 1c-syntax/bsl-language-server#3993: The platform collector's use of TypeService#memberAt(...) and #expressionTypesAt(...) depends on the positional API changes introduced in this PR.
  • 1c-syntax/bsl-language-server#4106: The default-value hint logic for skipped/missing arguments in PlatformMethodCallHintRenderer mirrors the conditional emission logic from this PR.
  • 1c-syntax/bsl-language-server#4158: The platform collector's filtering out of user-defined types when resolving constructors aligns with the suppression logic added in this PR.

Poem

A supplier once carried the weight of two jobs,
now split into collectors, no more mixed-up blobs.
Source-defined hops left, platform hops right,
flags in one burrow keep the config tight.
Renderer paints labels with tooltip delight —
this rabbit hops on, review's done, good night! 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately summarizes the main change: unifying method-call hint suppliers under the single methodCall setting.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/inlayhint-methodcall-toggle

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2311db8 and dd386a4.

📒 Files selected for processing (6)
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/AbstractMethodCallInlayHintSupplier.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/InlayHintSupplier.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/infrastructure/InlayHintsConfiguration.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/InlayHintProvider.java
  • src/test/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintSupplierUnitTest.java
  • src/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]>
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Test Results

 3 582 files   3 582 suites   1h 42m 18s ⏱️
 3 535 tests  3 517 ✅  18 💤 0 ❌
21 210 runs  21 098 ✅ 112 💤 0 ❌

Results for commit 897d0be.

♻️ This comment has been updated with latest results.

sfaqer and others added 3 commits June 30, 2026 17:43
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]>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollector.java (1)

53-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Javadoc describes a calling scenario instead of the class contract.

"Коллектор используется единым {@link MethodCallInlayHintSupplier}" 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 win

Field name supplier is stale after the Supplier→Collector rename.

The field type was retyped to PlatformMethodCallInlayHintCollector, but the variable is still named supplier, which is confusing given this PR's whole point is splitting the old supplier into collectors + a separate aggregating MethodCallInlayHintSupplier.

✏️ 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

📥 Commits

Reviewing files that changed from the base of the PR and between 55466b6 and 5bc39d2.

📒 Files selected for processing (8)
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/MethodCallInlayHintFlags.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallHintRenderer.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollector.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/SourceDefinedMethodCallInlayHintCollector.java
  • src/main/resources/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallHintRenderer_en.properties
  • src/main/resources/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallHintRenderer_ru.properties
  • src/test/java/com/github/_1c_syntax/bsl/languageserver/inlayhints/PlatformMethodCallInlayHintCollectorTest.java
  • src/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]>
@sfaqer
sfaqer requested a review from nixel2007 July 1, 2026 03:33
@sfaqer sfaqer changed the title fix(inlayhints): honor methodCall: false to disable method-call hints fix(inlayhints): unify method-call suppliers under a single methodCall setting Jul 1, 2026
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]>
@nixel2007
nixel2007 enabled auto-merge July 1, 2026 06:29
@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2026

Copy link
Copy Markdown

@nixel2007
nixel2007 merged commit 37f0dfb into develop Jul 1, 2026
37 checks passed
@nixel2007
nixel2007 deleted the fix/inlayhint-methodcall-toggle branch July 1, 2026 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants