Resolve requirements through lookup-backed witnesses - #12752
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughChangesWitness lookup resolution
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized compiler change resolves lookup-backed associated-type witnesses without any identified merge-blocking risk; it is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The change satisfies issue ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 🟡 Has issues — 2 gap(s) + 1 clarity note
This PR teaches tryLookUpRequirementWitness to resolve requirements through LookupDeclRef-backed declared subtype witnesses by delegating to the canonical getUnspecializedLookupRec / specializeLookedUpRec helpers (the path already used by LookupDeclRef::tryResolve). The delegation and branch ordering are sound and security/recursion concerns were checked and cleared, but the new early-return on a none result changes control flow for every lookup-backed witness, and the fix ships without a non-GPU regression test.
Changes Overview
Lookup-backed witness resolution (source/slang/slang-syntax.cpp)
- What changed: adds a branch to
tryLookUpRequirementWitnessthat detects aDeclaredSubtypeWitnesswhose decl-ref base is aLookupDeclRefand routes it through the shared recursive lookup/specialization helpers, so a nested associated-type conformance selected through a witness-table lookup (IContext.Primitive : IPrimitive→Attributes) resolves instead of returning an empty witness. Fixes #12751.
Findings (3 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | source/slang/slang-syntax.cpp:741 |
No non-GPU regression test for the changed path; existing look-alike CPU/INTERPRET tests apparently don't trigger the branch — verify via revert drill |
| 🟡 Gap | source/slang/slang-syntax.cpp:757 |
Early return RequirementWitness() on a none lookup exits the function, skipping the ThisType/ThisTypeConstraint fallbacks at lines 828/836 for lookup-backed witnesses |
| 🔵 Clarity | source/slang/slang-syntax.cpp:754-758 |
Undocumented flavor-forwarding divergence from the two other callers; local requirementWitness names an unspecialized entry |
reviewed: e1374a8 · diff sha256 764b70b0c639
|
|
||
| if (auto declaredSubtypeWitness = as<DeclaredSubtypeWitness>(subtypeWitness)) | ||
| { | ||
| if (as<LookupDeclRef>(declaredSubtypeWitness->getDeclRef().declRefBase)) |
There was a problem hiding this comment.
🟡 Gap: No regression test for the changed resolution path
This PR changes how tryLookUpRequirementWitness resolves a requirement through a LookupDeclRef-backed declared witness (the IContext.Primitive : IPrimitive → Attributes chain in the comment), but adds no test under tests/. tmp/pr-files.txt lists only source/slang/slang-syntax.cpp. Per CLAUDE.md ("Include tests: Add regression tests as .slang files under tests/"), and because the PR body states the observable coverage is downstream ray-tracing tests (Vulkan/D3D12/OptiX), the changed path is not exercised by the default non-GPU CI. A future regression in this branch would pass all non-GPU CI and only surface as a broken shader.
This scenario is expressible without a GPU (associated-type resolution happens at semantic-check/lowering time), so a -cpu or INTERPRET regression test is feasible.
Important caveat: the repo already has near-identical non-GPU tests — tests/compute/assoctype-complex.slang (-cpu, nested U : IBase / U.V) and tests/language-feature/interfaces/derived-interface-constraint-multilevel.slang (INTERPRET, multi-level A.B.C) — that apparently do not trigger this branch (they pass without this fix). Please do the revert drill: confirm the new test actually fails without this branch, and identify what distinguishes the failing shape (likely a concrete, non-generic context whose Primitive : IPrimitive witness resolves via a witness-table lookup) from those existing look-alikes, so the test provides real coverage rather than a false sense of it.
Suggestion: add a test modeled on the comment's IPrimitive/IContext example that resolves the nested associated type and verify (revert drill) it regresses without this branch.
| auto requirementWitness = | ||
| getUnspecializedLookupRec(astBuilder, requirementKey, declaredSubtypeWitness); | ||
| if (requirementWitness.getFlavor() == RequirementWitness::Flavor::none) | ||
| return RequirementWitness(); |
There was a problem hiding this comment.
🟡 Gap: Early return bypasses the ThisType/ThisTypeConstraint fallbacks for every lookup-backed witness
The new branch's guard (if (as<LookupDeclRef>(declaredSubtypeWitness->getDeclRef().declRefBase))) depends only on the witness's decl-ref shape, not on requirementKey, so it is entered for any requirement key on a lookup-backed witness — including ThisTypeDecl/ThisTypeConstraintDecl. getUnspecializedLookupRec only resolves keys stored in a witness-table requirement dictionary, and the ThisType requirement is deliberately not stored there. For those keys it returns none, and this branch then return RequirementWitness()s — exiting the whole function and skipping the shared tail fallbacks at lines 828 (if (as<ThisTypeDecl>(requirementKey)) → returns subtypeWitness->getSub()) and 836 (ThisTypeConstraintDecl → returns the witness itself).
Before this PR these keys reached the tail: a lookup-backed witness either skipped the InheritanceDecl branch entirely, or entered it, missed the dictionary lookup, and fell through — either way landing on line 828/836. After this PR the same query returns an empty RequirementWitness.
Example: tryLookUpRequirementWitness(lookupBackedWitness, IPrimitive.ThisTypeDecl) for the PR's own Primitive : IPrimitive conformance now returns none instead of getSub(), which can surface downstream as an unresolved this-type. (I have not proven this exact query is issued in practice on a lookup-backed witness, so this is a definite control-flow divergence whose reachability I could not confirm — a This-type resolution test on the lookup-backed shape would settle it.)
Suggested fix: don't consume control flow when the lookup yields nothing — let none fall through to the shared tail:
auto requirementWitness =
getUnspecializedLookupRec(astBuilder, requirementKey, declaredSubtypeWitness);
if (requirementWitness.getFlavor() != RequirementWitness::Flavor::none)
return specializeLookedUpRec(astBuilder, declaredSubtypeWitness, requirementWitness);
// else fall through to the ThisType / ThisTypeConstraint special cases belowNote this requires restructuring so the none case continues to lines 827-842 rather than returning inside the declaredSubtypeWitness block.
| auto requirementWitness = | ||
| getUnspecializedLookupRec(astBuilder, requirementKey, declaredSubtypeWitness); | ||
| if (requirementWitness.getFlavor() == RequirementWitness::Flavor::none) | ||
| return RequirementWitness(); | ||
| return specializeLookedUpRec(astBuilder, declaredSubtypeWitness, requirementWitness); |
There was a problem hiding this comment.
🔵 Clarity: document the flavor-forwarding divergence and rename the unspecialized local
Two small readability points on this block:
-
Flavor divergence from the other callers. The two existing callers of this helper pair guard the
specializeLookedUpReccall by flavor:LookupDeclRef::tryResolvecalls it only forval/declRef, andWitnessLookupIntVal::tryFoldOrNullonly forval. This branch forwards any non-noneflavor, includingwitnessTable. That is in fact correct here —tryResolvereturnsVal*(which awitnessTablecannot be, so it declines that flavor), whereastryLookUpRequirementWitnessreturns aRequirementWitnessthat legitimately can be a witness table, andspecializeLookedUpRec→RequirementWitness::specializehandlesFlavor::witnessTable(slang-syntax.cpp:693). But a reader comparing the three call sites cannot infer that without the trace. A one-line comment stating that awitnessTableresult is expected and specialized viaWitnessTable::specializewould prevent the divergence from reading as an oversight. -
Local name.
requirementWitnessholds the unspecialized lookup result that is then specialized. The other two callers name the equivalent variableunspecializedEntry/lookedUpVal, reserving "requirement witness" for the fully-resolved result. ConsiderunspecializedEntryhere for consistency with the unspecialized-vs-specialized distinction the surrounding code relies on.
2 participants
Motivation
Consider this checked source shape:
After a concrete context selects its primitive, the compiler can obtain the witness for
Primitive : IPrimitive. A later call totryLookUpRequirementWitnessshould then obtainAttributes, but it returns no witness when that conformance is represented by aLookupDeclRef. This blocks consumers that query an already-checked nested associated-type conformance. Fixes #12751.Proposed solution
When a declared subtype witness is lookup-backed, resolve the requirement with the same
getUnspecializedLookupRecandspecializeLookedUpRecpath already used byLookupDeclRef::tryResolve. This keeps the witness table as the semantic source of truth and applies the substitutions recorded along the lookup path.Change summary
tryLookUpRequirementWitnessto traverse lookup-backed declared subtype witnesses.Concepts and vocabulary
DeclaredSubtypeWitnesswhose declaration reference reaches an inheritance declaration through a witness-table lookup.IPrimitive.Attributes, used to select a witness-table entry.Process report
The front end already produces a valid conformance. For a concrete
IContext, its witness table stores the selectedPrimitivetype and the constraint witness forPrimitive : IPrimitive. That constraint witness is intentionally represented by aLookupDeclRef; ordinary declaration lookup already handles this shape inLookupDeclRef::tryResolve.The failure was in the consumer.
tryLookUpRequirementWitnessonly handled a declared witness when its declaration reference directly cast toInheritanceDecl, so the second lookup stopped before reading the primitive witness table. The fix detects the valid lookup-backed form and sends it through the existing recursive lookup helpers. It does not scan concrete declarations, rebuild checked syntax, or add a second equivalence rule.The downstream structural ray-tracing tests provide the observable regression: removing this branch makes 25 procedural-layout directives fail while resolving the primitive attribute type; retaining it restores all 216 directives. On this isolated branch, a clean Debug
slangcandslang-testbuild passed. A focused associated-type/interface run passed all compile, Vulkan, and CUDA cases; 17 LLVM execution cases could not launch the local LLVM runtime and produced no shader output.