Bug 342906 - No warning if only one of an if/else pair has a return statement

codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java

@@ -21,17 +21,23 @@ import org.eclipse.cdt.codan.core.model.cfg.IControlFlowGraph;
 import org.eclipse.cdt.codan.core.model.cfg.IExitNode;
 import org.eclipse.cdt.core.dom.ast.ASTVisitor;
 import org.eclipse.cdt.core.dom.ast.DOMException;
+import org.eclipse.cdt.core.dom.ast.IASTCompoundStatement;
 import org.eclipse.cdt.core.dom.ast.IASTDeclSpecifier;
 import org.eclipse.cdt.core.dom.ast.IASTDeclaration;
 import org.eclipse.cdt.core.dom.ast.IASTDeclarator;
+import org.eclipse.cdt.core.dom.ast.IASTDoStatement;
 import org.eclipse.cdt.core.dom.ast.IASTExpression;
+import org.eclipse.cdt.core.dom.ast.IASTForStatement;
 import org.eclipse.cdt.core.dom.ast.IASTFunctionDeclarator;
 import org.eclipse.cdt.core.dom.ast.IASTFunctionDefinition;
+import org.eclipse.cdt.core.dom.ast.IASTIfStatement;
 import org.eclipse.cdt.core.dom.ast.IASTNamedTypeSpecifier;
 import org.eclipse.cdt.core.dom.ast.IASTReturnStatement;
 import org.eclipse.cdt.core.dom.ast.IASTSimpleDeclSpecifier;
 import org.eclipse.cdt.core.dom.ast.IASTStatement;
+import org.eclipse.cdt.core.dom.ast.IASTSwitchStatement;
 import org.eclipse.cdt.core.dom.ast.IASTTypeId;
+import org.eclipse.cdt.core.dom.ast.IASTWhileStatement;
 import org.eclipse.cdt.core.dom.ast.IBasicType;
 import org.eclipse.cdt.core.dom.ast.IBinding;
 import org.eclipse.cdt.core.dom.ast.IType;
@@ -83,15 +89,18 @@ public class ReturnChecker extends AbstractAstFunctionChecker {
 		}
 		public int visit(IASTStatement stmt) {
 			if (stmt instanceof IASTReturnStatement) {
-				hasret = true;
 				IASTReturnStatement ret = (IASTReturnStatement) stmt;
+				boolean hasValue = ret.getReturnValue() != null;
+				if (hasret==false && hasValue) {
+					hasret=true;
+				}
 				if (!isVoid(func) && !isConstructorDestructor()) {
 					if (checkImplicitReturn(RET_NO_VALUE_ID) || isExplicitReturn(func)) {
-						if (ret.getReturnValue() == null)
+						if (!hasValue)
 							reportProblem(RET_NO_VALUE_ID, ret);
 					}
 				} else {
-					if (ret.getReturnValue() != null) {
+					if (hasValue) {
 						IType type = ret.getReturnValue().getExpressionType();
 						if (isVoid(type))
 							return PROCESS_SKIP;
@@ -130,15 +139,41 @@ public class ReturnChecker extends AbstractAstFunctionChecker {
 			return; // if it is template get out of here
 		ReturnStmpVisitor visitor = new ReturnStmpVisitor(func);
 		func.accept(visitor);
-		if (!visitor.hasret) {
+		boolean nonVoid = !isVoid(func);
-			// no return at all
+		if (nonVoid) {
-			if (!isVoid(func) && (checkImplicitReturn(RET_NORET_ID) || isExplicitReturn(func))) {
+			if (!visitor.hasret) {
-				if (endsWithNoExitNode(func))
+				// no return at all
-					reportProblem(RET_NORET_ID, func.getDeclSpecifier());
+				if (checkImplicitReturn(RET_NORET_ID) || isExplicitReturn(func)) {
+					if (endsWithNoExitNode(func))
+						reportProblem(RET_NORET_ID, func.getDeclSpecifier());
+				}
+			} else {
+				// there a return but maybe it is only on one branch
+				IASTStatement body = func.getBody();
+				if (body instanceof IASTCompoundStatement) {
+					IASTStatement[] statements = ((IASTCompoundStatement) body).getStatements();
+					if (statements.length > 0) {
+						IASTStatement last = statements[statements.length - 1];
+						// now check if last statement if complex (for optimization reasons, building CFG is expensive)
+						if (isCompoundStatement(last)) {
+							if (endsWithNoExitNode(func))
+								reportProblem(RET_NORET_ID, func.getDeclSpecifier());
+						}
+					}
+				}
 			}
 		}
 	}
 
+	/**
+	 * @param last
+	 * @return
+	 */
+	private boolean isCompoundStatement(IASTStatement last) {
+		return last instanceof IASTIfStatement || last instanceof IASTWhileStatement || last instanceof IASTDoStatement
+				|| last instanceof IASTForStatement || last instanceof IASTSwitchStatement;
+	}
+
 	/**
 	 * @param if - problem id
 	 * @return true if need to check inside functions with implicit return
@@ -160,7 +195,7 @@ public class ReturnChecker extends AbstractAstFunctionChecker {
 			IExitNode node = exitNodeIterator.next();
 			if (((ICfgData) node).getData() == null) {
 				// if it real exit node such as return, exit or throw data
-				// will be an ast node, it is null it is fake node added by the
+				// will be an ast node, if it is null it is a fake node added by the
 				// graph builder
 				noexitop = true;
 				break;

codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/ReturnCheckerTest.java

@@ -186,4 +186,28 @@ public class ReturnCheckerTest extends CheckerTestCase {
 		loadCodeAndRunCpp(getAboveComment());
 		checkErrorLine(1);
 	}
+
+//	int f()
+//	{
+//	    if (g())
+//	        h();
+//	    else
+//	        return 0;
+//	}
+	public void testBranches_Bug342906() {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(1);
+	}
+
+//	int f()
+//	{
+//	    switch (g()) {
+//	      case 1: h(); break;
+//	      case 2:
+//	        return 0;
+//	}
+	public void testSwitch() {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(1);
+	}
 }