Suya lola field support implement skeleton field set handler

[saksham-chawla]

Implement RegisterSetHandler() method to allow users to register callbacks for handling remote Set requests on skeleton fields. The Set handler wraps user-provided validation/modification logic and automatically broadcasts accepted values to all field subscribers via Update().

Issue: SWP-249527

---

Implement RegisterSetHandler() for SkeletonField with conditional setter support

This pull request introduces the RegisterSetHandler() method to the SkeletonField class, enabling users to register callbacks for handling remote Set requests. The handler wraps user-provided validation and modification logic, automatically broadcasting accepted values to all field subscribers via Update().

To achieve this, new template parameters (EnableSet, EnableNotifier) have been added to SkeletonField for conditional compilation, ensuring a zero-cost abstraction when setter functionality is not required. This involves significant refactoring of SkeletonField's constructors using delegating constructors and tag dispatch, and the addition of a new virtual method IsSetHandlerRegistered() to SkeletonFieldBase.

Architecture Diagram:

flowchart TD
    subgraph Core_Components
        SkeletonBase["SkeletonBase"]
        SkeletonFieldBase["SkeletonFieldBase (+9 lines)"]
        SkeletonEvent["SkeletonEvent (+5 lines)"]
        SkeletonMethod["SkeletonMethod"]
    end

    subgraph Communication_Layer
        SkeletonField["SkeletonField (+155 lines)"]
    end

    subgraph API_Layer
        SkeletonTrait["SkeletonTrait (+2 lines)"]
    end

    SkeletonBase -->|contains| SkeletonFieldBase
    SkeletonFieldBase -->|inherits| SkeletonBase
    SkeletonField -->|inherits| SkeletonFieldBase
    SkeletonField -->|uses| SkeletonEvent
    SkeletonField -->|registers handler with| SkeletonMethod
    SkeletonTrait -->|defines| SkeletonField

Loading

---

This summary was automatically generated by @propel-code-bot

[propel-code-bot]

Setter handler support is close, but key constructor and state-management bugs need fixes.

Status: Changes Suggested | Risk: Medium | Readiness: 6/10 (Needs Polish)

Issues Identified & Suggestions

• Add setter-enabled test constructor or SFINAE away invalid overloads: score/mw/com/impl/skeleton_field.h
• Set registration flag only after successful handler registration: score/mw/com/impl/skeleton_field.h
• Move/copy handler-registered flag in move operations: score/mw/com/impl/skeleton_field.h

Review Details

📁 5 files reviewed | 💬 3 comments

👍 / 👎 individual comments to help improve reviews for you

[propel-code-bot]

[Logic] The constructor intended for tests (SkeletonField(..., std::unique_ptr<SkeletonEventBinding>)) always delegates to the no-setter private constructor (SkeletonField{parent, event, field_name}). When EnableSet is true, that overload is removed via enable_if_t<!ES>, so this definition becomes ill-formed and setter-enabled fields cannot be instantiated with an injected binding (e.g. in unit tests). Provide a setter-enabled variant that also accepts a SkeletonMethod (or SFINAE this constructor away when EnableSet is true) so that the new setter functionality remains constructible for testing.

-template <typename SampleDataType, bool EnableSet, bool EnableNotifier>
-SkeletonField<SampleDataType, EnableSet, EnableNotifier>::SkeletonField(
-    SkeletonBase& skeleton_base,
-    const std::string_view field_name,
-    std::unique_ptr<SkeletonEventBinding<FieldType>> binding)
-    : SkeletonField{skeleton_base,
-                    std::make_unique<SkeletonEvent<FieldType>>(skeleton_base, field_name, std::move(binding)),
-                    field_name}
-{
-}
+template <typename SampleDataType, bool EnableSet, bool EnableNotifier>
+template <bool ES, typename>
+SkeletonField<SampleDataType, EnableSet, EnableNotifier>::SkeletonField(
+    SkeletonBase& skeleton_base,
+    const std::string_view field_name,
+    std::unique_ptr<SkeletonEventBinding<FieldType>> binding,
+    std::unique_ptr<SkeletonMethod<Result<FieldType>(FieldType)>> set_method)
+    : SkeletonField{skeleton_base,
+                    std::make_unique<SkeletonEvent<FieldType>>(skeleton_base, field_name, std::move(binding)),
+                    std::move(set_method),
+                    field_name}
+{
+}
+
+template <typename SampleDataType, bool EnableSet, bool EnableNotifier>
+template <bool ES, typename>
+SkeletonField<SampleDataType, EnableSet, EnableNotifier>::SkeletonField(
+    SkeletonBase& skeleton_base,
+    const std::string_view field_name,
+    std::unique_ptr<SkeletonEventBinding<FieldType>> binding)
+    : SkeletonField{skeleton_base,
+                    std::make_unique<SkeletonEvent<FieldType>>(skeleton_base, field_name, std::move(binding)),
+                    field_name}
+{
+}

Context for Agents

The constructor intended for tests (`SkeletonField(..., std::unique_ptr<SkeletonEventBinding>)`) always delegates to the no-setter private constructor (`SkeletonField{parent, event, field_name}`). When `EnableSet` is `true`, that overload is removed via `enable_if_t<!ES>`, so this definition becomes ill-formed and setter-enabled fields cannot be instantiated with an injected binding (e.g. in unit tests). Provide a setter-enabled variant that also accepts a `SkeletonMethod` (or SFINAE this constructor away when `EnableSet` is true) so that the new setter functionality remains constructible for testing.

```diff
-template <typename SampleDataType, bool EnableSet, bool EnableNotifier>
-SkeletonField<SampleDataType, EnableSet, EnableNotifier>::SkeletonField(
-    SkeletonBase& skeleton_base,
-    const std::string_view field_name,
-    std::unique_ptr<SkeletonEventBinding<FieldType>> binding)
-    : SkeletonField{skeleton_base,
-                    std::make_unique<SkeletonEvent<FieldType>>(skeleton_base, field_name, std::move(binding)),
-                    field_name}
-{
-}
+template <typename SampleDataType, bool EnableSet, bool EnableNotifier>
+template <bool ES, typename>
+SkeletonField<SampleDataType, EnableSet, EnableNotifier>::SkeletonField(
+    SkeletonBase& skeleton_base,
+    const std::string_view field_name,
+    std::unique_ptr<SkeletonEventBinding<FieldType>> binding,
+    std::unique_ptr<SkeletonMethod<Result<FieldType>(FieldType)>> set_method)
+    : SkeletonField{skeleton_base,
+                    std::make_unique<SkeletonEvent<FieldType>>(skeleton_base, field_name, std::move(binding)),
+                    std::move(set_method),
+                    field_name}
+{
+}
+
+template <typename SampleDataType, bool EnableSet, bool EnableNotifier>
+template <bool ES, typename>
+SkeletonField<SampleDataType, EnableSet, EnableNotifier>::SkeletonField(
+    SkeletonBase& skeleton_base,
+    const std::string_view field_name,
+    std::unique_ptr<SkeletonEventBinding<FieldType>> binding)
+    : SkeletonField{skeleton_base,
+                    std::make_unique<SkeletonEvent<FieldType>>(skeleton_base, field_name, std::move(binding)),
+                    field_name}
+{
+}
```


File: score/mw/com/impl/skeleton_field.h
Line: 250

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.95
• Likelihood Score: 5/5
• Likelihood Reason: When EnableSet=true, the testing constructor delegates to a 3-arg private constructor that is SFINAE'd away by enable_if_t<!ES>. The 4-arg private ctor doesn't match either. This will inevitably cause a compilation error whenever SkeletonField<T, true, N> is instantiated with the testing constructor (e.g., in unit tests for setter-enabled fields).
• Suggested Action: keep
• Tool Calls Used: 3

Reasoning

The comment correctly identifies a real compilation issue introduced by this PR.

Analysis of the delegation chain:

1. The testing constructor (line 79-81) is a non-template public constructor that takes (SkeletonBase&, string_view, unique_ptr<SkeletonEventBinding>).

2. Its out-of-line definition (lines 243-252) delegates to SkeletonField{skeleton_base, make_unique<SkeletonEvent>(...), field_name} — a 3-argument call matching signature (SkeletonBase&, unique_ptr<SkeletonEvent>, string_view).

3. The only private constructor matching this 3-arg signature is at lines 197-200, guarded by template <bool ES = EnableSet, typename = std::enable_if_t<!ES>>.

4. When EnableSet=true: enable_if_t<!true> = enable_if_t<false> → SFINAE failure, removing this overload.

5. The 4-arg private constructor (lines 190-194) requires a SkeletonMethod parameter and takes 4 arguments, so it doesn't match the 3-argument delegation.

6. No other constructors match either. The public constructors take different types (e.g., EnableSetOnlyTag, EnableNeitherTag).

Conclusion: This is a genuine compile-time error for SkeletonField<T, true, N>. The PR introduces EnableSet template parameter with SFINAE-guarded private constructors but fails to provide a valid delegation target for the testing constructor when EnableSet=true.

The comment is actionable: it suggests either adding a setter-enabled testing constructor that also accepts a SkeletonMethod, or SFINAE-gating the existing testing constructor. Concrete code is provided in the suggested fix.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/impl/skeleton_field.h
• Lines: 76-105

76:     /// Constructor that allows to set the binding directly.
77:     ///
78:     /// This is used only used for testing.
79:     SkeletonField(SkeletonBase& skeleton_base,
80:                   const std::string_view field_name,
81:                   std::unique_ptr<SkeletonEventBinding<FieldType>> binding);
82: 
83:     ~SkeletonField() override = default;
84: 
85:     SkeletonField(const SkeletonField&) = delete;
...

2. File Snapshot (HEAD)

• File: score/mw/com/impl/skeleton_field.h
• Lines: 186-286

186: 
187:     /// \brief Private delegating constructor used by the setter-enabled public ctor.
188:     /// Receives already-constructed objects so that we can pass the event pointer to SkeletonFieldBase before
189:     /// storing it here, mirroring the ProxyField pattern.
190:     template <bool ES = EnableSet, typename = std::enable_if_t<ES>>
191:     SkeletonField(SkeletonBase& parent,
192:                   std::unique_ptr<SkeletonEvent<FieldType>> skeleton_event_dispatch,
193:                   std::unique_ptr<SkeletonMethod<Result<FieldType>(FieldType)>> set_method,
194:                   const std::string_view field_name);
195: 
...

3. File Snapshot (HEAD)

• File: score/mw/com/impl/skeleton_field.h
• Lines: 36-82

36: 
37: namespace detail
38: {
39: /// Tag types for constructor overload disambiguation based on EnableSet template parameter.
40: struct EnableSetOnlyTag
41: {
42: };
43: struct EnableNeitherTag
44: {
45: };
...

[propel-code-bot]

[Logic] is_set_handler_registered_ is set to true before set_method_.get()->RegisterHandler(...) is called. If RegisterHandler fails and returns an error, the flag still indicates that a handler was successfully registered. This causes IsSetHandlerRegistered() to return true, which means PrepareOffer() in the base class will skip the kSetHandlerNotSet check and proceed as if a handler is registered — even though none is active.

Triggering scenario: RegisterSetHandler() is called, the underlying SkeletonMethod::RegisterHandler() fails (returns error), the user checks the returned error and tries again later, but PrepareOffer() / OfferService() is called in between. IsSetHandlerRegistered() returns true despite no handler being registered, leading to set requests arriving with no handler to process them.

The flag should only be set after confirming successful registration:

auto result = set_method_.get()->RegisterHandler(std::move(wrapped_callback));
if (result.has_value()) {
    is_set_handler_registered_ = true;
}
return result;

Context for Agents

[**Bug**] `is_set_handler_registered_` is set to `true` **before** `set_method_.get()->RegisterHandler(...)` is called. If `RegisterHandler` fails and returns an error, the flag still indicates that a handler was successfully registered. This causes `IsSetHandlerRegistered()` to return `true`, which means `PrepareOffer()` in the base class will skip the `kSetHandlerNotSet` check and proceed as if a handler is registered — even though none is active.

Triggering scenario: `RegisterSetHandler()` is called, the underlying `SkeletonMethod::RegisterHandler()` fails (returns error), the user checks the returned error and tries again later, but `PrepareOffer()` / `OfferService()` is called in between. `IsSetHandlerRegistered()` returns `true` despite no handler being registered, leading to set requests arriving with no handler to process them.

The flag should only be set after confirming successful registration:
```cpp
auto result = set_method_.get()->RegisterHandler(std::move(wrapped_callback));
if (result.has_value()) {
    is_set_handler_registered_ = true;
}
return result;
```

File: score/mw/com/impl/skeleton_field.h
Line: 148

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.97
• Likelihood Score: 4/5
• Likelihood Reason: The RegisterHandler call delegates to binding_->RegisterHandler() which returns ResultBlank — it can fail. If it does, is_set_handler_registered_ remains true, causing PrepareOffer() to skip the kSetHandlerNotSet guard. The scenario requires RegisterHandler to fail (binding-dependent), which is plausible in production (e.g., resource exhaustion, binding misconfiguration).
• Suggested Action: keep
• Tool Calls Used: 4

Reasoning

The comment accurately identifies a bug in newly introduced code (lines 147-148 of skeleton_field.h at HEAD):

is_set_handler_registered_ = true;
return set_method_.get()->RegisterHandler(std::move(wrapped_callback));

Verification of claims:

1. Flag set before registration: Confirmed — line 147 sets is_set_handler_registered_ = true unconditionally, and line 148 then calls RegisterHandler() and returns its result.

2. RegisterHandler can fail: Confirmed — SkeletonMethod::RegisterHandler() (line 126-172 in skeleton_method.h) ultimately calls binding_->RegisterHandler(std::move(type_erased_callable)) which returns ResultBlank. The binding layer can return an error.

3. IsSetHandlerRegistered() checks the flag: Confirmed — lines 178-185 show IsSetHandlerRegistered() returns is_set_handler_registered_ when EnableSet is true.

4. PrepareOffer() uses IsSetHandlerRegistered(): Confirmed — lines 67-71 of skeleton_field_base.h show:

if (!IsSetHandlerRegistered())
{
    return MakeUnexpected(ComErrc::kSetHandlerNotSet);
}

If the flag is incorrectly true, this guard is bypassed and the system proceeds as if a handler is registered when none is active.

5. This is new code introduced by this PR: Confirmed — the entire RegisterSetHandler() method, the is_set_handler_registered_ flag, and IsSetHandlerRegistered() override are all newly added in this PR.

6. The fix is concrete and correct: The suggested fix checks the result before setting the flag, which correctly prevents the inconsistent state.

This is a genuine bug introduced by this PR with a clear, actionable fix.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/impl/skeleton_field.h
• Lines: 128-185

128:     // SFINAE-gated: only exists when EnableSet == true
129:     template <bool ES = EnableSet,
130:               typename std::enable_if<ES, int>::type = 0>
131:     ResultBlank RegisterSetHandler(SetHandlerType handler) {
132:         auto wrapped_callback = [this, user_callback = std::move(handler)]
133:                                 (FieldType new_value) -> Result<FieldType> {
134:             // Allow user to validate/modify the value in-place
135:             user_callback(new_value);
136: 
137:             // Store the (possibly modified) value as the latest field value
...

2. File Snapshot (HEAD)

• File: score/mw/com/impl/skeleton_field_base.h

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

3. File Lookup

• Filename: skeleton_method.h

Found 3 file(s) matching 'skeleton_method.h':
- score/mw/com/impl/bindings/lola/skeleton_method.h
- score/mw/com/impl/bindings/mock_binding/skeleton_method.h
- score/mw/com/impl/methods/skeleton_method.h

4. File Snapshot (HEAD)

• File: score/mw/com/impl/methods/skeleton_method.h

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

[propel-code-bot]

[Maintainability] The new is_set_handler_registered_ state is never transferred in the move constructor/assignment: after the move, the flag stays at its default false while the underlying SkeletonMethod (moved via set_method_) still holds the registered handler. When a generated skeleton type is moved—which happens routinely inside SkeletonWrapperClass—subsequent OfferService() calls will fail with ComErrc::kSetHandlerNotSet even though a handler is already installed. Please move/copy the flag alongside set_method_ (and clear it on the moved-from object) in both the move constructor and move assignment so setter-enabled fields remain offerable after object moves.

Context for Agents

The new `is_set_handler_registered_` state is never transferred in the move constructor/assignment: after the move, the flag stays at its default `false` while the underlying `SkeletonMethod` (moved via `set_method_`) still holds the registered handler. When a generated skeleton type is moved—which happens routinely inside `SkeletonWrapperClass`—subsequent `OfferService()` calls will fail with `ComErrc::kSetHandlerNotSet` even though a handler is already installed. Please move/copy the flag alongside `set_method_` (and clear it on the moved-from object) in both the move constructor and move assignment so setter-enabled fields remain offerable after object moves.

File: score/mw/com/impl/skeleton_field.h
Line: 289

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.97
• Likelihood Score: 4/5
• Likelihood Reason: When EnableSet=true and a SkeletonField is moved (which happens routinely per the comment about SkeletonWrapperClass), the is_set_handler_registered_ flag will always be false in the moved-to object, causing OfferService() to fail with kSetHandlerNotSet even though the handler is registered. This will affect any production use of setter-enabled fields that undergo moves.
• Suggested Action: keep
• Tool Calls Used: 3

Reasoning

The comment correctly identifies a bug introduced by this PR:

1. New member is_set_handler_registered_ (line 175): Declared with {} default initialization, meaning false for bool when EnableSet=true.

2. Move constructor (lines 288-298): Moves initial_field_value_, skeleton_field_mock_, and set_method_, but does NOT move/copy is_set_handler_registered_. It stays at its default false.

3. Move assignment operator (lines 305-314): Same issue — moves initial_field_value_, skeleton_field_mock_, and set_method_, but does NOT transfer is_set_handler_registered_.

4. Impact path: RegisterSetHandler() sets is_set_handler_registered_ = true (line 147). After a move, IsSetHandlerRegistered() (line 178-185) returns the default false when EnableSet=true. PrepareOffer() in SkeletonFieldBase (line 67-70) checks IsSetHandlerRegistered() and returns MakeUnexpected(ComErrc::kSetHandlerNotSet) if false.

5. Introduced by this PR: Both is_set_handler_registered_ and the IsSetHandlerRegistered() check are new in this PR. The move constructor/assignment previously didn't need to handle these members. This is clearly a bug introduced by this diff.

6. Actionable: The fix is clear — add is_set_handler_registered_{other.is_set_handler_registered_} to the move constructor's initializer list, add is_set_handler_registered_ = other.is_set_handler_registered_; to the move assignment, and optionally clear the flag on the moved-from object.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/impl/skeleton_field.h
• Lines: 163-200

163: 
164:     std::unique_ptr<FieldType> initial_field_value_;
165:     ISkeletonField<FieldType>* skeleton_field_mock_;
166: 
167:     // Zero-cost conditional storage: unique_ptr when EnableSet=true, zero-size tag when false.
168:     using SetMethodType = std::conditional_t<EnableSet,
169:                                              std::unique_ptr<SkeletonMethod<Result<FieldType>(FieldType)>>,
170:                                              detail::EnableSetOnlyTag>;
171:     SetMethodType set_method_;
172: 
...

2. File Snapshot (HEAD)

• File: score/mw/com/impl/skeleton_field_base.h

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

3. File Snapshot (HEAD)

• File: score/mw/com/impl/skeleton_field.h
• Lines: 286-320

286: }
287: 
288: template <typename SampleDataType, bool EnableSet, bool EnableNotifier>
289: SkeletonField<SampleDataType, EnableSet, EnableNotifier>::SkeletonField(SkeletonField&& other) noexcept
290:     : SkeletonFieldBase(static_cast<SkeletonFieldBase&&>(other)),
291:       // known llvm bug (https://github.com/llvm/llvm-project/issues/63202)
292:       // This usage is safe because the previous line only moves the base class portion via static_cast.
293:       // The derived class member 'initial_field_value_' remains untouched by the base class move constructor,
294:       // so it's still valid to access it here for moving into our own member.
295:       // coverity[autosar_cpp14_a12_8_3_violation] This is a false-positive.
...