Conversation
cirras
left a comment
cirras
left a comment
There was a problem hiding this comment.
Some feedback items to address, but this is looking great. Looking forward to getting this new rule into the analysis.
| import org.sonar.plugins.communitydelphi.api.ast.DelphiNode; | ||
| import org.sonar.plugins.communitydelphi.api.symbol.declaration.NameDeclaration; | ||
|
|
||
| public interface LiveVariable extends DelphiNode { |
There was a problem hiding this comment.
Does this really need to extend the whole DelphiNode interface?
There was a problem hiding this comment.
I think I can improve the modelling a little to remove this extension. I will have a go.
| @@ -0,0 +1,19 @@ | |||
| { | |||
| "title": "Redundant assignments should be removed.", | |||
| "type": "BUG", | |||
There was a problem hiding this comment.
Redundant assignments are useless, but they don't impact reliability and they're not bugs.
There was a problem hiding this comment.
Does that make it a "CODE_SMELL"?
| @Test | ||
| void testRepeatedAssignmentsInSameBlockShouldAddIssue() { | ||
| DelphiTestUnitBuilder unitBuilder = | ||
| new DelphiTestUnitBuilder() | ||
| .appendImpl("procedure Test;") | ||
| .appendImpl("begin") | ||
| .appendImpl(" var A := 10; // Noncompliant") | ||
| .appendImpl(" A := 11; // Noncompliant") | ||
| .appendImpl(" A := 12;") | ||
| .appendImpl(" Writeln(A);") | ||
| .appendImpl("end;"); | ||
|
|
||
| CheckVerifier.newVerifier().withCheck(new DeadStoreCheck()).onFile(unitBuilder).verifyIssues(); | ||
| } |
There was a problem hiding this comment.
Not terribly important, but this all-in-one pattern is more common in the project.
| @Test | |
| void testRepeatedAssignmentsInSameBlockShouldAddIssue() { | |
| DelphiTestUnitBuilder unitBuilder = | |
| new DelphiTestUnitBuilder() | |
| .appendImpl("procedure Test;") | |
| .appendImpl("begin") | |
| .appendImpl(" var A := 10; // Noncompliant") | |
| .appendImpl(" A := 11; // Noncompliant") | |
| .appendImpl(" A := 12;") | |
| .appendImpl(" Writeln(A);") | |
| .appendImpl("end;"); | |
| CheckVerifier.newVerifier().withCheck(new DeadStoreCheck()).onFile(unitBuilder).verifyIssues(); | |
| } | |
| @Test | |
| void testRepeatedAssignmentsInSameBlockShouldAddIssue() { | |
| CheckVerifier.newVerifier() | |
| .withCheck(new DeadStoreCheck()) | |
| .onFile( | |
| new DelphiTestUnitBuilder() | |
| .appendImpl("procedure Test;") | |
| .appendImpl("begin") | |
| .appendImpl(" var A := 10; // Noncompliant") | |
| .appendImpl(" A := 11; // Noncompliant") | |
| .appendImpl(" A := 12;") | |
| .appendImpl(" Writeln(A);") | |
| .appendImpl("end;")) | |
| .verifyIssues(); | |
| } |
There was a problem hiding this comment.
Fair. Most other instances of this pattern were me, too. Would you prefer them all to be the all-in-one approach?
| .appendImpl("procedure Test;") | ||
| .appendImpl("begin") | ||
| .appendImpl(" for var I := 1 to 10 do begin") | ||
| .appendImpl(" I := 10; // Noncompliant") |
There was a problem hiding this comment.
This is kind of a weird dicey one, isn't it? What if we're at the end of the final loop?
There was a problem hiding this comment.
It is a bit of a dicey case. However, it is actually a non-issue. I realise now that I never tried to compile this code. It is actually a compiler error.
E2081 Assignment to FOR-Loop variable 'I'
In this case, I think it is probably fine to leave the test. But you could also argue that it should just be deleted as invalid code.
There was a problem hiding this comment.
I'd argue it should be deleted in that case.
|
Oops, I didn't mean to commit the test that failed CI. |
Raise statements have an optional `at expression` which should be accessible through the API.
A control flow graph is tied more closely to a statement list, rather than a routine implementation. That is because all statement lists can have a control flow graph, particularly `initialization` and `finalization` sections.
This provides the node the control flow graph was constructed from. This will allow consumers to reason about the surrounding context, e.g., subroutines, when using a `ControlFlowGraph`.
2 participants
This PR implements #455.
There are two main parts to this PR. Other than the Check, nothing is slated to be added to the public API; everything remains internal.
1. Live variable analysis
The
BlockDataFlowVisitorinspects a single Block, notifying its call site of usages and assignments to variables.LocalDataFlowPropertiesrepresent what variables are assigned or used before assignment for a block. This is a fundamental building block for live variable analysis.LiveVariablescontain the information about which variables are alive coming in and going out of a given block. Additionally, there is some other information about the entire graph that comes in handy when implementing the dead store check.To support these changes, a
LiveVariableinterface was added to abstract across name declarations and references.2. Dead store check
This is the first check that uses variable liveness to add issues. High-level, this check raises issues on assignments of variables that aren't subsequently used, either by going out of scope or being reassigned.
Other changes
Some other changes were made in service of improving the modelling of control flow graphs.