X Tutup
Skip to content

Fix prototype call check for nullable types #1230

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.

Sign up for GitHub

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

Merged
Perryvw merged 2 commits into from
Mar 17, 2022
Merged

Fix prototype call check for nullable types #1230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8dc845e
Fix prototype call check for nullable types
Perryvw Mar 13, 2022
8403960
Added extra test covering more types
Perryvw Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/transformation/builtins/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
hasStandardLibrarySignature,
isArrayType,
isFunctionType,
isNullableType,
isNumberType,
isStandardLibraryType,
isStringType,
Expand Down Expand Up @@ -115,22 +116,34 @@ export function transformBuiltinCallExpression(
}
}

if (isStringType(context, ownerType) && hasStandardLibrarySignature(context, node)) {
const isStringFunction =
isStringType(context, ownerType) ||
(expression.questionDotToken && isNullableType(context, ownerType, isStringType));
if (isStringFunction && hasStandardLibrarySignature(context, node)) {
if (isOptionalCall) return unsupportedOptionalCall();
return transformStringPrototypeCall(context, node);
}

if (isNumberType(context, ownerType) && hasStandardLibrarySignature(context, node)) {
const isNumberFunction =
isNumberType(context, ownerType) ||
(expression.questionDotToken && isNullableType(context, ownerType, isNumberType));
if (isNumberFunction && hasStandardLibrarySignature(context, node)) {
if (isOptionalCall) return unsupportedOptionalCall();
return transformNumberPrototypeCall(context, node);
}

if (isArrayType(context, ownerType) && hasStandardLibrarySignature(context, node)) {
const isArrayFunction =
isArrayType(context, ownerType) ||
(expression.questionDotToken && isNullableType(context, ownerType, isArrayType));
if (isArrayFunction && hasStandardLibrarySignature(context, node)) {
if (isOptionalCall) return unsupportedOptionalCall();
return transformArrayPrototypeCall(context, node);
}

if (isFunctionType(ownerType) && hasStandardLibrarySignature(context, node)) {
const isFunctionFunction =
isFunctionType(ownerType) ||
(expression.questionDotToken && isNullableType(context, ownerType, (_, t) => isFunctionType(t)));
if (isFunctionFunction && hasStandardLibrarySignature(context, node)) {
if (isOptionalCall) return unsupportedOptionalCall();
return transformFunctionPrototypeCall(context, node);
}
Expand Down
15 changes: 15 additions & 0 deletions src/transformation/utils/typescript/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ export function typeCanSatisfy(
return false;
}

export function isNullishType(context: TransformationContext, type: ts.Type): boolean {
return isTypeWithFlags(context, type, ts.TypeFlags.Undefined | ts.TypeFlags.Null | ts.TypeFlags.VoidLike);
}

export function isStringType(context: TransformationContext, type: ts.Type): boolean {
return isTypeWithFlags(context, type, ts.TypeFlags.String | ts.TypeFlags.StringLike | ts.TypeFlags.StringLiteral);
}
Expand All @@ -68,6 +72,17 @@ export function isNumberType(context: TransformationContext, type: ts.Type): boo
return isTypeWithFlags(context, type, ts.TypeFlags.Number | ts.TypeFlags.NumberLike | ts.TypeFlags.NumberLiteral);
}

export function isNullableType(
context: TransformationContext,
type: ts.Type,
isType: (c: TransformationContext, t: ts.Type) => boolean
): boolean {
return (
typeCanSatisfy(context, type, t => isType(context, t)) &&
typeAlwaysSatisfies(context, type, t => isType(context, t) || isNullishType(context, t))
);
}

function isExplicitArrayType(context: TransformationContext, type: ts.Type): boolean {
if (type.symbol) {
const baseConstraint = context.checker.getBaseConstraintOfType(type);
Expand Down
12 changes: 12 additions & 0 deletions test/unit/builtins/array.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,3 +700,15 @@ test.each([
])("trailing undefined or null are allowed in array literal (%p)", literal => {
util.testExpression(literal).expectToHaveNoDiagnostics();
});

// Issue #1218: https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1218
test.each(["[1, 2, 3]", "undefined"])("prototype call on nullable array (%p)", value => {
util.testFunction`
function find(arr?: number[]) {
return arr?.indexOf(2);
}
return find(${value});
`
.setOptions({ strictNullChecks: true })
.expectToMatchJsResult();
});
12 changes: 12 additions & 0 deletions test/unit/builtins/numbers.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,3 +143,15 @@ test.each([
])("parseInt with base and trailing text (%p)", ({ numberString, base }) => {
util.testExpression`parseInt("${numberString}", ${base})`.expectToMatchJsResult();
});

// Issue #1218: https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1218
test.each(["42", "undefined"])("prototype call on nullable number (%p)", value => {
util.testFunction`
function toString(n?: number) {
return n?.toString();
}
return toString(${value});
`
.setOptions({ strictNullChecks: true })
.expectToMatchJsResult();
});
27 changes: 27 additions & 0 deletions test/unit/builtins/string.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,3 +367,30 @@ test("string intersected method", () => {
return ({ abc: () => "a" } as Vector).abc();
`.expectToMatchJsResult();
});

// Issue #1218: https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1218
test.each(['"foo"', "undefined"])("prototype call on nullable string (%p)", value => {
util.testFunction`
function toUpper(str?: string) {
Copy link

@aSemy aSemy Mar 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str?: string is one way of triggering the bug in #1218 - Is it also worth having tests for the other potential triggers, str: string | undefined and str: string | null? I only see tests for str?: string.

Copy link
Member Author

@Perryvw Perryvw Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should all resolve to the same type (kinda), but you are right, adding some more tests wouldn't hurt. I added some more.

return str?.toUpperCase();
}
return toUpper(${value});
`
.setOptions({ strictNullChecks: true })
.expectToMatchJsResult();
});

// Issue #1218: https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1218
test.each(["string | undefined", "string | null", "null | string", "null | undefined | string"])(
"prototype call on nullable string type (%p)",
type => {
util.testFunction`
function toUpper(str: ${type}) {
return str?.toUpperCase();
}
return toUpper("foo");
`
.setOptions({ strictNullChecks: true })
.expectToMatchJsResult();
}
);
12 changes: 12 additions & 0 deletions test/unit/functions/functions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,18 @@ test("function apply without arguments should not lead to exception", () => {
`.expectToMatchJsResult();
});

// Issue #1218: https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1218
test.each(["() => 4", "undefined"])("prototype call on nullable function (%p)", value => {
util.testFunction`
function call(f?: () => number) {
return f?.apply(3);
}
return call(${value});
`
.setOptions({ strictNullChecks: true })
.expectToMatchJsResult();
});

test("Function call", () => {
util.testFunction`
const abc = function (this: { a: number }, a: string) { return this.a + a; }
Expand Down
X Tutup