diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantJumpCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantJumpCheck.java index ff10a6300..f5660d606 100644 --- a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantJumpCheck.java +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantJumpCheck.java @@ -23,12 +23,10 @@ import au.com.integradev.delphi.cfg.api.Block; import au.com.integradev.delphi.cfg.api.ControlFlowGraph; import au.com.integradev.delphi.cfg.api.Finally; -import au.com.integradev.delphi.cfg.api.Linear; import au.com.integradev.delphi.cfg.api.UnconditionalJump; import org.sonar.check.Rule; import org.sonar.plugins.communitydelphi.api.ast.AnonymousMethodNode; import org.sonar.plugins.communitydelphi.api.ast.ArgumentListNode; -import org.sonar.plugins.communitydelphi.api.ast.CompoundStatementNode; import org.sonar.plugins.communitydelphi.api.ast.DelphiNode; import org.sonar.plugins.communitydelphi.api.ast.NameReferenceNode; import org.sonar.plugins.communitydelphi.api.ast.RoutineImplementationNode; @@ -53,12 +51,8 @@ public DelphiCheckContext visit(RoutineImplementationNode routine, DelphiCheckCo @Override public DelphiCheckContext visit(AnonymousMethodNode routine, DelphiCheckContext context) { - CompoundStatementNode compoundStatementNode = - routine.getFirstChildOfType(CompoundStatementNode.class); - if (compoundStatementNode != null) { - ControlFlowGraph cfg = ControlFlowGraphFactory.create(compoundStatementNode); - cfg.getBlocks().forEach(block -> checkBlock(block, context)); - } + ControlFlowGraph cfg = ControlFlowGraphFactory.create(routine.getStatementBlock()); + cfg.getBlocks().forEach(block -> checkBlock(block, context)); return super.visit(routine, context); } @@ -69,19 +63,20 @@ private void checkBlock(Block block, DelphiCheckContext context) { } UnconditionalJump jump = (UnconditionalJump) block; - Block successorWithoutJump = jump.getSuccessorIfRemoved(); DelphiNode terminator = jump.getTerminator(); RoutineNameDeclaration routineNameDeclaration = getRoutineNameDeclaration(terminator); + if (routineNameDeclaration == null) { + return; + } + if (!isContinueOrExit(routineNameDeclaration) || isExitWithExpression(routineNameDeclaration, terminator)) { return; } Block successor = jump.getSuccessor(); - successorWithoutJump = nonEmptySuccessor(successorWithoutJump); - - if (!successorWithoutJump.equals(successor)) { + if (!successor.equals(jump.getSuccessorIfRemoved())) { return; } @@ -116,46 +111,31 @@ private static boolean onlyFinallyBlocksBeforeEnd(Block finallyBlock) { } private static RoutineNameDeclaration getRoutineNameDeclaration(DelphiNode node) { - if (!(node instanceof NameReferenceNode)) { - return null; + NameDeclaration nameDeclaration = null; + if (node instanceof NameReferenceNode) { + nameDeclaration = ((NameReferenceNode) node).getNameDeclaration(); } - NameDeclaration nameDeclaration = ((NameReferenceNode) node).getNameDeclaration(); - if (!(nameDeclaration instanceof RoutineNameDeclaration)) { - return null; + if (nameDeclaration instanceof RoutineNameDeclaration) { + return (RoutineNameDeclaration) nameDeclaration; } - return (RoutineNameDeclaration) nameDeclaration; + return null; } private static boolean isContinueOrExit(RoutineNameDeclaration routineNameDeclaration) { - if (routineNameDeclaration == null) { - return false; - } String fullyQualifiedName = routineNameDeclaration.fullyQualifiedName(); return fullyQualifiedName.equals("System.Continue") || fullyQualifiedName.equals("System.Exit"); } private static boolean isExitWithExpression( RoutineNameDeclaration routineNameDeclaration, DelphiNode statement) { - if (routineNameDeclaration == null) { - return false; - } String fullyQualifiedName = routineNameDeclaration.fullyQualifiedName(); if (!fullyQualifiedName.equals("System.Exit")) { return false; } - ArgumentListNode argumentList = - statement.getParent().getFirstChildOfType(ArgumentListNode.class); - if (argumentList == null) { - return false; - } - return argumentList.getArgumentNodes().size() == 1; - } - private static Block nonEmptySuccessor(Block initialBlock) { - Block result = initialBlock; - while (result.getElements().isEmpty() && result instanceof Linear) { - result = ((Linear) result).getSuccessor(); - } - return result; + var argumentList = statement.getParent().getFirstChildOfType(ArgumentListNode.class); + int arguments = argumentList == null ? 0 : argumentList.getArgumentNodes().size(); + + return arguments > 0; } } diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantJumpCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantJumpCheckTest.java index d710c83b8..d9bb22007 100644 --- a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantJumpCheckTest.java +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantJumpCheckTest.java @@ -181,6 +181,21 @@ void testExitWithValueBeforeEndShouldNotAddIssue() { doExitTest(List.of("Exit(42);", "Foo;"), false); } + @Test + void testExitWithEmptyArgumentListShouldAddIssue() { + doExitTest(List.of("Exit(); // Noncompliant"), true); + } + + @Test + void testExitWithEmptyArgumentListAtEndShouldAddIssue() { + doExitTest(List.of("Foo;", "Exit(); // Noncompliant"), true); + } + + @Test + void testExitWithEmptyArgumentListBeforeEndShouldNotAddIssue() { + doExitTest(List.of("Exit();", "Foo;"), false); + } + @Test void testExitShouldAddIssue() { doExitTest(List.of("Exit; // Noncompliant"), true); @@ -193,7 +208,7 @@ void testExitAtEndShouldAddIssue() { @Test void testExitBeforeEndShouldNotAddIssue() { - doExitTest(List.of("Exit; // Noncompliant", "Foo;"), false); + doExitTest(List.of("Exit;", "Foo;"), false); } @Test @@ -353,4 +368,33 @@ void testAnonymousMethodExitShouldAddIssue() { doExitTest( List.of("var A := procedure", " begin", " Exit; // Noncompliant", " end;"), true); } + + @Test + void testGotoShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new RedundantJumpCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Test;") + .appendImpl("label NextLine;") + .appendImpl("begin") + .appendImpl(" goto NextLine;") + .appendImpl(" NextLine:") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testAsmRoutineShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new RedundantJumpCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure AsmRoutine;") + .appendImpl("asm") + .appendImpl(" JMP @NextLine") + .appendImpl(" @NextLine:") + .appendImpl("end;")) + .verifyNoIssues(); + } }