walkmod / walkmod-core

walkmod: an open source tool to fix coding style issues
http://www.walkmod.com
GNU Lesser General Public License v3.0
153 stars 23 forks source link

Non-Semantic rule can break semantic rules #91

Open alefhar opened 4 years ago

alefhar commented 4 years ago

Applying a non-semantic rule before a semantic rule can break that semantic rule. For the source

public class Main {

  public int get() {
    int a = 1, b = 2;
    if (a == 1) {
      if (b == 2) {
        int c = b + a;
        return c;
      }
    }
    return -1;
  }
}

the transformation chain

<walkmod>
    <chain name="default">
        <transformation type="sonar:CollapsibleIfStatements"/>
        <transformation type="sonar:RemoveUselessVariables"/>
    </chain>
</walkmod>

results in the source code

public class Main {

  public int get() {
    int a = 1, b = 2;
    if ((a == 1) && (b == 2)) {
      return c;
    }
    return -1;
  }
}

The declaration of the variable c was removed, the transformation causes compilation errors. Changing the order of the applied rules yields the correct results. I debugged the problem and the rule RemoveUselessVariables does not find any usages for the variable c. I then had a look at the rule CollapsibleIfStatements, where the then-block of the inner if is cloned and set as then-block of the parent-if:

if (stmt instanceof BlockStmt) {
  BlockStmt block = (BlockStmt) stmt;
  List<Statement> stmts = block.getStmts();
  if (stmts.size() == 1) {
    parentIf.setThenStmt(n.getThenStmt().clone());
    parentIf.setCondition(condition);
  }
}

In clone() the child statements and expressions are cloned as well. When the VariableDeclaration for c is cloned only the variable name and the definition are taken over; the usages however are not cloned (this seems to be the case for all sibling classes). The then-block then contains statements with erased semantic analysis. The block seems to get passed to the next rules which then lacks the required information.