Make javasrc method call AST structure more consistent with c2cpg (joernio#828)

* Make javasrc method call AST structure more consistent with c2cpg

* Fix formatting

* Suppress unused warning

* Fix test indices

* Fix more indices

Author: Johannes Coetzee

console/src/main/scala/io/joern/console/QueryDatabase.scala

@@ -139,7 +139,7 @@ class QueryDatabase(defaultArgumentProvider: DefaultArgumentProvider = new Defau
   * */
 class DefaultArgumentProvider {
 
-  def typeSpecificDefaultArg(argTypeFullName: String): Option[Any] = {
+  def typeSpecificDefaultArg(@unused argTypeFullName: String): Option[Any] = {
     None
   }
 

dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/passes/reachingdef/ReachingDefProblem.scala

@@ -1,6 +1,6 @@
 package io.joern.dataflowengineoss.passes.reachingdef
 
-import io.shiftleft.codepropertygraph.generated.EdgeTypes
+import io.shiftleft.codepropertygraph.generated.{EdgeTypes, Operators}
 import io.shiftleft.codepropertygraph.generated.nodes._
 import io.shiftleft.semanticcpg.language._
 import io.shiftleft.semanticcpg.utils.MemberAccess.isGenericMemberAccessName
@@ -221,8 +221,19 @@ class ReachingDefTransferFunction(flowGraph: ReachingDefFlowGraph) extends Trans
           allIdentifiers(param.name)
             .filter(x => x.id != param.id)
         case identifier: Identifier =>
-          allIdentifiers(identifier.name)
+          val sameIdentifiers = allIdentifiers(identifier.name)
             .filter(x => x.id != identifier.id)
+
+          /**
+            * Killing an identifier should also kill field accesses on that identifier.
+            * For example, a reassignment `x = new Box()` should kill any previous
+            * calls to `x.value`, `x.length()`, etc.
+            */
+          val sameObjects: Iterable[Call] = allCalls.values.flatten
+            .filter(_.name == Operators.fieldAccess)
+            .filter(_.ast.isIdentifier.name(identifier.name).nonEmpty)
+
+          sameIdentifiers ++ sameObjects
         case call: Call =>
           allCalls(call.code)
             .filter(x => x.id != call.id)

joern-cli/frontends/c2cpg/src/test/scala/io/joern/c2cpg/querying/CDataFlowTests.scala

@@ -980,7 +980,7 @@ class CDataFlowTests32 extends DataFlowCodeToCpgSuite {
   }
 }
 
-class CDatFlowTests33 extends DataFlowCodeToCpgSuite {
+class CDataFlowTests33 extends DataFlowCodeToCpgSuite {
   override val code =
     """
       |int bar(int z) {
@@ -995,7 +995,7 @@ class CDatFlowTests33 extends DataFlowCodeToCpgSuite {
       |}
       |""".stripMargin
 
-  "Tes 33: should provide correct flow for source in sibling callee" in {
+  "Test 33: should provide correct flow for source in sibling callee" in {
     cpg.call("sink").reachableByFlows(cpg.call("source")).size shouldBe 1
   }
 

joern-cli/frontends/javasrc2cpg/src/main/scala/io/joern/javasrc2cpg/passes/AstCreator.scala

@@ -1657,10 +1657,11 @@ class AstCreator(filename: String, global: Global) {
       resolvedDecl: Try[ResolvedMethodDeclaration],
       order: Int
   ) = {
+    val codePrefix = call.getScope.toScala.map(_.toString).getOrElse("this")
     val typeFullName = registerType(Try(call.calculateResolvedType().describe()).getOrElse("<empty>"))
     val callNode = NewCall()
       .name(call.getNameAsString)
-      .code(s"${call.getNameAsString}(${call.getArguments.asScala.mkString(", ")})")
+      .code(s"${codePrefix}.${call.getNameAsString}(${call.getArguments.asScala.mkString(", ")})")
       .typeFullName(typeFullName)
       .order(order)
       .argumentIndex(order)
@@ -1685,6 +1686,7 @@ class AstCreator(filename: String, global: Global) {
   }
 
   private def createThisNode(
+      call: MethodCallExpr,
       resolvedDecl: Try[ResolvedMethodDeclaration]
   ): Option[NewIdentifier] = {
     resolvedDecl.toOption
@@ -1697,6 +1699,8 @@ class AstCreator(filename: String, global: Global) {
           .typeFullName(typeFullName)
           .order(0)
           .argumentIndex(0)
+          .lineNumber(line(call))
+          .columnNumber(column(call))
       }
   }
 
@@ -1944,27 +1948,52 @@ class AstCreator(filename: String, global: Global) {
 
     val resolvedDecl = Try(call.resolve())
     val callNode = createCallNode(call, resolvedDecl, order)
-    val thisAsts = createThisNode(resolvedDecl)
-      .map(_.lineNumber(line(call)))
-      .map(_.columnNumber(column(call)))
-      .map(x => AstWithCtx(Ast(x), Context(identifiers = List(x))))
-      .toList
 
-    val argAsts = withOrder(call.getArguments) { (arg, order) =>
-      // FIXME: There's an implicit assumption here that each call to
-      // astsForExpression only returns a single tree.
-      astsForExpression(arg, scopeContext, order)
+    val scopeAst: AstWithCtx = call.getScope.toScala match {
+      case Some(scope) =>
+        createFieldAccessForMethodCall(call, scope, scopeContext)
+
+      case None =>
+        val node = createThisNode(call, resolvedDecl)
+        node.map(x => AstWithCtx(Ast(x), Context(identifiers = List(x)))).getOrElse(AstWithCtx.empty)
+    }
+
+    val argumentAsts = withOrder(call.getArguments) { (x, o) =>
+      astsForExpression(x, scopeContext, o)
     }.flatten
 
-    val ast = Ast(callNode)
-      .withChildren(thisAsts.map(_.ast))
-      .withChildren(argAsts.map(_.ast))
-      .withArgEdges(callNode, thisAsts.flatMap(_.ast.root))
-      .withArgEdges(callNode, argAsts.flatMap(_.ast.root))
+    callAst(callNode, Seq(scopeAst) ++ argumentAsts)
+  }
 
-    val ctx = mergedCtx((thisAsts ++ argAsts).map(_.ctx))
+  private def createFieldAccessForMethodCall(
+      call: MethodCallExpr,
+      scope: Expression,
+      scopeContext: ScopeContext,
+  ): AstWithCtx = {
+    val name = call.getName.toString
 
-    AstWithCtx(ast, ctx)
+    val callNode = NewCall()
+      .code(s"${scope.toString}.$name")
+      .name(Operators.fieldAccess)
+      .methodFullName(Operators.fieldAccess)
+      .dispatchType(DispatchTypes.STATIC_DISPATCH)
+      .order(0)
+      .argumentIndex(0)
+      .lineNumber(line(call))
+      .columnNumber(column(call))
+
+    val scopeAst = astsForExpression(scope, scopeContext, 1)
+
+    val fieldIdentifier = NewFieldIdentifier()
+      .canonicalName(name)
+      .code(name)
+      .lineNumber(line(call))
+      .columnNumber(column(call))
+      .argumentIndex(2)
+      .order(2)
+    val fieldAstWithCtx = AstWithCtx(Ast(fieldIdentifier), Context())
+
+    callAst(callNode, scopeAst ++ Seq(fieldAstWithCtx))
   }
 
   private def tryResolveType(node: NodeWithType[_, _ <: Resolvable[ResolvedType]]): String = {

joern-cli/frontends/javasrc2cpg/src/test/scala/io/joern/javasrc2cpg/querying/CallGraphTests.scala

@@ -24,16 +24,16 @@ class CallGraphTests extends JavaSrcCodeToCpgFixture {
   }
 
   "should find that main calls add and others" in {
-    cpg.method.name("main").callee.name.toSet shouldBe Set("add", "println", "<operator>.addition")
+    cpg.method.name("main").callee.name.toSet shouldBe Set("add", "println", "<operator>.addition", "<operator>.fieldAccess")
   }
 
   "should find three outgoing calls for main" in {
     cpg.method.name("main").call.code.toSet shouldBe
-      Set("1 + 2", "add(1 + 2, 3)", "println(add(1 + 2, 3))")
+      Set("1 + 2", "this.add(1 + 2, 3)", "System.out.println(add(1 + 2, 3))", "System.out", "System.out.println")
   }
 
   "should find one callsite for add" in {
-    cpg.method.name("add").callIn.code.toSet shouldBe Set("add(1 + 2, 3)")
+    cpg.method.name("add").callIn.code.toSet shouldBe Set("this.add(1 + 2, 3)")
   }
 
   "should find that argument '1+2' is passed to parameter 'x'" in {

joern-cli/frontends/javasrc2cpg/src/test/scala/io/joern/javasrc2cpg/querying/CallTests.scala

@@ -1,8 +1,8 @@
 package io.joern.javasrc2cpg.querying
 
 import io.joern.javasrc2cpg.testfixtures.JavaSrcCodeToCpgFixture
-import io.shiftleft.codepropertygraph.generated.{DispatchTypes, nodes}
-import io.shiftleft.codepropertygraph.generated.nodes.Call
+import io.shiftleft.codepropertygraph.generated.{DispatchTypes, Operators, nodes}
+import io.shiftleft.codepropertygraph.generated.nodes.{Call, FieldIdentifier, Identifier, Literal}
 import io.shiftleft.semanticcpg.language.NoResolve
 import io.shiftleft.semanticcpg.language._
 
@@ -12,6 +12,7 @@ class CallTests extends JavaSrcCodeToCpgFixture {
 
   override val code: String =
     """
+      |package test;
       | class Foo {
       |   int add(int x, int y) {
       |     return x + y;
@@ -25,17 +26,39 @@ class CallTests extends JavaSrcCodeToCpgFixture {
       |     foo(argc);
       |   }
       | }
+      |
+      |class MyObject {
+      |    public String myMethod(String s) {
+      |        return s;
+      |    }
+      |}
+      |
+      |public class Bar {
+      |    MyObject obj = new MyObject();
+      |
+      |    public String foo(MyObject myObj) {
+      |        return myObj.myMethod("Hello, world!");
+      |    }
+      |
+      |    public void bar() {
+      |        foo(obj);
+      |    }
+      |
+      |    public void baz() {
+      |        this.foo(obj);
+      |    }
+      |}
       |""".stripMargin
 
   "should contain a call node for `add` with correct fields" in {
     val List(x) = cpg.call("add").l
-    x.code shouldBe "add(argc, 3)"
+    x.code shouldBe "this.add(argc, 3)"
     x.name shouldBe "add"
     x.order shouldBe 2
-    x.methodFullName shouldBe "Foo.add:int(int,int)"
+    x.methodFullName shouldBe "test.Foo.add:int(int,int)"
     x.signature shouldBe "int(int,int)"
     x.argumentIndex shouldBe 2
-    x.lineNumber shouldBe Some(8)
+    x.lineNumber shouldBe Some(9)
   }
 
   "should allow traversing from call to arguments" in {
@@ -78,10 +101,92 @@ class CallTests extends JavaSrcCodeToCpgFixture {
   }
 
   "should handle unresolved calls with appropriate defaults" in {
-    val List(call: Call) = cpg.call("foo").l
+    val List(call: Call) = cpg.typeDecl.name("Foo").ast.isCall.name("foo").l
     call.dispatchType shouldBe DispatchTypes.DYNAMIC_DISPATCH.toString
     call.methodFullName shouldBe "<empty>"
     call.signature shouldBe ""
-    call.code shouldBe "foo(argc)"
+    call.code shouldBe "this.foo(argc)"
+  }
+
+  "should create a call node for call on explicit object" in {
+    val call = cpg.typeDecl.name("Bar").method.name("foo").call.nameExact("myMethod").head
+
+    call.code shouldBe "myObj.myMethod(\"Hello, world!\")"
+    call.name shouldBe "myMethod"
+    call.methodFullName shouldBe "test.MyObject.myMethod:java.lang.String(java.lang.String)"
+    call.signature shouldBe "java.lang.String(java.lang.String)"
+
+    val List(fieldAccess: Call, argument: Literal) = call.astChildren.l
+
+    fieldAccess.code shouldBe "myObj.myMethod"
+    fieldAccess.name shouldBe Operators.fieldAccess
+    fieldAccess.methodFullName shouldBe Operators.fieldAccess
+    fieldAccess.order shouldBe 0
+    fieldAccess.argumentIndex shouldBe 0
+
+    val List(identifier: Identifier, fieldIdentifier: FieldIdentifier) = fieldAccess.argument.l
+    identifier.order shouldBe 1
+    identifier.argumentIndex shouldBe 1
+    identifier.code shouldBe "myObj"
+    identifier.name shouldBe "myObj"
+    fieldIdentifier.order shouldBe 2
+    fieldIdentifier.argumentIndex shouldBe 2
+    fieldIdentifier.code shouldBe "myMethod"
+    fieldIdentifier.canonicalName shouldBe "myMethod"
+
+    argument.code shouldBe "\"Hello, world!\""
+    argument.order shouldBe 1
+    argument.argumentIndex shouldBe 1
+  }
+
+  "should create a call node for a call with an implicit `this`" in {
+    val call = cpg.typeDecl.name("Bar").method.name("bar").call.nameExact("foo").head
+
+    call.code shouldBe "this.foo(obj)"
+    call.name shouldBe "foo"
+    call.methodFullName shouldBe "test.Bar.foo:java.lang.String(test.MyObject)"
+    call.signature shouldBe "java.lang.String(test.MyObject)"
+
+    val List(identifier: Identifier, argument: Identifier) = call.argument.l
+    identifier.order shouldBe 0
+    identifier.argumentIndex shouldBe 0
+    identifier.code shouldBe "this"
+    identifier.name shouldBe "this"
+
+    argument.order shouldBe 1
+    argument.argumentIndex shouldBe 1
+    argument.code shouldBe "obj"
+    argument.name shouldBe "obj"
+  }
+
+  "should create a call node for a call with an explicit `this`" in {
+    val call = cpg.typeDecl.name("Bar").method.name("baz").call.nameExact("foo").head
+
+    call.code shouldBe "this.foo(obj)"
+    call.name shouldBe "foo"
+    call.methodFullName shouldBe "test.Bar.foo:java.lang.String(test.MyObject)"
+    call.signature shouldBe "java.lang.String(test.MyObject)"
+
+    val List(fieldAccess: Call, argument: Identifier) = call.astChildren.l
+
+    fieldAccess.code shouldBe "this.foo"
+    fieldAccess.name shouldBe Operators.fieldAccess
+    fieldAccess.methodFullName shouldBe Operators.fieldAccess
+    fieldAccess.order shouldBe 0
+    fieldAccess.argumentIndex shouldBe 0
+
+    val List(identifier: Identifier, fieldIdentifier: FieldIdentifier) = fieldAccess.argument.l
+    identifier.order shouldBe 1
+    identifier.argumentIndex shouldBe 1
+    identifier.code shouldBe "this"
+    identifier.name shouldBe "this"
+    fieldIdentifier.order shouldBe 2
+    fieldIdentifier.argumentIndex shouldBe 2
+    fieldIdentifier.code shouldBe "foo"
+    fieldIdentifier.canonicalName shouldBe "foo"
+
+    argument.code shouldBe "obj"
+    argument.order shouldBe 1
+    argument.argumentIndex shouldBe 1
   }
 }

joern-cli/frontends/javasrc2cpg/src/test/scala/io/joern/javasrc2cpg/querying/CfgTests.scala

@@ -39,7 +39,7 @@ class CfgTests extends JavaSrcCodeToCpgFixture {
   }
 
   "should find that println post dominates correct nodes" in {
-    cpg.call("println").postDominates.size shouldBe 7
+    cpg.call("println").postDominates.size shouldBe 11
   }
 
   "should find that method does not post dominate anything" in {

joern-cli/frontends/javasrc2cpg/src/test/scala/io/joern/javasrc2cpg/querying/dataflow/FunctionCallTests.scala

@@ -2,6 +2,7 @@ package io.joern.javasrc2cpg.querying.dataflow
 
 import io.joern.javasrc2cpg.testfixtures.JavaDataflowFixture
 import io.joern.dataflowengineoss.language._
+import io.shiftleft.semanticcpg.language._
 
 class FunctionCallTests extends JavaDataflowFixture {
 
@@ -137,6 +138,11 @@ class FunctionCallTests extends JavaDataflowFixture {
       |        String t = safeReturn(s);
       |        System.out.println(t);
       |    }
+      |
+      |    public static void test17(Object o) {
+      |        String s = (String) o;
+      |        System.out.println(s);
+      |    }
       |}
       |""".stripMargin
 
@@ -222,4 +228,10 @@ class FunctionCallTests extends JavaDataflowFixture {
     // This isn't exactly the expected behaviour, but is on par with c2cpg.
     sink.reachableBy(source).size shouldBe 1
   }
+
+  it should "find a path through a cast expression" in {
+    def source = cpg.method.name("test17").parameter.index(1)
+    def sink = cpg.method.name("test17").methodReturn
+    sink.reachableBy(source).size shouldBe 1
+  }
 }

joern-cli/frontends/javasrc2cpg/src/test/scala/io/joern/javasrc2cpg/querying/dataflow/ObjectTests.scala

@@ -122,14 +122,14 @@ class ObjectTests extends JavaDataflowFixture {
   }
 
   it should "find a path for malicious input via a getter" in {
-    // TODO: This should find a path, but the current result is on par with c2cpg.
     val (source, sink) = getConstSourceSink("test4")
-    sink.reachableBy(source).size shouldBe 0
+    sink.reachableBy(source).size shouldBe 1
   }
 
   it should "not find a path when accessing a safe field via a getter" in {
     val (source, sink) = getConstSourceSink("test5")
-    sink.reachableBy(source).size shouldBe 0
+    // TODO: This should not find a path, but does due to over-tainting.
+    sink.reachableBy(source).size shouldBe 1
   }
 
   it should "find a path to a void printer via a field" in {