Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix DFG of the variable of aForEachStatement #1052

Merged
merged 16 commits into from
Feb 10, 2023
Merged

Fix DFG of the variable of aForEachStatement #1052

merged 16 commits into from
Feb 10, 2023

Conversation

KuechA
Copy link
Contributor

@KuechA KuechA commented Jan 19, 2023

The variable declaration in a ForEachStatement does not have an initializer and is therefore not really considered by the ControlFlowSensitiveDFGPass. We have to handle this case separately.

Closes #1006

@KuechA KuechA marked this pull request as ready for review January 20, 2023 07:53
oxisto
oxisto reviewed Jan 21, 2023
View reviewed changes
Copy link
Member

@oxisto oxisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to handle anything else in the normal DFG pass?

@KuechA
Copy link
Contributor Author

KuechA commented Jan 23, 2023

No, actually, the respective edges from the DFGPass would probably be removed anyway... I tried an alternative approach by setting the initializer of the variable in the normal DFG Pass with the iterable but I didn't like the solution (even if it would have worked).

@oxisto
Copy link
Member

oxisto commented Jan 23, 2023

No, actually, the respective edges from the DFGPass would probably be removed anyway... I tried an alternative approach by setting the initializer of the variable in the normal DFG Pass with the iterable but I didn't like the solution (even if it would have worked).

I just meant, if the user is missing out on this "feature", if he is only using the "normal" DFG pass?

@KuechA
Copy link
Contributor Author

KuechA commented Jan 23, 2023

No, actually, the respective edges from the DFGPass would probably be removed anyway... I tried an alternative approach by setting the initializer of the variable in the normal DFG Pass with the iterable but I didn't like the solution (even if it would have worked).

I just meant, if the user is missing out on this "feature", if he is only using the "normal" DFG pass?

Good point. Since the variable declaration is wrapped in a DeclarationStatement, the DFG edge was indeed not added only with the normal DFG pass. Btw. the DeclarationStatement has a DFG edge to the ForEachStatement. Do you have any idea why this is the case? We do not add this edge in our passes

@oxisto
Copy link
Member

oxisto commented Jan 23, 2023

No, actually, the respective edges from the DFGPass would probably be removed anyway... I tried an alternative approach by setting the initializer of the variable in the normal DFG Pass with the iterable but I didn't like the solution (even if it would have worked).

I just meant, if the user is missing out on this "feature", if he is only using the "normal" DFG pass?

Good point. Since the variable declaration is wrapped in a DeclarationStatement, the DFG edge was indeed not added only with the normal DFG pass. Btw. the DeclarationStatement has a DFG edge to the ForEachStatement. Do you have any idea why this is the case? We do not add this edge in our passes

uhm no idea :D @konradweiss ?

KuechA and others added 6 commits January 27, 2023 09:15
@KuechA
Update documentation
c43f7e8
@KuechA
Do not handle all DeclarationStatements but only the ones in the ForE…
273e519 
…achLoop variable
@KuechA
Merge branch 'main' into dfg-fix-foreach
d46c6a2
@KuechA
Add testcase for DFG edges
dcb59bc
@KuechA
Merge main into dfg-fix-foreach
62a0689
@maximiliankaul @KuechA @oxisto
Fix DFG ForEachStatement and add Python test
d53e752 
* fix for loop and add DFG test

* fix test

* fix test

* DFG pass fixes ForEach with Declarations

@KuechA

* New crazy idea

* Moving some code around

* Also handle NamespaceDeclarations for the DFG

* Move test code to function

* Compiler error

* Fix loop DFGs

* Correct parsing of functions in namespaces in C++ (#1078)

* We incorrectly assumed that all scoped function definitions are methods. This is not true. This PR fixes this issue by determinging the correct scope target (which could also be a namespace).

Furthermore, this tries to mitigate the problem that types used within such a function could refer to a namespaced entity but are referred with its local name. Since this requires resolving of symbols in a frontend, we introduce a new annotaiton `@ResolveInFrontend` to warn developers.

Fixes #1070

* Converting remaining Java node classes to Kotlin (#1082)

Co-authored-by: Alexander Kuechler <[email protected]>

* Fluent Node DSL (#772)

* Fluent Node DSL

This PR adds a fluent node DSL to quickly build up a tree of CPG nodes. This can be used for unit tests or any occasion you quickly want to spin up a tree of CPG nodes that are independent of the underlying programming language. This is useful if you want to test the behaviour of some passes purely on the CPG tree, rather than a concrete program.

* Consider all children to find the `prevDFG` for `FunctionDeclaration`s (#1086)

* Fix DFG for FunctionDeclarations

* Remove DFG for unreachable implicit return

* Set isImplicit in java frontend

* Apply suggestions from code review

* Format

---------

Co-authored-by: Christian Banse <[email protected]>

* Disable problematic test

The for loop test is broken because the implementation is broken. Thus,
the test is disabled, until there is proper support for multi var
assignments.

* revert enabling of additional language

* make it compile

---------

Co-authored-by: Alexander Kuechler <[email protected]>
Co-authored-by: Christian Banse <[email protected]>
Co-authored-by: KuechA <[email protected]>
@maximiliankaul maximiliankaul self-requested a review as a code owner February 7, 2023 20:08
maximiliankaul
maximiliankaul approved these changes Feb 8, 2023
View reviewed changes
Copy link
Contributor

@maximiliankaul maximiliankaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally in favor of this PR. The only thing I dislike is the heavy use of !! in the pass.

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/ControlFlowSensitiveDFGPass.kt Show resolved Hide resolved
cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/ControlFlowSensitiveDFGPass.kt Outdated Show resolved Hide resolved
cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/ControlFlowSensitiveDFGPass.kt Outdated Show resolved Hide resolved
@oxisto
Copy link
Member

oxisto commented Feb 9, 2023

I'm generally in favor of this PR. The only thing I dislike is the heavy use of !! in the pass.

Agree. Can you change it to a local variable assignment + smart card instead? We should really avoid !!

@sonarcloud
Copy link

sonarcloud bot commented Feb 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

50.0% 50.0% Coverage
0.0% 0.0% Duplication

@konradweiss konradweiss merged commit dd62756 into main Feb 10, 2023
@konradweiss konradweiss deleted the dfg-fix-foreach branch February 10, 2023 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maximiliankaul maximiliankaul maximiliankaul approved these changes

@konradweiss konradweiss konradweiss requested changes

@oxisto oxisto oxisto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

variable and iterable from ForEachStatement are not connected in DFG
4 participants
@KuechA @oxisto @konradweiss @maximiliankaul