fix: fix invalid if model with empty statements in then/else branch (#…

…2733)

src/main/java/spoon/support/compiler/jdt/ContextBuilder.java

@@ -129,7 +129,10 @@ void exit(ASTNode node) {
 		if (!stack.isEmpty()) {
 			this.jdtTreeBuilder.getExiter().setChild(current);
 			this.jdtTreeBuilder.getExiter().setChild(pair.node);
-			this.jdtTreeBuilder.getExiter().scan(stack.peek().element);
+			ASTPair parentPair = stack.peek();
+			this.jdtTreeBuilder.getExiter().setParent(parentPair.node);
+			//visit ParentExiter using parent Spoon node, while it has access to parent's JDT node and child Spoon and JDT node
+			this.jdtTreeBuilder.getExiter().scan(parentPair.element);
 		}
 	}
 

src/main/java/spoon/support/compiler/jdt/JDTCommentBuilder.java

@@ -431,10 +431,12 @@ public <E> void visitCtSwitch(CtSwitch<E> e) {
 
 			@Override
 			public void visitCtIf(CtIf e) {
-				if (!(e.getThenStatement() instanceof CtBlock)) {
-					if (comment.getPosition().getSourceEnd() <= e.getThenStatement().getPosition().getSourceStart()) {
-						e.getThenStatement().addComment(comment);
-						return;
+				if (e.getThenStatement() != null) {
+					if (!(e.getThenStatement() instanceof CtBlock)) {
+						if (comment.getPosition().getSourceEnd() <= e.getThenStatement().getPosition().getSourceStart()) {
+							e.getThenStatement().addComment(comment);
+							return;
+						}
 					}
 				}
 				if (e.getElseStatement() != null) {

src/main/java/spoon/support/compiler/jdt/JDTTreeBuilder.java

@@ -142,6 +142,7 @@
 import spoon.reflect.declaration.CtAnonymousExecutable;
 import spoon.reflect.declaration.CtClass;
 import spoon.reflect.declaration.CtConstructor;
+import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtEnumValue;
 import spoon.reflect.declaration.CtField;
 import spoon.reflect.declaration.CtMethod;
@@ -449,14 +450,18 @@ public void endVisit(LabeledStatement labeledStatement, BlockScope scope) {
 							newSourceStart, oldPos.getSourceEnd(),
 							oldPos.getCompilationUnit().getLineSeparatorPositions()));
 				}
+				//call exit with origin labeled statement
+				//because some listeners needs origin one
+				//we cannot call exit on unexpected child
+				context.exit(labeledStatement);
 				//use childStmt instead of helper block
-				//1) disconnect childStmt from it's helper block
-				childStmt.setParent(null);
-				//2) inject childStmt instead of block into context.stack,
-				//so ParentExiter will use it instead of block
-				pair.element = childStmt;
-				pair.node = labeledStatement.statement;
-				context.exit(pair.node);
+				CtElement parent = block.getParent();
+				//remember whether parent was implicit
+				boolean parentIsImplicit = parent.isImplicit();
+				//because replace resets CtBlock#isImplicit to false
+				block.replace(childStmt);
+				//but we need to keep it as it was before
+				parent.setImplicit(parentIsImplicit);
 				return;
 			}
 		}

src/main/java/spoon/support/compiler/jdt/ParentExiter.java

@@ -28,6 +28,7 @@
 import org.eclipse.jdt.internal.compiler.ast.ExplicitConstructorCall;
 import org.eclipse.jdt.internal.compiler.ast.Expression;
 import org.eclipse.jdt.internal.compiler.ast.ForStatement;
+import org.eclipse.jdt.internal.compiler.ast.IfStatement;
 import org.eclipse.jdt.internal.compiler.ast.MessageSend;
 import org.eclipse.jdt.internal.compiler.ast.MethodDeclaration;
 import org.eclipse.jdt.internal.compiler.ast.QualifiedAllocationExpression;
@@ -37,6 +38,8 @@
 import org.eclipse.jdt.internal.compiler.ast.TypeReference;
 import org.eclipse.jdt.internal.compiler.ast.UnionTypeReference;
 import org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding;
+
+import spoon.SpoonException;
 import spoon.reflect.code.BinaryOperatorKind;
 import spoon.reflect.code.CtArrayAccess;
 import spoon.reflect.code.CtArrayRead;
@@ -115,6 +118,7 @@ public class ParentExiter extends CtInheritanceScanner {
 
 	private CtElement child;
 	private ASTNode childJDT;
+	private ASTNode parentJDT;
 	private Map<CtTypedElement<?>, List<CtAnnotation>> annotationsMap = new HashMap<>();
 
 	/**
@@ -132,6 +136,10 @@ public void setChild(ASTNode child) {
 		this.childJDT = child;
 	}
 
+	public void setParent(ASTNode parent) {
+		this.parentJDT = parent;
+	}
+
 	@Override
 	public void scanCtElement(CtElement e) {
 		if (child instanceof CtAnnotation && this.jdtTreeBuilder.getContextBuilder().annotationValueName.isEmpty()) {
@@ -605,17 +613,24 @@ public void visitCtIf(CtIf ifElement) {
 			return;
 		} else if (child instanceof CtStatement) {
 			CtStatement child = (CtStatement) this.child;
+			// we create implicit blocks everywhere for facilitating transformation
 			if (!(this.child instanceof CtBlock)) {
 				child = jdtTreeBuilder.getFactory().Code().createCtBlock(child);
 				child.setImplicit(true);
 				child.setPosition(this.child.getPosition());
 			}
-			if (ifElement.getThenStatement() == null) {
+
+			IfStatement ifJDT = (IfStatement) this.parentJDT;
+			if (ifJDT.thenStatement == this.childJDT) {
+				//we are visiting `then` of `if`
 				ifElement.setThenStatement(child);
 				return;
-			} else if (ifElement.getElseStatement() == null) {
+			} else if (ifJDT.elseStatement == this.childJDT) {
+				//we are visiting `else` of `if`
 				ifElement.setElseStatement(child);
 				return;
+			} else {
+				throw new SpoonException("Unexpected call of ParentExiter on CtIf");
 			}
 		}
 		super.visitCtIf(ifElement);

src/test/java/spoon/test/comment/CommentTest.java

@@ -1067,4 +1067,17 @@ public void testCommentAssociationAndPrettyPrint() {
 		CtMethod<?> method = nestedIface.getMethodsByName("mytest").get(0);
 		assertEquals(1, method.getComments().size());
 	}
+
+	@Test
+	public void testEmptyStatementComments() {
+		//contract: model building should not produce NPE, comments should exist
+		Launcher launcher = new Launcher();
+		launcher.addInputResource("./src/test/java/spoon/test/comment/testclasses/EmptyStatementComments.java");
+		launcher.getEnvironment().setCommentEnabled(true);
+
+		CtModel model = launcher.buildModel();
+		List<CtIf> conditions = model.getElements(new TypeFilter<>(CtIf.class));
+		assertEquals("comment", conditions.get(0).getComments().get(0).getContent());
+		assertEquals("comment", conditions.get(1).getComments().get(0).getContent());
+	}
 }

src/test/java/spoon/test/comment/testclasses/EmptyStatementComments.java

@@ -0,0 +1,11 @@
+package spoon.test.comment.testclasses;
+
+public class EmptyStatementComments {
+    void m1() {
+        if (true) // comment
+            ;
+
+        if (true) /* comment */
+            ;
+    }
+}

src/test/java/spoon/test/condition/ConditionalTest.java

@@ -16,7 +16,15 @@
  */
 package spoon.test.condition;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import java.util.List;
+
 import org.junit.Test;
+
 import spoon.reflect.code.CtBlock;
 import spoon.reflect.code.CtConditional;
 import spoon.reflect.code.CtIf;
@@ -25,13 +33,9 @@
 import spoon.reflect.declaration.CtType;
 import spoon.reflect.visitor.filter.TypeFilter;
 import spoon.test.condition.testclasses.Foo;
+import spoon.test.condition.testclasses.Foo2;
 import spoon.testing.utils.ModelUtils;
 
-import java.util.List;
-
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
 public class ConditionalTest {
 	String newLine = System.getProperty("line.separator");
 
@@ -97,4 +101,34 @@ public void testNoBlockInConditionAndLoop() throws Exception {
 				+ "    java.lang.System.out.println();" + newLine,
 				method.getBody().getStatement(0).toString());
 	}
+
+	@Test
+	public void testNoThenBlock() throws Exception {
+		// contract: corner cases provided by @Egor18 are well handled
+		final CtType<Foo> aFoo = ModelUtils.buildClass(Foo.class);
+		CtMethod<Object> method = aFoo.getMethod("m4");
+		final List<CtIf> conditions = method.getElements(new TypeFilter<>(CtIf.class));
+
+		assertNotNull(conditions.get(0).getThenStatement());
+		assertNull(conditions.get(0).getElseStatement());
+
+		assertNull(conditions.get(1).getThenStatement());
+		assertNull(conditions.get(1).getElseStatement());
+	}
+
+	@Test
+	public void testNoThenBlockBug() throws Exception {
+		// contract: empty statement are correctly handled
+		// the fix consists of addind visit(EmptyStatement) in JDTTreeBuilder
+		final CtType aFoo = ModelUtils.buildClass(Foo2.class);
+		CtMethod<Object> method = aFoo.getMethod("bug");
+		final List<CtIf> conditions = method.getElements(new TypeFilter<>(CtIf.class));
+
+		// contract: an empty statement is transformed into an empty block with a comment
+		assertNull(conditions.get(0).getThenStatement());
+
+		// and we have the correct else statement
+		assertNotNull(conditions.get(0).getElseStatement());
+		assertEquals("java.lang.System.out.println();", conditions.get(0).getElseStatement().toString().trim());
+	}
 }

src/test/java/spoon/test/condition/testclasses/Foo.java

@@ -43,4 +43,10 @@ else if (true)
 			break;
 		} while (true);
 	}
+
+	void m4() {
+		if (false) {}
+
+		if (false);
+	}
 }

src/test/java/spoon/test/condition/testclasses/Foo2.java

@@ -0,0 +1,10 @@
+package spoon.test.condition.testclasses;
+
+public class Foo2 {
+	// bug case kindly provided by @Egor18
+	// in https://github.com/INRIA/spoon/pull/2733
+	void bug() {
+		if (false);
+		else System.out.println();
+	}
+}