diff --git a/src/linter/ui5Types/fix/CallExpressionBaseFix.ts b/src/linter/ui5Types/fix/CallExpressionBaseFix.ts index 9ce4d7c5e..6b1d06a67 100644 --- a/src/linter/ui5Types/fix/CallExpressionBaseFix.ts +++ b/src/linter/ui5Types/fix/CallExpressionBaseFix.ts @@ -1,7 +1,7 @@ import ts from "typescript"; import {PositionInfo} from "../../LinterContext.js"; import { - countChildNodesRecursive, findNodeRecursive, isExpectedValueExpression, isSideEffectFree, + countChildNodesRecursive, findNodeRecursive, isReturnValueUsed, isSideEffectFree, } from "../utils/utils.js"; import BaseFix, {BaseFixParams} from "./BaseFix.js"; import {FixHelpers} from "./Fix.js"; @@ -28,7 +28,7 @@ export default abstract class CallExpressionBaseFix extends BaseFix { } // If requested, check whether the return value of the call expression is assigned to a variable, // passed to another function or used elsewhere. - if (this.params.mustNotUseReturnValue && isExpectedValueExpression(node)) { + if (this.params.mustNotUseReturnValue && isReturnValueUsed(node)) { return false; } const containedCallExpression = findNodeRecursive(node.expression, this.nodeTypes); diff --git a/src/linter/ui5Types/utils/utils.ts b/src/linter/ui5Types/utils/utils.ts index 2ed6ece84..93ef2c06e 100644 --- a/src/linter/ui5Types/utils/utils.ts +++ b/src/linter/ui5Types/utils/utils.ts @@ -232,20 +232,46 @@ export function isAssignment(node: ts.AccessExpression): boolean { return false; } -export function isExpectedValueExpression(node: ts.Node): boolean { - let isExpectedValue = false; - while (node && !isExpectedValue) { - // Whether the function returns a value - if (!ts.isReturnStatement(node) && node.parent && ts.isBlock(node.parent)) { +export function isReturnValueUsed(node: ts.Node): boolean { + let isReturnValueUsed = false; + while (node && !isReturnValueUsed) { + if (!ts.isReturnStatement(node) && !ts.isThrowStatement(node) && node.parent && ts.isBlock(node.parent)) { + // If the node is not a return statement, but is part of a block, we can assume that it is not used + break; + } + if (node.parent && ts.isConditionalExpression(node.parent) && node === node.parent.condition) { + // If the node is part of a conditional expression, and is the condition, the return value is used + // Other positions in the conditional expression depend on how the whole expression is used + // Example: `method() ? 1 : 2;` + isReturnValueUsed = true; + break; + } + + if (node.parent && (ts.isIfStatement(node.parent) || ts.isWhileStatement(node.parent) || + ts.isDoStatement(node.parent) || ts.isSwitchStatement(node.parent)) && node !== node.parent.expression) { + // If the node is part of an if or while statement, but not the condition we can assume + // that the return value is not used + // Example: `while(1 === 1) method();` does not use the return value of `method()` + break; + } + if (node.parent && ts.isForStatement(node.parent) && node === node.parent.statement) { + // If the node is part of a for statement but part of the statement, we can assume + // that the return value is not used + // Example: `for (let i = 0; i < 10; i++) method();` does not use the return value of `method()` break; } if (ts.isVariableDeclaration(node) || ts.isBinaryExpression(node) || + ts.isIfStatement(node) || + ts.isWhileStatement(node) || + ts.isDoStatement(node) || + ts.isForStatement(node) || + ts.isSwitchStatement(node) || ts.isVariableStatement(node) || - ts.isConditionalExpression(node) || ts.isParenthesizedExpression(node) || ts.isReturnStatement(node) || + ts.isThrowStatement(node) || // () => jQuery.sap.log.error("FOO"); Explicit return in an arrow function (ts.isArrowFunction(node) && !ts.isBlock(node.body)) || // Argument of a function call @@ -257,12 +283,12 @@ export function isExpectedValueExpression(node: ts.Node): boolean { ts.isPropertyAccessExpression(node.expression.parent) && ts.isCallExpression(node.expression.parent.expression)) ) { - isExpectedValue = true; + isReturnValueUsed = true; } node = node.parent; } - return isExpectedValue; + return isReturnValueUsed; } export function isConditionalAccess(node: ts.Node): boolean { diff --git a/test/fixtures/autofix/jQuerySapLog.js b/test/fixtures/autofix/jQuerySapLog.js index f8dcb90e1..c8a9e1f3c 100644 --- a/test/fixtures/autofix/jQuerySapLog.js +++ b/test/fixtures/autofix/jQuerySapLog.js @@ -24,6 +24,10 @@ sap.ui.define([], async function () { baseLog.debug("This is a debug log message"); const level = jQuery.sap.log.getLevel(); + if (jQuery.sap.log.debug("This is a debug log message")) {} + while(jQuery.sap.log.debug("This is a debug log message")) {} + for (let i = 0; jQuery.sap.log.debug("This is a debug log message"); i++) {} + if (level === jQuery.sap.log.LogLevel.DEBUG) { jQuery.sap.log.debug(`This is a debug (${jQuery.sap.Level.DEBUG}) log message`); } diff --git a/test/lib/autofix/snapshots/autofix.fixtures.ts.md b/test/lib/autofix/snapshots/autofix.fixtures.ts.md index 510861e1d..dc92278dc 100644 --- a/test/lib/autofix/snapshots/autofix.fixtures.ts.md +++ b/test/lib/autofix/snapshots/autofix.fixtures.ts.md @@ -5260,6 +5260,10 @@ Generated by [AVA](https://avajs.dev). const baseLog = BaseLog.getLogger();␊ baseLog.debug("This is a debug log message");␊ const level = BaseLog.getLevel();␊ + ␊ + if (jQuery.sap.log.debug("This is a debug log message")) {}␊ + while(jQuery.sap.log.debug("This is a debug log message")) {}␊ + for (let i = 0; jQuery.sap.log.debug("This is a debug log message"); i++) {}␊ ␊ if (level === BaseLog.Level.DEBUG) {␊ BaseLog.debug(\`This is a debug (${jQuery.sap.Level.DEBUG}) log message\`);␊ @@ -5312,8 +5316,26 @@ Generated by [AVA](https://avajs.dev). line: 17, message: 'Unable to analyze this method call because the type of identifier "fatal" in "jQuery.sap.log.fatal("This is a fatal log message")" could not be determined', }, + { + category: 1, + column: 6, + line: 27, + message: 'Unable to analyze this method call because the type of identifier "debug" in "jQuery.sap.log.debug("This is a debug log message")" could not be determined', + }, + { + category: 1, + column: 8, + line: 28, + message: 'Unable to analyze this method call because the type of identifier "debug" in "jQuery.sap.log.debug("This is a debug log message")" could not be determined', + }, + { + category: 1, + column: 18, + line: 29, + message: 'Unable to analyze this method call because the type of identifier "debug" in "jQuery.sap.log.debug("This is a debug log message")" could not be determined', + }, ], - errorCount: 2, + errorCount: 5, fatalErrorCount: 0, filePath: 'jQuerySapLog.js', messages: [ @@ -5326,8 +5348,32 @@ Generated by [AVA](https://avajs.dev). severity: 2, }, { - column: 37, + column: 6, + line: 27, + message: 'Use of deprecated API \'jQuery.sap.log.debug\'', + messageDetails: 'Deprecated test message', + ruleId: 'no-deprecated-api', + severity: 2, + }, + { + column: 8, line: 28, + message: 'Use of deprecated API \'jQuery.sap.log.debug\'', + messageDetails: 'Deprecated test message', + ruleId: 'no-deprecated-api', + severity: 2, + }, + { + column: 18, + line: 29, + message: 'Use of deprecated API \'jQuery.sap.log.debug\'', + messageDetails: 'Deprecated test message', + ruleId: 'no-deprecated-api', + severity: 2, + }, + { + column: 37, + line: 32, message: 'Use of deprecated API \'jQuery.sap.Level.DEBUG\'', messageDetails: 'Deprecated test message', ruleId: 'no-deprecated-api', diff --git a/test/lib/autofix/snapshots/autofix.fixtures.ts.snap b/test/lib/autofix/snapshots/autofix.fixtures.ts.snap index a3c9fe608..7eaa356ba 100644 Binary files a/test/lib/autofix/snapshots/autofix.fixtures.ts.snap and b/test/lib/autofix/snapshots/autofix.fixtures.ts.snap differ diff --git a/test/lib/linter/ui5Types/utils.ts b/test/lib/linter/ui5Types/utils.ts index 3f0488d68..e63f8812b 100644 --- a/test/lib/linter/ui5Types/utils.ts +++ b/test/lib/linter/ui5Types/utils.ts @@ -8,6 +8,7 @@ import { getSymbolForPropertyInConstructSignatures, isClassMethod, isConditionalAccess, + isReturnValueUsed, } from "../../../../src/linter/ui5Types/utils/utils.js"; function createProgram(code: string) { @@ -380,6 +381,243 @@ test("getPropertyAssignmentsInObjectLiteralExpression - unsupported elements", ( t.is(result[1], test2); }); +test("isReturnValueUsed (negative): Block", (t) => { + const program = createProgram(` + method(); + `); + const expressionStatement = program.getSourceFile("test.ts")!.statements[0] as ts.ExpressionStatement; + t.false(isReturnValueUsed(expressionStatement)); +}); + +test("isReturnValueUsed: CallExpression", (t) => { + const program = createProgram(` + foo(method()); + `); + const expressionStatement = program.getSourceFile("test.ts")!.statements[0] as ts.ExpressionStatement; + const callExpression = expressionStatement.expression as ts.CallExpression; + const method = callExpression.arguments[0] as ts.CallExpression; + t.true(isReturnValueUsed(method)); +}); + +test("isReturnValueUsed: BinaryExpression", (t) => { + const program = createProgram(` + true && method(); + `); + const expressionStatement = program.getSourceFile("test.ts")!.statements[0] as ts.ExpressionStatement; + const binaryExpression = expressionStatement.expression as ts.BinaryExpression; + t.true(isReturnValueUsed(binaryExpression.right)); +}); + +test("isReturnValueUsed: VariableDeclaration", (t) => { + const program = createProgram(` + const result = method(); + `); + const variableStatement = program.getSourceFile("test.ts")!.statements[0] as ts.VariableStatement; + const variableDeclaration = variableStatement.declarationList.declarations[0]; + t.true(isReturnValueUsed(variableDeclaration.initializer!)); +}); + +test("isReturnValueUsed: IfStatement", (t) => { + const program = createProgram(` + if (method()) {} + `); + const ifStatement = program.getSourceFile("test.ts")!.statements[0] as ts.IfStatement; + t.true(isReturnValueUsed(ifStatement.expression)); +}); + +test("isReturnValueUsed: ConditionalExpression", (t) => { + const program = createProgram(` + method() ? 1 : undefined; + `); + const expressionStatement = program.getSourceFile("test.ts")!.statements[0] as ts.ExpressionStatement; + const conditionalExpression = expressionStatement.expression as ts.ConditionalExpression; + t.true(isReturnValueUsed(conditionalExpression.condition)); +}); + +test("isReturnValueUsed (negative): ConditionalExpression - whenTrue", (t) => { + const program = createProgram(` + condition ? method() : undefined; + `); + const expressionStatement = program.getSourceFile("test.ts")!.statements[0] as ts.ExpressionStatement; + const conditionalExpression = expressionStatement.expression as ts.ConditionalExpression; + t.false(isReturnValueUsed(conditionalExpression.whenTrue)); +}); + +test("isReturnValueUsed: ForStatement", (t) => { + const program = createProgram(` + for (let i = 0; method(); i++) {} + `); + const forStatement = program.getSourceFile("test.ts")!.statements[0] as ts.ForStatement; + t.true(isReturnValueUsed(forStatement.condition!)); +}); + +test("isReturnValueUsed: WhileStatement", (t) => { + const program = createProgram(` + while (method()) {} + `); + const whileStatement = program.getSourceFile("test.ts")!.statements[0] as ts.WhileStatement; + t.true(isReturnValueUsed(whileStatement.expression)); +}); + +test("isReturnValueUsed (negative): WhileStatement statement", (t) => { + const program = createProgram(` + while (true) method() + `); + const whileStatement = program.getSourceFile("test.ts")!.statements[0] as ts.WhileStatement; + t.false(isReturnValueUsed(whileStatement.statement)); +}); + +test("isReturnValueUsed: DoStatement", (t) => { + const program = createProgram(` + do {} while (method()); + `); + const whileStatement = program.getSourceFile("test.ts")!.statements[0] as ts.WhileStatement; + t.true(isReturnValueUsed(whileStatement.expression)); +}); + +test("isReturnValueUsed: VariableStatement - multiple declarations", (t) => { + const program = createProgram(` + const result1 = method(), result2 = method(); + `); + const variableStatement = program.getSourceFile("test.ts")!.statements[0] as ts.VariableStatement; + const variableDeclarations = variableStatement.declarationList.declarations; + t.true(isReturnValueUsed(variableDeclarations[0].initializer!)); + t.true(isReturnValueUsed(variableDeclarations[1].initializer!)); +}); + +test("isReturnValueUsed: ParenthesizedExpression", (t) => { + const program = createProgram(` + (method()); + `); + const expressionStatement = program.getSourceFile("test.ts")!.statements[0] as ts.ExpressionStatement; + const parenthesizedExpression = expressionStatement.expression as ts.ParenthesizedExpression; + t.true(isReturnValueUsed(parenthesizedExpression.expression)); +}); + +test("isReturnValueUsed: ReturnStatement", (t) => { + const program = createProgram(` + function foo() { + return method(); + } + `); + const functionDeclaration = program.getSourceFile("test.ts")!.statements[0] as ts.FunctionDeclaration; + const returnStatement = functionDeclaration.body!.statements[0] as ts.ReturnStatement; + t.true(isReturnValueUsed(returnStatement.expression!)); +}); + +test("isReturnValueUsed: ThrowStatement", (t) => { + const program = createProgram(` + function foo() { + throw method(); + } + `); + const functionDeclaration = program.getSourceFile("test.ts")!.statements[0] as ts.FunctionDeclaration; + const throwStatement = functionDeclaration.body!.statements[0] as ts.ThrowStatement; + t.true(isReturnValueUsed(throwStatement.expression)); +}); + +test("isReturnValueUsed: ArrowFunction", (t) => { + const program = createProgram(` + const foo = () => method(); + `); + const variableStatement = program.getSourceFile("test.ts")!.statements[0] as ts.VariableStatement; + const variableDeclaration = variableStatement.declarationList.declarations[0]; + const arrowFunction = variableDeclaration.initializer as ts.ArrowFunction; + t.true(isReturnValueUsed(arrowFunction.body)); +}); + +test("isReturnValueUsed: PropertyAssignment in ObjectLiteralExpression", (t) => { + const program = createProgram(` + const obj = { method: method() }; + `); + + const variableStatement = program.getSourceFile("test.ts")!.statements[0] as ts.VariableStatement; + const variableDeclaration = variableStatement.declarationList.declarations[0]; + const objectLiteralExpression = variableDeclaration.initializer as ts.ObjectLiteralExpression; + const propertyAssignment = objectLiteralExpression.properties[0] as ts.PropertyAssignment; + + t.true(isReturnValueUsed(propertyAssignment.initializer)); +}); + +test("isReturnValueUsed: ComputedPropertyName in ObjectLiteralExpression", (t) => { + const program = createProgram(` + const obj = { [method()]: "value" }; + `); + + const variableStatement = program.getSourceFile("test.ts")!.statements[0] as ts.VariableStatement; + const variableDeclaration = variableStatement.declarationList.declarations[0]; + const objectLiteralExpression = variableDeclaration.initializer as ts.ObjectLiteralExpression; + const propertyAssignment = objectLiteralExpression.properties[0] as ts.PropertyAssignment; + const computedPropertyName = propertyAssignment.name as ts.ComputedPropertyName; + + t.true(isReturnValueUsed(computedPropertyName.expression)); +}); + +test("isReturnValueUsed (negative): CallExpression nested in ObjectLiteralExpression", (t) => { + const program = createProgram(` + const obj = { + method: () => { + method() + } + }; + `); + + const variableStatement = program.getSourceFile("test.ts")!.statements[0] as ts.VariableStatement; + const variableDeclaration = variableStatement.declarationList.declarations[0]; + const objectLiteralExpression = variableDeclaration.initializer as ts.ObjectLiteralExpression; + const propertyAssignment = objectLiteralExpression.properties[0] as ts.PropertyAssignment; + const arrowFunc = propertyAssignment.initializer as ts.ArrowFunction; + const block = arrowFunc.body as ts.Block; + + t.false(isReturnValueUsed(block.statements[0] as ts.ExpressionStatement)); +}); + +test("isReturnValueUsed: Chaining in PropertyAccessExpression", (t) => { + const program = createProgram(` + sap.ui.getCore().getConfiguration().getLanguage(); + `); + + const expressionStatement = program.getSourceFile("test.ts")!.statements[0] as ts.ExpressionStatement; + const firstPropertyAccessExpression = expressionStatement.expression as ts.PropertyAccessExpression; + const secondPropertyAccessExpression = firstPropertyAccessExpression.expression as ts.PropertyAccessExpression; + + t.true(isReturnValueUsed(secondPropertyAccessExpression)); +}); + +test("isReturnValueUsed: SwitchStatement", (t) => { + const program = createProgram(` + switch (method()) { + case 1: + break; + case 2: + break; + default: + break; + } + `); + + const switchStatement = program.getSourceFile("test.ts")!.statements[0] as ts.SwitchStatement; + t.true(isReturnValueUsed(switchStatement.expression)); +}); + +test("isReturnValueUsed (negative): SwitchStatement", (t) => { + const program = createProgram(` + switch (foo) { + case 1: + method(); + break; + case 2: + break; + default: + break; + } + `); + + const switchStatement = program.getSourceFile("test.ts")!.statements[0] as ts.SwitchStatement; + const clause = switchStatement.caseBlock.clauses[0] as ts.CaseClause; + t.false(isReturnValueUsed(clause.statements[0])); +}); + // Positive tests for isConditionalAccess test("isConditionalAccess: AccessExpression in IfStatement", (t) => { const program = createProgram(`