diff --git a/TSLANG_ISSUES.md b/TSLANG_ISSUES.md index b0f7044a0..38f97a654 100644 --- a/TSLANG_ISSUES.md +++ b/TSLANG_ISSUES.md @@ -24,6 +24,7 @@ https://github.com/ASDAlexander77/native_ts_graphics | 16 | `u64 %` goes through f64 (wrong above 2^53); `u64 /` has no integer division | - | `%` **fixed** by #571; `/` stays a number, as in JavaScript | | 17 | Storing through `Ref` indexing (`bytes[i] = x`) is rejected: "saving to constant object" | - | **answered**: `Deref(bytes[i]) = x` stores; the error says so since #573 | | 18 | `ns.f()` resolves to a local variable named `f` declared earlier in the function: "not a function to call" | - | **fixed** by #569 (pre-alpha100: open) | +| 19 | A @cpp overload call rejects a class argument for a `T \| null` or base-class parameter; a call whose argument the first overload can't take fails to compile | - | **fixed** (this PR; pre-alpha101: open) | Verified downstream: native_ts_graphics dropped its 71 forwarding constructors (PR #49) and rebuilt everything with pre-alpha97 (all objects deleted first): Windows build clean, CTest 61/61, Android @@ -291,6 +292,56 @@ the bare name `value`. Workaround: name the local differently. **Fixed by #569**: the member is looked up in the namespace first. A name the namespace doesn't have still falls back to the general lookup, so `ns.missing` can still find a local `missing`. +### 19. A @cpp overload call rejects a class for `T | null`, and checks the first overload's parameters + +Found on pre-alpha101 in native_ts_graphics' `core/nvrhi.ts`, after tsbindgen started writing +NVRHI's overloads under one name: `device.createGraphicsPipeline(Ref(desc), framebuffer)` with +`framebuffer: IFramebuffer` fits neither overload, `(Ref, Ref)` +nor `(Ref, IFramebuffer | null)`: + +```ts +@cpp +class Shape { + @slot(1) area(): s32; +} + +@cpp +class Square extends Shape { + @slot(2) side(): s32; +} + +@cpp +class Canvas { + @slot(1) draw(x: s32): s32; + @slot(2) draw(shape: Shape | null): s32; +} + +// canvas: Canvas, shape: Shape, square: Square +canvas.draw(shape); // error: invalid cast from Shape to s32 +``` + +``` +error: no overload of 'createGraphicsPipeline' takes these arguments (Reference<...>, nvrhi.IFramebuffer); it has: + createGraphicsPipeline(Reference<...>, Reference<...>) + createGraphicsPipeline(Reference<...>, nvrhi.IFramebuffer | null) +``` + +Two causes: +- an argument fit a parameter only by its own type, a number conversion or `canCastFromTo` (tuples + and interfaces): a `Shape` didn't fit `Shape | null`, nor a `Square` a `Shape`; +- the overload was picked in the full pass only. The partial resolve before it (finding the + function's types) called the first overload with the arguments, and an argument it can't be cast + to (`Shape` to `s32` above) failed the function. The NVRHI call gave the "no overload" error + instead because a class does cast to a `Ref`. + +Workaround: pass the argument through a local of the parameter's type +(`const arg: IFramebuffer | null = framebuffer;`). + +**Fixed** (this PR): a class fits its own type and a `T | null` holding it (3, the same C++ +pointer) and a base class, alone or in a union (2, a derived-to-base conversion). The overload is +picked in a partial resolve too; there a call that fits none (or two) keeps the first overload +without an error, and the full pass reports it. + --- ## 8. A union of literals in arithmetic (found live on pre-alpha97) diff --git a/docs/cpp-classes.md b/docs/cpp-classes.md index b405dcb41..74a7743ee 100644 --- a/docs/cpp-classes.md +++ b/docs/cpp-classes.md @@ -103,7 +103,9 @@ Writing one by hand: - **Overloads.** Declare each C++ overload under the C++ name, with its own `@slot` (or `@linkname` for a non-virtual one). A call picks the overload from its arguments, at compile time as C++ does: an argument of the parameter's own type fits best, then an integer for an - integer or a float for a float, then any other conversion. A call that fits none, or fits two + integer or a float for a float, or a class derived from the parameter's class, then any other + conversion. A class is a C++ pointer, so `T` and `T | null` fit a `T | null` parameter alike. A + call that fits none, or fits two as well, is an error that lists the overloads. MSVC groups overloads in the vtable, in reverse declaration order: take the numbers from the layout report (tsbindgen does). diff --git a/tslang/lib/TypeScript/MLIRGenAccessCall.cpp b/tslang/lib/TypeScript/MLIRGenAccessCall.cpp index b552d3e99..1c4f80286 100644 --- a/tslang/lib/TypeScript/MLIRGenAccessCall.cpp +++ b/tslang/lib/TypeScript/MLIRGenAccessCall.cpp @@ -1707,13 +1707,11 @@ namespace mlirgen return mlir::success(); } - if (!genContext.allowPartialResolve) + // in a partial resolve too: the first overload's parameters can't take another's arguments + actualFuncRefValue = resolveCppOverload(location, actualFuncRefValue, operands, genContext); + if (!actualFuncRefValue) { - actualFuncRefValue = resolveCppOverload(location, actualFuncRefValue, operands, genContext); - if (!actualFuncRefValue) - { - return mlir::failure(); - } + return mlir::failure(); } return mlirGenCall(location, actualFuncRefValue, operands, genContext); @@ -1811,8 +1809,22 @@ namespace mlirgen return funcRefValue; } + // whether derivedClass is baseClass or derives from it + std::function isOrDerivesFrom = + [&](mlir_ts::ClassType derivedClass, mlir_ts::ClassType baseClass) { + if (derivedClass == baseClass) + { + return true; + } + + auto derivedInfo = getClassInfoByFullName(derivedClass.getName().getValue()); + return derivedInfo && llvm::any_of(derivedInfo->baseClasses, [&](ClassInfo::TypePtr base) { + return isOrDerivesFrom(base->classType, baseClass); + }); + }; + // how well an argument fits a parameter: 3 its type, 2 an integer for an integer or a float - // for a float, 1 any other conversion, 0 none + // for a float, or a derived class for a base class, 1 any other conversion, 0 none auto fit = [&](mlir::Type argType, mlir::Type paramType) -> int { if (argType == paramType) { @@ -1828,6 +1840,30 @@ namespace mlirgen return 3; } + // a class is a C++ pointer, and `T | null` the same pointer: a `T` fits it as `T` does, + // and a class derived from T as a derived-to-base conversion + if (auto argClass = dyn_cast(baseType)) + { + auto classFit = [&](mlir::Type type) { + auto paramClass = dyn_cast(type); + return !paramClass ? 0 : argClass == paramClass ? 3 : isOrDerivesFrom(argClass, paramClass) ? 2 : 0; + }; + + auto classBest = classFit(paramType); + if (auto unionType = dyn_cast(paramType)) + { + for (auto member : unionType.getTypes()) + { + classBest = std::max(classBest, classFit(member)); + } + } + + if (classBest) + { + return classBest; + } + } + // an integer for an integer, a float for a float, before a conversion between the two // (C++ ranks a promotion or an integral conversion above a floating-integral one) auto isFloat = [&](mlir::Type type) { return isDouble(type) || isa(type); }; @@ -1907,6 +1943,12 @@ namespace mlirgen if (best.empty() || best.size() > 1) { + // a partial resolve may not know the arguments' types yet: the full one reports it + if (genContext.allowPartialResolve) + { + return funcRefValue; + } + auto diagnostic = emitError(location); diagnostic << (best.empty() ? "no overload of '" : "the call of '") << name << (best.empty() ? "' takes these arguments (" : "' is ambiguous: its arguments ("); diff --git a/tslang/test/tester/tests/00cpp_overloads.ts b/tslang/test/tester/tests/00cpp_overloads.ts index 060f884ea..3ed4cee02 100644 --- a/tslang/test/tester/tests/00cpp_overloads.ts +++ b/tslang/test/tester/tests/00cpp_overloads.ts @@ -17,6 +17,26 @@ class Derived extends Base { @slot(5) write(s: string): s32; // an overload of its own } +// classes are C++ pointers: a class fits its own type and `T | null` alike, and a derived class +// fits its base as a derived-to-base conversion, below an exact fit +@cpp +class Shape { + @slot(1) area(): s32; +} + +@cpp +class Square extends Shape { + @slot(2) side(): s32; +} + +@cpp +class Canvas { + @slot(1) draw(x: s32): s32; + @slot(2) draw(shape: Shape | null): s32; + @slot(3) fill(shape: Square): s32; + @slot(4) fill(shape: Shape): s32; +} + type ObjectData = [vtable: Opaque, value: s32]; function slot0(self: Opaque): s32 { return -1; } @@ -26,6 +46,11 @@ function slot3(self: Opaque, x: s32, y: s32): s32 { return 300 + x * y; } function slot4(self: Opaque): s32 { return Deref(>self).value; } function slot5(self: Opaque, s: string): s32 { return 500 + s.length; } +function canvasDrawInt(self: Opaque, x: s32): s32 { return 1000 + x; } +function canvasDrawShape(self: Opaque, shape: Opaque): s32 { return 2000; } +function canvasFillSquare(self: Opaque, shape: Opaque): s32 { return 3000; } +function canvasFillShape(self: Opaque, shape: Opaque): s32 { return 4000; } + function main() { let vtable: Opaque[] = [slot0, slot1, slot2, slot3, slot4, slot5]; let data: ObjectData = [Ref(vtable[0]), 7]; @@ -46,5 +71,19 @@ function main() { assert(derived.write("abc") == 503, "the derived class's own overload"); assert(derived.write(2, 3) == 306, "another base overload"); + let canvasVtable: Opaque[] = [slot0, canvasDrawInt, canvasDrawShape, + canvasFillSquare, canvasFillShape]; + let canvasData: ObjectData = [Ref(canvasVtable[0]), 0]; + const canvas = Ref(canvasData); + const shape = Ref(data); + const square = Ref(data); + const maybeShape: Shape | null = shape; + assert(canvas.draw(3) == 1003, "an integer: draw(s32)"); + assert(canvas.draw(maybeShape) == 2000, "a Shape | null: draw(Shape | null)"); + assert(canvas.draw(shape) == 2000, "a Shape: draw(Shape | null)"); + assert(canvas.draw(square) == 2000, "a Square: draw(Shape | null), derived to base"); + assert(canvas.fill(square) == 3000, "a Square: fill(Square) over fill(Shape)"); + assert(canvas.fill(shape) == 4000, "a Shape: fill(Shape)"); + print("done."); }