Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import au.com.integradev.delphi.cfg.api.Branch;
import au.com.integradev.delphi.cfg.api.ControlFlowGraph;
import au.com.integradev.delphi.cfg.api.Terminated;
import au.com.integradev.delphi.cfg.api.Terminus;
import au.com.integradev.delphi.utils.ControlFlowGraphUtils;
import java.util.ArrayDeque;
import java.util.ArrayList;
Expand Down Expand Up @@ -244,7 +245,7 @@ private static boolean jumpsBeforeLoop(ControlFlowGraph cfg, Block loopBlock, De
return true;
}
if ((search.getSuccessors().size() == 1 && search.getSuccessors().contains(jumpTarget))
|| search.equals(cfg.getExitBlock())) {
|| search instanceof Terminus) {
return false;
}

Expand Down
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The previous implementation passes all tests if we simply change the implementation of notFinallyOrExit:

private static boolean notFinallyOrExit(Block block) {
  return !(block instanceof Finally || block instanceof ExceptionalRoutineExit);
}

What's the benefit of the full-on implementation rejig?
If there's some case that this handles that the previous implementation fundamentally doesn't, can we add unit tests to demonstrate that?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The rejig made more sense to me. After playing with the existing implementation a bit more, I think the change you suggested is sufficient. I will revert it to that.

Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@

import au.com.integradev.delphi.cfg.api.Block;
import au.com.integradev.delphi.cfg.api.ControlFlowGraph;
import au.com.integradev.delphi.cfg.api.ExceptionalRoutineExit;
import au.com.integradev.delphi.cfg.api.Finally;
import au.com.integradev.delphi.cfg.api.PossibleException;
import au.com.integradev.delphi.cfg.api.RoutineExit;
import au.com.integradev.delphi.cfg.api.Terminated;
import au.com.integradev.delphi.cfg.api.UnknownException;
import au.com.integradev.delphi.cfg.block.TerminatorKind;
Expand Down Expand Up @@ -50,11 +52,11 @@ public DelphiCheckContext visit(RoutineImplementationNode routine, DelphiCheckCo

private static boolean canReturnNormally(RoutineImplementationNode routine) {
ControlFlowGraph cfg = ControlFlowGraphUtils.findContainingCFG(routine);
return cfg != null && canReturnNormally(cfg, cfg.getEntryBlock());
return cfg != null && canReturnNormally(cfg.getEntryBlock());
}

private static boolean canReturnNormally(ControlFlowGraph cfg, Block block) {
if (block.equals(cfg.getExitBlock())) {
private static boolean canReturnNormally(Block block) {
if (block instanceof RoutineExit) {
return true;
}

Expand All @@ -63,18 +65,20 @@ private static boolean canReturnNormally(ControlFlowGraph cfg, Block block) {
if (isGuaranteedException(block)) {
successors =
block.getSuccessors().stream()
.filter(s -> notFinallyOrExit(cfg, s))
.filter(NoreturnContractCheck::notFinallyOrExit)
.collect(Collectors.toSet());
} else if (block instanceof PossibleException) {
successors = new HashSet<>();
successors.add(((PossibleException) block).getSuccessor());
((PossibleException) block)
.getExceptions().stream().filter(s -> notFinallyOrExit(cfg, s)).forEach(successors::add);
.getExceptions().stream()
.filter(NoreturnContractCheck::notFinallyOrExit)
.forEach(successors::add);
} else {
successors = block.getSuccessors();
}

return successors.stream().anyMatch(s -> canReturnNormally(cfg, s));
return successors.stream().anyMatch(NoreturnContractCheck::canReturnNormally);
}

private static boolean isGuaranteedException(Block block) {
Expand All @@ -83,7 +87,7 @@ private static boolean isGuaranteedException(Block block) {
&& ((Terminated) block).getTerminatorKind() == TerminatorKind.RAISE);
}

private static boolean notFinallyOrExit(ControlFlowGraph cfg, Block block) {
return !(block instanceof Finally || block.equals(cfg.getExitBlock()));
private static boolean notFinallyOrExit(Block block) {
return !(block instanceof Finally || block instanceof ExceptionalRoutineExit);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@

import au.com.integradev.delphi.cfg.api.Block;
import au.com.integradev.delphi.cfg.api.ControlFlowGraph;
import au.com.integradev.delphi.cfg.api.ExceptionalRoutineExit;
import au.com.integradev.delphi.cfg.api.Finally;
import au.com.integradev.delphi.cfg.api.RoutineExit;
import au.com.integradev.delphi.cfg.api.Terminated;
import au.com.integradev.delphi.cfg.api.UnconditionalJump;
import au.com.integradev.delphi.utils.ControlFlowGraphUtils;
Expand Down Expand Up @@ -134,7 +136,11 @@ private boolean isViolation(ControlFlowGraph cfg) {
}
if (!finallyBlock.getSuccessor().equals(finallyBlock.getExceptionSuccessor())) {
// multiple paths after the finally corresponds to code that would be skipped from the jump
return false;
if (!(finallyBlock.getSuccessor() instanceof RoutineExit
&& finallyBlock.getExceptionSuccessor() instanceof ExceptionalRoutineExit)) {
// if both of the paths are routine exits, then they are effectively equivalent
return false;
}
}
}
// if no invalidating try-finally blocks are found, the use is a violation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -341,15 +341,15 @@ void testNoreturnRoutineWithRaiseToIrrelevantExceptShouldAddIssue() {
}

@Test
void testFailOnUpgradeNoreturnRoutineWithRaiseToSpecificEmptyExceptShouldNotAddIssue() {
void testNoreturnRoutineWithRaiseToSpecificEmptyExceptShouldAddIssue() {
CheckVerifier.newVerifier()
.withCheck(new NoreturnContractCheck())
.onFile(
new DelphiTestUnitBuilder()
.appendImpl("uses")
.appendImpl(" System.SysUtils;")
.appendImpl("")
.appendImpl("procedure RaiseToSpecificExcept; noreturn;")
.appendImpl("procedure RaiseToSpecificExcept; noreturn; // Noncompliant")
.appendImpl("begin")
.appendImpl(" try")
.appendImpl(" raise Exception.Create('Error');")
Expand All @@ -359,6 +359,6 @@ void testFailOnUpgradeNoreturnRoutineWithRaiseToSpecificEmptyExceptShouldNotAddI
.appendImpl(" end;")
.appendImpl(" end;")
.appendImpl("end;"))
.verifyNoIssues();
.verifyIssues();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

import au.com.integradev.delphi.cfg.api.Block;
import au.com.integradev.delphi.cfg.api.ControlFlowGraph;
import au.com.integradev.delphi.cfg.api.ExceptionalRoutineExit;
import au.com.integradev.delphi.cfg.api.RoutineExit;
import au.com.integradev.delphi.cfg.block.BlockImpl;
import au.com.integradev.delphi.cfg.block.ProtoBlock;
import au.com.integradev.delphi.cfg.block.ProtoBlockFactory;
Expand Down Expand Up @@ -47,6 +49,7 @@
public class ControlFlowGraphBuilder {
private final List<ProtoBlock> blocks = new ArrayList<>();
private final Deque<ProtoBlock> exitBlocks = new ArrayDeque<>();
private final Deque<ProtoBlock> exceptionalExitBlocks = new ArrayDeque<>();
private final Deque<ProtoBlock> breakTargets = new ArrayDeque<>();
private final Deque<ProtoBlock> continueTargets = new ArrayDeque<>();
private final Map<String, ProtoBlock> labelTargets = new HashMap<>();
Expand All @@ -57,9 +60,14 @@ public class ControlFlowGraphBuilder {

@SuppressWarnings("this-escape")
public ControlFlowGraphBuilder() {
ProtoBlock exceptionalExitBlock = ProtoBlockFactory.exceptionalExitBlock();
addBlock(exceptionalExitBlock);
exceptionalExitBlocks.add(exceptionalExitBlock);

ProtoBlock exitBlock = ProtoBlockFactory.exitBlock();
addBlock(exitBlock);
exitBlocks.add(exitBlock);

addBlockBefore(exitBlock);
}

Expand All @@ -73,8 +81,7 @@ public ControlFlowGraph build() {
}

ControlFlowGraphImpl cfg =
new ControlFlowGraphImpl(
map.get(currentBlock), map.get(exitBlocks.peek()), new ArrayList<>(map.values()));
new ControlFlowGraphImpl(map.get(currentBlock), new ArrayList<>(map.values()));
cfg.prune();

populatePredecessors(cfg);
Expand All @@ -93,15 +100,28 @@ private static void populatePredecessors(ControlFlowGraph cfg) {

private static void populateIds(ControlFlowGraph cfg) {
List<Block> blocks = Lists.reverse(cfg.getBlocks());
for (int blockId = 0; blockId < blocks.size(); blockId++) {
((BlockImpl) blocks.get(blockId)).setId(blockId);
int blockId = 0;
for (Block block : blocks) {
if (block instanceof RoutineExit) {
// Ensure `RoutineExit` is always id 0
((BlockImpl) block).setId(0);
} else if (block instanceof ExceptionalRoutineExit) {
// Ensure `ExceptionalRoutineExit` is always id -1
((BlockImpl) block).setId(-1);
} else {
((BlockImpl) block).setId(++blockId);
}
}
}

public ProtoBlock getExitBlock() {
return exitBlocks.peek();
}

public ProtoBlock getExceptionalExitBlock() {
return exceptionalExitBlocks.peek();
}

public ProtoBlock getBreakTarget() {
return breakTargets.peek();
}
Expand All @@ -110,6 +130,10 @@ public ProtoBlock getContinueTarget() {
return continueTargets.peek();
}

public boolean isInLoop() {
return !continueTargets.isEmpty();
}

public void pushLoopContext(ProtoBlock continueTarget, ProtoBlock breakTarget) {
continueTargets.push(continueTarget);
breakTargets.push(breakTarget);
Expand All @@ -122,10 +146,12 @@ public void popLoopContext() {

public void pushExitBlock(ProtoBlock target) {
exitBlocks.push(target);
exceptionalExitBlocks.push(target);
}

public void popExitBlock() {
exitBlocks.pop();
exceptionalExitBlocks.pop();
}

private static class UnresolvedLabel {
Expand Down Expand Up @@ -193,15 +219,15 @@ public boolean inTryContext() {

public ProtoBlock getCatchTarget(Type exceptionType) {
if (tryContexts.isEmpty()) {
return getExitBlock();
return getExceptionalExitBlock();
}
TryContext tryContext = tryContexts.peek();
return tryContext.catches.keySet().stream()
.filter(catchType -> isCompatibleType(exceptionType, catchType))
.findFirst()
.map(tryContext.catches::get)
.or(() -> Optional.ofNullable(tryContext.elseBlock))
.orElse(getExitBlock());
.orElse(getExceptionalExitBlock());
}

private static boolean isCompatibleType(Type exceptionType, Type catchType) {
Expand All @@ -214,7 +240,7 @@ public Set<ProtoBlock> getAllCatchTargets() {
}
TryContext context = tryContexts.peek();
Stream<ProtoBlock> elseOrExit =
Stream.of(Optional.ofNullable(context.elseBlock).orElse(getExitBlock()));
Stream.of(Optional.ofNullable(context.elseBlock).orElse(getExceptionalExitBlock()));
return Stream.concat(context.catches.values().stream(), elseOrExit)
.filter(Objects::nonNull)
.collect(Collectors.toSet());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import au.com.integradev.delphi.cfg.api.Block;
import au.com.integradev.delphi.cfg.api.ControlFlowGraph;
import au.com.integradev.delphi.cfg.api.ExceptionalRoutineExit;
import au.com.integradev.delphi.cfg.api.Terminated;
import au.com.integradev.delphi.cfg.api.UnconditionalJump;
import au.com.integradev.delphi.cfg.block.BlockImpl;
Expand All @@ -32,12 +33,10 @@

public class ControlFlowGraphImpl implements ControlFlowGraph {
private Block entry;
private final Block exit;
private final List<Block> blocks;

public ControlFlowGraphImpl(Block entry, Block exit, List<Block> blocks) {
public ControlFlowGraphImpl(Block entry, List<Block> blocks) {
this.entry = entry;
this.exit = exit;
this.blocks = blocks;
}

Expand All @@ -46,11 +45,6 @@ public Block getEntryBlock() {
return entry;
}

@Override
public Block getExitBlock() {
return exit;
}

@Override
public List<Block> getBlocks() {
return Collections.unmodifiableList(Lists.reverse(blocks));
Expand All @@ -75,6 +69,20 @@ public void prune() {

blocks.forEach(inactiveBlocks::remove);
} while (!inactiveBlocks.isEmpty());

// Remove the exceptional exit if there are no paths to it
Block exceptionalExitBlock =
Lists.reverse(blocks).stream()
.filter(ExceptionalRoutineExit.class::isInstance)
.findFirst()
.orElseThrow(
() -> new IllegalStateException("Couldn't find the exceptional exit block"));

if (blocks.stream()
.flatMap(block -> block.getSuccessors().stream())
.noneMatch(successor -> successor.equals(exceptionalExitBlock))) {
blocks.remove(exceptionalExitBlock);
}
}

private boolean isInactive(Block block) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ private static void handleExceptionalPaths(ControlFlowGraphBuilder builder) {
private static Set<ProtoBlock> getUnknownExceptionTargets(ControlFlowGraphBuilder builder) {
Set<ProtoBlock> exceptions = builder.getAllCatchTargets();
if (exceptions.isEmpty()) {
return Set.of(builder.getExitBlock());
return Set.of(builder.getExceptionalExitBlock());
}
return exceptions;
}
Expand Down Expand Up @@ -548,10 +548,13 @@ private ControlFlowGraphBuilder buildTryFinally(
FinallyBlockNode finallyNode = node.getFinallyBlock();
builder.addBlock(
ProtoBlockFactory.finallyBlock(
node.getFinallyBlock(), builder.getCurrentBlock(), builder.getExitBlock()));
node.getFinallyBlock(), builder.getCurrentBlock(), builder.getExceptionalExitBlock()));

build(finallyNode.getStatementList(), builder);
builder.pushLoopContext(builder.getCurrentBlock(), builder.getCurrentBlock());
boolean inLoop = builder.isInLoop();
if (inLoop) {
builder.pushLoopContext(builder.getCurrentBlock(), builder.getCurrentBlock());
}
builder.pushExitBlock(builder.getCurrentBlock());

builder.addBlockBeforeCurrent();
Expand All @@ -565,6 +568,9 @@ private ControlFlowGraphBuilder buildTryFinally(
builder.addElement(node);

builder.popExitBlock();
if (inLoop) {
builder.popLoopContext();
}
return builder;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,6 @@ public interface ControlFlowGraph {
*/
Block getEntryBlock();

/**
* The final exit block of the control flow graph
*
* @return the final block of the control flow graph
*/
Block getExitBlock();

/**
* All the blocks within the control flow graph
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Sonar Delphi Plugin
* Copyright (C) 2026 Integrated Application Development
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02
*/
package au.com.integradev.delphi.cfg.api;

/** A block representing an exceptional end of a routine, e.g., an uncaught exception */
public interface ExceptionalRoutineExit extends Terminus {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Sonar Delphi Plugin
* Copyright (C) 2026 Integrated Application Development
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02
*/
package au.com.integradev.delphi.cfg.api;

/** A block representing the end of a routine */
public interface RoutineExit extends Terminus {}
Loading
Loading