From 5a4bac958f85dba56fb4b96a291b0895fa50f063 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 19 Sep 2023 10:59:12 +0100 Subject: [PATCH 1/3] Improve if/while/do-while validation --- .../apexlink/cst/BodyDeclarations.scala | 9 +-- .../nawforce/apexlink/cst/Expressions.scala | 13 +--- .../nawforce/apexlink/cst/Statements.scala | 42 +++++++++--- .../com/nawforce/apexlink/cst/Variables.scala | 3 +- .../apexlink/cst/stmts/ControlFlow.scala | 6 +- .../scala/com/nawforce/apexlink/org/OPM.scala | 2 +- .../apexlink/analysis/OrgAnalysisTest.scala | 5 +- .../nawforce/apexlink/cst/DoWhileTest.scala | 41 ++++++++++++ .../com/nawforce/apexlink/cst/IfTest.scala | 64 +++++++++++++++++-- .../com/nawforce/apexlink/cst/WhileTest.scala | 40 ++++++++++++ .../apexlink/org/IssueManagerTest.scala | 8 +-- .../nawforce/pkgforce/diagnostics/Issue.scala | 12 ++-- .../pkgforce/stream/LabelGenerator.scala | 10 ++- .../pkgforce/stream/SObjectGenerator.scala | 6 +- 14 files changed, 201 insertions(+), 60 deletions(-) create mode 100644 jvm/src/test/scala/com/nawforce/apexlink/cst/DoWhileTest.scala create mode 100644 jvm/src/test/scala/com/nawforce/apexlink/cst/WhileTest.scala diff --git a/jvm/src/main/scala/com/nawforce/apexlink/cst/BodyDeclarations.scala b/jvm/src/main/scala/com/nawforce/apexlink/cst/BodyDeclarations.scala index 1c8640748..195839cd8 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/cst/BodyDeclarations.scala +++ b/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( diff --git a/jvm/src/main/scala/com/nawforce/apexlink/cst/Expressions.scala b/jvm/src/main/scala/com/nawforce/apexlink/cst/Expressions.scala index 88e2721a1..6aa22b482 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/cst/Expressions.scala +++ b/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 _ => } diff --git a/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala b/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala index 1c083e610..056f6adb2 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala +++ b/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala @@ -152,6 +152,15 @@ object LocalVariableDeclarationStatement { final case class IfStatement(expression: Expression, statements: Seq[Statement]) extends Statement { override def verify(context: BlockVerifyContext): Unit = { val expr = expression.verify(context) + if (expr.isDefined && expr.typeName != TypeNames.Boolean) { + context.log( + Issue( + ERROR_CATEGORY, + expression.location, + s"If expression should return a Boolean value, not a '${expr.typeName}'" + ) + ) + } // This is replicating a feature where non-block statements can pass declarations forward val stmtRootContext = new InnerBlockVerifyContext(context).withBranchingControl() @@ -338,7 +347,17 @@ object ForUpdate { final case class WhileStatement(expression: Expression, statement: Option[Statement]) extends Statement { override def verify(context: BlockVerifyContext): Unit = { - expression.verify(context) + val expr = expression.verify(context) + if (expr.isDefined && expr.typeName != TypeNames.Boolean) { + context.log( + Issue( + ERROR_CATEGORY, + expression.location, + s"While expression should return a Boolean value, not a '${expr.typeName}'" + ) + ) + } + statement.foreach(_.verify(context)) } } @@ -352,10 +371,20 @@ 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)) + val expr = expression.verify(context) + if (expr.isDefined && expr.typeName != TypeNames.Boolean) { + context.log( + Issue( + ERROR_CATEGORY, + expression.location, + s"While expression should return a Boolean value, not a '${expr.typeName}'" + ) + ) + } + statement.foreach(_.verify(context)) } } @@ -364,8 +393,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) } } @@ -461,9 +489,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)) } diff --git a/jvm/src/main/scala/com/nawforce/apexlink/cst/Variables.scala b/jvm/src/main/scala/com/nawforce/apexlink/cst/Variables.scala index af9734be7..62fda90d5 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/cst/Variables.scala +++ b/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}'" ) ) diff --git a/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlFlow.scala b/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlFlow.scala index 37c59193a..63038535d 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlFlow.scala +++ b/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlFlow.scala @@ -46,9 +46,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 +72,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))) } } diff --git a/jvm/src/main/scala/com/nawforce/apexlink/org/OPM.scala b/jvm/src/main/scala/com/nawforce/apexlink/org/OPM.scala index e5f87235a..c2bfeeaae 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/org/OPM.scala +++ b/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 = { diff --git a/jvm/src/test/scala/com/nawforce/apexlink/analysis/OrgAnalysisTest.scala b/jvm/src/test/scala/com/nawforce/apexlink/analysis/OrgAnalysisTest.scala index 712b34d9b..8bfabd57b 100644 --- a/jvm/src/test/scala/com/nawforce/apexlink/analysis/OrgAnalysisTest.scala +++ b/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 '' expecting {'extends', 'implements', '{'}" ) ) diff --git a/jvm/src/test/scala/com/nawforce/apexlink/cst/DoWhileTest.scala b/jvm/src/test/scala/com/nawforce/apexlink/cst/DoWhileTest.scala new file mode 100644 index 000000000..3f1da7262 --- /dev/null +++ b/jvm/src/test/scala/com/nawforce/apexlink/cst/DoWhileTest.scala @@ -0,0 +1,41 @@ +/* + * 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 Boolean value, not a 'System.String'\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 Boolean value, not a 'null'\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); }}") + } + +} diff --git a/jvm/src/test/scala/com/nawforce/apexlink/cst/IfTest.scala b/jvm/src/test/scala/com/nawforce/apexlink/cst/IfTest.scala index 2006848a4..a75629683 100644 --- a/jvm/src/test/scala/com/nawforce/apexlink/cst/IfTest.scala +++ b/jvm/src/test/scala/com/nawforce/apexlink/cst/IfTest.scala @@ -19,10 +19,66 @@ import org.scalatest.funsuite.AnyFunSuite class IfTest extends AnyFunSuite with TestHelper { - test("Block bug") { - typeDeclaration("public class Dummy {{ if (false) String a = ''; else a =''; }}") - assert(!hasIssues) + test("Bad conditional") { + typeDeclaration("public class Dummy {{ if (foo) {} }}") + assert( + dummyIssues == "Missing: line 1 at 26-29: No variable or type found for 'foo' on 'Dummy'\n" + ) + } + + test("Non boolean conditional") { + typeDeclaration("public class Dummy {{ if ('') {} }}") + assert( + dummyIssues == "Error: line 1 at 26-28: If expression should return a Boolean value, not a 'System.String'\n" + ) + } + + test("Null boolean conditional") { + typeDeclaration("public class Dummy {{ if (null) {} }}") + assert( + dummyIssues == "Error: line 1 at 26-30: If expression should return a Boolean value, not a 'null'\n" + ) + } + + test("Single statement") { + happyTypeDeclaration("public class Dummy {{ if (true) System.debug(''); }}") + } + + test("Else statement") { + happyTypeDeclaration( + "public class Dummy {{ if (true) System.debug(''); else System.debug(''); }}" + ) + } + + test("Scoping bug") { + happyTypeDeclaration("public class Dummy {{ if (false) String a = ''; else a =''; }}") + } + + test("Scoping bug with block") { + typeDeclaration("public class Dummy {{ if (false) {String a = '';} else a =''; }}") + assert( + dummyIssues == "Missing: line 1 at 55-56: No variable or type found for 'a' on 'Dummy'\n" + ) + } + + test("Scoping bug with else block") { + happyTypeDeclaration("public class Dummy {{ if (false) String a = ''; else { a ='';} }}") + } + + test("Single block") { + happyTypeDeclaration("public class Dummy {{ if (true) {System.debug('');} }}") + } + + test("Else block") { + happyTypeDeclaration( + "public class Dummy {{ if (true) System.debug(''); else {System.debug('');} }}" + ) + } + + test("Dual block") { + happyTypeDeclaration( + "public class Dummy {{ if (true) {System.debug('');} else {System.debug('');} }}" + ) } - // TODO: Write some tests! } diff --git a/jvm/src/test/scala/com/nawforce/apexlink/cst/WhileTest.scala b/jvm/src/test/scala/com/nawforce/apexlink/cst/WhileTest.scala new file mode 100644 index 000000000..b6f9d0a9e --- /dev/null +++ b/jvm/src/test/scala/com/nawforce/apexlink/cst/WhileTest.scala @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2023 Certinia Inc. All rights reserved. + */ +package com.nawforce.apexlink.cst + +import com.nawforce.apexlink.TestHelper +import org.scalatest.funsuite.AnyFunSuite + +class WhileTest extends AnyFunSuite with TestHelper { + + test("Bad conditional") { + typeDeclaration("public class Dummy {{ while (foo) {} }}") + assert( + dummyIssues == "Missing: line 1 at 29-32: No variable or type found for 'foo' on 'Dummy'\n" + ) + } + + test("Non boolean conditional") { + typeDeclaration("public class Dummy {{ while ('') {} }}") + assert( + dummyIssues == "Error: line 1 at 29-31: While expression should return a Boolean value, not a 'System.String'\n" + ) + } + + test("Null boolean conditional") { + typeDeclaration("public class Dummy {{ while (null) {} }}") + assert( + dummyIssues == "Error: line 1 at 29-33: While expression should return a Boolean value, not a 'null'\n" + ) + } + + test("Single statement") { + happyTypeDeclaration("public class Dummy {{ while (true) System.debug(''); }}") + } + + test("Single block") { + happyTypeDeclaration("public class Dummy {{ while (true) {System.debug('');} }}") + } + +} diff --git a/jvm/src/test/scala/com/nawforce/apexlink/org/IssueManagerTest.scala b/jvm/src/test/scala/com/nawforce/apexlink/org/IssueManagerTest.scala index 95fe82a2e..7852894b1 100644 --- a/jvm/src/test/scala/com/nawforce/apexlink/org/IssueManagerTest.scala +++ b/jvm/src/test/scala/com/nawforce/apexlink/org/IssueManagerTest.scala @@ -16,7 +16,7 @@ package com.nawforce.apexlink.org import com.nawforce.apexlink.TestHelper import com.nawforce.pkgforce.PathInterpolator.PathInterpolator import com.nawforce.pkgforce.diagnostics.{Issue, SYNTAX_CATEGORY} -import com.nawforce.pkgforce.path.{Location, PathLike} +import com.nawforce.pkgforce.path.{Location, PathLike, PathLocation} import com.nawforce.runtime.FileSystemHelper import org.scalatest.funsuite.AnyFunSuite @@ -47,9 +47,8 @@ class IssueManagerTest extends AnyFunSuite with TestHelper { assert(org.issues.hasUpdatedIssues.isEmpty) val expectedIssue = Issue( - root.join("Dummy.cls"), SYNTAX_CATEGORY, - Location(1, 18), + PathLocation(root.join("Dummy.cls"), Location(1, 18)), "mismatched input '' expecting {'extends', 'implements', '{'}" ) @@ -119,9 +118,8 @@ class IssueManagerTest extends AnyFunSuite with TestHelper { val expectedIssues = Array( Issue( - root.join("Dummy.cls"), SYNTAX_CATEGORY, - Location(1, 18), + PathLocation(root.join("Dummy.cls"), Location(1, 18)), "mismatched input '' expecting {'extends', 'implements', '{'}" ) ) diff --git a/shared/src/main/scala/com/nawforce/pkgforce/diagnostics/Issue.scala b/shared/src/main/scala/com/nawforce/pkgforce/diagnostics/Issue.scala index 3d595dcf0..ec4e1dbf8 100644 --- a/shared/src/main/scala/com/nawforce/pkgforce/diagnostics/Issue.scala +++ b/shared/src/main/scala/com/nawforce/pkgforce/diagnostics/Issue.scala @@ -30,7 +30,7 @@ package com.nawforce.pkgforce.diagnostics import io.github.apexdevtools.api.{IssueLocation, Rule} import com.nawforce.pkgforce.diagnostics.DiagnosticCategory.isErrorType -import com.nawforce.pkgforce.path.{Location, PathLike} +import com.nawforce.pkgforce.path.{Location, PathLike, PathLocation} import com.nawforce.runtime.platform.Path import upickle.default.{macroRW, ReadWriter => RW} @@ -74,14 +74,10 @@ object Issue { .orElseBy(_.diagnostic.location.startLine) .orElseBy(_.diagnostic.location.startPosition) - def apply( - path: PathLike, - category: DiagnosticCategory, - location: Location, - message: String - ): Issue = { - new Issue(path, Diagnostic(category, location, message)) + def apply(category: DiagnosticCategory, pathLocation: PathLocation, message: String): Issue = { + new Issue(pathLocation.path, Diagnostic(category, pathLocation.location, message)) } + } sealed case class IssuesAnd[T](issues: ArraySeq[Issue], value: T) diff --git a/shared/src/main/scala/com/nawforce/pkgforce/stream/LabelGenerator.scala b/shared/src/main/scala/com/nawforce/pkgforce/stream/LabelGenerator.scala index f43077c4f..17bd8d589 100644 --- a/shared/src/main/scala/com/nawforce/pkgforce/stream/LabelGenerator.scala +++ b/shared/src/main/scala/com/nawforce/pkgforce/stream/LabelGenerator.scala @@ -65,7 +65,11 @@ object LabelGenerator { ) } catch { case e: XMLException => - Iterator(IssuesEvent(Issue(document.path, ERROR_CATEGORY, e.where, e.msg))) + Iterator( + IssuesEvent( + Issue(ERROR_CATEGORY, PathLocation(document.path, e.where), e.msg) + ) + ) } }) labels ++ Iterator( @@ -73,7 +77,9 @@ object LabelGenerator { ) } catch { case e: XMLException => - Iterator(IssuesEvent(Issue(document.path, ERROR_CATEGORY, e.where, e.msg))) + Iterator( + IssuesEvent(Issue(ERROR_CATEGORY, PathLocation(document.path, e.where), e.msg)) + ) } } }) diff --git a/shared/src/main/scala/com/nawforce/pkgforce/stream/SObjectGenerator.scala b/shared/src/main/scala/com/nawforce/pkgforce/stream/SObjectGenerator.scala index d69269cfe..9d575e71b 100644 --- a/shared/src/main/scala/com/nawforce/pkgforce/stream/SObjectGenerator.scala +++ b/shared/src/main/scala/com/nawforce/pkgforce/stream/SObjectGenerator.scala @@ -203,9 +203,8 @@ object SObjectGenerator { IssuesAnd( ArraySeq( Issue( - doc.path, ERROR_CATEGORY, - Location(doc.rootElement.line), + PathLocation(doc.path, Location(doc.rootElement.line)), s"Unexpected customSettingsType value '$x', should be 'List' or 'Hierarchy'" ) ), @@ -236,9 +235,8 @@ object SObjectGenerator { IssuesAnd( ArraySeq( Issue( - doc.path, ERROR_CATEGORY, - Location(doc.rootElement.line), + PathLocation(doc.path, Location(doc.rootElement.line)), s"Unexpected sharingModel value '${sharingModel.get}'" ) ), From 3d0a1e5e9b4672f96ae427bfbd73c91d0d9cebc7 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 19 Sep 2023 11:06:29 +0100 Subject: [PATCH 2/3] Fix some IntelliJ warnings --- .../com/nawforce/apexlink/cst/Statements.scala | 2 +- .../nawforce/apexlink/cst/stmts/ControlFlow.scala | 3 +-- .../apexlink/cst/stmts/ControlPatterns.scala | 15 ++++++--------- .../com/nawforce/apexlink/cst/stmts/Switch.scala | 1 - 4 files changed, 8 insertions(+), 13 deletions(-) diff --git a/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala b/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala index 056f6adb2..8c31a228c 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala +++ b/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala @@ -411,7 +411,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)) } } diff --git a/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlFlow.scala b/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlFlow.scala index 63038535d..5b10cd316 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlFlow.scala +++ b/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 diff --git a/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlPatterns.scala b/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlPatterns.scala index 6eb4ad80d..24e334d9f 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/ControlPatterns.scala +++ b/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) } } diff --git a/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/Switch.scala b/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/Switch.scala index 61d343736..08a176061 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/cst/stmts/Switch.scala +++ b/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) From 543202604b5767b680bccab10d24cf2178baf8e2 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Wed, 20 Sep 2023 11:46:21 +0100 Subject: [PATCH 3/3] Add isStatic handling on if/while/do-while --- .../nawforce/apexlink/cst/Statements.scala | 71 ++++++++++--------- .../nawforce/apexlink/cst/DoWhileTest.scala | 11 ++- .../com/nawforce/apexlink/cst/IfTest.scala | 11 ++- .../com/nawforce/apexlink/cst/WhileTest.scala | 12 +++- 4 files changed, 65 insertions(+), 40 deletions(-) diff --git a/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala b/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala index 8c31a228c..820db0fe6 100644 --- a/jvm/src/main/scala/com/nawforce/apexlink/cst/Statements.scala +++ b/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,16 +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) - if (expr.isDefined && expr.typeName != TypeNames.Boolean) { - context.log( - Issue( - ERROR_CATEGORY, - expression.location, - s"If expression should return a Boolean value, not a '${expr.typeName}'" - ) - ) - } + 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() @@ -177,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)) } } @@ -347,17 +372,7 @@ object ForUpdate { final case class WhileStatement(expression: Expression, statement: Option[Statement]) extends Statement { override def verify(context: BlockVerifyContext): Unit = { - val expr = expression.verify(context) - if (expr.isDefined && expr.typeName != TypeNames.Boolean) { - context.log( - Issue( - ERROR_CATEGORY, - expression.location, - s"While expression should return a Boolean value, not a '${expr.typeName}'" - ) - ) - } - + verifyExpressionIs(expression, context, TypeNames.Boolean, isStatic = false, "While") statement.foreach(_.verify(context)) } } @@ -374,17 +389,7 @@ object WhileStatement { final case class DoWhileStatement(statement: Option[Statement], expression: Expression) extends Statement { override def verify(context: BlockVerifyContext): Unit = { - val expr = expression.verify(context) - if (expr.isDefined && expr.typeName != TypeNames.Boolean) { - context.log( - Issue( - ERROR_CATEGORY, - expression.location, - s"While expression should return a Boolean value, not a '${expr.typeName}'" - ) - ) - } - + verifyExpressionIs(expression, context, TypeNames.Boolean, isStatic = false, "While") statement.foreach(_.verify(context)) } } diff --git a/jvm/src/test/scala/com/nawforce/apexlink/cst/DoWhileTest.scala b/jvm/src/test/scala/com/nawforce/apexlink/cst/DoWhileTest.scala index 3f1da7262..7608c6951 100644 --- a/jvm/src/test/scala/com/nawforce/apexlink/cst/DoWhileTest.scala +++ b/jvm/src/test/scala/com/nawforce/apexlink/cst/DoWhileTest.scala @@ -18,14 +18,21 @@ class DoWhileTest extends AnyFunSuite with TestHelper { test("Non boolean conditional") { typeDeclaration("public class Dummy {{ do {} while (''); }}") assert( - dummyIssues == "Error: line 1 at 35-37: While expression should return a Boolean value, not a 'System.String'\n" + 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 Boolean value, not a 'null'\n" + 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" ) } diff --git a/jvm/src/test/scala/com/nawforce/apexlink/cst/IfTest.scala b/jvm/src/test/scala/com/nawforce/apexlink/cst/IfTest.scala index a75629683..3a222929a 100644 --- a/jvm/src/test/scala/com/nawforce/apexlink/cst/IfTest.scala +++ b/jvm/src/test/scala/com/nawforce/apexlink/cst/IfTest.scala @@ -29,14 +29,21 @@ class IfTest extends AnyFunSuite with TestHelper { test("Non boolean conditional") { typeDeclaration("public class Dummy {{ if ('') {} }}") assert( - dummyIssues == "Error: line 1 at 26-28: If expression should return a Boolean value, not a 'System.String'\n" + dummyIssues == "Error: line 1 at 26-28: If expression should return a 'System.Boolean' instance, not a 'System.String' instance\n" ) } test("Null boolean conditional") { typeDeclaration("public class Dummy {{ if (null) {} }}") assert( - dummyIssues == "Error: line 1 at 26-30: If expression should return a Boolean value, not a 'null'\n" + dummyIssues == "Error: line 1 at 26-30: If expression should return a 'System.Boolean' instance, not a 'null' instance\n" + ) + } + + test("Static boolean conditional") { + typeDeclaration("public class Dummy {{ if (Boolean) {} }}") + assert( + dummyIssues == "Error: line 1 at 26-33: If expression should return a 'System.Boolean' instance, not a 'System.Boolean' type\n" ) } diff --git a/jvm/src/test/scala/com/nawforce/apexlink/cst/WhileTest.scala b/jvm/src/test/scala/com/nawforce/apexlink/cst/WhileTest.scala index b6f9d0a9e..93d709a46 100644 --- a/jvm/src/test/scala/com/nawforce/apexlink/cst/WhileTest.scala +++ b/jvm/src/test/scala/com/nawforce/apexlink/cst/WhileTest.scala @@ -18,14 +18,21 @@ class WhileTest extends AnyFunSuite with TestHelper { test("Non boolean conditional") { typeDeclaration("public class Dummy {{ while ('') {} }}") assert( - dummyIssues == "Error: line 1 at 29-31: While expression should return a Boolean value, not a 'System.String'\n" + dummyIssues == "Error: line 1 at 29-31: While expression should return a 'System.Boolean' instance, not a 'System.String' instance\n" ) } test("Null boolean conditional") { typeDeclaration("public class Dummy {{ while (null) {} }}") assert( - dummyIssues == "Error: line 1 at 29-33: While expression should return a Boolean value, not a 'null'\n" + dummyIssues == "Error: line 1 at 29-33: While expression should return a 'System.Boolean' instance, not a 'null' instance\n" + ) + } + + test("Static boolean conditional") { + typeDeclaration("public class Dummy {{ while (Boolean) {} }}") + assert( + dummyIssues == "Error: line 1 at 29-36: While expression should return a 'System.Boolean' instance, not a 'System.Boolean' type\n" ) } @@ -36,5 +43,4 @@ class WhileTest extends AnyFunSuite with TestHelper { test("Single block") { happyTypeDeclaration("public class Dummy {{ while (true) {System.debug('');} }}") } - }