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 <[email protected]>

Signed-off-by: Adam Glustein <[email protected]>

…ance bug Signed-off-by: Adam Glustein <[email protected]>

…ucts

Signed-off-by: Adam Glustein <[email protected]>

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 <[email protected]>

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

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 <[email protected]>

Signed-off-by: Adam Glustein <[email protected]>

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?

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

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

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

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

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

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

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

Add unset field semantics (disc #536)

6884894

Signed-off-by: Simon Beyzerov <[email protected]>

Fix up PR

7bd22fc

Signed-off-by: Adam Glustein <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

robambalu requested changes Dec 23, 2025

View reviewed changes

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AdamGlustein commented Oct 10, 2025 •

edited

Loading

ptomecek commented Oct 27, 2025 •

edited

Loading