Skip to content

Commit

Permalink
[javasrc2cpg] Don't add unknown nodes to cpg for unhandled captured v…
Browse files Browse the repository at this point in the history
…ariables (#4617)

* Add reproducing test

* Add tests for new lambdas

* Never add Unknown node for captured variable

* Add todos to comments about lambda parameter capturing
  • Loading branch information
johannescoetzee committed May 30, 2024
1 parent 2386dd3 commit b8538e7
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -97,22 +97,17 @@ trait AstForNameExpressionsCreator { this: AstCreator =>
val variable = capturedVariable.variable
val typeDeclChain = capturedVariable.typeDeclChain

scope.enclosingMethod.map(_.lookupVariable("this")) match {
case None | Some(NotInScope) | Some(CapturedVariable(_, _)) =>
scope.lookupVariable("this") match {
case NotInScope | CapturedVariable(_, _) =>
logger.warn(
s"Attempted to create AST for captured variable ${variable.name}, but could not find `this` param in direct scope."
)
Ast(NewUnknown().code(variable.name).lineNumber(line(nameExpr)).columnNumber(column(nameExpr)))

case Some(SimpleVariable(ScopeParameter(thisNode: NewMethodParameterIn))) =>
val thisIdentifier = identifierNode(
nameExpr,
thisNode.name,
thisNode.code,
thisNode.typeFullName,
thisNode.dynamicTypeHintFullName
)
val thisAst = Ast(thisIdentifier).withRefEdge(thisIdentifier, thisNode)
Ast(identifierNode(nameExpr, variable.name, variable.name, variable.typeFullName))

case SimpleVariable(scopeVariable) =>
val thisIdentifier =
identifierNode(nameExpr, scopeVariable.name, scopeVariable.name, scopeVariable.typeFullName)
val thisAst = Ast(thisIdentifier).withRefEdge(thisIdentifier, scopeVariable.node)

val lineNumber = line(nameExpr)
val columnNumber = column(nameExpr)
Expand All @@ -139,12 +134,6 @@ trait AstForNameExpressionsCreator { this: AstCreator =>

val captureFieldIdentifier = fieldIdentifierNode(nameExpr, variable.name, variable.name)
callAst(finalFieldAccess, List(outerClassChain, Ast(captureFieldIdentifier)))

case Some(SimpleVariable(thisNode)) =>
logger.warn(
s"Attempted to create AST for captured variable ${variable.name}, but found non-parameter `this`: ${thisNode}."
)
Ast(NewUnknown().code(variable.name).lineNumber(line(nameExpr)).columnNumber(column(nameExpr)))
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -742,4 +742,72 @@ class LambdaTests extends JavaSrcCode2CpgFixture {
cpg.call.nameExact("<init>").count(_.argument.isEmpty) shouldBe 0
}
}

"calls on captured variables in lambdas contained in anonymous classes" should {
val cpg = code("""
|
|public class Foo {
|
| public static void sink(String s) {};
|
| public static Object test(Bar captured) {
| Visitor v = new Visitor() {
| public void visit(Visited visited) {
| visited.getList().forEach(lambdaParam -> captured.remove(lambdaParam));
| }
| };
| }
|}
|""".stripMargin)

// TODO: This behaviour isn't exactly correct, but is on par with how we currently handle field captures in lambdas.
"have the correct receiver ast" in {
inside(cpg.call.name("remove").receiver.l) { case List(fieldAccessCall: Call) =>
fieldAccessCall.name shouldBe Operators.fieldAccess

inside(fieldAccessCall.argument.l) { case List(identifier: Identifier, fieldIdentifier: FieldIdentifier) =>
identifier.name shouldBe "this"
identifier.typeFullName shouldBe "Foo.test.Visitor$0"

fieldIdentifier.canonicalName shouldBe "captured"

fieldAccessCall.typeFullName shouldBe "<unresolvedNamespace>.Bar"
}
}
}
}

// TODO: These tests exist to document current behaviour, but the current behaviour is wrong.
"lambdas capturing parameters" should {
val cpg = code("""
|import java.util.function.Consumer;
|
|public class Foo {
| public String capturedField;
|
| public void foo() {
| Consumer<String> consumer = lambdaParam -> System.out.println(capturedField);
| }
|}
|""".stripMargin)

"represent the captured field as a field access" in {
inside(cpg.method.name(".*lambda.*").call.name("println").argument.l) { case List(_, fieldAccessCall: Call) =>
fieldAccessCall.name shouldBe Operators.fieldAccess

inside(fieldAccessCall.argument.l) { case List(identifier: Identifier, fieldIdentifier: FieldIdentifier) =>
identifier.name shouldBe "this"
identifier.typeFullName shouldBe "Foo"

fieldIdentifier.canonicalName shouldBe "capturedField"
}
}
}

// TODO: It should, but it doesn't.
"have a captured local for the enclosing class" in {
// There should be an `outerClass` local which captures the outer method `this`.
cpg.method.name(".*lambda.*").local.name("this").typeFullName(".*Foo.*").isEmpty shouldBe true
}
}
}

0 comments on commit b8538e7

Please sign in to comment.