Variable finality highlighting fixes

Unreachable branches are analyzed for twice assignment, but constants are evaluated.
Fixes IDEA-186321 Bad code green: non-iterating for-loop update
Fixes IDEA-186304 good code red: field might not have been initialized in unreachable branch
Fixes (mostly) IDEA-186305 good code red: variable might already have been assigned to

java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/HighlightControlFlowUtil.java

@@ -20,7 +20,9 @@ import com.intellij.psi.util.FileTypeUtils;
 import com.intellij.psi.util.PsiTreeUtil;
 import com.intellij.psi.util.PsiUtil;
 import com.intellij.util.BitUtil;
+import com.intellij.util.ObjectUtils;
 import com.intellij.util.Processor;
+import com.siyeh.ig.psiutils.VariableAccessUtils;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
@@ -569,7 +571,7 @@ public class HighlightControlFlowUtil {
     Collection<ControlFlowUtil.VariableInfo> codeBlockProblems = finalVarProblems.get(codeBlock);
     if (codeBlockProblems == null) {
       try {
-        final ControlFlow controlFlow = getControlFlowNoConstantEvaluate(codeBlock);
+        final ControlFlow controlFlow = getControlFlow(codeBlock);
         codeBlockProblems = ControlFlowUtil.getInitializedTwice(controlFlow);
       }
       catch (AnalysisCanceledException e) {
@@ -597,45 +599,31 @@ public class HighlightControlFlowUtil {
 
   @Nullable
   static HighlightInfo checkCannotWriteToFinal(@NotNull PsiExpression expression, @NotNull PsiFile containingFile) {
-    PsiReferenceExpression reference = null;
-    boolean readBeforeWrite = false;
+    PsiExpression operand = null;
     if (expression instanceof PsiAssignmentExpression) {
-      final PsiAssignmentExpression assignmentExpression = (PsiAssignmentExpression)expression;
-      final PsiExpression left = PsiUtil.skipParenthesizedExprDown(assignmentExpression.getLExpression());
-      if (left instanceof PsiReferenceExpression) {
-        reference = (PsiReferenceExpression)left;
-      }
-      readBeforeWrite = assignmentExpression.getOperationTokenType() != JavaTokenType.EQ;
+      operand = ((PsiAssignmentExpression)expression).getLExpression();
     }
-    else if (expression instanceof PsiUnaryExpression) {
-      final PsiExpression operand = PsiUtil.skipParenthesizedExprDown(((PsiUnaryExpression)expression).getOperand());
-      final IElementType sign = ((PsiUnaryExpression)expression).getOperationTokenType();
-      if (operand instanceof PsiReferenceExpression && (sign == JavaTokenType.PLUSPLUS || sign == JavaTokenType.MINUSMINUS)) {
-        reference = (PsiReferenceExpression)operand;
-      }
-      readBeforeWrite = true;
+    else if (PsiUtil.isIncrementDecrementOperation(expression)) {
+      operand = ((PsiUnaryExpression)expression).getOperand();
     }
-    final PsiElement resolved = reference == null ? null : reference.resolve();
-    PsiVariable variable = resolved instanceof PsiVariable ? (PsiVariable)resolved : null;
+    PsiReferenceExpression reference = ObjectUtils.tryCast(PsiUtil.skipParenthesizedExprDown(operand), PsiReferenceExpression.class);
+    PsiVariable variable = reference == null ? null : ObjectUtils.tryCast(reference.resolve(), PsiVariable.class);
     if (variable == null || !variable.hasModifierProperty(PsiModifier.FINAL)) return null;
     final boolean canWrite = canWriteToFinal(variable, expression, reference, containingFile) && checkWriteToFinalInsideLambda(variable, reference) == null;
-    if (readBeforeWrite || !canWrite) {
-      final String name = variable.getName();
-      String description = JavaErrorMessages.message(canWrite ? "variable.not.initialized" : "assignment.to.final.variable", name);
-      final HighlightInfo highlightInfo =
-        HighlightInfo.newHighlightInfo(HighlightInfoType.ERROR).range(reference.getTextRange()).descriptionAndTooltip(description).create();
-      final PsiElement innerClass = getInnerClassVariableReferencedFrom(variable, expression);
-      if (innerClass == null || variable instanceof PsiField) {
-        QuickFixAction.registerQuickFixAction(highlightInfo,
-                                              QUICK_FIX_FACTORY.createModifierListFix(variable, PsiModifier.FINAL, false, false));
-      }
-      else {
-        QuickFixAction.registerQuickFixAction(highlightInfo, QUICK_FIX_FACTORY.createVariableAccessFromInnerClassFix(variable, innerClass));
-      }
-      return highlightInfo;
+    if (canWrite) return null;
+    final String name = variable.getName();
+    String description = JavaErrorMessages.message("assignment.to.final.variable", name);
+    final HighlightInfo highlightInfo =
+      HighlightInfo.newHighlightInfo(HighlightInfoType.ERROR).range(reference.getTextRange()).descriptionAndTooltip(description).create();
+    final PsiElement innerClass = getInnerClassVariableReferencedFrom(variable, expression);
+    if (innerClass == null || variable instanceof PsiField) {
+      QuickFixAction.registerQuickFixAction(highlightInfo,
+                                            QUICK_FIX_FACTORY.createModifierListFix(variable, PsiModifier.FINAL, false, false));
     }
-
-    return null;
+    else {
+      QuickFixAction.registerQuickFixAction(highlightInfo, QUICK_FIX_FACTORY.createVariableAccessFromInnerClassFix(variable, innerClass));
+    }
+    return highlightInfo;
   }
 
   private static boolean canWriteToFinal(@NotNull PsiVariable variable,

java/java-psi-impl/src/com/intellij/psi/controlFlow/ControlFlowAnalyzer.java

@@ -731,8 +731,7 @@ class ControlFlowAnalyzer extends JavaElementVisitor {
       conditionExpression.accept(this);
     }
 
-    boolean generateElseFlow = true;
-    boolean generateThenFlow = true;
+    boolean thenReachable = true;
     boolean generateConditionalJump = true;
     /*
      * if() statement generated instructions outline:
@@ -748,16 +747,15 @@ class ControlFlowAnalyzer extends JavaElementVisitor {
     if (myEvaluateConstantIfCondition) {
       final Object value = myConstantEvaluationHelper.computeConstantExpression(conditionExpression);
       if (value instanceof Boolean) {
-        boolean condition = ((Boolean)value).booleanValue();
-        generateThenFlow = condition;
-        generateElseFlow = !condition;
+        thenReachable = ((Boolean)value).booleanValue();
         generateConditionalJump = false;
         myCurrentFlow.setConstantConditionOccurred(true);
       }
     }
-    if (generateConditionalJump) {
+    if (generateConditionalJump || !thenReachable) {
       BranchingInstruction.Role role = elseBranch == null ? BranchingInstruction.Role.END : BranchingInstruction.Role.ELSE;
-      Instruction instruction = new ConditionalGoToInstruction(0, role, conditionExpression);
+      Instruction instruction = generateConditionalJump ? new ConditionalGoToInstruction(0, role, conditionExpression) :
+        new GoToInstruction(0, role);
       myCurrentFlow.addInstruction(instruction);
       if (elseBranch == null) {
         addElementOffsetLater(statement, false);
@@ -766,16 +764,13 @@ class ControlFlowAnalyzer extends JavaElementVisitor {
         addElementOffsetLater(elseBranch, true);
       }
     }
-    if (thenBranch != null && generateThenFlow) {
+    if (thenBranch != null) {
       thenBranch.accept(this);
     }
-    if (elseBranch != null && generateElseFlow) {
-      if (generateThenFlow) {
-        // make jump to end after then branch (only if it has been generated)
-        Instruction instruction = new GoToInstruction(0);
-        myCurrentFlow.addInstruction(instruction);
-        addElementOffsetLater(statement, false);
-      }
+    if (elseBranch != null) {
+      Instruction instruction = new GoToInstruction(0);
+      myCurrentFlow.addInstruction(instruction);
+      addElementOffsetLater(statement, false);
       elseBranch.accept(this);
     }
 
@@ -996,11 +991,11 @@ class ControlFlowAnalyzer extends JavaElementVisitor {
 
     final PsiExpression condition = statement.getAssertCondition();
     boolean generateCondition = true;
-    boolean generateThrow = true;
+    boolean throwReachable = true;
     if (myEvaluateConstantIfCondition) {
       Object conditionValue = myConstantEvaluationHelper.computeConstantExpression(condition);
       if (conditionValue instanceof Boolean) {
-        generateThrow = !((Boolean)conditionValue);
+        throwReachable = !((Boolean)conditionValue);
         generateCondition = false;
         emitEmptyInstruction();
       }
@@ -1025,18 +1020,21 @@ class ControlFlowAnalyzer extends JavaElementVisitor {
       Instruction ifTrue = new ConditionalGoToInstruction(0, BranchingInstruction.Role.END, statement.getAssertCondition());
       myCurrentFlow.addInstruction(ifTrue);
       addElementOffsetLater(statement, false);
-    }
-    if (generateThrow) {
-      PsiExpression description = statement.getAssertDescription();
-      if (description != null) {
-        description.accept(this);
+    } else {
+      if (!throwReachable) {
+        myCurrentFlow.addInstruction(new GoToInstruction(0, BranchingInstruction.Role.END));
+        addElementOffsetLater(statement, false);
       }
-      // if description is evaluated, the assert statement cannot complete normally
-      // though non-necessarily AssertionError will be thrown (description may throw something, or AssertionError ctor, etc.)
-      PsiClassType exceptionClass = JavaPsiFacade.getElementFactory(statement.getProject()).createTypeByFQClassName(
-        CommonClassNames.JAVA_LANG_THROWABLE, statement.getResolveScope());
-      addThrowInstructions(findThrowToBlocks(exceptionClass));
     }
+    PsiExpression description = statement.getAssertDescription();
+    if (description != null) {
+      description.accept(this);
+    }
+    // if description is evaluated, the assert statement cannot complete normally
+    // though non-necessarily AssertionError will be thrown (description may throw something, or AssertionError ctor, etc.)
+    PsiClassType exceptionClass = JavaPsiFacade.getElementFactory(statement.getProject()).createTypeByFQClassName(
+      CommonClassNames.JAVA_LANG_THROWABLE, statement.getResolveScope());
+    addThrowInstructions(findThrowToBlocks(exceptionClass));
 
     myStartStatementStack.popStatement();
     myEndStatementStack.popStatement();

java/java-psi-impl/src/com/intellij/psi/controlFlow/ControlFlowUtil.java

@@ -1487,6 +1487,15 @@ public class ControlFlowUtil {
     return visitor.getResult().intValue();
   }
 
+  private static int findUnprocessed(int startOffset, int endOffset, InstructionClientVisitor<?> visitor) {
+    for (int i = startOffset; i < endOffset; i++) {
+      if (!visitor.processedInstructions[i]) {
+        return i;
+      }
+    }
+    return endOffset;
+  }
+
   private static void depthFirstSearch(ControlFlow flow, InstructionClientVisitor visitor) {
     depthFirstSearch(flow, visitor, 0, flow.getSize());
   }
@@ -1905,9 +1914,16 @@ public class ControlFlowUtil {
 
   @NotNull
   public static Collection<VariableInfo> getInitializedTwice(@NotNull ControlFlow flow, int startOffset, int endOffset) {
-    InitializedTwiceClientVisitor visitor = new InitializedTwiceClientVisitor(flow, startOffset);
-    depthFirstSearch(flow, visitor, startOffset, endOffset);
-    return visitor.getResult();
+    while (startOffset < endOffset) {
+      InitializedTwiceClientVisitor visitor = new InitializedTwiceClientVisitor(flow, startOffset);
+      depthFirstSearch(flow, visitor, startOffset, endOffset);
+      Collection<VariableInfo> result = visitor.getResult();
+      if(!result.isEmpty()) {
+        return result;
+      }
+      startOffset = findUnprocessed(startOffset, endOffset, visitor);
+    }
+    return Collections.emptyList();
   }
 
   private static class InitializedTwiceClientVisitor extends InstructionClientVisitor<Collection<VariableInfo>> {

java/java-tests/testData/codeInsight/daemonCodeAnalyzer/advHighlighting/AssignToFinal.java

@@ -143,3 +143,76 @@ class TX {
     (<error descr="Variable 'k' might not have been initialized">k</error>)++;
   }
 }
+// IDEA-186321
+class ForLoop {
+  private final int i;
+  {
+    i = 1;
+    for(;;i = 2, <error descr="Variable 'i' might already have been assigned to">i</error> = 3) {
+      break;
+    }
+  }
+
+  private final int j;
+  {
+    for(;;j = 2) {
+      <error descr="Variable 'j' might already have been assigned to">j</error> = 1;
+      break;
+    }
+  }
+}
+// IDEA-186305
+class Asserts {
+  final int x;
+  {
+    x = 1;
+    assert true : x = 2;
+  }
+
+  final int x1;
+  {
+    x1 = 1;
+    assert false : <error descr="Variable 'x1' might already have been assigned to">x1</error> = 2;
+  }
+
+  final int y;
+  {
+    try {
+      assert true : y = 2;
+    }
+    catch (Throwable t) {}
+    // javac accepts this, though this looks strange
+    <error descr="Variable 'y' might already have been assigned to">y</error> = 1;
+  }
+
+  final int y1;
+  {
+    try {
+      assert false : y1 = 2;
+    }
+    catch (Throwable t) {}
+    <error descr="Variable 'y1' might already have been assigned to">y1</error> = 1;
+  }
+}
+// IDEA-186304
+class IncrementInUnreachableBranch {
+  private final int i;
+  {
+    if (true) {
+      i = 2;
+    } else {
+      System.out.println(i); // unreachable
+      i++;
+    }
+  }
+
+  private final int j;
+  {
+    if (true) {
+      j = 2;
+    } else {
+      System.out.println(j); // unreachable
+      j = j + 1;
+    }
+  }
+}

plugins/InspectionGadgets/test/com/siyeh/igtest/style/field_final/FieldMayBeFinal.java

@@ -485,7 +485,7 @@ class T5 {
   }
 }
 class T5a {
-  private int x; // javac accepts if this is final, as x = 2 is never executed, not sure whether this is according to spec
+  private int <warning descr="Field 'x' may be 'final'">x</warning>; // may be final -- javac accepts this
 
   {
     x = 1;
@@ -800,7 +800,7 @@ class T43 {
   }
 }
 class T44 {
-  private int <warning descr="Field 'i' may be 'final'">i</warning>; // may not be final -- false-positive
+  private int i; // may not be final
   {
     for (; true ; i = 1, i = 2) {
       i = 2 ;
@@ -809,7 +809,9 @@ class T44 {
   }
 }
 class T45 {
-  private int <warning descr="Field 'i' may be 'final'">i</warning>; // does not compile in javac (probably javac error)
+  // dubious: does not compile in javac when final. Probably javac error - JDK-8198245, but seems logical
+  // our behavior is consistent with javac now
+  private int i;
   {
     for (; true; i = 1) {
       i = 2;
@@ -848,7 +850,7 @@ class T49 {
   }
 }
 class T50 {
-  private boolean b; // may not be final, but green when it is.
+  private boolean <warning descr="Field 'b' may be 'final'">b</warning>; // may be final
   T50(int i) {
     if (false && (b = true)) {
 
@@ -900,7 +902,7 @@ class T55 {
   }
 }
 class T56 {
-  private boolean b; // may not be final, but green when it is
+  private boolean <warning descr="Field 'b' may be 'final'">b</warning>; // may be final
   {
     if (false && (b = false)) ;
     if (true && (b = false)) ;