Merge pull request #230 from apex-dev-tools/112-missing-validations-o…

…n-ifwhile

112 missing validations on if, while & do-while

jvm/src/main/scala/com/nawforce/apexlink/cst/BodyDeclarations.scala

@@ -358,14 +358,7 @@ final case class ApexFieldDeclaration(
     val staticContext = if (isStatic) Some(true) else None
 
     if (isStatic && modifiers.contains(PROTECTED_MODIFIER)) {
-      context.log(
-        Issue(
-          location.path,
-          ERROR_CATEGORY,
-          location.location,
-          s"protected field '${id.name}' cannot be static"
-        )
-      )
+      context.log(Issue(ERROR_CATEGORY, location, s"protected field '${id.name}' cannot be static"))
     }
 
     variableDeclarator.verify(

jvm/src/main/scala/com/nawforce/apexlink/cst/Expressions.scala

@@ -421,14 +421,7 @@ final case class MethodCallWithId(target: Id, arguments: ArraySeq[Expression]) e
       case Left(err) =>
         if (callee.isComplete) {
           if (argTypes.contains(TypeNames.Any)) {
-            context.log(
-              Issue(
-                location.path,
-                WARNING_CATEGORY,
-                location.location,
-                s"$err, likely due to unknown type"
-              )
-            )
+            context.log(Issue(WARNING_CATEGORY, location, s"$err, likely due to unknown type"))
           } else {
             context.logMissing(location, s"$err")
           }
@@ -645,9 +638,7 @@ final case class BinaryExpression(lhs: Expression, rhs: Expression, op: String)
           ArithmeticAddSubtractAssignmentOperation | ArithmeticMultiplyDivideAssignmentOperation =>
         operation
           .getReadOnlyError(leftInter, context)
-          .foreach(msg =>
-            context.log(Issue(location.path, WARNING_CATEGORY, location.location, msg))
-          )
+          .foreach(msg => context.log(Issue(WARNING_CATEGORY, location, msg)))
       case _ =>
     }
 

jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala

@@ -31,6 +31,39 @@ import scala.collection.mutable
 
 abstract class Statement extends CST with ControlFlow {
   def verify(context: BlockVerifyContext): Unit
+
+  /** Verify an expression result type matches a specific type logging an issue if not
+    *
+    * @param expression to verify
+    * @param context verify context to use
+    * @param typeName to check for
+    * @param isStatic check for static or instance value
+    * @param prefix for the log issue
+    */
+  def verifyExpressionIs(
+    expression: Expression,
+    context: BlockVerifyContext,
+    typeName: TypeName,
+    isStatic: Boolean,
+    prefix: String
+  ): (Boolean, ExprContext) = {
+    val expr = expression.verify(context)
+    if (expr.isDefined && (!expr.isStatic.contains(isStatic) || expr.typeName != typeName)) {
+      val qualifier       = if (isStatic) "type" else "instance"
+      val resultQualifier = if (expr.isStatic.contains(true)) "type" else "instance"
+      context.log(
+        Issue(
+          ERROR_CATEGORY,
+          expression.location,
+          s"$prefix expression should return a '$typeName' $qualifier, not a '${expr.typeName}' $resultQualifier"
+        )
+      )
+      (false, expr)
+    } else {
+      (true, expr)
+    }
+  }
+
 }
 
 // Treat Block as Statement for blocks in blocks
@@ -151,7 +184,8 @@ object LocalVariableDeclarationStatement {
 
 final case class IfStatement(expression: Expression, statements: Seq[Statement]) extends Statement {
   override def verify(context: BlockVerifyContext): Unit = {
-    val expr = expression.verify(context)
+    val exprResult =
+      verifyExpressionIs(expression, context, TypeNames.Boolean, isStatic = false, "If")
 
     // This is replicating a feature where non-block statements can pass declarations forward
     val stmtRootContext = new InnerBlockVerifyContext(context).withBranchingControl()
@@ -168,7 +202,7 @@ final case class IfStatement(expression: Expression, statements: Seq[Statement])
         )
     })
 
-    verifyControlPath(stmtRootContext, BranchControlPattern(Some(expr), 2))
+    verifyControlPath(stmtRootContext, BranchControlPattern(Some(exprResult._2), 2))
   }
 }
 
@@ -338,7 +372,7 @@ object ForUpdate {
 final case class WhileStatement(expression: Expression, statement: Option[Statement])
     extends Statement {
   override def verify(context: BlockVerifyContext): Unit = {
-    expression.verify(context)
+    verifyExpressionIs(expression, context, TypeNames.Boolean, isStatic = false, "While")
     statement.foreach(_.verify(context))
   }
 }
@@ -352,10 +386,10 @@ object WhileStatement {
   }
 }
 
-final case class DoWhileStatement(statement: Option[Statement], expression: Option[Expression])
+final case class DoWhileStatement(statement: Option[Statement], expression: Expression)
     extends Statement {
   override def verify(context: BlockVerifyContext): Unit = {
-    expression.foreach(_.verify(context))
+    verifyExpressionIs(expression, context, TypeNames.Boolean, isStatic = false, "While")
     statement.foreach(_.verify(context))
   }
 }
@@ -364,8 +398,7 @@ object DoWhileStatement {
   def construct(parser: CodeParser, statement: DoWhileStatementContext): DoWhileStatement = {
     DoWhileStatement(
       Statement.construct(parser, statement.statement(), isTrigger = false),
-      Option(statement.parExpression())
-        .map(parExpression => Expression.construct(parExpression.expression()))
+      Expression.construct(statement.parExpression.expression())
     ).withContext(statement)
   }
 }
@@ -383,7 +416,7 @@ final case class TryStatement(block: Block, catches: Seq[CatchClause], finallyBl
       .ArrayBuffer(true)
       .addAll(catches.map(_ => true))
     finallyBlock.foreach(_ => a.addOne(false))
-    verifyControlPath(tryContext, BranchControlPattern(None, a.toArray))
+    verifyControlPath(tryContext, BranchControlPattern(a.toArray))
   }
 }
 
@@ -461,9 +494,7 @@ object CatchClause {
 final case class ReturnStatement(expression: Option[Expression]) extends Statement {
   override def verify(context: BlockVerifyContext): Unit = {
     assertReturnType(context, expression.map(_.verify(context)))
-      .foreach(msg =>
-        context.log(Issue(this.location.path, ERROR_CATEGORY, this.location.location, msg))
-      )
+      .foreach(msg => context.log(Issue(ERROR_CATEGORY, location, msg)))
     verifyControlPath(context, ExitControlPattern(exitsMethod = true, exitsBlock = true))
   }
 

jvm/src/main/scala/com/nawforce/apexlink/cst/Variables.scala

@@ -49,9 +49,8 @@ final case class VariableDeclarator(
         ) {
           context.log(
             Issue(
-              location.path,
               ERROR_CATEGORY,
-              location.location,
+              location,
               s"Incompatible types in assignment, from '${rhsCtx.typeDeclaration.typeName}' to '${lhsType.typeName}'"
             )
           )

jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlFlow.scala

@@ -29,13 +29,12 @@ trait ControlFlowContext {
     path
   }
 
-  def getControlRoot(): ControlFlowContext = root
   def setControlRoot(context: BlockVerifyContext with ControlFlowContext): BlockVerifyContext = {
     root = context
     this
   }
 
-  def hasBranchingControl(): Boolean = root.branching
+  def hasBranchingControl: Boolean = root.branching
   def withBranchingControl(): BlockVerifyContext = {
     root.branching = true
     this
@@ -46,9 +45,7 @@ trait ControlFlowContext {
     getPaths
       .filter(_.unreachable)
       .flatMap(_.location)
-      .foreach(pl =>
-        log(Issue(pl.path, ERROR_CATEGORY, pl.location, s"Unreachable block or statement"))
-      )
+      .foreach(location => log(Issue(ERROR_CATEGORY, location, s"Unreachable block or statement")))
   }
 }
 
@@ -74,7 +71,7 @@ trait OuterControlFlowContext {
       case _: UnknownPath => s"Expected return statement"
       case _              => s"Code path does not return a value"
     })
-    path.location.foreach(pl => log(Issue(pl.path, ERROR_CATEGORY, pl.location, message)))
+    path.location.foreach(location => log(Issue(ERROR_CATEGORY, location, message)))
   }
 }
 

jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlPatterns.scala

@@ -39,7 +39,7 @@ sealed trait ControlPattern {
   protected def nextPathUnreachable(context: BlockVerifyContext): Boolean = {
     val paths   = context.getPaths
     val nonVoid = context.returnType != TypeNames.Void
-    !context.hasBranchingControl() && (paths.lastOption.exists(_.unreachable) || paths.exists {
+    !context.hasBranchingControl && (paths.lastOption.exists(_.unreachable) || paths.exists {
       // some block returns depend on entry to the block
       // we cannot be certain in all cases
       case s: StatementPath => s.returns || s.exitsBlock
@@ -137,11 +137,8 @@ object BlockControlPattern {
 }
 
 // if/else, try/catch
-class BranchControlPattern(
-  expr: Option[ExprContext],
-  requiredBranches: Array[Boolean],
-  exclusive: Boolean
-) extends BlockControlPattern {
+class BranchControlPattern(requiredBranches: Array[Boolean], exclusive: Boolean)
+    extends BlockControlPattern {
   override protected def collectFailedPaths(
     context: BlockVerifyContext,
     paths: Array[ControlPath]
@@ -171,10 +168,10 @@ class BranchControlPattern(
 }
 
 object BranchControlPattern {
-  def apply(expr: Option[ExprContext], requiredBranches: Array[Boolean]): BranchControlPattern = {
-    new BranchControlPattern(expr, requiredBranches, false)
+  def apply(requiredBranches: Array[Boolean]): BranchControlPattern = {
+    new BranchControlPattern(requiredBranches, false)
   }
   def apply(expr: Option[ExprContext], requiredBranches: Int): BranchControlPattern = {
-    new BranchControlPattern(expr, Array.fill(requiredBranches)(true), true)
+    new BranchControlPattern(Array.fill(requiredBranches)(true), true)
   }
 }

jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/Switch.scala

@@ -260,7 +260,6 @@ final case class SwitchStatement(expression: Expression, whenControls: List[When
     verifyControlPath(
       switchContext,
       BranchControlPattern(
-        None,
         Option
           .when(whenControls.isEmpty) { Array(x = true) }
           .getOrElse(whenControls.map(_ => true).toArray)

jvm/src/main/scala/com/nawforce/apexlink/org/OPM.scala

@@ -800,7 +800,7 @@ object OPM extends TriHierarchy {
       writer.append(message)
       writer.append(": ")
       ex.printStackTrace(new PrintWriter(writer))
-      log(Issue(path, ERROR_CATEGORY, Location.empty, writer.toString))
+      log(Issue(ERROR_CATEGORY, PathLocation(path, Location.empty), writer.toString))
     }
 
     override def toString: String = {

jvm/src/test/scala/com/nawforce/apexlink/analysis/OrgAnalysisTest.scala

@@ -6,7 +6,7 @@ package com.nawforce.apexlink.analysis
 import com.nawforce.apexlink.TestHelper
 import com.nawforce.apexlink.api.{LoadAndRefreshAnalysis, NoAnalysis, RefreshAnalysis}
 import com.nawforce.pkgforce.diagnostics.{ERROR_CATEGORY, Issue => InternalIssue}
-import com.nawforce.pkgforce.path.{Location, PathLike}
+import com.nawforce.pkgforce.path.{Location, PathLike, PathLocation}
 import com.nawforce.runtime.FileSystemHelper
 import com.nawforce.runtime.platform.Path
 import io.github.apexdevtools.api.Issue
@@ -75,9 +75,8 @@ class OrgAnalysisTest extends AnyFunSuite with BeforeAndAfter with TestHelper {
           val testClassPath = root.join("foo").join("Dummy.cls")
           MockAnalysisProvider.issues = Array(
             InternalIssue(
-              testClassPath,
               ERROR_CATEGORY,
-              Location(1, 18),
+              PathLocation(testClassPath, Location(1, 18)),
               "mismatched input '<EOF>' expecting {'extends', 'implements', '{'}"
             )
           )

jvm/src/test/scala/com/nawforce/apexlink/cst/DoWhileTest.scala

@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2023 Certinia Inc. All rights reserved.
+ */
+package com.nawforce.apexlink.cst
+
+import com.nawforce.apexlink.TestHelper
+import org.scalatest.funsuite.AnyFunSuite
+
+class DoWhileTest extends AnyFunSuite with TestHelper {
+
+  test("Bad conditional") {
+    typeDeclaration("public class Dummy {{ do {} while (foo); }}")
+    assert(
+      dummyIssues == "Missing: line 1 at 35-38: No variable or type found for 'foo' on 'Dummy'\n"
+    )
+  }
+
+  test("Non boolean conditional") {
+    typeDeclaration("public class Dummy {{ do {} while (''); }}")
+    assert(
+      dummyIssues == "Error: line 1 at 35-37: While expression should return a 'System.Boolean' instance, not a 'System.String' instance\n"
+    )
+  }
+
+  test("Null boolean conditional") {
+    typeDeclaration("public class Dummy {{ do {} while (null); }}")
+    assert(
+      dummyIssues == "Error: line 1 at 35-39: While expression should return a 'System.Boolean' instance, not a 'null' instance\n"
+    )
+  }
+
+  test("Static boolean conditional") {
+    typeDeclaration("public class Dummy {{ do {} while (Boolean); }}")
+    assert(
+      dummyIssues == "Error: line 1 at 35-42: While expression should return a 'System.Boolean' instance, not a 'System.Boolean' type\n"
+    )
+  }
+
+  test("Single statement") {
+    // TODO: This should fail a block is required, apex-parser is over general
+    happyTypeDeclaration("public class Dummy {{ do System.debug(''); while (true); }}")
+  }
+
+  test("Single block") {
+    happyTypeDeclaration("public class Dummy {{ do {System.debug('');} while (true);  }}")
+  }
+
+}