Merge pull request #279 from apex-dev-tools/278-provide-a-means-to-de…

…tect-where-behaviour-changes-may-occur-due-to-new-handling-of-private-methods-overrides-in-v61

278 provide a means to detect where behaviour changes may occur due to new handling of private methods overrides in v61

jvm/src/main/scala/com/nawforce/apexlink/plugins/OverridePlugin.scala

@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2024 Certinia Inc. All rights reserved.
+ */
+package com.nawforce.apexlink.plugins
+
+import com.nawforce.apexlink.cst._
+import com.nawforce.apexlink.plugins.OverridePlugin.extensibleModifiers
+import com.nawforce.apexlink.types.apex.ApexMethodLike
+import com.nawforce.apexlink.types.core.DependentType
+import com.nawforce.pkgforce.diagnostics.{Diagnostic, ERROR_CATEGORY, Issue}
+import com.nawforce.pkgforce.modifiers._
+
+/** Plugin for detecting where a private method override is occurring in pre-v61 code. Flags both super class
+  * and base class methods to make easier to spot.
+  *
+  * @param td type being handled by this plugin
+  */
+class OverridePlugin(td: DependentType) extends Plugin(td) {
+
+  override def onEnumValidated(td: EnumDeclaration): Seq[DependentType] = Seq.empty
+
+  override def onInterfaceValidated(td: InterfaceDeclaration): Seq[DependentType] = Seq.empty
+
+  override def onClassValidated(td: ClassDeclaration): Seq[DependentType] = {
+    // Bail early if not extending or virtual/abstract
+    if (td.modifiers.intersect(extensibleModifiers).isEmpty && td.superClass.isEmpty)
+      return Seq.empty
+
+    // Hack: Analysis requires a methodMap as it establishes shadow relationships
+    td.methodMap
+
+    td.localMethods
+      .collect { case m: ApexMethodLike => m }
+      .foreach(method => {
+        // This private method is being overridden
+        if (method.visibility == PRIVATE_MODIFIER && method.shadowedBy.nonEmpty) {
+          val overrides = method.shadowedBy.flatMap(_.thisTypeIdOpt).map(_.toString).mkString(", ")
+          td.module.pkg.org.issues.log(
+            new Issue(
+              method.location.path,
+              Diagnostic(
+                ERROR_CATEGORY,
+                method.idLocation,
+                s"The overrides of this private method will fail in v61, see $overrides"
+              )
+            )
+          )
+        }
+
+        // This method is overriding a private method
+        findPrivateShadow(method)
+          .foreach(shadow => {
+            td.module.pkg.org.issues.log(
+              new Issue(
+                method.location.path,
+                Diagnostic(
+                  ERROR_CATEGORY,
+                  method.idLocation,
+                  s"This override of a private method will fail in v61, see ${shadow.location.toString}"
+                )
+              )
+            )
+          })
+      })
+
+    // No dependent processing needed, this is a standalone analysis
+    Seq.empty
+  }
+
+  private def findPrivateShadow(method: ApexMethodLike): Option[ApexMethodLike] = {
+    val shadows       = method.shadows.collect { case m: ApexMethodLike => m }
+    val privateShadow = shadows.find(_.visibility == PRIVATE_MODIFIER)
+    privateShadow.orElse(shadows.collectFirst(Function.unlift(findPrivateShadow)))
+  }
+}
+
+object OverridePlugin {
+  final val extensibleModifiers = Seq(ABSTRACT_MODIFIER, VIRTUAL_MODIFIER)
+}

jvm/src/main/scala/com/nawforce/apexlink/plugins/PluginsManager.scala

@@ -56,7 +56,8 @@ class PluginsManager {
 }
 
 object PluginsManager {
-  private val defaultPlugins     = Seq[Class[_ <: Plugin]](classOf[UnusedPlugin])
+  private val defaultPlugins =
+    Seq[Class[_ <: Plugin]](classOf[UnusedPlugin], classOf[OverridePlugin])
   private var plugins            = defaultPlugins
   private var pluginConstructors = defaultPlugins.map(_.getConstructor(classOf[DependentType]))
 

jvm/src/main/scala/com/nawforce/apexlink/types/apex/ApexDeclaration.scala

@@ -91,8 +91,8 @@ trait ApexMethodLike extends ApexVisibleMethodLike with Referenceable with IdLoc
   private var _shadows: SkinnyWeakSet[MethodDeclaration]    = new SkinnyWeakSet()
   private var _shadowedBy: SkinnyWeakSet[MethodDeclaration] = new SkinnyWeakSet()
 
-  def shadows: Set[MethodDeclaration]            = _shadows.toSet
-  private def shadowedBy: Set[MethodDeclaration] = _shadowedBy.toSet
+  def shadows: Set[MethodDeclaration]    = _shadows.toSet
+  def shadowedBy: Set[MethodDeclaration] = _shadowedBy.toSet
 
   def resetShadows(): Unit = {
     _shadows = new SkinnyWeakSet()

jvm/src/test/scala/com/nawforce/apexlink/TestHelper.scala

@@ -22,7 +22,7 @@ import com.nawforce.apexlink.api.{
   TypeSummary
 }
 import com.nawforce.apexlink.org.{OPM, OrgInfo}
-import com.nawforce.apexlink.plugins.{PluginsManager, UnusedPlugin}
+import com.nawforce.apexlink.plugins.{Plugin, PluginsManager, UnusedPlugin}
 import com.nawforce.apexlink.rpc.{LocationLink, TargetLocation}
 import com.nawforce.apexlink.types.apex.{ApexClassDeclaration, ApexFullDeclaration, FullDeclaration}
 import com.nawforce.apexlink.types.core.TypeDeclaration
@@ -59,8 +59,8 @@ trait TestHelper {
     }
   }
 
-  def createOrgWithUnused(path: PathLike): OPM.OrgImpl = {
-    val plugins = PluginsManager.overridePlugins(Seq(classOf[UnusedPlugin]))
+  def createOrgWithPlugin(path: PathLike, cls: Class[_ <: Plugin]): OPM.OrgImpl = {
+    val plugins = PluginsManager.overridePlugins(Seq(cls))
     try {
       ParserHelper.setParser()
       defaultOrg = Org.newOrg(path).asInstanceOf[OPM.OrgImpl]

jvm/src/test/scala/com/nawforce/apexlink/plugin/OverrideTest.scala

@@ -0,0 +1,269 @@
+/*
+ * Copyright (c) 2024 Certinia Inc. All rights reserved.
+ */
+
+package com.nawforce.apexlink.plugin
+import com.nawforce.apexlink.TestHelper
+import com.nawforce.apexlink.org.OPM
+import com.nawforce.apexlink.plugins.OverridePlugin
+import com.nawforce.pkgforce.path.PathLike
+import com.nawforce.runtime.FileSystemHelper
+import org.scalatest.Inspectors.forAll
+import org.scalatest.funsuite.AnyFunSuite
+import org.scalatest.matchers.should.Matchers
+
+class OverrideTest extends AnyFunSuite with Matchers with TestHelper {
+
+  def createOrgWithOverride(root: PathLike): OPM.OrgImpl = {
+    createOrgWithPlugin(root, classOf[OverridePlugin])
+  }
+
+  def orgIssuesFor(org: OPM.OrgImpl, path: PathLike): String = {
+    val messages = org.issueManager.issuesForFileInternal(path).map(_.asString()).mkString("\n")
+    if (messages.nonEmpty) messages + "\n" else ""
+  }
+
+  forAll(Set("", "private")) { baseVisibility =>
+    val baseVisibilityDescribe = if (baseVisibility.isEmpty) "implicit private" else "private"
+    forAll(Set("private", "protected", "public")) { overrideVisibility =>
+      test(s"Override $baseVisibilityDescribe with $overrideVisibility") {
+        FileSystemHelper.run(
+          Map(
+            "Base.cls" -> s"public virtual class Base {$baseVisibility void foo() {}}",
+            "Over.cls" -> s"public class Over extends Base {$overrideVisibility void foo() {}}"
+          )
+        ) { root: PathLike =>
+          val org = createOrgWithOverride(root)
+          orgIssuesFor(org, root.join("Base.cls")) should include(
+            "The overrides of this private method will fail in v61, see Over\n"
+          )
+          orgIssuesFor(org, root.join("Over.cls")) should include(
+            "This override of a private method will fail in v61, see /Base.cls: line 1 at"
+          )
+        }
+      }
+
+      test(s"Override $baseVisibilityDescribe with $overrideVisibility in same file") {
+        FileSystemHelper.run(
+          Map(
+            "Base.cls" -> s"public virtual class Base {$baseVisibility void foo() {} public class Over extends Base {$overrideVisibility void foo() {}}}"
+          )
+        ) { root: PathLike =>
+          val org = createOrgWithOverride(root)
+          orgIssuesFor(org, root.join("Base.cls")) should include(
+            "Method 'foo' can not override non-virtual method\n"
+          )
+        }
+      }
+
+      test(s"Override $baseVisibilityDescribe virtual with $overrideVisibility") {
+        FileSystemHelper.run(
+          Map(
+            "Base.cls" -> s"public virtual class Base {$baseVisibility virtual void foo() {}}",
+            "Over.cls" -> s"public class Over extends Base {$overrideVisibility void foo() {}}"
+          )
+        ) { root: PathLike =>
+          val org = createOrgWithOverride(root)
+          orgIssuesFor(org, root.join("Base.cls")) should include(
+            "The overrides of this private method will fail in v61, see Over\n"
+          )
+          orgIssuesFor(org, root.join("Over.cls")) should include(
+            "This override of a private method will fail in v61, see /Base.cls: line 1"
+          )
+        }
+      }
+
+      test(s"Override $baseVisibilityDescribe virtual with $overrideVisibility in same file") {
+        FileSystemHelper.run(
+          Map(
+            "Base.cls" -> s"public virtual class Base {private virtual void foo() {} public class Over extends Base {$overrideVisibility override void foo() {}}}"
+          )
+        ) { root: PathLike =>
+          val org = createOrgWithOverride(root)
+          orgIssuesFor(org, root.join("Base.cls")) should include(
+            "The overrides of this private method will fail in v61, see Base.Over\n"
+          )
+          orgIssuesFor(org, root.join("Base.cls")) should include(
+            "This override of a private method will fail in v61, see /Base.cls: line 1 at"
+          )
+        }
+      }
+
+      test(s"Override $baseVisibilityDescribe abstract with $overrideVisibility") {
+        FileSystemHelper.run(
+          Map(
+            "Base.cls" -> s"public abstract class Base {$baseVisibility abstract void foo();}",
+            "Over.cls" -> s"public class Over extends Base {$overrideVisibility void foo() {}}"
+          )
+        ) { root: PathLike =>
+          val org = createOrgWithOverride(root)
+          orgIssuesFor(org, root.join("Base.cls")) should include(
+            "The overrides of this private method will fail in v61, see Over\n"
+          )
+          orgIssuesFor(org, root.join("Over.cls")) should include(
+            "This override of a private method will fail in v61, see /Base.cls: line 1"
+          )
+        }
+      }
+
+      test(s"Override $baseVisibilityDescribe abstract with $overrideVisibility in same file") {
+        FileSystemHelper.run(
+          Map(
+            "Base.cls" -> s"public abstract class Base {private abstract void foo(); public class Over extends Base {$overrideVisibility override void foo() {}}}"
+          )
+        ) { root: PathLike =>
+          val org = createOrgWithOverride(root)
+          orgIssuesFor(org, root.join("Base.cls")) should include(
+            "The overrides of this private method will fail in v61, see Base.Over\n"
+          )
+          orgIssuesFor(org, root.join("Base.cls")) should include(
+            "This override of a private method will fail in v61, see /Base.cls: line 1 at"
+          )
+        }
+      }
+    }
+  }
+
+  test(s"Three trier private/private/private override ") {
+    FileSystemHelper.run(
+      Map(
+        "A.cls" -> s"public virtual class A {private void foo() {} }",
+        "B.cls" -> s"public virtual class B extends A {private void foo() {} }",
+        "C.cls" -> s"public virtual class C extends B {private void foo() {} }"
+      )
+    ) { root: PathLike =>
+      val org = createOrgWithOverride(root)
+      assert(
+        orgIssuesFor(org, root.join("A.cls"))
+          == "Error: line 1 at 37-40: The overrides of this private method will fail in v61, see B\n"
+      )
+      assert(
+        orgIssuesFor(org, root.join("B.cls"))
+          == "Error: line 1 at 47-50: This override of a private method will fail in v61, see /A.cls: line 1 at 32-45\n" +
+          "Error: line 1 at 47-50: The overrides of this private method will fail in v61, see C\n"
+      )
+      assert(
+        orgIssuesFor(org, root.join("C.cls"))
+          == "Error: line 1 at 47-50: This override of a private method will fail in v61, see /B.cls: line 1 at 42-55\n"
+      )
+    }
+  }
+
+  test(s"Three trier private/protected/protected override ") {
+    FileSystemHelper.run(
+      Map(
+        "A.cls" -> s"public virtual class A {private void foo() {} }",
+        "B.cls" -> s"public virtual class B extends A {protected virtual void foo() {} }",
+        "C.cls" -> s"public virtual class C extends B {protected override void foo() {} }"
+      )
+    ) { root: PathLike =>
+      val org = createOrgWithOverride(root)
+      assert(
+        orgIssuesFor(org, root.join("A.cls"))
+          == "Error: line 1 at 37-40: The overrides of this private method will fail in v61, see B\n"
+      )
+      assert(
+        orgIssuesFor(org, root.join("B.cls"))
+          == "Error: line 1 at 57-60: This override of a private method will fail in v61, see /A.cls: line 1 at 32-45\n"
+      )
+      assert(
+        orgIssuesFor(org, root.join("C.cls"))
+          == "Error: line 1 at 58-61: This override of a private method will fail in v61, see /A.cls: line 1 at 32-45\n"
+      )
+    }
+  }
+
+  test(s"Three trier private/protected/public override ") {
+    FileSystemHelper.run(
+      Map(
+        "A.cls" -> "public virtual class A {private void foo() {} }",
+        "B.cls" -> "public virtual class B extends A {protected virtual void foo() {} }",
+        "C.cls" -> "public virtual class C extends B {public override void foo() {} }"
+      )
+    ) { root: PathLike =>
+      val org = createOrgWithOverride(root)
+      assert(
+        orgIssuesFor(org, root.join("A.cls"))
+          == "Error: line 1 at 37-40: The overrides of this private method will fail in v61, see B\n"
+      )
+      assert(
+        orgIssuesFor(org, root.join("B.cls"))
+          == "Error: line 1 at 57-60: This override of a private method will fail in v61, see /A.cls: line 1 at 32-45\n"
+      )
+      assert(
+        orgIssuesFor(org, root.join("C.cls"))
+          == "Error: line 1 at 55-58: This override of a private method will fail in v61, see /A.cls: line 1 at 32-45\n"
+      )
+    }
+  }
+
+  test(s"Three trier private/public/public override ") {
+    FileSystemHelper.run(
+      Map(
+        "A.cls" -> "public virtual class A {private void foo() {} }",
+        "B.cls" -> "public virtual class B extends A {public virtual void foo() {} }",
+        "C.cls" -> "public virtual class C extends B {public override void foo() {} }"
+      )
+    ) { root: PathLike =>
+      val org = createOrgWithOverride(root)
+      assert(
+        orgIssuesFor(org, root.join("A.cls"))
+          == "Error: line 1 at 37-40: The overrides of this private method will fail in v61, see B\n"
+      )
+      assert(
+        orgIssuesFor(org, root.join("B.cls"))
+          == "Error: line 1 at 54-57: This override of a private method will fail in v61, see /A.cls: line 1 at 32-45\n"
+      )
+      assert(
+        orgIssuesFor(org, root.join("C.cls"))
+          == "Error: line 1 at 55-58: This override of a private method will fail in v61, see /A.cls: line 1 at 32-45\n"
+      )
+    }
+  }
+
+  test(s"Dual overrides") {
+    FileSystemHelper.run(
+      Map(
+        "A.cls" -> "public virtual class A {private void foo() {} }",
+        "B.cls" -> "public virtual class B extends A {private void foo() {} }",
+        "C.cls" -> "public virtual class C extends A {private void foo() {} }"
+      )
+    ) { root: PathLike =>
+      val org = createOrgWithOverride(root)
+      assert(
+        orgIssuesFor(org, root.join("A.cls"))
+          == "Error: line 1 at 37-40: The overrides of this private method will fail in v61, see B, C\n"
+      )
+      assert(
+        orgIssuesFor(org, root.join("B.cls"))
+          == "Error: line 1 at 47-50: This override of a private method will fail in v61, see /A.cls: line 1 at 32-45\n"
+      )
+      assert(
+        orgIssuesFor(org, root.join("C.cls"))
+          == "Error: line 1 at 47-50: This override of a private method will fail in v61, see /A.cls: line 1 at 32-45\n"
+      )
+    }
+  }
+
+  test(s"Dual overrides (same file)") {
+    FileSystemHelper.run(
+      Map(
+        "A.cls" ->
+          """public virtual class A {private virtual void foo() {}
+            | public virtual class B extends A {private override void foo() {} }
+            | public virtual class C extends A {private override void foo() {} }
+            |}
+            |""".stripMargin
+      )
+    ) { root: PathLike =>
+      val org = createOrgWithOverride(root)
+      assert(
+        orgIssuesFor(org, root.join("A.cls"))
+          == "Error: line 1 at 45-48: The overrides of this private method will fail in v61, see A.B, A.C\n" +
+          "Error: line 2 at 57-60: This override of a private method will fail in v61, see /A.cls: line 1 at 40-53\n" +
+          "Error: line 3 at 57-60: This override of a private method will fail in v61, see /A.cls: line 1 at 40-53\n"
+      )
+    }
+  }
+
+}