-
Notifications
You must be signed in to change notification settings - Fork 345
Fix FunctionReflection getName() returning null for overloaded functions #8113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com>
Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com>
@@ -0,0 +1,90 @@ | |||
// unit-test-find-function-by-name-in-type-overloaded.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to add a new test file, just amend unit-test-function-lookup-resolution.cpp
with a SLANG_CHECK()
to call getName on func1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Removed the separate test files and added SLANG_CHECK()
calls to verify getName()
works on func1
in the existing unit-test-function-lookup-resolution.cpp
. The test passes successfully. (commit: 9934ebd)
Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com>
@tdavidovicNV for viz. @ArielG-NV can you review? |
I'll review the PR right away. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 suggestion for code-styling
Changes look good.
Co-authored-by: ArielG-NV <159081215+ArielG-NV@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-approval
/format |
🌈 Formatted, please merge the changes from this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapproval
…ons (shader-slang#8113) When using Slang reflection API to find functions by name in a type, `FunctionReflection::getName()` would return `nullptr` for overloaded functions instead of the expected function name. The issue occurred in `spReflectionFunction_GetName` when `findFunctionByNameInType` returned a `SlangReflectionFunction` wrapping an `OverloadedExpr` (for overloaded functions) instead of a single `DeclRef<FunctionDeclBase>`. The `convertToFunc()` function would fail for `OverloadedExpr` objects, causing `getName()` to return `nullptr`. **Example of the bug:** ```cpp // This code would fail before the fix public interface IBase { public void step(inout float f); } public struct Impl : IBase { public void step(inout float f) { f += 1.0f; } } // Using reflection API: auto implType = reflection->findTypeByName("Impl"); auto stepFunction = reflection->findFunctionByNameInType(implType, "step"); auto name = stepFunction->getName(); // Would return nullptr ``` **Fix:** Modified `spReflectionFunction_GetName` to handle overloaded functions by falling back to the name of the first overload candidate when `convertToFunc` fails. This follows the same pattern already used by `spReflectionFunction_specializeWithArgTypes`. The fix ensures that: - Overloaded function containers return the correct function name - Individual overload candidates also return their names correctly - Non-overloaded functions continue to work as before - No regression in existing functionality **Testing:** Added comprehensive test cases covering both the original issue scenario and explicit function overloading. All existing reflection tests continue to pass. Fixes shader-slang#8047. <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> Co-authored-by: Yong He <yonghe@outlook.com> Co-authored-by: ArielG-NV <159081215+ArielG-NV@users.noreply.github.com> Co-authored-by: slangbot <ellieh+slangbot@nvidia.com> Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com>
When using Slang reflection API to find functions by name in a type,
FunctionReflection::getName()
would returnnullptr
for overloaded functions instead of the expected function name.The issue occurred in
spReflectionFunction_GetName
whenfindFunctionByNameInType
returned aSlangReflectionFunction
wrapping anOverloadedExpr
(for overloaded functions) instead of a singleDeclRef<FunctionDeclBase>
. TheconvertToFunc()
function would fail forOverloadedExpr
objects, causinggetName()
to returnnullptr
.Example of the bug:
Fix:
Modified
spReflectionFunction_GetName
to handle overloaded functions by falling back to the name of the first overload candidate whenconvertToFunc
fails. This follows the same pattern already used byspReflectionFunction_specializeWithArgTypes
.The fix ensures that:
Testing:
Added comprehensive test cases covering both the original issue scenario and explicit function overloading. All existing reflection tests continue to pass.
Fixes #8047.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.