Skip to content

Use forward_property for property forwarding in resource_ref wrappers#2328

Merged
rapids-bot[bot] merged 1 commit intorapidsai:mainfrom
bdice:fix/forward-property-get-property-ambiguity
Mar 23, 2026
Merged

Use forward_property for property forwarding in resource_ref wrappers#2328
rapids-bot[bot] merged 1 commit intorapidsai:mainfrom
bdice:fix/forward-property-get-property-ambiguity

Conversation

@bdice
Copy link
Collaborator

@bdice bdice commented Mar 23, 2026

Description

Replace the hand-rolled friend get_property templates in cccl_resource_ref and cccl_async_resource_ref with inheritance from cuda::forward_property. This delegates property forwarding to CCCL's own machinery, which correctly handles dynamic_accessibility_property (NVIDIA/cccl#7727) and any future properties without ambiguity.

Each wrapper now exposes upstream_resource() returning the inner ResourceType, as required by forward_property for stateful properties.

Tests add minimal forward_property adaptors using RMM resource refs as upstream, exercising the exact scenario that causes the ambiguity.

Note: this is a temporary solution for the main branch -- resolving #2323 / #2325 will remove this code on the staging branch while I continue working on CCCL MR migrations.

Closes #2322.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Replace the hand-rolled friend get_property templates in
cccl_resource_ref and cccl_async_resource_ref with inheritance from
cuda::forward_property. This delegates property forwarding to CCCL's
own machinery, which correctly handles dynamic_accessibility_property
(NVIDIA/cccl#7727) and any future properties without ambiguity.

Each wrapper now exposes upstream_resource() returning the inner
ResourceType, as required by forward_property for stateful properties.

Closes rapidsai#2322
@bdice bdice requested a review from a team as a code owner March 23, 2026 16:08
@bdice bdice requested review from PointKernel and rongou March 23, 2026 16:08
@bdice bdice moved this to In Progress in RMM Project Board Mar 23, 2026
@bdice bdice self-assigned this Mar 23, 2026
@bdice bdice added bug Something isn't working non-breaking Non-breaking change labels Mar 23, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f7a08566-3481-444c-97a0-062edad0b237

📥 Commits

Reviewing files that changed from the base of the PR and between 1bef459 and 8cedbad.

📒 Files selected for processing (2)
  • cpp/include/rmm/detail/cccl_adaptors.hpp
  • cpp/tests/mr/resource_ref_conversion_tests.cpp

📝 Walkthrough

Summary by CodeRabbit

  • Refactor

    • Updated internal resource reference classes to improve property forwarding capabilities and better align with CUDA memory resource standards.
    • Simplified property delegation mechanisms through modernized accessor patterns to enhance code maintainability and clarity.
  • Tests

    • Added comprehensive validation tests for resource reference type erasure, property forwarding behavior, and async/sync resource adaptor patterns.

Walkthrough

The PR refactors property forwarding in two CCCL adaptor classes (cccl_resource_ref and cccl_async_resource_ref) by replacing friend get_property templates with public inheritance from cuda::forward_property and introducing upstream_resource() accessor methods.

Changes

Cohort / File(s) Summary
CCCL Adaptor Property Forwarding
cpp/include/rmm/detail/cccl_adaptors.hpp
Updated cccl_resource_ref and cccl_async_resource_ref to inherit from cuda::forward_property<> and removed friend get_property templates in favor of upstream_resource() const accessor methods returning ResourceType const&.
Property Forwarding Validation Tests
cpp/tests/mr/resource_ref_conversion_tests.cpp
Added forwarding adaptor structs wrapping RMM resource refs, validated cuda::forward_property concept compliance via static_assert, and introduced three test cases for type-erasure and property access through the forwarding chain.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Suggested labels

non-breaking, improvement

Suggested reviewers

  • harrism
  • rongou
  • ttnghia
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.73% 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
Title check ✅ Passed The title 'Use forward_property for property forwarding in resource_ref wrappers' accurately summarizes the main change: replacing hand-rolled friend get_property templates with cuda::forward_property inheritance.
Description check ✅ Passed The description clearly explains the motivation (resolving ambiguity with CCCL's dynamic_accessibility_property), the implementation approach (inheritance from cuda::forward_property, adding upstream_resource()), and references the related issue (#2322).
Linked Issues check ✅ Passed The PR directly addresses issue #2322 by replacing unconstrained friend get_property templates with cuda::forward_property inheritance and adding upstream_resource() accessors, which eliminates the ambiguity in property overload resolution.
Out of Scope Changes check ✅ Passed Changes are focused and in-scope: modifications to cccl_resource_ref and cccl_async_resource_ref in cccl_adaptors.hpp, and test additions using forward_property adaptors to validate the solution, all directly supporting the stated objective.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@bdice
Copy link
Collaborator Author

bdice commented Mar 23, 2026

/merge

@rapids-bot rapids-bot bot merged commit ad99c11 into rapidsai:main Mar 23, 2026
87 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in RMM Project Board Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Non-breaking change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Ambiguous get_property overload with CCCL's dynamic_accessibility_property

2 participants