Add unset field semantics for structs ("strict structs") #607

AdamGlustein · 2025-10-10T18:04:49Z

Cleanup and a few fixes to #574, thanks @sim15 for the contribution!

Signed-off-by: Simon Beyzerov <simon.beyzerov@point72.com>

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

…ance bug Signed-off-by: Adam Glustein <adam.glustein@point72.com>

…ucts

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

docs/wiki/api-references/csp.Struct-API.md

ptomecek · 2025-10-27T13:08:20Z

Previously, when all fields on structs were optional, we hard-coded these two flags for pydantic validation

Can we only apply this logic when allow_unset=True?
I think that when allow_unset is False, pydantic should infer it correctly based on the declared type and the default value, though we should add a unit test.

AdamGlustein · 2025-11-03T18:37:23Z

Previously, when all fields on structs were optional, we hard-coded these two flags for pydantic validation

https://github.com/Point72/csp/blob/strict-structs/csp/impl/struct.py#L125

https://github.com/Point72/csp/blob/strict-structs/csp/impl/struct.py#L130

Can we only apply this logic when allow_unset=True? I think that when allow_unset is False, pydantic should infer it correctly based on the declared type and the default value, though we should add a unit test.

I tested out this approach and it doesn't validate correctly in the case of a required field with a default value.

class MyStrictStruct(csp.Struct, allow_unset=False):
            req_default_str: str = "default"

E       pydantic_core._pydantic_core.SchemaError: Error building "list" validator:
E         SchemaError: Error building "function-wrap" validator:
E         SchemaError: Error building "typed-dict" validator:
E         SchemaError: Field 'req_default_str': a required field cannot have a default value

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

robambalu · 2025-11-10T16:53:38Z

cpp/csp/engine/InputAdapter.h

 template<typename T>
 bool InputAdapter::consumeTick( const T & value )
 {
+    if constexpr( CspType::Type::fromCType<T>::type == CspType::TypeTraits::STRUCT )


is there a central place we can do this check on push rather than consume? Will be much harder to track down on consumption

I did not see such a place - we could add it to each adapter type (push, pull, pushpull and managers) separately in their pushTick impls, but I think it's cleaner to have the validation logic in one spot.

I agree, but my point stands that it would be a lot harder to tell where the error is coming from
Also unfortunate to vall validate on all input adapters, even python based ones which were already validated due to PyStruct creation in python

As to the second point here, we could store a "dirty" flag on the struct to avoid validating it twice if it has a) been validated already and b) not had any modifications since the last validation.

And actually, since you can't delete fields on a strict struct, you only ever need to validate it once so point b) is irrelevant.

right, so we could store a bit to indicate its already validated, but im not sure its necessary at this point since we almost always be validating once up front

Really we can skip the validation check in InputAdapter::consume for any PyStruct.
In a somewhat hacky way we already have this info as we could check struct -> dialectPtr() != nullptr

If we are concerned about the double validation I would go with adding a validated bit, but maybe in a separate PR

cpp/csp/engine/Struct.h

cpp/csp/engine/Struct.cpp

csp/impl/struct.py

cpp/csp/python/PyStruct.cpp

csp/impl/wiring/edge.py

cpp/csp/python/PyStruct.cpp

ptomecek · 2025-11-11T16:31:37Z

Previously, when all fields on structs were optional, we hard-coded these two flags for pydantic validation

https://github.com/Point72/csp/blob/strict-structs/csp/impl/struct.py#L125

https://github.com/Point72/csp/blob/strict-structs/csp/impl/struct.py#L130

Can we only apply this logic when allow_unset=True? I think that when allow_unset is False, pydantic should infer it correctly based on the declared type and the default value, though we should add a unit test.

I tested out this approach and it doesn't validate correctly in the case of a required field with a default value.
class MyStrictStruct(csp.Struct, allow_unset=False):
            req_default_str: str = "default"
E       pydantic_core._pydantic_core.SchemaError: Error building "list" validator:
E         SchemaError: Error building "function-wrap" validator:
E         SchemaError: Error building "typed-dict" validator:
E         SchemaError: Field 'req_default_str': a required field cannot have a default value

I think this just means that if there is a default value, we leave it as "optional" (as the user doesn't have to provide it), but if there isn't, then it's not. I guess I was wrong about pydantic inferring it. However, the behavior for non-defaulted fields will be different for the two types of Struct.

…ucts

…ield is set to None in the struct's bitmask Signed-off-by: Adam Glustein <adam.glustein@point72.com>

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

robambalu · 2025-12-16T21:31:47Z

cpp/csp/adapters/websocket/ClientAdapterManager.cpp

+                    PushBatch batch( m_engine -> rootEngine() );
+                    m_inputAdapter -> processMessage( c, t, &batch );
+                }
+                catch( csp::Exception & err )


why is this try/catch here now if we are throwing in consume?

I don't think this is doing anything, right. Reverted the change here

robambalu · 2025-12-16T21:34:31Z

cpp/csp/engine/InputAdapter.h

+    if constexpr( CspType::Type::fromCType<T>::type == CspType::TypeTraits::STRUCT )
+    {
+        if( unlikely( !( value -> validate() ) ) )
+            CSP_THROW( ValueError, "Struct " << value -> meta() -> name() << " is not valid; required fields " << value -> formatAllUnsetStrictFields() << " were not set on init" );


Since we're likely leaving this in consume, can we add something i meant to add.
Please add a virtual const char * name() const; method on InputAdapter, can default it to return "InputAdapter"
we should really implement name where we can, and use name() in the exception here

for purposes of this PR we can just define name() as "InputAdapter" and separately we can implement it

Done and added to the error message

robambalu · 2025-12-16T21:42:06Z

cpp/csp/engine/Struct.cpp

    m_partialSize  = m_size - baseSize;
    m_maskLoc      = m_size - m_maskSize;
+
+    uint8_t numRemainingBits = ( m_fields.size() - m_firstPartialField + optionalFieldCount ) % 8;


looks like m_firstPartialField is always 0 at this point.
In any case I dont think its needed here

Not needed and removed

robambalu · 2025-12-16T22:35:21Z

cpp/csp/engine/Struct.cpp

-    for( auto & f : m_fields )
+
+    // Set optional fields first so that their 2-bits never cross a byte boundary
+    m_optionalFieldsSetBits  = new uint8_t[ m_maskSize ]();


std::unique_ptr or just use std::vector
also consider merging into one allocation for both masks to avoid potential fragmentation

Changed to use vectors. I could store it as one vector containing both the set bitmasks and the none bitmasks, but I think the added complexity with that approach is not really worth it, considering this is a member of the meta.

robambalu · 2025-12-16T22:37:18Z

cpp/csp/engine/Struct.cpp

+        }
+    }
+    // deal with last (partial) byte filled with optional fields
+    auto lastOptIndex = maskLoc - m_maskLoc;


i forget if we have a way to run coverage right now but would be good to ensure all of these code paths are being tested or at least executed

I simplified this logic a lot to just set the bits programmatically rather than using these precalculated constants, not sure why I didn't do that to start with

it is a member of the meta so space isnt a concern, its more the frequent access of these vectors.
if the allocations did happen to fragment it would be unfortunate

Ok I see what you mean, if those vectors get allocated on separate cachelines then each validate call will be loading in both.
I am fine to make it a single vector and just use offset logic to access each mask.

I made it so that all the set/none masks are in the same vector.

robambalu · 2025-12-23T16:17:53Z

cpp/csp/python/PyStructToDict.cpp

+        if( field -> isNone( struct_ptr ) )
+            py_obj = PyObjectPtr::incref( Py_None );
+        else
+            py_obj = switchCspType( field -> type(), [field, struct_ptr, this]( auto tag )


if we do keep None processing, brackets around multi-line clause

robambalu · 2025-12-23T16:20:21Z

cpp/csp/python/PyStructToJson.cpp

+    for( const auto & field: fields )
    {
-        if( !field -> isSet( struct_ptr ) )
+        if( !field -> isSet( struct_ptr ) && !field -> isNone( struct_ptr ) )


Same Q, do we want to propagate None here

I don't see what else we would do, just treat it as unset? Makes more sense to pass it through as a null for consistency.

robambalu · 2025-12-23T16:25:03Z

docs/wiki/api-references/csp.Struct-API.md

+
+- **Default:** `False` (for backwards compatibility)
+- **When set to `True`:** Enforces *strict struct semantics* — required fields must be set at initialization.
+  - Note that `del struct.field` is not allowed for strict structs — this will raise an error. As a result, for any strict struct, `hasattr(struct, field)` always returns True for any defined field.


I would move these two bullets below to the semantics section

robambalu · 2025-12-23T16:25:32Z

docs/wiki/api-references/csp.Struct-API.md

+
+- **Default:** `False` (for backwards compatibility)
+- **When set to `True`:** Enforces *strict struct semantics* — required fields must be set at initialization.
+  - Note that `del struct.field` is not allowed for strict structs — this will raise an error. As a result, for any strict struct, `hasattr(struct, field)` always returns True for any defined field.


for any strict struct, hasattr(struct, field) always returns True for any defined field.
" and never has to be checked"

robambalu · 2025-12-23T16:26:05Z

docs/wiki/api-references/csp.Struct-API.md

+1. **Example**
+
+   ```python
+   class MyStruct(csp.Struct, allow_unset=False):


fix param name

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

Simon Beyzerov and others added 4 commits September 16, 2025 12:03

Add unset field semantics (disc #536)

6884894

Signed-off-by: Simon Beyzerov <simon.beyzerov@point72.com>

Fix up PR

7bd22fc

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

Merge remote-tracking branch 'origin' into strict-structs

5b318c1

Further clean up of PR; add docs, improve error messages, fix inherit…

c4c14ed

…ance bug Signed-off-by: Adam Glustein <adam.glustein@point72.com>

AdamGlustein requested review from alexddobkin, robambalu and svatasoiu as code owners October 10, 2025 18:04

AdamGlustein mentioned this pull request Oct 10, 2025

allow opt-in for new unset field semantics on csp.Struct (disc #536) #574

Closed

AdamGlustein added 2 commits October 10, 2025 14:06

Merge branch 'main' of https://github.com/Point72/csp into strict-str…

34619f4

…ucts

Change lint to use 3.11

e5889d2

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

AdamGlustein requested review from ptomecek and timkpaine as code owners October 10, 2025 18:16

timkpaine added the type: enhancement Issues and PRs related to improvements to existing features label Oct 10, 2025

Fix 3.10+ Union annotations

da7599d

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

ptomecek reviewed Oct 27, 2025

View reviewed changes

docs/wiki/api-references/csp.Struct-API.md Outdated Show resolved Hide resolved

AdamGlustein requested a review from ptomecek November 3, 2025 20:59

Fix typo in docs

05d6ea7

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

robambalu reviewed Nov 11, 2025

View reviewed changes

cpp/csp/python/PyStruct.cpp Outdated Show resolved Hide resolved

robambalu reviewed Nov 11, 2025

View reviewed changes

cpp/csp/python/PyStruct.cpp Outdated Show resolved Hide resolved

Merge branch 'main' of https://github.com/Point72/csp into strict-str…

02ab5f3

…ucts

AdamGlustein marked this pull request as draft November 19, 2025 21:23

Store optionality on the field object, and embed whether or not the f…

c343b5a

…ield is set to None in the struct's bitmask Signed-off-by: Adam Glustein <adam.glustein@point72.com>

AdamGlustein force-pushed the strict-structs branch from b86157d to c343b5a Compare November 21, 2025 14:38

More PR comments

4a16bab

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

AdamGlustein marked this pull request as ready for review November 21, 2025 17:46

AdamGlustein requested a review from robambalu November 21, 2025 17:46

Fix str/repr for strict structs

b0d3741

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

robambalu requested changes Dec 23, 2025

View reviewed changes

AdamGlustein added 3 commits December 24, 2025 11:45

WIP PR comments

ed8318d

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

PR comments

cbe70bc

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

Store set and none masks in the same vector, provide accessors

72338cc

Signed-off-by: Adam Glustein <adam.glustein@point72.com>

Add unset field semantics for structs ("strict structs") #607

Are you sure you want to change the base?

Add unset field semantics for structs ("strict structs") #607

Uh oh!

Conversation

AdamGlustein commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ptomecek commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AdamGlustein commented Nov 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ptomecek commented Nov 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamGlustein commented Oct 10, 2025 •

edited

Loading

ptomecek commented Oct 27, 2025 •

edited

Loading