Fixing Java field access #1862

oxisto · 2024-11-21T10:09:20Z

This PR consists of a major overhaul of Java reference and member expression parsing. It removes a LOT of old weird legacy code that was full of weird things.

It also introduces a new Java-specific pass, that takes care of deciding when a member access is actually static or not. This might be something that we could decide to adapt for other/all languages (maybe also tied to the discussion in #1863).

It also contains some adjustments to the import system, more specifically when the importedSymbols field is populated. This might have implications for other languages and if wanted I can extract this into a separate PR.

oxisto · 2024-11-21T14:13:13Z

Blocked by #1586

codecov · 2024-11-21T15:10:17Z

Codecov Report

Attention: Patch coverage is 82.05128% with 14 lines in your changes missing coverage. Please review.

Project coverage is 77.07%. Comparing base (e62d7af) to head (0494bdb).

Files with missing lines	Patch %	Lines
...n/de/fraunhofer/aisec/cpg/passes/SymbolResolver.kt	16.66%	4 Missing and 1 partial ⚠️
...in/de/fraunhofer/aisec/cpg/passes/JavaExtraPass.kt	70.58%	0 Missing and 5 partials ⚠️
...n/de/fraunhofer/aisec/cpg/passes/ImportResolver.kt	85.71%	0 Missing and 2 partials ⚠️
...raunhofer/aisec/cpg/frontends/java/JavaLanguage.kt	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
...tlin/de/fraunhofer/aisec/cpg/frontends/Language.kt	`92.24% <100.00%> (+0.49%)`	⬆️
...lin/de/fraunhofer/aisec/cpg/passes/TypeResolver.kt	`95.83% <100.00%> (+0.18%)`	⬆️
...s/kotlin/de/fraunhofer/aisec/cpg/test/TestUtils.kt	`71.27% <ø> (ø)`
...ofer/aisec/cpg/frontends/java/ExpressionHandler.kt	`78.09% <100.00%> (+4.87%)`	⬆️
...r/aisec/cpg/frontends/java/JavaLanguageFrontend.kt	`70.46% <100.00%> (-4.22%)`	⬇️
...n/de/fraunhofer/aisec/cpg/passes/ImportResolver.kt	`89.65% <85.71%> (-0.46%)`	⬇️
...raunhofer/aisec/cpg/frontends/java/JavaLanguage.kt	`84.09% <60.00%> (-3.09%)`	⬇️
...n/de/fraunhofer/aisec/cpg/passes/SymbolResolver.kt	`86.62% <16.66%> (-1.34%)`	⬇️
...in/de/fraunhofer/aisec/cpg/passes/JavaExtraPass.kt	`70.58% <70.58%> (ø)`

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

KuechA

I fail to understand most of the changes since the old code is hard to understand and the new code requires in-depth knowledge and understanding of the java parser and frontend which I do not have. However, I have some nits which should already be resolved and may support subsequent more detailed reviews.

KuechA · 2024-12-02T09:45:25Z

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/SymbolResolver.kt

@@ -209,7 +209,7 @@ open class SymbolResolver(ctx: TranslationContext) : ComponentPass(ctx) {
        // Preparation for a future without legacy call resolving. Taking the first candidate is not
        // ideal since we are running into an issue with function pointers here (see workaround
        // below).
-        var wouldResolveTo = ref.candidates.singleOrNull()
+        var wouldResolveTo = ref.candidates.firstOrNull()


This change does no longer account for ambiguities. The old code with singleOrNull() prefers not to resolve anything if it's not sure about the candidate. I think this was also intended, wasn't it?

It was semi-intended. I am ok with the old behavior, but we should at least

have a warning if we do not resolve it

forward the decision to the Language. In Java it is possible to have an ambiguity here if you statically import a field and a function and both can have the same name and end up in candidates. I need to deal with that here

KuechA · 2024-12-02T09:49:35Z

cpg-language-java/src/test/kotlin/de/fraunhofer/aisec/cpg/enhancements/calls/SuperCallTest.kt

        val fieldRef = findByUniquePredicate(refs) { "field" == it.code }
        val getSuperField = findByUniqueName(methods, "getSuperField")
        refs = getSuperField.allChildren<MemberExpression>()
        val superFieldRef = findByUniquePredicate(refs) { "super.field" == it.code }
-        assertTrue(fieldRef.base is Reference)
-        assertRefersTo(fieldRef.base, getField.receiver)
+        // See https://github.com/Fraunhofer-AISEC/cpg/issues/1863.


I don't see how this relates to the case of java.

KuechA · 2024-12-02T09:50:36Z

cpg-language-java/src/test/kotlin/de/fraunhofer/aisec/cpg/enhancements/calls/SuperCallTest.kt

+        // See https://github.com/Fraunhofer-AISEC/cpg/issues/1863.
+        // We only have a regular call expression here, if we do not resolve anything else in the
+        // frontend
+        /*assertTrue(fieldRef.base is Reference)


commented out code. -> Remove or fix. What do we expect?

KuechA · 2024-12-02T09:54:04Z

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

@@ -889,3 +658,21 @@ class ExpressionHandler(lang: JavaLanguageFrontend) :
        }
    }
 }
+
+fun ResolvedFieldDeclaration.toFieldAccessExpr(expr: NameExpr): FieldAccessExpr {


Documentation missing

KuechA · 2024-12-02T10:00:03Z

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

-        //    }
-        val name = this.parseName(nameExpr.nameAsString)
-        return try {
+        // Try to resolve it. We will remove this in a future where we do not really in the


Is the TODO of this method resolved? If so, please delete it.

Also, please document the method according to the new standards: #1683 (comment)

This sentence is broken

KuechA · 2024-12-02T10:03:41Z

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

-                    ?: newProblemExpression("Could not parse base")
-        }
-        var fieldType: Type?
+


Please document the method according to the new standards: #1683 (comment)

KuechA · 2024-12-02T10:07:00Z

cpg-language-java/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/JavaExtraPass.kt

+
+@DependsOn(TypeResolver::class)
+@ExecuteBefore(SymbolResolver::class)
+class JavaExtraPass(ctx: TranslationContext) : TranslationUnitPass(ctx) {


Documentation missing for this whole file

oxisto · 2024-12-18T08:56:02Z

I fail to understand most of the changes since the old code is hard to understand and the new code requires in-depth knowledge and understanding of the java parser and frontend which I do not have. However, I have some nits which should already be resolved and may support subsequent more detailed reviews.

Agreed. I will extract the remaining changes that are not Java-specific to extra PRs (#1908 and #1909)

…t of candidates This adds a new function `Language.bestViableReferenceCandidate` which takes the old implementation as a default behaviour.

…tion` This introduces a new extension function `ImportDeclaration.updateImportedSymbols`. The idea behind that is that we might need to update the `importedSymbols` of an import declaration, if the symbols that "live" in the associated namespace change. This is mainly the case once we infer records out of known `Type` nodes. Therefore, the type resolver will update all the imported symbols before executing all the remaining passes. This way, the remainder of the passes can access imported symbols that were inferred.

oxisto mentioned this pull request Nov 21, 2024

Removing legacy resolveReference function #1861

Draft

oxisto added the blocked Blocked by an external factor label Nov 21, 2024

oxisto force-pushed the java-fix-static branch from c781e33 to faaa036 Compare November 21, 2024 14:34

oxisto force-pushed the java-fix-static branch from 4c7279c to 952f56c Compare November 28, 2024 14:00

oxisto removed the blocked Blocked by an external factor label Nov 28, 2024

oxisto force-pushed the java-fix-static branch from 952f56c to 5f75416 Compare November 28, 2024 14:05

oxisto marked this pull request as ready for review November 28, 2024 14:05

oxisto requested review from konradweiss and KuechA as code owners November 28, 2024 14:05

oxisto added the java Related to Java language parsing label Nov 28, 2024

oxisto force-pushed the java-fix-static branch from 5f75416 to c7f72a6 Compare November 28, 2024 20:56

KuechA reviewed Dec 2, 2024

View reviewed changes

oxisto force-pushed the java-fix-static branch from c7f72a6 to 592b8ce Compare December 18, 2024 08:49

oxisto mentioned this pull request Dec 18, 2024

Providing an API to update the imported symbols for an ImportDeclaration #1908

Open

oxisto force-pushed the java-fix-static branch from 592b8ce to d4cd2a8 Compare December 19, 2024 20:09

oxisto added 4 commits December 19, 2024 21:10

Provide a function for languages to influence reference resolution ou…

8d7a4ca

…t of candidates This adds a new function `Language.bestViableReferenceCandidate` which takes the old implementation as a default behaviour.

Fixing Java field access

6316a28

Better solution for reference candidate

0494bdb

oxisto force-pushed the java-fix-static branch from d4cd2a8 to 0494bdb Compare December 19, 2024 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing Java field access #1862

Fixing Java field access #1862

oxisto commented Nov 21, 2024 •

edited

Loading

oxisto commented Nov 21, 2024

codecov bot commented Nov 21, 2024 •

edited

Loading

KuechA left a comment

KuechA Dec 2, 2024

oxisto Dec 18, 2024

KuechA Dec 2, 2024

KuechA Dec 2, 2024

KuechA Dec 2, 2024

KuechA Dec 2, 2024

KuechA Dec 2, 2024

KuechA Dec 2, 2024

oxisto commented Dec 18, 2024 •

edited

Loading

Fixing Java field access #1862

Are you sure you want to change the base?

Fixing Java field access #1862

Conversation

oxisto commented Nov 21, 2024 • edited Loading

oxisto commented Nov 21, 2024

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

KuechA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxisto commented Dec 18, 2024 • edited Loading

oxisto commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

oxisto commented Dec 18, 2024 •

edited

Loading