-
Notifications
You must be signed in to change notification settings - Fork 379
Fix Enum Constructor Specialization in Generic Types #8996
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
base: master
Are you sure you want to change the base?
Conversation
dc0921f to
49b0816
Compare
49b0816 to
061665a
Compare
tangent-vector
left a comment
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.
This looks great. Thank you again for digging down (well, up in terms of the compiler's dataflow) to the root cause of the problem.
I would like to hear your response to the two questions I raised about the code, but I don't know if either of those can/should turn into an actual code change.
| } | ||
| for (auto decor : innerInst->getDecorations()) | ||
|
|
||
| if (innerInst) |
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.
Do we think this conditional is still good/necessary?
Unless we have a reason to believe that innerInst could be null for valid input code, I would rather see a SLANG_ASSERT(innerInst) than an if(innerInst). Either way, this code seems unrelated to the actual fix.
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.
Looks like we still need to keep this conditional check.
As the innerInst = findInnerMostGenericReturnVal(genInst); could return nullptr for the generic that doesn't have return val ( enum in this case?)
The genInst causing the trouble has only a basic block contains a T Op_Param.
We would have to keep check, unless this inst should not be here in the first place..
gtong-nv
left a comment
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.
Thanks for the feedback! Sorry for the late response. I have addressed the comment and update the way we generate memberDeclRef.
| } | ||
| for (auto decor : innerInst->getDecorations()) | ||
|
|
||
| if (innerInst) |
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.
Looks like we still need to keep this conditional check.
As the innerInst = findInnerMostGenericReturnVal(genInst); could return nullptr for the generic that doesn't have return val ( enum in this case?)
The genInst causing the trouble has only a basic block contains a T Op_Param.
We would have to keep check, unless this inst should not be here in the first place..
f8b6dc7 to
7f3d41a
Compare
Fix Enum Constructor Specialization in Generic Types
This PR fixes an issue where synthesized constructors for enums nested within generic types were failing to specialize correctly.
The Problem
When
slangsynthesizes a requirement witness (like a constructor) for an enum, it creates a new function declaration (synFunc) and adds it to the AST. The original code was creating aDirectDeclRefto this new function:A
DirectDeclRefrepresents a reference to a declaration without any context. It effectively treats the function as if it exists globally or independently.This becomes a problem when the enum is nested within a generic type (e.g.,
struct Test<T> { enum Inner { ... } }). The constructor forInnerinherently depends on the generic parameters of its parent (e.g.,T). When the compiler later attempts to specializeTest<int>, it encounters theDirectDeclRef. Because this reference has no base or parent pointer, the compiler cannot trace it back to the genericTest<T>, and thus has no way to substituteintforT. This results in an invalid reference to an unspecialized generic constructor within a specialized type context, leading to verification failures or incorrect IR.The Fix
The fix is to construct the witness using a
MemberDeclRefthat is rooted at the parent declaration:getDefaultDeclRef(context->parentDecl)retrieves the correct reference to the parent enum (which correctly links back to the generic struct).getMemberDeclRefcreates a reference tosynFuncrelative to that parent.This constructs a proper chain of references:
Constructor -> Enum -> Generic Struct. When the compiler specializesTest<int>, it can now traverse this chain, identify that the constructor is part of a generic instantiation, and correctly apply substitutions to produce the specialized constructor forTest<int>.Inner.This PR also adds a few nullptr checks and removes the duplicated
findWitnessValwithfindWitnessTableEntry.Fixes: #8887