Various fixes for declarations

[oxisto]

• Properly parses declarations that are part of the declaration specifier now.
• Adding problem declarations that are parsed as part of a record to the current translation unit
• Added a handler for field declarations for better consistency and less warnings
• Mark implicit constructors as implicit and correctly set their type
• Fixed a small issue of the scope manager in method definitions
• Correctly replacing the ast node of a scope if the node is "upgraded", i.e. from a function declaration to a method declaration or a constructor

In this process, I have decided to merge the DeclarationListHandler into the DeclarationHandler. Since an handler can only return Node types or List<Node> types, I have introduced the DeclarationSequence node. This node should not end up in the final graph and will be "flattened" during CPG transformation.

Closes #178

[oxisto]

Ok, I have no idea, why the CFG tests fail. They also only fail sometimes it seems :(

[oxisto]

The test design here does not make sense in my opinion. It tries to get all AST in getByLineNr using SubgraphWalker.flattenAST, including also declarations, which cannot even receive CFG edges. If then someone is a declaration on the same line as a statement and parsed before the statement it returns the declaration in this line.

[JulianSchuette]

This PR breaks half of the tests in Codyze.
Starting with commit b08930a3ce7536d53b942ead94b93c211777480f ("Minor fixes to method and constructor definitions"), various things break:

• Type.getTypeName() returning null is not expected by Codyze
• Nodes not having a code property
• Endless loops somewhere
• ...

I'd recommend to quickly test each CPG version against Codyze to see what breaks and then clearly document the changes so it gets clear what has to be done to upgrade Codyze. Currently, the API of CPG is a constantly moving target and it is very difficult to keep up as users of CPG do not know what they can rely on and no documentation is available.

[oxisto]

The main problem is, that I cannot really test codyze because for some reason, this branch leads to Fraunhofer-AISEC/codyze#37.

All changes are documented above and they should not introduce anything that "breaks", if they do, then it was always broken and probably worked around in codyze. Seems we have a little bit of a chicken-and-egg problem here.

[oxisto]

So it looks like that it all started with #164 which somehow causes the OGM wrapper within codyze to malfunction and do all kind of crazy things such as not recognising relationships or not finding property values and so on. It seems to have an issue with the annotations field within Node, which looks perfectly fine to me.

cpg/src/main/java/de/fraunhofer/aisec/cpg/graph/Node.java

Lines 118 to 119 in 6af6499

	@SubGraph("AST")
	protected List<Annotation> annotations;

[oxisto]

Two field relationships were incorrectly defines as incoming, although they should be outgoing. Together with Fraunhofer-AISEC/codyze#41 this should be enough to successfully run all tests on codyze.

[JulianSchuette]

Codyze tests are now passing, thank you. The newly DeclarationSequence (and its implication on Codyze) is not yet clear - see my other comment.

[JulianSchuette]

This is a breaking change in the CPG structure. What is the rationale behind it? Before, there was a DeclarationStatement node that had one or multiple declarations (in case of a sequence). There are even extra methods for it (DeclarationStatement.getDeclarations(), DeclarationStatement.isSingleDeclaration(), etc.)

This PR now introduces a separate node for DeclarationSequence. It is unclear how that relates to the existing structure of having 1-n declarations attached to a DeclarationStatement.

Also, the comment raises more questions: It should not end up in the final graph -> What is the final graph? As this class inherits from Declaration which is a Node, it will indeed be part of the graph. What is the point of having nodes that are not part of the graph?

[oxisto]

Declarations can exist without a declaration statement, i.e. directly attached to a translation unit. The problem is that one simple declaration in C++ can actually hold more than one declaration (for example when declaring multiple variables or a class declaration inside a variable declaration).

In the past, we tried to work around this by having two distinct handlers, a DeclarationHandler, which returns a Declaration and a DeclarationListHandler, which returns a List<Declaration>. This is a constraint, because a handler can either return a type of N or List<N> but not both in the same handler.

A C++ simple declaration was first handled by the DeclarationListHandler's handleSimpleDeclaration and then again by the DeclarationHandler's handleSimpleDeclaration, which lead to various errors - because some functionality needed to exist in both handlers (and it didn't).

My workaround thus was to create a "virtual" CPG node, called a declaration sequence, which is actually a declaration itself and can hold 0..n declarations. Referring to the final graph means that the declaration sequence's children are being added to the parent's list of declarations before translation is finished. Thus, it has no implications on codyze.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
6 Code Smells

84.4% Coverage
0.0% Duplication

Changes are approved