Add tests for dynamic dispatch with generic interfaces (#9885)#10075
Add tests for dynamic dispatch with generic interfaces (#9885)#10075NV-xiaoyongs wants to merge 7 commits intomasterfrom
Conversation
Add 7 test files covering the full scope of generic interface dynamic dispatch: - generic-interface-4: ICalculator<float> with Adder/Multiplier (basic scenario) - generic-interface-5: ICalculator<int> with 3 concrete types - generic-interface-6: ITransform<Vec2> with user-defined struct type argument - generic-interface-7: IConverter<T,U> with multiple type parameters - generic-interface-8: diagnostic test for createDynamicObject<ICalculator<float>> (error 39999) - generic-interface-9: IAccumulator<each Ts> with variadic generics - generic-interface-10: nested IProcessor<T> taking IValue<T> parameter All compute tests run on slang, vk, cuda, and cpu backends. Discovered that createDynamicObject does not support specialized generic interfaces.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds seven Slang shader tests that exercise dynamic dispatch for generic interfaces across single- and multi-parameter generics, variadic generics, user-defined types, nested generics, and createDynamicObject error cases; each file introduces interfaces, implementations, factory functions, test helpers, and compute kernels or compile-time checks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Claude finished @NV-xiaoyongs's task in 7m 20s —— View job Review of PR #10075
SummaryTest-only PR adding 7 new
Key FindingsAll expected values verified correct:
No bugs, correctness issues, or design concerns found. No inline comments needed. Maintainer feedback addressed: generic-interface-8 now covers all 3 cases @csyonghe requested — concrete specialization ( Positive Aspects
Overall AssessmentClean test-only PR. All expected values are mathematically correct, FileCheck patterns follow existing conventions, and the diagnostic test properly addresses the maintainer's review feedback. LGTM. |
There was a problem hiding this comment.
Pull request overview
Adds a suite of Slang language-feature tests to validate dynamic dispatch behavior when interfaces are generic (including multi-parameter, variadic packs, nested composition), plus a diagnostic for an unsupported createDynamicObject case.
Changes:
- Added compute tests that exercise dynamic dispatch for specialized generic interfaces across multiple backends (slang/vk/cuda/cpu).
- Added coverage for generic type arguments beyond scalars (user-defined struct), multiple type parameters, and variadic generics.
- Added a negative/diagnostic test asserting
createDynamicObject<ICalculator<float>>fails with error39999.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/language-feature/dynamic-dispatch/generic-interface-4.slang | Baseline dynamic dispatch test for ICalculator<float> with 2 concrete implementations |
| tests/language-feature/dynamic-dispatch/generic-interface-5.slang | Extends generic-interface dispatch to ICalculator<int> with 3 concrete types |
| tests/language-feature/dynamic-dispatch/generic-interface-6.slang | Validates dynamic dispatch through a generic interface specialized on a user-defined Vec2 |
| tests/language-feature/dynamic-dispatch/generic-interface-7.slang | Exercises dynamic dispatch for a 2-parameter generic interface IConverter<T,U> |
| tests/language-feature/dynamic-dispatch/generic-interface-8.slang | Diagnostic test asserting createDynamicObject fails for a specialized generic interface |
| tests/language-feature/dynamic-dispatch/generic-interface-9.slang | Validates dynamic dispatch for variadic generic interfaces (each Ts) |
| tests/language-feature/dynamic-dispatch/generic-interface-10.slang | Tests nested/composed generic interfaces where one interface method consumes another interface type |
| @@ -0,0 +1,43 @@ | |||
| // createDynamicObject does not currently support generic interface types. | |||
There was a problem hiding this comment.
The comment says createDynamicObject does not support generic interface types, but the test is specifically about a specialized generic interface (ICalculator<float>). Consider rewording to avoid implying that non-specialized generic interfaces are unsupported in general, e.g. 'does not currently support specialized generic interface types'.
| // createDynamicObject does not currently support generic interface types. | |
| // createDynamicObject does not currently support specialized generic interface types. |
|
|
||
| struct SquareProcessor : IProcessor<float> | ||
| { | ||
| float process(IValue<float> input) { return input.get() * input.get(); } |
There was a problem hiding this comment.
This calls input.get() twice, which can change semantics for other IValue<float> implementations and also introduces an extra interface call. Consider storing the result once (e.g., let x = input.get(); return x * x;) to keep the nested-dispatch test deterministic and easier to reason about.
| float process(IValue<float> input) { return input.get() * input.get(); } | |
| float process(IValue<float> input) | |
| { | |
| let x = input.get(); | |
| return x * x; | |
| } |
| float process(IValue<float> input) { return input.get() + 1.0; } | ||
| } | ||
|
|
||
| struct SquareProcessor : IProcessor<float> | ||
| { | ||
| float process(IValue<float> input) { return input.get() * input.get(); } |
There was a problem hiding this comment.
This test is described as 'nested generic interfaces', but it only asserts a dispatch-site report at the outer call (proc.process(val)). Since nested dispatch is exercised via input.get() inside the processors, consider adding REPORT/REPORT-DAG expectations at the input.get() call sites too, so the test explicitly verifies the inner interface dispatch is generated.
| float process(IValue<float> input) { return input.get() + 1.0; } | |
| } | |
| struct SquareProcessor : IProcessor<float> | |
| { | |
| float process(IValue<float> input) { return input.get() * input.get(); } | |
| float process(IValue<float> input) { return input.get() + 1.0; } | |
| // REPORT: ([[# @LINE-1]]): {{.*}} generated {{.*}} dispatch code{{.*}} 2 possible types: | |
| } | |
| struct SquareProcessor : IProcessor<float> | |
| { | |
| float process(IValue<float> input) { return input.get() * input.get(); } | |
| // REPORT: ([[# @LINE-1]]): {{.*}} generated {{.*}} dispatch code{{.*}} 2 possible types: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds 7 new test files covering generic interface dynamic dispatch scenarios, expanding coverage from the existing generic-interface-{1,2,3} tests. The new tests cover:
| Test | Scenario |
|---|---|
generic-interface-4 |
ICalculator<float> with 2 concrete types (basic) |
generic-interface-5 |
ICalculator<int> with 3 concrete types |
generic-interface-6 |
ITransform<Vec2> with user-defined struct type argument |
generic-interface-7 |
IConverter<T,U> with multiple type parameters |
generic-interface-8 |
Diagnostic test: createDynamicObject with specialized generic interface (error 39999) |
generic-interface-9 |
IAccumulator<each Ts> with variadic generics |
generic-interface-10 |
Nested IProcessor<T> taking IValue<T> parameter (composed interfaces) |
Key Findings
All expected values verified correct:
- generic-interface-4: Adder(3,4)=7, Multiplier(3,4)=12 ✓
- generic-interface-5: IntAdder(10,3)=13, IntMultiplier(10,3)=30, IntSubtractor(10,3)=7 ✓
- generic-interface-6: ScaleTransform(3,4)→reduce(6,8)=14, NegateTransform(3,4)→reduce(-3,-4)=12 ✓
- generic-interface-7: IntToFloatScale(4)=10.0, IntToFloatNegate(4)=-4.0 ✓
- generic-interface-9: SumTwo(3,4)=7.0, ProductTwo(3,4)=12.0 ✓
- generic-interface-10: All 4 processor×value combos verified ✓
No bugs or correctness issues found.
Minor Observations (non-blocking)
-
Broader backend coverage: The new tests run on 4 backends (
-slang,-vk,-cuda,-cpu) while the existinggeneric-interface-{1,2,3}only run on-slang. This is good for coverage, though it will increase CI time. -
Error 39999 fragility in generic-interface-8: The diagnostic test checks for
error 39999, which is a shared error code used by many different diagnostics (cyclic references, expected constant, no applicable overload, etc.). The test would pass even if the error origin changed to a completely different diagnostic. Consider matching part of the error message text to make this more robust. However, this is consistent with existing patterns in the codebase, so it's acceptable. -
REPORT-DAG usage in generic-interface-6: Correctly uses
REPORT-DAGfor two dispatch sites where ordering may vary, consistent withderived-interface.slang.
Positive Aspects
- Well-structured tests with clear header comments explaining each scenario
- Good test matrix: primitive types (float, int), user-defined structs, multi-parameter generics, variadics, nested/composed interfaces, and a diagnostic edge case
- FileCheck patterns are consistent with existing conventions
- Properly documents a discovered limitation (createDynamicObject + generic interfaces → issue #10063)
Overall Assessment
Clean test-only PR. The tests are well-organized, cover meaningful scenarios, expected values are correct, and patterns are consistent with the existing test suite. LGTM.
| ICalculator<float> createCalc(uint id) | ||
| { | ||
| // CHECK: ([[# @LINE+1]]): error 39999 | ||
| return createDynamicObject<ICalculator<float>>(id); |
There was a problem hiding this comment.
what if the code does ICalculator<T> or ICalculator<IFoo>, will it crash?
There was a problem hiding this comment.
Both createDynamicObject<ICalculator> and createDynamicObject<ICalculator> produce the same clean error 39999 ("could not specialize generic for arguments of type (uint)") — no crash.
Additional tests are added to cover these test gaps.
There was a problem hiding this comment.
This error message is confusing and mystic, and this should be treated as a bug that we need to fix separately. Where are we tracking the bugs found that need fixing?
There was a problem hiding this comment.
This error message is confusing and mystic, and this should be treated as a bug that we need to fix separately. Where are we tracking the bugs found that need fixing?
We track the bug here:https://docs.google.com/spreadsheets/d/17OQbCrogJVAVT3oM51nzsUtEpQ8x7APiq-Q1EHlrCKE/edit?gid=1420633408#gid=1420633408
For this particular issue in the above description:
Discovered that createDynamicObject does not support specialized generic interfaces. (created issue #10063 to follow up)
There was a problem hiding this comment.
add comments to clarify.
… interface type arg Per reviewer feedback, test that createDynamicObject also produces clean errors (not crashes) when used with: - ICalculator<T> (open type parameter) - ICalculator<IFoo> (interface type as type argument) Both produce error 39999 like the existing ICalculator<float> case.
There was a problem hiding this comment.
Summary
This PR adds 7 new test files covering generic interface dynamic dispatch scenarios, extending the existing generic-interface-{1,2,3} test suite. The tests progressively increase in complexity:
| Test | Scenario | Backends |
|---|---|---|
generic-interface-4 |
ICalculator<float> with 2 concrete types |
slang/vk/cuda/cpu |
generic-interface-5 |
ICalculator<int> with 3 concrete types |
slang/vk/cuda/cpu |
generic-interface-6 |
ITransform<Vec2> with user-defined struct |
slang/vk/cuda/cpu |
generic-interface-7 |
IConverter<T,U> multi-parameter generics |
slang/vk/cuda/cpu |
generic-interface-8 |
createDynamicObject diagnostic (error 39999) |
spirv/hlsl/glsl/metal |
generic-interface-9 |
IAccumulator<each Ts> variadic generics |
slang/vk/cuda/cpu |
generic-interface-10 |
Nested IProcessor<T> + IValue<T> composition |
slang/vk/cuda/cpu |
Key Findings
All expected values verified correct — I manually checked the arithmetic for every test case and all CHECK expectations match.
Maintainer feedback addressed: generic-interface-8 now includes all 3 cases requested by @csyonghe (concrete specialization ICalculator<float>, open type parameter ICalculator<T>, and interface type argument ICalculator<IFoo>).
No bugs or correctness issues found.
Minor Observations (non-blocking)
-
Broader backend coverage: The new compute tests run on 4 backends (slang/vk/cuda/cpu) while existing
generic-interface-{1,2,3}only run on-slang. This is good for coverage but increases CI time. -
REPORT-DAG usage in generic-interface-6: Correctly uses
REPORT-DAGfor two dispatch sites where ordering may vary — consistent withderived-interface.slang. -
generic-interface-10 nested dispatch:
SquareProcessorcallsinput.get()twice, which exercises the inner dispatch path multiple times. Results are correct. See inline comment for a suggested clarifying comment. -
Error 39999 fragility in generic-interface-8: The error code is shared across many diagnostics. Matching partial message text would be more robust, but current pattern is consistent with existing tests. See inline comment.
Positive Aspects
- Well-structured progressive test suite covering primitive types → user-defined structs → multi-parameter generics → variadics → nested composition
- Clear header comments explaining each test scenario
- Properly documents a discovered limitation (
createDynamicObject+ generic interfaces → issue #10063) - FileCheck patterns are consistent with existing conventions
Overall Assessment
Clean test-only PR with good coverage of generic interface dynamic dispatch scenarios. All expected values are correct. The diagnostic test addresses maintainer feedback. LGTM — the inline comments are non-blocking suggestions for improved clarity.
|
|
||
| struct SquareProcessor : IProcessor<float> | ||
| { | ||
| float process(IValue<float> input) { return input.get() * input.get(); } |
There was a problem hiding this comment.
The SquareProcessor calls input.get() twice, which means the dynamic dispatch on IValue<float> occurs twice per invocation. This works correctly for these test implementations (both calls return the same value), so the expected results are correct.
However, this is actually a strength of the test — it exercises the inner dispatch path twice within a single outer dispatch. If the intention was to verify idempotent nested dispatch, consider adding a brief comment like // Deliberately calls get() twice to exercise nested dispatch so future readers know this is intentional and not an accidental inefficiency.
| ICalculator<float> createCalcFloat(uint id) | ||
| { | ||
| // CHECK: ([[# @LINE+1]]): error 39999 | ||
| return createDynamicObject<ICalculator<float>>(id); |
There was a problem hiding this comment.
Good that you added all three cases (concrete specialization, open type parameter, interface type argument) to address @csyonghe's feedback.
Minor note: error 39999 is a shared error code used by many different diagnostics. The test would pass even if the error origin changed to a completely different diagnostic. For more robust checking, you could match part of the error message text (e.g., error 39999:{{.*}}createDynamicObject{{.*}}). That said, this is consistent with existing diagnostic test patterns in the repo, so it's acceptable as-is.
There was a problem hiding this comment.
Summary
Test-only PR adding 7 new .slang files covering generic interface dynamic dispatch, extending the existing generic-interface-{1,2,3} suite. The tests progressively increase in complexity:
| Test | Scenario | Backends |
|---|---|---|
generic-interface-4 |
ICalculator<float> with 2 concrete types |
slang/vk/cuda/cpu |
generic-interface-5 |
ICalculator<int> with 3 concrete types |
slang/vk/cuda/cpu |
generic-interface-6 |
ITransform<Vec2> with user-defined struct |
slang/vk/cuda/cpu |
generic-interface-7 |
IConverter<T,U> multi-parameter generics |
slang/vk/cuda/cpu |
generic-interface-8 |
createDynamicObject diagnostic (error 39999) |
spirv/hlsl/glsl/metal |
generic-interface-9 |
IAccumulator<each Ts> variadic generics |
slang/vk/cuda/cpu |
generic-interface-10 |
Nested IProcessor<T> + IValue<T> composition |
slang/vk/cuda/cpu |
Key Findings
All expected values verified correct:
- generic-interface-4: Adder(3,4)=7.0, Multiplier(3,4)=12.0 ✓
- generic-interface-5: IntAdder(10,3)=13, IntMultiplier(10,3)=30, IntSubtractor(10,3)=7 ✓
- generic-interface-6: ScaleTransform(3,4)→(6,8)→14.0, NegateTransform(3,4)→(-3,-4)→12.0 ✓
- generic-interface-7: IntToFloatScale(4)=10.0, IntToFloatNegate(4)=-4.0 ✓
- generic-interface-9: SumTwo(3,4)=7.0, ProductTwo(3,4)=12.0 ✓
- generic-interface-10: All 4 processor×value combinations verified ✓
Maintainer feedback addressed: generic-interface-8 now includes all 3 cases requested by @csyonghe: concrete specialization (ICalculator<float>), open type parameter (ICalculator<T>), and interface type argument (ICalculator<IFoo>). All produce clean error 39999 with no crash.
No bugs or correctness issues found. No inline comments needed.
Positive Aspects
- Well-structured progressive test suite: primitive types → user-defined structs → multi-param generics → variadics → nested composition
- Clear header comments explaining each test scenario
- Proper use of
REPORT-DAGin generic-interface-6 for unordered dispatch site reports - Broader backend coverage (4 backends) compared to existing tests (1 backend) improves confidence
- Documents a discovered limitation via issue #10063
Overall Assessment
Clean test-only PR. All expected values are correct, FileCheck patterns follow existing conventions, and the diagnostic test properly addresses @csyonghe's feedback. LGTM.
… interface type arg Per reviewer feedback, test that createDynamicObject also produces clean errors (not crashes) when used with: - ICalculator<T> (open type parameter) - ICalculator<IFoo> (interface type as type argument) Both produce error 39999 like the existing ICalculator<float> case.
…ader-slang/slang into xsun/dynamic-dispatch.9885.2026-02-18
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/language-feature/dynamic-dispatch/generic-interface-8.slang`:
- Around line 7-13: Remove the duplicated TEST directives so each target appears
only once: find the repeated lines containing the test directives "-target
spirv", "-target glsl -stage compute -entry computeMain" and "-target metal
-stage compute -entry computeMain" in the generic-interface-8.slang test header
and delete the duplicate occurrences, leaving a single TEST line for each target
to avoid redundant CI runs.
tests/language-feature/dynamic-dispatch/generic-interface-8.slang
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Summary
Test-only PR adding 7 new .slang files that extend the existing generic-interface-{1,2,3} suite with comprehensive coverage of generic interface dynamic dispatch scenarios. The tests form a well-structured progression from basic to advanced:
| Test | Scenario | Backends |
|---|---|---|
generic-interface-4 |
ICalculator<float> with 2 concrete types |
slang/vk/cuda/cpu |
generic-interface-5 |
ICalculator<int> with 3 concrete types |
slang/vk/cuda/cpu |
generic-interface-6 |
ITransform<Vec2> with user-defined struct |
slang/vk/cuda/cpu |
generic-interface-7 |
IConverter<T,U> multi-parameter generics |
slang/vk/cuda/cpu |
generic-interface-8 |
createDynamicObject diagnostic (error 39999) |
spirv/hlsl/glsl/metal |
generic-interface-9 |
IAccumulator<each Ts> variadic generics |
slang/vk/cuda/cpu |
generic-interface-10 |
Nested IProcessor<T> + IValue<T> composition |
slang/vk/cuda/cpu |
Key Findings
All expected values verified correct:
- generic-interface-4: Adder(3,4)=7.0, Multiplier(3,4)=12.0 ✓
- generic-interface-5: IntAdder(10,3)=13, IntMultiplier(10,3)=30, IntSubtractor(10,3)=7 ✓
- generic-interface-6: ScaleTransform(3,4)→(6,8)→14.0, NegateTransform(3,4)→(-3,-4)→12.0 ✓
- generic-interface-7: IntToFloatScale(4)=10.0, IntToFloatNegate(4)=-4.0 ✓
- generic-interface-9: SumTwo(3,4)=7.0, ProductTwo(3,4)=12.0 ✓
- generic-interface-10: All 4 processor×value combinations verified ✓
No bugs, correctness issues, or design concerns found. No inline comments needed.
Maintainer feedback addressed: generic-interface-8 now covers all 3 cases @csyonghe requested — concrete specialization, open type parameter, and interface type argument. Duplicate TEST directives were already cleaned up in commit a6f3a32.
Positive Aspects
- Well-structured progressive test suite: primitive types → user-defined structs → multi-param generics → variadics → nested composition
- Broader backend coverage (4 backends) vs existing tests (1 backend) — good for catching backend-specific regressions
- Proper use of
REPORT-DAGin generic-interface-6 for unordered dispatch site matching - Clear inline comments explaining expected arithmetic in
computeMain - Consistent patterns with existing
generic-interface-{1,2,3}test conventions - Documents a discovered limitation via issue #10063
Overall Assessment
Clean test-only PR. All expected values are mathematically correct, FileCheck patterns follow existing conventions, and the diagnostic test properly addresses the maintainer's review feedback. LGTM.
2 participants
Fixes #9885
Add 7 test files covering the full scope of generic interface dynamic dispatch:
All compute tests run on slang, vk, cuda, and cpu backends.
Discovered that createDynamicObject does not support specialized generic interfaces. (created issue #10063 to follow up)