sonar-delphi icon indicating copy to clipboard operation
sonar-delphi copied to clipboard

Add control flow graph implementation

Open jgardn3r opened this issue 1 year ago • 2 comments

This PR adds the notion of a control flow graph. Control flow graphs are a stepping stone in data flow analysis and symbolic execution.

With just the control flow graph alone, two new rules could be implemented:

  • RedundantJump
  • LoopExecutingAtMostOnce

In addition to that, some more API traversal methods were added:

  • RepeatStatementNode::getGuardExpression
  • RepeatStatementNode::getStatements
  • CaseStatementNode::getSelectorExpression
  • CaseItemStatementNode::getStatement

The thinking is that the control flow graph will be internal for a while before being exposed in the stable API. In light of this, I have made a ControlFlowGraph interface in the API, but there are no public ways to create/interact with it.

jgardn3r avatar Aug 07 '24 05:08 jgardn3r

I didn't respond to each review comment because many things have changed and it is practically a rewrite. I have tried to:

  1. Improve documentation and explanatory comments
  2. Use immutability
  3. Create an API that isn't public

jgardn3r avatar Aug 19 '24 03:08 jgardn3r

There are a few aspects of the current shape that I am not completely happy with yet that I think would be worth discussing/brainstorming:

  • The modelling of the successors
    • getSuccessors()
      • Returns an interface that allows you to check what kind of successors there are
      • Is this worth it?
      • This replaces the sonar-java approach of having a flag for a bunch of different things, e.g., isFinallyBlock, isCatchBlock
    • getSuccessorBlocks()
      • This is the successor-type agnostic way of getting all of the successor blocks
      • Generically useful for traversing blocks in rules, etc.
    • Object reference permanence - not necessarily a bad thing, just worth noting
      • When constructing the blocks the successors are treated as immutable
      • If there is a change to the successors a new successor object is used to replace
      • Blocks reference each other using address, rather than ID
    • An idea I had is only making the strongly coupled object as the end and using IDs internally

jgardn3r avatar Sep 30 '24 05:09 jgardn3r

Also, I'm not sure if you've noticed @jgardn3r but there are still a few unresolved conversations from @Cirras 's first round of review.

fourls avatar Jan 21 '25 00:01 fourls

Went a little overzealous with the delete button and failed to compile again. All @Cirras comments should be addressed now.

jgardn3r avatar Feb 12 '25 22:02 jgardn3r