From c6e8b4b89acdaafe006f9a204c1868868956032d Mon Sep 17 00:00:00 2001 From: Perryvw Date: Sun, 6 Mar 2022 13:19:23 +0100 Subject: [PATCH 1/3] Add warning diagnostic when using condition variable that will never be false in Lua --- src/transformation/utils/diagnostics.ts | 4 ++ src/transformation/visitors/conditional.ts | 14 ++++++ src/transformation/visitors/loops/do-while.ts | 7 +++ test/unit/conditionals.spec.ts | 43 +++++++++++++++++++ 4 files changed, 68 insertions(+) diff --git a/src/transformation/utils/diagnostics.ts b/src/transformation/utils/diagnostics.ts index 6520694ed..7fc7d814b 100644 --- a/src/transformation/utils/diagnostics.ts +++ b/src/transformation/utils/diagnostics.ts @@ -147,6 +147,10 @@ export const annotationDeprecated = createWarningDiagnosticFactory( `See https://typescripttolua.github.io/docs/advanced/compiler-annotations#${kind.toLowerCase()} for more information.` ); +export const truthyOnlyConditionalValue = createWarningDiagnosticFactory( + "Numbers and strings will always evaluate to true in Lua. Explicitly check the value with ===." +); + export const notAllowedOptionalAssignment = createErrorDiagnosticFactory( "The left-hand side of an assignment expression may not be an optional property access." ); diff --git a/src/transformation/visitors/conditional.ts b/src/transformation/visitors/conditional.ts index 0c8434890..82d214cdd 100644 --- a/src/transformation/visitors/conditional.ts +++ b/src/transformation/visitors/conditional.ts @@ -5,6 +5,7 @@ import { transformInPrecedingStatementScope } from "../utils/preceding-statement import { performHoisting, ScopeType } from "../utils/scope"; import { transformBlockOrStatement } from "./block"; import { canBeFalsy } from "../utils/typescript"; +import { truthyOnlyConditionalValue } from "../utils/diagnostics"; type EvaluatedExpression = [precedingStatemens: lua.Statement[], value: lua.Expression]; @@ -39,6 +40,9 @@ function transformProtectedConditionalExpression( } export const transformConditionalExpression: FunctionVisitor = (expression, context) => { + // Check if we need to add diagnostic about Lua truthiness + checkOnlyTruthyCondition(expression.condition, context); + const condition = transformInPrecedingStatementScope(context, () => context.transformExpression(expression.condition) ); @@ -64,6 +68,10 @@ export const transformConditionalExpression: FunctionVisitor = (statement, context) => { + // Check if we need to add diagnostic about Lua truthiness + checkOnlyTruthyCondition(statement.expression, context); + const body = transformLoopBody(context, statement); let [conditionPrecedingStatements, condition] = transformInPrecedingStatementScope(context, () => @@ -37,6 +41,9 @@ export const transformWhileStatement: FunctionVisitor = (stat }; export const transformDoStatement: FunctionVisitor = (statement, context) => { + // Check if we need to add diagnostic about Lua truthiness + checkOnlyTruthyCondition(statement.expression, context); + const body = lua.createDoStatement(transformLoopBody(context, statement)); let [conditionPrecedingStatements, condition] = transformInPrecedingStatementScope(context, () => diff --git a/test/unit/conditionals.spec.ts b/test/unit/conditionals.spec.ts index a1a234cf6..b0aad763e 100644 --- a/test/unit/conditionals.spec.ts +++ b/test/unit/conditionals.spec.ts @@ -1,4 +1,5 @@ import * as tstl from "../../src"; +import { truthyOnlyConditionalValue } from "../../src/transformation/utils/diagnostics"; import * as util from "../util"; test.each([0, 1])("if (%p)", inp => { @@ -138,3 +139,45 @@ test.each([false, true])("Ternary conditional with preceding statements in false }) .expectToMatchJsResult(); }); + +test.each(["string", "number", "string | number"])( + "Warning when using if statement that cannot evaluate to false undefined or null (%p)", + type => { + util.testFunction` + if (condition) {} + ` + .setTsHeader(`declare var condition: ${type};`) + .expectToHaveDiagnostics([truthyOnlyConditionalValue.code]); + } +); + +test.each(["string", "number", "string | number"])( + "Warning when using while statement that cannot evaluate to false undefined or null (%p)", + type => { + util.testFunction` + while (condition) {} + ` + .setTsHeader(`declare var condition: ${type};`) + .expectToHaveDiagnostics([truthyOnlyConditionalValue.code]); + } +); + +test.each(["string", "number", "string | number"])( + "Warning when using do while statement that cannot evaluate to false undefined or null (%p)", + type => { + util.testFunction` + do {} while (condition) + ` + .setTsHeader(`declare var condition: ${type};`) + .expectToHaveDiagnostics([truthyOnlyConditionalValue.code]); + } +); + +test.each(["string", "number", "string | number"])( + "Warning when using ternary that cannot evaluate to false undefined or null (%p)", + type => { + util.testExpression`condition ? 1 : 0` + .setTsHeader(`declare var condition: ${type};`) + .expectToHaveDiagnostics([truthyOnlyConditionalValue.code]); + } +); From 7f1f25162854145e7ae6ae9553047b75e8ba3b3d Mon Sep 17 00:00:00 2001 From: Perryvw Date: Wed, 17 Aug 2022 21:53:33 +0200 Subject: [PATCH 2/3] Fix tests --- test/unit/conditionals.spec.ts | 8 +++++++- test/unit/identifiers.spec.ts | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/test/unit/conditionals.spec.ts b/test/unit/conditionals.spec.ts index b0aad763e..df7de5176 100644 --- a/test/unit/conditionals.spec.ts +++ b/test/unit/conditionals.spec.ts @@ -89,7 +89,9 @@ test.each([ { condition: false, lhs: 4, rhs: 5 }, { condition: 3, lhs: 4, rhs: 5 }, ])("Ternary Conditional (%p)", ({ condition, lhs, rhs }) => { - util.testExpressionTemplate`${condition} ? ${lhs} : ${rhs}`.expectToMatchJsResult(); + util.testExpressionTemplate`${condition} ? ${lhs} : ${rhs}` + .ignoreDiagnostics([truthyOnlyConditionalValue.code]) + .expectToMatchJsResult(); }); test.each(["true", "false", "a < 4", "a == 8"])("Ternary Conditional Delayed (%p)", condition => { @@ -147,6 +149,7 @@ test.each(["string", "number", "string | number"])( if (condition) {} ` .setTsHeader(`declare var condition: ${type};`) + .setOptions({ strict: true }) .expectToHaveDiagnostics([truthyOnlyConditionalValue.code]); } ); @@ -158,6 +161,7 @@ test.each(["string", "number", "string | number"])( while (condition) {} ` .setTsHeader(`declare var condition: ${type};`) + .setOptions({ strict: true }) .expectToHaveDiagnostics([truthyOnlyConditionalValue.code]); } ); @@ -169,6 +173,7 @@ test.each(["string", "number", "string | number"])( do {} while (condition) ` .setTsHeader(`declare var condition: ${type};`) + .setOptions({ strict: true }) .expectToHaveDiagnostics([truthyOnlyConditionalValue.code]); } ); @@ -178,6 +183,7 @@ test.each(["string", "number", "string | number"])( type => { util.testExpression`condition ? 1 : 0` .setTsHeader(`declare var condition: ${type};`) + .setOptions({ strict: true }) .expectToHaveDiagnostics([truthyOnlyConditionalValue.code]); } ); diff --git a/test/unit/identifiers.spec.ts b/test/unit/identifiers.spec.ts index 7c0722938..b5b627b4d 100644 --- a/test/unit/identifiers.spec.ts +++ b/test/unit/identifiers.spec.ts @@ -302,7 +302,7 @@ describe("lua keyword as identifier doesn't interfere with lua's value", () => { test("variable (elseif)", () => { util.testFunction` - const elseif = "foobar"; + const elseif: string | undefined = "foobar"; if (false) { } else if (elseif) { return elseif; @@ -350,7 +350,7 @@ describe("lua keyword as identifier doesn't interfere with lua's value", () => { test("variable (then)", () => { util.testFunction` - const then = "foobar"; + const then: string | undefined = "foobar"; if (then) { return then; } From e5039e6549de2648c508c742a6297889b55527ba Mon Sep 17 00:00:00 2001 From: Perryvw Date: Wed, 17 Aug 2022 21:58:32 +0200 Subject: [PATCH 3/3] Fix ugly import --- test/unit/conditionals.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/conditionals.spec.ts b/test/unit/conditionals.spec.ts index df7de5176..ffbff9ab4 100644 --- a/test/unit/conditionals.spec.ts +++ b/test/unit/conditionals.spec.ts @@ -1,6 +1,6 @@ import * as tstl from "../../src"; -import { truthyOnlyConditionalValue } from "../../src/transformation/utils/diagnostics"; import * as util from "../util"; +import { truthyOnlyConditionalValue } from "../../src/transformation/utils/diagnostics"; test.each([0, 1])("if (%p)", inp => { util.testFunction`