DYN-10693: Report unresolved calls in Code Block Node function bodies#17224
Open
RobertGlobant20 wants to merge 4 commits into
Open
DYN-10693: Report unresolved calls in Code Block Node function bodies#17224RobertGlobant20 wants to merge 4 commits into
RobertGlobant20 wants to merge 4 commits into
Conversation
An unresolved function call inside a Code Block Node function definition body was silently dropped instead of raising a "function not found" warning. Function bodies are only compiled during the CBN first pass, where the warning is suppressed to allow forward references to functions defined later or in sibling CBNs. Because bodies are never recompiled in the second pass, such warnings were never emitted and the node returned null with no feedback. Instead of suppressing the warning during the first pass, buffer the unresolved call and re-evaluate it once every function definition has been registered (in RecompileCodeBlockAST). Candidates that still fail to resolve are reported; those that now resolve are legitimate forward references and are discarded. Only calls inside function bodies are deferred - top-level calls keep the existing suppress-then-recompile behavior to avoid duplicate warnings. Mirrored in the associative and imperative code paths. Adds regression tests: unresolved call in a function body warns, while forward references within the same CBN and across sibling CBNs do not. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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-10693
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes DYN-10693 by ensuring Code Block Nodes (CBNs) report a FunctionNotFound warning when an unresolved call occurs inside a function definition body. It does this by deferring (buffering) unresolved call candidates during the CBN first-pass compile and re-resolving them after all function definitions are registered, avoiding false positives for forward references.
Changes:
- Buffer unresolved function-body calls during CBN first-pass compilation (Associative + Imperative codegen), instead of suppressing them permanently.
- Add
Coresupport for storing deferred call metadata and re-checking/logging unresolved candidates after function registration. - Add regression tests and new
.dynfixtures covering unresolved-in-body and valid forward-reference cases.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/DynamoCoreTests/CodeBlockNodeTests.cs | Adds regression tests for unresolved calls in function bodies and forward-reference non-warning cases. |
| test/core/dsevaluation/testfunction_nodefn_in_body.dyn | New fixture: unresolved call inside a CBN function body should warn. |
| test/core/dsevaluation/testfunction_forwardref_samecbn.dyn | New fixture: forward reference within same CBN function body should not warn. |
| test/core/dsevaluation/testfunction_forwardref_in_body.dyn | New fixture: forward reference to sibling CBN function should not warn. |
| src/Engine/ProtoImperative/CodeGen.cs | Defers unresolved function-body calls during CBN first pass for the imperative path. |
| src/Engine/ProtoAssociative/CodeGen.cs | Mirrors the same deferred-warning behavior for the associative path. |
| src/Engine/ProtoCore/Core.cs | Introduces deferred-candidate storage and a method to re-resolve/log still-unresolved calls post-registration. |
| src/DynamoCore/Graph/Nodes/CodeBlockNode.cs | Clears/drains per-core deferred candidates per CBN and logs deferred unresolved warnings during second-pass recompilation. |
Comment on lines
+801
to
+806
| try | ||
| { | ||
| // Start each compile with an empty deferred-resolution buffer so unresolved-call | ||
| // candidates never leak across code block nodes (e.g. if a prior compile bailed out | ||
| // before draining it). See DYN-10693. | ||
| libraryServices?.LibraryManagementCore?.DeferredFunctionResolutions.Clear(); |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
- Fix reliability finding: the deferred-buffer clear at the start of ProcessCode used a null-conditional on libraryServices, contradicting the unconditional dereference immediately after. Make it consistent with the surrounding code (libraryServices is non-null on this path). - Reduce code duplication: extract the deferred-candidate creation into Core.AddDeferredFunctionResolution, called from both the associative and imperative code paths instead of duplicating the initializer. - Deduplicate the DYN-10693 regression tests behind a shared OpenSingleFunctionCbn helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // guid used by the function-warning test fixtures. Shared by the DYN-10693 regression tests. | ||
| private CodeBlockNodeModel OpenSingleFunctionCbn(string dynFileName, int expectedNodeCount) | ||
| { | ||
| OpenModelInManualMode(Path.Combine(TestDirectory, @"core\dsevaluation", dynFileName)); |
Contributor
Author
|
GIF showing the expected behavior. |
SonarCloud still flagged the associative and imperative code paths as 100% duplicated because the warn-or-defer block (~16 lines) was nearly identical in both. Extract the entire decision into a single shared Core.LogOrDeferFunctionNotFound method so each code generator calls it in one line, eliminating the duplication. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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
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.
Purpose
Fixes DYN-10693.
A function call that cannot be resolved inside a Code Block Node (CBN) function definition body was silently dropped instead of raising a "Method '<name>()' not found" warning. The node just returned
nullwith no feedback, while the identical call at the CBN top level warned correctly.Root cause: CBNs compile in two passes.
deffunction bodies are only ever compiled during the first pass (IsCodeBlockNodeFirstPass == true), where theFunctionNotFoundwarning is deliberately suppressed so that forward references (to functions defined later in the same CBN or in a sibling CBN not yet compiled) do not produce false positives. Because function bodies are never revisited in the second pass, a genuinely unresolved call inside a body never surfaced a warning.Fix — defer the warning decision instead of suppressing it. During the first pass, an unresolved call inside a function body is buffered on
Core(name, argument types, message, location) rather than dropped. Once every function definition has been registered (inCodeBlockNodeModel.RecompileCodeBlockAST), each buffered candidate is re-resolved against the now-complete function table viaCoreUtils.GetFunctionBySignature:FunctionNotFoundwarning;Only calls inside function bodies are deferred; top-level calls keep the existing suppress-then-recompile behavior (the second pass already re-checks them), which avoids duplicate warnings. The change is mirrored in the associative and imperative code paths, and is inert outside CBN first-pass compilation, so runtime graph evaluation is unaffected.
Note on approach: an earlier iteration fixed this by recompiling the function definitions in the second pass. That approach was discarded in favor of the one in this PR after analyzing the implementation suggestion in the DYN-10693 comments (deferring the warning decision), which is lower risk: it stays confined to warning-emission bookkeeping and does not re-run codegen or mutate the function table. The only refinement over that suggestion is scoping the deferral to function bodies (via
globalProcIndex != kGlobalScope) to prevent double-warning top-level calls.Declarations
Check these if you believe they are true
No public API changes — all new members (
Core.DeferredFunctionResolution,Core.DeferredFunctionResolutions,Core.LogUnresolvedDeferredFunctionWarnings) areinternal.Release Notes
Code Block Nodes now report a "function not found" warning when a function body calls a function that does not exist, instead of silently returning null.
Reviewers
@QilongTang (or as assigned by the team). FYI @jasonstratton — this implements the deferred-warning approach suggested in the DYN-10693 comments.
FYIs
Test coverage added in
CodeBlockNodeTests:The imperative path shares the same mirrored code and is covered by the existing
ImperativeFunctionCall_*regression tests.