* High degree of code duplication in instanceof pattern code generation

* Fixes #2021

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/IfStatement.java

@@ -262,16 +262,7 @@ public void generateCode(BlockScope currentScope, CodeStream codeStream) {
 		this.elseStatement.generateCode(currentScope, codeStream);
 	} else {
 		// generate condition side-effects
-		if (this.condition.containsPatternVariable()) {
-			this.condition.generateOptimizedBoolean(
-				currentScope,
-				codeStream,
-				endifLabel,
-				null,
-				cst == Constant.NotAConstant);
-		} else {
-			this.condition.generateCode(currentScope, codeStream, false);
-		}
+		this.condition.generateCode(currentScope, codeStream, false);
 		codeStream.recordPositionsFrom(pc, this.sourceStart);
 	}
 	// May loose some local variable initializations : affecting the local variable attributes

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/InstanceOfExpression.java

@@ -104,80 +104,90 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
 @Override
 public void generateCode(BlockScope currentScope, CodeStream codeStream, boolean valueRequired) {
 
-	int pc = codeStream.position;
+	BranchLabel falseLabel, continueLabel;
 
-	this.expression.generateCode(currentScope, codeStream, true);
-	if (this.secretExpressionValue != null) {
-		codeStream.store(this.secretExpressionValue, true);
-		codeStream.addVariable(this.secretExpressionValue);
+	if (this.pattern != null) {
+		falseLabel = new BranchLabel(codeStream);
+		continueLabel = new BranchLabel(codeStream);
+	} else {
+		falseLabel = null;
+		continueLabel = null;
 	}
-	codeStream.instance_of(this.type, this.type.resolvedType);
+
+	generateOptimizedBoolean(currentScope, codeStream, null, falseLabel, true);
+
 	if (this.pattern != null) {
-		BranchLabel falseLabel = new BranchLabel(codeStream);
-		BranchLabel trueLabel = new BranchLabel(codeStream);
-		BranchLabel continueLabel = new BranchLabel(codeStream);
-		codeStream.ifeq(falseLabel);
 
-		if (this.secretExpressionValue != null) {
-			codeStream.load(this.secretExpressionValue);
-			codeStream.removeVariable(this.secretExpressionValue);
-		} else {
-			this.expression.generateCode(currentScope, codeStream, true);
+		if (valueRequired) {
+			codeStream.iconst_1();
+			codeStream.goto_(continueLabel);
 		}
-		this.pattern.generateOptimizedBoolean(currentScope, codeStream, trueLabel, falseLabel);
-
-		trueLabel.place();
-		codeStream.iconst_1();
-		codeStream.goto_(continueLabel);
 		falseLabel.place();
+		/* We are generating a "thunk" of sorts now, that flow analysis has no clue about.
+		   so, we need to manage the live variables manually. Pattern bindings are not definitely
+		   assigned here as we are in instanceof false region.
+	    */
 		for (LocalVariableBinding binding : this.pattern.bindingsWhenTrue()) {
 			binding.recordInitializationEndPC(codeStream.position);
 		}
-		codeStream.iconst_0();
-		continueLabel.place();
 
+		if (valueRequired)
+			codeStream.iconst_0();
+
+		continueLabel.place();
 	}
-	if (valueRequired) {
-		codeStream.generateImplicitConversion(this.implicitConversion);
-	} else {
-		codeStream.pop();
-	}
-	codeStream.recordPositionsFrom(pc, this.sourceStart);
+	codeStream.recordPositionsFrom(codeStream.position, this.sourceEnd);
 }
+
 @Override
 public void generateOptimizedBoolean(BlockScope currentScope, CodeStream codeStream, BranchLabel trueLabel, BranchLabel falseLabel, boolean valueRequired) {
-	// a label valued to nil means: by default we fall through the case...
-	// both nil means we leave the value on the stack
-	if (this.elementVariable == null && this.pattern == null) {
-		super.generateOptimizedBoolean(currentScope, codeStream, trueLabel, falseLabel, valueRequired);
-		return;
-	}
+
+	/* A label valued to null is supposed to mean: by default we fall through the case ...
+	   Both null means no "optimization" and we leave the value on the stack
+	   But we have trouble when
+	        this.pattern != null && trueLabel != null && falseLabel == null
+
+	   In this case, since we have no control over placement of the trueLabel, we won't know where to emit the pattern binding code.
+	   So what we do is always emit ifeq even when optimization would call for ifne and treat the whole "blob" of pattern matching
+	   code as part of the predicate. if you think about long and hard you can convince yourself this is semantically correct.
+	*/
 
 	int pc = codeStream.position;
+	boolean optimize = trueLabel != null || falseLabel != null;
 
 	this.expression.generateCode(currentScope, codeStream, true);
 	if (this.secretExpressionValue != null) {
 		codeStream.store(this.secretExpressionValue, true);
 		codeStream.addVariable(this.secretExpressionValue);
 	}
 
-	BranchLabel nextSibling = falseLabel != null ? falseLabel : new BranchLabel(codeStream);
+	BranchLabel internalFalseLabel = falseLabel != null ? falseLabel : new BranchLabel(codeStream);
 	codeStream.instance_of(this.type, this.type.resolvedType);
-	codeStream.ifeq(nextSibling);
-	if (this.secretExpressionValue != null) {
-		codeStream.load(this.secretExpressionValue);
-		codeStream.removeVariable(this.secretExpressionValue);
-	} else {
-		this.expression.generateCode(currentScope, codeStream, true);
+
+	boolean shouldPop;
+	if (optimize) {
+		codeStream.ifeq(internalFalseLabel);
+		shouldPop = false; /* drained already by ifeq */
+	}
+	else {
+		assert this.pattern == null;
+		shouldPop = !valueRequired;
 	}
 
-	this.pattern.generateOptimizedBoolean(currentScope, codeStream, trueLabel, nextSibling);
+	if (this.pattern != null) {
+		if (this.secretExpressionValue != null) {
+			codeStream.load(this.secretExpressionValue);
+			codeStream.removeVariable(this.secretExpressionValue);
+		} else {
+			this.expression.generateCode(currentScope, codeStream, true);
+		}
+		this.pattern.generateOptimizedBoolean(currentScope, codeStream, trueLabel, internalFalseLabel);
+	}
 
-	if (valueRequired) {
-		codeStream.generateImplicitConversion(this.implicitConversion);
-	} else {
+	if (shouldPop) {
 		codeStream.pop();
 	}
+
 	codeStream.recordPositionsFrom(pc, this.sourceStart);
 
 	int position = codeStream.position;
@@ -191,14 +201,14 @@ public void generateOptimizedBoolean(BlockScope currentScope, CodeStream codeStr
 			if (trueLabel == null) {
 				// Implicit falling through the TRUE case
 			} else {
-				// No implicit fall through TRUE/FALSE --> should never occur
+				// No implicit fall through TRUE/FALSE --> classic instanceof code generation called from generateCode above
 			}
 		}
 	}
 	codeStream.recordPositionsFrom(position, this.sourceEnd);
 
-	if (nextSibling != falseLabel)
-		nextSibling.place();
+	if (internalFalseLabel != falseLabel)
+		internalFalseLabel.place();
 }
 
 @Override

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java

@@ -988,7 +988,10 @@ private void generateCodePatternCaseEpilogue(CodeStream codeStream, int caseInde
 			Pattern pattern = (Pattern) caseStatement.constantExpressions[caseStatement.patternIndex];
 			pattern.elseTarget.place();
 			if (!pattern.isAlwaysTrue()) {
-				// at the trampoline here, pattern bindings are not definitely assigned.
+				/* We are generating a "thunk"/"trampoline" of sorts now, that flow analysis has no clue about.
+				   We need to manage the live variables manually. Pattern bindings are not definitely
+				   assigned here as we are in the else region.
+			    */
 				final LocalVariableBinding[] bindingsWhenTrue = pattern.bindingsWhenTrue();
 				Stream.of(bindingsWhenTrue).forEach(v->v.recordInitializationEndPC(codeStream.position));
 				codeStream.loadInt(this.nullProcessed ? caseIndex - 1 : caseIndex);