Skip to content

Commit 17b66ef

Browse files
author
Tim Foley
authored
Unify all generic parameters, even if some mismatch (shader-slang#454)
* Fix decl-ref printing to handling NULL pointers If the underlying decl, or its name is NULL, then use an empty string for the declaration name. This issue was found when debugging, but could bite non-debug cases too, if we ever try to print something like a generic type constraint, which has no name. * Unify all generic parameters, even if some mismatch Fixes shader-slang#449 The front end tries to infer the right generic arguments to use at a call site using a sloppily implemented "unification" approach. The basic idea is that if you pass a `vector<float,3>` into a function that operates ona `vector<T,N>` where `T` and `N` are generic paameters, then the unification will try to unify `vector<float,3>` with `vector<T,N>` which will lead to it recursively unifying `float` with `T` and `3` with `N`, at which point we have viable values to substitute in for those parameters. Where the existing approach is maybe not quite right is in how it handles obvious unification failures. So if we ask the code to unify, say, `float` with `uint`, it will bail out immediately because those can't be unified. This sounds right superficially, but in some cases with might be calling a function that takes a `vector<float,N>` and passing a `vector<uint,3>` and we'd like to at least get far enough along with unification to see that `N` should be `3` so that the front end can maybe decide to call the function anyway, with some amount of implicit conversion. Over time I've had to modify a lot of the "unification" logic so that it doesn't treat the obvious failures as a hard stop, and instead just returns the failure as a boolean status, but keeps on trying to unify things even after such a failure. When doing unification as part of inference for generic arguments, there will usually be subsequent steps (e.g., type conversions for function aguments) that will catch the type errors that arise. This specific change is to make is so that when unifying the substitutions for a generic decl-ref, we try to unify all the pair-wise arguments, and don't bail out on the first mismatch (so that the `float`-vs-`uint` failure above doesn't lead to us skipping the `3` and `N` pairing). The one case we need to watch out for in all of this is when unification is used to check if an `extension` declaration (which might be generic) is actually application to a concrete type. In that case we obviously don't want an extension for `vector<float,N>` to apply to `vector<uint,3>`, so it is important that the extension case check the return status from the unification logic (*or* in the future, it could just confirm that the substituted type is equivalent to the original as a post-process...). I've added a test case that reproduces the original failure that surfaced the bug. * fixup: add expected test output
1 parent 6d400b6 commit 17b66ef

File tree

4 files changed

+46
-6
lines changed

4 files changed

+46
-6
lines changed

source/slang/check.cpp

+8-3
Original file line numberDiff line numberDiff line change
@@ -5419,17 +5419,22 @@ namespace Slang
54195419
// Their arguments must unify
54205420
SLANG_RELEASE_ASSERT(fstGen->args.Count() == sndGen->args.Count());
54215421
UInt argCount = fstGen->args.Count();
5422+
bool okay = true;
54225423
for (UInt aa = 0; aa < argCount; ++aa)
54235424
{
54245425
if (!TryUnifyVals(constraints, fstGen->args[aa], sndGen->args[aa]))
5425-
return false;
5426+
{
5427+
okay = false;
5428+
}
54265429
}
54275430

54285431
// Their "base" specializations must unify
54295432
if (!TryUnifySubstitutions(constraints, fstGen->outer, sndGen->outer))
5430-
return false;
5433+
{
5434+
okay = false;
5435+
}
54315436

5432-
return true;
5437+
return okay;
54335438
}
54345439

54355440
bool TryUnifyTypeParam(

source/slang/syntax.cpp

+6-3
Original file line numberDiff line numberDiff line change
@@ -2011,10 +2011,13 @@ void Type::accept(IValVisitor* visitor, void* extra)
20112011

20122012
String DeclRefBase::toString() const
20132013
{
2014-
StringBuilder sb;
2015-
sb << this->getDecl()->getName()->text;
2014+
if (!decl) return "";
2015+
2016+
auto name = decl->getName();
2017+
if (!name) return "";
2018+
20162019
// TODO: need to print out substitutions too!
2017-
return sb.ProduceString();
2020+
return name->text;
20182021
}
20192022

20202023
RefPtr<ThisTypeSubstitution> getThisTypeSubst(DeclRefBase & declRef, bool insertSubstEntry)

tests/bugs/gh-449.slang

+25
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// gh-449.slang
2+
//TEST:SIMPLE:
3+
4+
// Issue when dealing with binary operations that
5+
// mix scalars with vectors that have a different
6+
// element type.
7+
8+
struct S { int dummy; };
9+
10+
void foo(S s);
11+
12+
void main()
13+
{
14+
// This works fine right now. The `uint` gets converted to a
15+
// `float`, and then we do the addition.
16+
float2 a = float2(1, 2);
17+
uint b = 3;
18+
foo(a + b);
19+
20+
// This used to get confused, with the `f` getting converted
21+
// to a `uint` before the addition.
22+
uint2 u = uint2(1, 2);
23+
float f = 3.0;
24+
foo(u + f);
25+
}

tests/bugs/gh-449.slang.expected

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
result code = -1
2+
standard error = {
3+
tests/bugs/gh-449.slang(18): error 30019: expected an expression of type 'S', got 'vector<float,2>'
4+
tests/bugs/gh-449.slang(24): error 30019: expected an expression of type 'S', got 'vector<float,2>'
5+
}
6+
standard output = {
7+
}

0 commit comments

Comments
 (0)