Clarifying setting location and code in the Node builders (#1404)

* Clarifying setting location and code in the Node builders

This PR adds a clarifying comment to the node builder, why set set the location and code there. It also removes the latest remnants of setting the location in the handlerClarifying setting location and code in the Node builders

* Added more rawNode to node builders in LLVM frontend

* Adding more rawNode specifiers to typescript frontend

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/Handler.kt

@@ -106,12 +106,6 @@ abstract class Handler<ResultNode : Node?, HandlerNode, L : LanguageFrontend<in
         if (handler != null) {
             val s = handler.handle(ctx)
             if (s != null) {
-                // The language frontend might set a location, which we should respect. Otherwise,
-                // we will
-                // set the location here.
-                if (s.location == null) {
-                    frontend.setCodeAndLocation(s, ctx)
-                }
                 frontend.setComment(s, ctx)
             }
             ret = s

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/graph/NodeBuilder.kt

@@ -92,9 +92,7 @@ interface NamespaceProvider : MetadataProvider {
  * - Setting [Node.scope]. if a [ScopeProvider] is given
  * - Setting [Node.isInferred], if an [IsInferredProvider] is given
  *
- * Note, that one provider can implement multiple provider interfaces. Additionally, if
- * [codeOverride] is specified, the supplied source code is used to override anything from the
- * provider.
+ * Note, that one provider can implement multiple provider interfaces.
  */
 fun Node.applyMetadata(
     provider: MetadataProvider?,
@@ -103,6 +101,12 @@ fun Node.applyMetadata(
     localNameOnly: Boolean = false,
     defaultNamespace: Name? = null,
 ) {
+    // We try to set the code and especially the location as soon as possible because the hashCode
+    // implementation of the Node class relies on it. Otherwise, we could have a problem that the
+    // location is not yet set, but the node is put into a hashmap. In this case the hashCode is
+    // calculated based on an empty location and if we would later set the location, we would have a
+    // mismatch. Each language frontend and also each handler implements CodeAndLocationProvider, so
+    // calling a node builder from these should already set the location.
     if (provider is CodeAndLocationProvider<*> && rawNode != null) {
         (provider as CodeAndLocationProvider<Any>).setCodeAndLocation(this, rawNode)
     }

cpg-language-java/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/java/ExpressionHandler.kt

@@ -255,7 +255,7 @@ class ExpressionHandler(lang: JavaLanguageFrontend) :
     // Not sure how to handle this exactly yet
     private fun handleVariableDeclarationExpr(expr: Expression): DeclarationStatement {
         val variableDeclarationExpr = expr.asVariableDeclarationExpr()
-        val declarationStatement = newDeclarationStatement()
+        val declarationStatement = newDeclarationStatement(rawNode = expr)
         for (variable in variableDeclarationExpr.variables) {
             val declaration = frontend.declarationHandler.handleVariableDeclarator(variable)
             frontend.setCodeAndLocation(declaration, variable)
@@ -401,7 +401,8 @@ class ExpressionHandler(lang: JavaLanguageFrontend) :
                     fieldAccessExpr.name.identifier,
                     base,
                     fieldType,
-                    "." // there is only "." in java
+                    ".", // there is only "." in java
+                    rawNode = expr
                 )
             memberExpression.isStaticAccess = true
             return memberExpression
@@ -417,14 +418,26 @@ class ExpressionHandler(lang: JavaLanguageFrontend) :
                     unknownType()
                 }
             val memberExpression =
-                newMemberExpression(fieldAccessExpr.name.identifier, base, fieldType, ".")
+                newMemberExpression(
+                    fieldAccessExpr.name.identifier,
+                    base,
+                    fieldType,
+                    ".",
+                    rawNode = expr
+                )
             memberExpression.isStaticAccess = true
             return memberExpression
         }
         if (base.location == null) {
             base.location = frontend.locationOf(fieldAccessExpr)
         }
-        return newMemberExpression(fieldAccessExpr.name.identifier, base, fieldType, ".")
+        return newMemberExpression(
+            fieldAccessExpr.name.identifier,
+            base,
+            fieldType,
+            ".",
+            rawNode = expr
+        )
     }
 
     private fun handleLiteralExpression(expr: Expression): Literal<*>? {

cpg-language-java/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/java/StatementHandler.kt

@@ -102,7 +102,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
                 frontend.expressionHandler.handle(expr)
                     as? de.fraunhofer.aisec.cpg.graph.statements.expressions.Expression
         }
-        val returnStatement = this.newReturnStatement()
+        val returnStatement = this.newReturnStatement(rawNode = stmt)
         // JavaParser seems to add implicit return statements, that are not part of the original
         // source code. We mark it as such
         returnStatement.isImplicit = !returnStmt.tokenRange.isPresent
@@ -119,7 +119,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
         val conditionExpression = ifStmt.condition
         val thenStatement = ifStmt.thenStmt
         val optionalElseStatement = ifStmt.elseStmt
-        val ifStatement = this.newIfStatement()
+        val ifStatement = newIfStatement(rawNode = stmt)
         frontend.scopeManager.enterScope(ifStatement)
         ifStatement.thenStatement = handle(thenStatement)
         ifStatement.condition =
@@ -134,7 +134,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
         val assertStmt = stmt.asAssertStmt()
         val conditionExpression = assertStmt.check
         val thenStatement = assertStmt.message
-        val assertStatement = this.newAssertStatement()
+        val assertStatement = newAssertStatement(rawNode = stmt)
         assertStatement.condition =
             frontend.expressionHandler.handle(conditionExpression)
                 as de.fraunhofer.aisec.cpg.graph.statements.expressions.Expression
@@ -148,7 +148,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
         val whileStmt = stmt.asWhileStmt()
         val conditionExpression = whileStmt.condition
         val statement = whileStmt.body
-        val whileStatement = this.newWhileStatement()
+        val whileStatement = newWhileStatement(rawNode = stmt)
         frontend.scopeManager.enterScope(whileStatement)
         whileStatement.statement = handle(statement)
         whileStatement.condition =
@@ -159,7 +159,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
     }
 
     private fun handleForEachStatement(stmt: Statement): ForEachStatement {
-        val statement = this.newForEachStatement()
+        val statement = newForEachStatement(rawNode = stmt)
         frontend.scopeManager.enterScope(statement)
         val forEachStmt = stmt.asForEachStmt()
         val variable = frontend.expressionHandler.handle(forEachStmt.variable)
@@ -177,7 +177,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
 
     private fun handleForStatement(stmt: Statement): ForStatement {
         val forStmt = stmt.asForStmt()
-        val statement = this.newForStatement()
+        val statement = this.newForStatement(rawNode = stmt)
         frontend.setCodeAndLocation(statement, stmt)
         frontend.scopeManager.enterScope(statement)
         if (forStmt.initialization.size > 1) {
@@ -283,7 +283,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
         val doStmt = stmt.asDoStmt()
         val conditionExpression = doStmt.condition
         val statement = doStmt.body
-        val doStatement = this.newDoStatement()
+        val doStatement = newDoStatement(rawNode = stmt)
         frontend.scopeManager.enterScope(doStatement)
         doStatement.statement = handle(statement)
         doStatement.condition =
@@ -294,12 +294,12 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
     }
 
     private fun handleEmptyStatement(stmt: Statement): EmptyStatement {
-        return this.newEmptyStatement()
+        return this.newEmptyStatement(rawNode = stmt)
     }
 
     private fun handleSynchronizedStatement(stmt: Statement): SynchronizedStatement {
         val synchronizedJava = stmt.asSynchronizedStmt()
-        val synchronizedCPG = this.newSynchronizedStatement()
+        val synchronizedCPG = newSynchronizedStatement(rawNode = stmt)
         synchronizedCPG.expression =
             frontend.expressionHandler.handle(synchronizedJava.expression)
                 as de.fraunhofer.aisec.cpg.graph.statements.expressions.Expression
@@ -311,22 +311,22 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
         val labelStmt = stmt.asLabeledStmt()
         val label = labelStmt.label.identifier
         val statement = labelStmt.statement
-        val labelStatement = this.newLabelStatement()
+        val labelStatement = newLabelStatement(rawNode = stmt)
         labelStatement.subStatement = handle(statement)
         labelStatement.label = label
         return labelStatement
     }
 
     private fun handleBreakStatement(stmt: Statement): BreakStatement {
         val breakStmt = stmt.asBreakStmt()
-        val breakStatement = this.newBreakStatement()
+        val breakStatement = newBreakStatement(rawNode = stmt)
         breakStmt.label.ifPresent { label: SimpleName -> breakStatement.label = label.toString() }
         return breakStatement
     }
 
     private fun handleContinueStatement(stmt: Statement): ContinueStatement {
         val continueStmt = stmt.asContinueStmt()
-        val continueStatement = this.newContinueStatement()
+        val continueStatement = newContinueStatement(rawNode = stmt)
         continueStmt.label.ifPresent { label: SimpleName ->
             continueStatement.label = label.toString()
         }
@@ -337,7 +337,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
         val blockStmt = stmt.asBlockStmt()
 
         // first of, all we need a compound statement
-        val compoundStatement = this.newBlock(rawNode = stmt)
+        val compoundStatement = newBlock(rawNode = stmt)
         frontend.scopeManager.enterScope(compoundStatement)
         for (child in blockStmt.statements) {
             val statement = handle(child)
@@ -373,7 +373,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
                         getNextTokenWith(":", optionalTokenRange.get().begin)
                     )
             }
-            val defaultStatement = this.newDefaultStatement()
+            val defaultStatement = newDefaultStatement()
             defaultStatement.location =
                 getLocationsFromTokens(parentLocation, caseTokens.a, caseTokens.b)
             return defaultStatement
@@ -461,7 +461,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
 
     fun handleSwitchStatement(stmt: Statement): SwitchStatement {
         val switchStmt = stmt.asSwitchStmt()
-        val switchStatement = this.newSwitchStatement()
+        val switchStatement = newSwitchStatement(rawNode = stmt)
 
         // make sure location is set
         frontend.setCodeAndLocation(switchStatement, switchStmt)
@@ -539,7 +539,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
 
     private fun handleTryStatement(stmt: Statement): TryStatement {
         val tryStmt = stmt.asTryStmt()
-        val tryStatement = this.newTryStatement()
+        val tryStatement = newTryStatement(rawNode = stmt)
         frontend.scopeManager.enterScope(tryStatement)
         val resources =
             tryStmt.resources.mapNotNull { ctx: Expression ->
@@ -573,7 +573,7 @@ class StatementHandler(lang: JavaLanguageFrontend?) :
     private fun handleCatchClause(
         catchCls: CatchClause
     ): de.fraunhofer.aisec.cpg.graph.statements.CatchClause {
-        val cClause = this.newCatchClause()
+        val cClause = newCatchClause(rawNode = catchCls)
         frontend.scopeManager.enterScope(cClause)
         val possibleTypes = mutableSetOf<Type>()
         val concreteType: Type

cpg-language-llvm/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/llvm/StatementHandler.kt

@@ -74,7 +74,7 @@ class StatementHandler(lang: LLVMIRLanguageFrontend) :
 
         when (opcode) {
             LLVMRet -> {
-                val ret = newReturnStatement()
+                val ret = newReturnStatement(rawNode = instr)
 
                 val numOps = LLVMGetNumOperands(instr)
                 if (numOps != 0) {
@@ -98,7 +98,7 @@ class StatementHandler(lang: LLVMIRLanguageFrontend) :
             }
             LLVMUnreachable -> {
                 // Does nothing
-                return newEmptyStatement()
+                return newEmptyStatement(rawNode = instr)
             }
             LLVMCallBr -> {
                 // Maps to a call but also to a goto statement? Barely used => not relevant
@@ -133,7 +133,7 @@ class StatementHandler(lang: LLVMIRLanguageFrontend) :
             }
             LLVMPHI -> {
                 frontend.phiList.add(instr)
-                return newEmptyStatement()
+                return newEmptyStatement(rawNode = instr)
             }
             LLVMSelect -> {
                 return declarationOrNot(frontend.expressionHandler.handleSelect(instr), instr)
@@ -143,7 +143,7 @@ class StatementHandler(lang: LLVMIRLanguageFrontend) :
                 log.info(
                     "userop instruction is not a real instruction. Replacing it with empty statement"
                 )
-                return newEmptyStatement()
+                return newEmptyStatement(rawNode = instr)
             }
             LLVMVAArg -> {
                 return handleVaArg(instr)
@@ -244,7 +244,7 @@ class StatementHandler(lang: LLVMIRLanguageFrontend) :
             gotoStatement.name = name
             gotoStatement
         } else {
-            val emptyStatement = newEmptyStatement()
+            val emptyStatement = newEmptyStatement(rawNode = instr)
             emptyStatement.name = name
             emptyStatement
         }
@@ -1034,7 +1034,7 @@ class StatementHandler(lang: LLVMIRLanguageFrontend) :
     private fun handleBrStatement(instr: LLVMValueRef): Statement {
         if (LLVMGetNumOperands(instr) == 3) {
             // if(op) then {goto label1} else {goto label2}
-            val ifStatement = newIfStatement()
+            val ifStatement = newIfStatement(rawNode = instr)
             val condition = frontend.getOperandValueAtIndex(instr, 0)
             ifStatement.condition = condition
 
@@ -1466,7 +1466,9 @@ class StatementHandler(lang: LLVMIRLanguageFrontend) :
             // add it to our bindings cache
             frontend.bindingsCache[symbolName] = decl
 
-            val declStatement = DeclarationStatement()
+            // Since the declaration statement only contains the single declaration, we can use the
+            // same raw node, so we end up with the same code and location
+            val declStatement = newDeclarationStatement(rawNode = valueRef)
             declStatement.singleDeclaration = decl
             declStatement
         } else {

cpg-language-typescript/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/typescript/StatementHandler.kt

@@ -58,7 +58,7 @@ class StatementHandler(lang: TypeScriptLanguageFrontend) :
     private fun handleFunctionDeclaration(node: TypeScriptNode): Statement {
         // typescript allows to declare function on a statement level, e.g. within a compound
         // statement. We can wrap it into a declaration statement
-        val statement = newDeclarationStatement()
+        val statement = newDeclarationStatement(rawNode = node)
 
         val decl = this.frontend.declarationHandler.handle(node)
 
@@ -72,7 +72,7 @@ class StatementHandler(lang: TypeScriptLanguageFrontend) :
     }
 
     private fun handleReturnStatement(node: TypeScriptNode): ReturnStatement {
-        val returnStmt = newReturnStatement()
+        val returnStmt = newReturnStatement(rawNode = node)
 
         node.children?.first()?.let {
             returnStmt.returnValue = this.frontend.expressionHandler.handle(it)
@@ -98,7 +98,7 @@ class StatementHandler(lang: TypeScriptLanguageFrontend) :
     }
 
     private fun handleVariableStatement(node: TypeScriptNode): DeclarationStatement {
-        val statement = newDeclarationStatement()
+        val statement = newDeclarationStatement(rawNode = node)
 
         // the declarations are contained in a VariableDeclarationList
         val nodes = node.firstChild("VariableDeclarationList")?.children