Move TaggedValue::ResourceHash to macro definition site

[timon-schelling]

No description provided.

[timon-schelling]

@TrueDoctor any reason not to do this?

[gemini-code-assist]

Code Review

This pull request moves the ResourceHash variant from the auto-generated variants to the manual non-serialized variants within the tagged_value! macro. A critical issue was identified where this change prevents ResourceHash from being downcast back to a TaggedValue in try_from_any and try_from_std_any_ref, which will lead to conversion errors. It is recommended to add manual handling for ResourceHash in both of these methods to avoid this regression.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[cubic-dev-ai]

1 issue found across 1 file

Confidence score: 2/5

• In node-graph/graph-craft/src/document/value.rs, try_from_std_any_ref is missing a ResourceHash match arm, so std::any::Any downcasts for that type can fail at runtime and break value conversion paths in normal execution — add the ResourceHash arm (mirroring the fix for try_from_any) and validate with a targeted conversion test before merging.

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[0HyperCube]

Why move to the variant macro definition site?

The tagged_value macro will accept the #[serde(skip)] and use it in the definition of the enum variant (as you can see for other variants such as DAffine2).

This approach seems to just add more duplicated code. If you do not care about the duplicated code, you might as well get rid of the tagged_value macro.

[timon-schelling]

Why move to the variant macro definition site?

Not 100% sure, @Keavon said I should have done this when originally introducing this variant.

[TrueDoctor]

I agree with @0HyperCube, I don't see a technical reason for this change and if this is purely about code organization, there are other way of achieving that

[Keavon]

This is a runtime-only, ephemeral variant that is not serialized, so this fixes its incorrect placement amongst the serialized variants to be now correctly amongst its runtime-only variants. That was the original purpose of splitting these up between the ones that describe value node input widget persisted values in a .graphite document versus those that are internal machinery.

[TrueDoctor]

This is a runtime-only, ephemeral variant that is not serialized, so this fixes its incorrect placement amongst the serialized variants to be now correctly amongst its runtime-only variants. That was the original purpose of splitting these up between the ones that describe value node input widget persisted values in a .graphite document versus those that are internal machinery.

Then the original split was not the right way to enforce this. The current solution seems pretty fragile with a non compiler enforced invariant upheld by you. If this is something you have strong opinions on, we should either add nesting to the tagged value, or enforce this in the generation macro using two input lists

[Keavon]

I like the latter option.