Skip to content

Conversation

alexshtin
Copy link
Contributor

@alexshtin alexshtin commented May 15, 2025

What changed?

CHASM: Add basic ComponentPointer support to CHASM component.

Why?

Part of CHASM workstream. Few things are still pending:

  1. RefC and RefD should return error.
  2. Implementation of RefC and RefD needs to be revisited. It is not clear if it is possible to implement w/o deserializing entire component starting from root and all the way down, because n.value is needed to compare with passed argument.
  3. Dangling pointers protection is not implemented.

How did you test it?

  • built
  • run locally and tested manually
  • covered by existing tests
  • added new unit test(s)
  • added new functional test(s)

@alexshtin alexshtin force-pushed the feature/chasm-component-pointer branch from a0704ae to 685e4f3 Compare May 29, 2025 14:33
@alexshtin alexshtin force-pushed the feature/chasm-component-pointer branch from 685e4f3 to 8b2525a Compare May 30, 2025 06:19
@alexshtin alexshtin requested a review from yycptt May 31, 2025 06:13
@alexshtin alexshtin marked this pull request as ready for review May 31, 2025 06:14
@alexshtin alexshtin requested a review from a team as a code owner May 31, 2025 06:14
switch internal.value().(type) {
case []string: // Pointer
default: // Component | Data
if err = assertStructPointer(reflect.TypeOf(internal.value())); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I think it is the case based on my reading of the code but want to double check: If Component or Data field type, it (T) needs to be pointer to a struct. For Pointer field type, T can be an interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a value of pointer itself. And it must be of []string type. It is set internally, so it is always must be the case.

For other call of the assertStructPointer, it asserts on actual type of the value which is passed to deserialize method, which is always concrete type, even if the field is defined using interface. You can check TestFieldInterface.

Copy link
Member

@yycptt yycptt Jun 2, 2025

Choose a reason for hiding this comment

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

ok I see. The type the T in Field[T] can always be an interface. But when creating a new Field, it needs to be a concrete type (i.e. pointer to struct) when creating the field ().

Then I guess what I want to confirm is: do Component|DataPointerTo() have the same requirement? Looks like the answer is yes because the Ref() impl only check = node.value and node value is always concrete type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the answer is "yes" :-).

In

func ComponentPointerTo[C Component](
	ctx MutableContext,
	c C,
) (Field[C], error) {

C can be and interface, but c will be always an instance of a concrete type which should match the type of what it points to. If they are mismatch, it can be hard to debug, I guess.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks for confirming.

I guess usually the component using Pointer won't actually care or know the concrete type of the component being pointed to. So the use case I have in mind is the component's constructor will just accept an interface and then create a pointer to the passed in interface value, which means c is an interface.
It's easy to support that I think, just need to check for the try to match the underlying value in Ref().

@alexshtin alexshtin enabled auto-merge (squash) June 1, 2025 01:04
@alexshtin alexshtin merged commit e44606a into temporalio:main Jun 1, 2025
52 checks passed
@alexshtin alexshtin deleted the feature/chasm-component-pointer branch June 1, 2025 01:45
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