DynamoDS/DYN-8473: #15837 String.Concat inconsistent behaviour of subsequent lists#17152
Draft
jnealb wants to merge 7 commits into
Draft
DynamoDS/DYN-8473: #15837 String.Concat inconsistent behaviour of subsequent lists#17152jnealb wants to merge 7 commits into
jnealb wants to merge 7 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8473
… coupling Adds TestConcatStringNestedListInputIsIndependentOfScalarInput to capture the bug where a nested list connected to one input of String.Concat dictates the structure of every subsequent input. The test is [Ignore]'d for now and will be re-enabled once the AST fix lands in TASK 2.
… replication
Special-cases DSCore.String.Concat@string[] in
ZeroTouchVarArgNodeController.BuildOutputAst so that, when the node has
>=2 inputs and is fully applied, the AST is lowered to a left-associative
chain of binary `+` operator calls (s0 + s1 + s2 + ...). Each operand
participates in Dynamo's normal per-argument replication, so a nested-
list input no longer dictates the structure of subsequent inputs and
list-level overrides work as users expect (DYN-8473).
Tests updated to reflect the new, correct behaviour:
- TestConcatStringMultipleInput now pairs parallel flat lists
element-wise ({"abef","cdgh"}) instead of packing them into a single
string[] argument.
- TestConcatStringInvalidInput now asserts the coerced "a1" preview that
DesignScript's `+` operator produces from (string, int) rather than the
old node-level warning.
- TestConcatStringNestedListInputIsIndependentOfScalarInput is re-enabled
and now loads test/core/string/TestConcatString_nestedList.dyn instead
of building the graph programmatically.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| [Test] | ||
| public void TestConcatStringNestedListInputIsIndependentOfScalarInput() | ||
| { | ||
| string testFilePath = Path.Combine(localDynamoStringTestFolder, "TestConcatString_nestedList.dyn"); |
Renames the `params string[]` parameter on `DSCore.String.Concat` from `strings` to `lists` so the variadic input ports surface as `list0`, `list1`, ... — matching the user request in DYN-8473 that the labels reflect that each port can accept either a string or a list of strings (replication happens per-port). The label derivation in ZeroTouchVarInputController/ZeroTouchVarArgNodeController is parameter- name-based, so the rename is the only code change required. The mangled name (`DSCore.String.Concat@string[]`) is type-based and unchanged, so the binary-`+` lowering special case from TASK 2 still matches and existing graphs resolve the same function. VariableInputNode serialization only persists `inputcount` / port GUIDs+state — port Name is not restored from disk — so old `.dyn` graphs serialized with `string0/string1` labels load cleanly with fresh `list0/list1` labels and all wires remain connected. Adds `TestConcatStringPortsAreNamedListN` asserting the first two ports of a loaded String.Concat node are `list0` and `list1`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
| [Test] | ||
| public void TestConcatStringPortsAreNamedListN() | ||
| { | ||
| string testFilePath = Path.Combine(localDynamoStringTestFolder, "TestConcatString_nestedList.dyn"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes https://autodesk.atlassian.net/browse/DYN-8473
Implementation Plan
Goal: Fix String.Concat so list levels are honoured and each concatenation is independent of the previous input, and improve confusing input port names.
Approach: Locate the String.Concat node implementation in the CoreNodes string library, replace/wrap the underlying .NET String.Concat call so that nested-list inputs respect Dynamo list-level semantics (lacing/replication) rather than letting an earlier input's shape dictate subsequent inputs. Rename the variadic input ports from string0, string1, ... to clearer names while preserving backward compatibility ([AlsoKnownAs] or equivalent), and add NUnit tests covering nested-list inputs with and without list levels applied.
TASK 1: Reproduce and locate the defect
Status: DONE
Milestone: A failing NUnit test that demonstrates String.Concat ignoring list levels / coupling subsequent inputs to the first nested input's structure.
Findings:
DSCore.String.Concat(params string[] strings)atsrc/Libraries/CoreNodes/String.cslines 52-61.Dynamo.Graph.Nodes.ZeroTouch.DSVarArgFunction(src/DynamoCore/Graph/Nodes/ZeroTouch/DSVarArgFunction.cs);ZeroTouchVarArgNodeController.BuildOutputAstpacks all input ports into a singlestring[]argument, which couples nested structure on port 0 to ports 1..N.ZeroTouchVarInputController.GetInputNametrims trailingsfrom the last parameter name (strings->string0,string1, ...)..dynfiles:DSCore.String.Concat@string[].TestConcatStringNestedListInputIsIndependentOfScalarInputintest/DynamoCoreTests/Nodes/StringTests.cs(currently[Ignore]'d with DYN-8473 reason; TASK 2 will enable once the fix lands).TASK 2: Fix list-level / replication behaviour
Status: DONE
Milestone: String.Concat honours list levels per input and concatenation of each input is independent of the others; the failing test from TASK 1 now passes.
TestConcatStringNestedListInputIsIndependentOfScalarInput(remove the[Ignore]).Findings:
DSCore.String.Concat@string[]inZeroTouchVarArgNodeController.BuildOutputAst(src/DynamoCore/Graph/Nodes/ZeroTouch/DSVarArgFunction.cs). When the node has >=2 inputs and is fully applied, the AST is lowered to a left-associative chain of binary+operator calls (s0 + s1 + s2 + ...) usingOp.GetOpFunction(Operator.add). Each operand participates in Dynamo's normal per-argument replication, so a nested-list input no longer dictates the structure of subsequent inputs. Partial application and the 1-input case continue to fall through to the originalparams-packing path.TestConcatStringMultipleInputupdated: two parallel flat lists now pair element-wise ({"abef","cdgh"}) instead of being packed into a singlestring[]({"abcd","efgh"}).TestConcatStringInvalidInputupdated:("a", 1)now produces"a1"via DesignScript+coercion instead of a node-level warning (the oldparams string[]path raised a warning on type mismatch).test/core/string/TestConcatString_nestedList.dyn(XML format matching the rest of the folder), with a nested{{"a","b"},{"c","d"}}codeblock on port 0 and a scalar"X"on port 1.Dynamo.Tests.StringTestspass with the fix in place.TASK 3: Clarify input port names
Status: DONE
Milestone: Input ports are named in a way that reflects accepted input (e.g., list0, list1, ... or a similarly clear scheme) without breaking existing graphs.
Findings:
params string[]parameter onDSCore.String.Concatfromstringstolists(src/Libraries/CoreNodes/String.cs). Port labels are derived from the parameter name byZeroTouchVarArgNodeController.InitializeFunctionParameters(arg.Name.Remove(arg.Name.Length - 1) + "0") andZeroTouchVarInputController.GetInputName(Parameters.Last().Name.TrimEnd('s') + index), so the new name surfaces aslist0,list1,list2, ... without any other change.DSCore.String.Concat@string[]is type-based, not name-based, so the special-case inDSVarArgFunction.BuildOutputAst(the binary+chain from TASK 2) continues to match and existing.dyngraphs resolve the same function.inputcount(XML) / the deserialized port list (JSON viaSerializationConverters.setPortDataOnNewPort); portNameis not copied from the deserialized port back onto the new port, and connectors reference port GUIDs (JSON) or indices (XML). Old graphs with serialized labelsstring0/string1therefore load with fresh labelslist0/list1and all wires remain connected..resxlocalisation of port labels/tooltips marked as skipped: the existing CoreNodes pattern derives port labels and tooltips from C# parameter names andType.ToShortString(), with no .resx hook for individual zero-touch parameters. Moving String.Concat's labels into.resxwould be a one-off divergence from the convention used by every other zero-touch node; deferred to a broader effort.TestConcatStringPortsAreNamedListNintest/DynamoCoreTests/Nodes/StringTests.csasserts the first two ports of a loaded String.Concat node are labelledlist0andlist1.Dynamo.Tests.StringTestspass (including the new port-name assertion and the legacy.dynfixtures previously authored withstring0/string1labels).TASK 4: Tests, docs, and API surface
Status: NOT STARTED
Milestone: Test suite covers the fix and the renamed ports; public API additions are recorded; node help artefacts are updated if the node behaviour changed visibly.
Verification