Skip to content

Commit

Permalink
Replace libdparse in IfElseSameCheck (#81)
Browse files Browse the repository at this point in the history 
* Fix & extend IfElseSameCheck

* Enable debug session

* Revert "Enable debug session"

This reverts commit e703fbe.

* Replace common code with template

* Investigate failing workflows

* Revert "Investigate failing workflows"

This reverts commit 11e1dbf.

* Remove check extension

* Trigger assign error only for the ternary operator

* Fix assignment error
  • Loading branch information
@Vladiwostok
Vladiwostok authored Mar 18, 2024
1 parent a805955 commit a3c4736
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 59 deletions.
161 changes: 106 additions & 55 deletions src/dscanner/analysis/ifelsesame.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,111 +5,162 @@

module dscanner.analysis.ifelsesame;

import std.stdio;
import dparse.ast;
import dparse.lexer;
import dscanner.analysis.base;
import dscanner.analysis.helpers;
import dsymbol.scope_ : Scope;
import dmd.hdrgen : toChars;
import dmd.tokens : EXP;
import std.conv : to;
import std.string : format;
import std.typecons : Tuple, tuple;

/**
* Checks for duplicated code in conditional and logical expressions.
* $(UL
* $(LI If statements whose "then" block is the same as the "else" block)
* $(LI || and && expressions where the left and right are the same)
* $(LI == expressions where the left and right are the same)
* $(LI == and != expressions where the left and right are the same)
* $(LI >, <, >=, and <= expressions where the left and right are the same)
* $(LI Assignments where the left and right are the same)
* )
*/
final class IfElseSameCheck : BaseAnalyzer
extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd
{
alias visit = BaseAnalyzer.visit;

alias visit = BaseAnalyzerDmd.visit;
mixin AnalyzerInfo!"if_else_same_check";

this(BaseAnalyzerArguments args)
private enum IF_KEY = "dscanner.bugs.if_else_same";
private enum IF_MESSAGE = "'Else' branch is identical to 'Then' branch.";

private enum LOGICAL_EXP_KEY = "dscanner.bugs.logic_operator_operands";
private enum LOGICAL_EXP_MESSAGE = "Left side of logical %s is identical to right side.";

private enum ASSIGN_KEY = "dscanner.bugs.self_assignment";
private enum ASSIGN_MESSAGE = "Left side of assignment operation is identical to the right side.";

private bool inAssignment = false;

extern (D) this(string fileName, bool skipTests = false)
{
super(args);
super(fileName, skipTests);
}

override void visit(const IfStatement ifStatement)
override void visit(AST.IfStatement ifStatement)
{
if (ifStatement.thenStatement && (ifStatement.thenStatement == ifStatement.elseStatement))
super.visit(ifStatement);

if (ifStatement.ifbody is null || ifStatement.elsebody is null)
return;

auto thenBody = to!string(toChars(ifStatement.ifbody));
auto elseBody = to!string(toChars(ifStatement.elsebody));

if (thenBody == elseBody)
{
const(Token)[] tokens = ifStatement.elseStatement.tokens;
// extend 1 past, so we include the `else` token
tokens = (tokens.ptr - 1)[0 .. tokens.length + 1];
addErrorMessage(tokens,
"dscanner.bugs.if_else_same", "'Else' branch is identical to 'Then' branch.");
auto lineNum = cast(ulong) ifStatement.loc.linnum;
auto charNum = cast(ulong) ifStatement.loc.charnum;
addErrorMessage(lineNum, charNum, IF_KEY, IF_MESSAGE);
}
ifStatement.accept(this);
}

override void visit(const AssignExpression assignExpression)
override void visit(AST.AssignExp assignExp)
{
auto e = cast(const AssignExpression) assignExpression.expression;
if (e !is null && assignExpression.operator == tok!"="
&& e.ternaryExpression == assignExpression.ternaryExpression)
{
addErrorMessage(assignExpression, "dscanner.bugs.self_assignment",
"Left side of assignment operatior is identical to the right side.");
}
assignExpression.accept(this);
bool oldInAssignment = inAssignment;
inAssignment = true;
super.visit(assignExp);
inAssignment = oldInAssignment;
}

override void visit(const AndAndExpression andAndExpression)
override void visit(AST.CondExp condExp)
{
if (andAndExpression.left !is null && andAndExpression.right !is null
&& andAndExpression.left == andAndExpression.right)
super.visit(condExp);
if (inAssignment)
handleBinaryExpression(condExp);
}

override void visit(AST.LogicalExp logicalExpr)
{
super.visit(logicalExpr);
handleBinaryExpression(logicalExpr);
}

private void handleBinaryExpression(AST.BinExp expr)
{
auto expr1 = to!string(toChars(expr.e1));
auto expr2 = to!string(toChars(expr.e2));

if (expr1 == expr2)
{
addErrorMessage(andAndExpression.right,
"dscanner.bugs.logic_operator_operands",
"Left side of logical and is identical to right side.");
auto lineNum = cast(ulong) expr.loc.linnum;
auto charNum = cast(ulong) expr.loc.charnum;
auto errorInfo = getErrorInfo(expr.op);
addErrorMessage(lineNum, charNum, errorInfo[0], errorInfo[1]);
}
andAndExpression.accept(this);
}

override void visit(const OrOrExpression orOrExpression)
private extern (D) Tuple!(string, string) getErrorInfo(EXP op)
{
if (orOrExpression.left !is null && orOrExpression.right !is null
&& orOrExpression.left == orOrExpression.right)
switch (op)
{
addErrorMessage(orOrExpression.right,
"dscanner.bugs.logic_operator_operands",
"Left side of logical or is identical to right side.");
case EXP.orOr:
return tuple(LOGICAL_EXP_KEY, LOGICAL_EXP_MESSAGE.format("or"));
case EXP.andAnd:
return tuple(LOGICAL_EXP_KEY, LOGICAL_EXP_MESSAGE.format("and"));
case EXP.question:
return tuple(ASSIGN_KEY, ASSIGN_MESSAGE);
default:
assert(0);
}
orOrExpression.accept(this);

}
}

unittest
{
import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig;
import dscanner.analysis.helpers : assertAnalyzerWarningsDMD;
import std.stdio : stderr;

StaticAnalysisConfig sac = disabledConfig();
sac.if_else_same_check = Check.enabled;
assertAnalyzerWarnings(q{
void testSizeT()

assertAnalyzerWarningsDMD(q{
void testThenElseSame()
{
string person = "unknown";
if (person == "unknown")
person = "bobrick"; /* same */
if (person == "unknown") // [warn]: 'Else' branch is identical to 'Then' branch.
person = "bobrick";
else
person = "bobrick"; /* same */ /+
^^^^^^^^^^^^^^^^^^^^^^^ [warn]: 'Else' branch is identical to 'Then' branch. +/
// note: above ^^^ line spans over multiple lines, so it starts at start of line, since we don't have any way to test this here
// look at the tests using 1-wide tab width for accurate visuals.
person = "bobrick";

if (person == "unknown") // ok
person = "ricky"; // not same
if (person == "unknown")
person = "ricky";
else
person = "bobby"; // not same
person = "bobby";
}
}c, sac);

assertAnalyzerWarningsDMD(q{
void testLogicalExp()
{
int a = 5, b = 5;
if (a == b || a == b) // [warn]: Left side of logical or is identical to right side.
a = 6;
if (a == b && a == b) // [warn]: Left side of logical and is identical to right side.
a = 6;
}
}c, sac);

assertAnalyzerWarningsDMD(q{
void testAssignExp()
{
int a = 5, b = 5;
a = b > 5 ? b : b; // [warn]: Left side of assignment operation is identical to the right side.
}
}c, sac);

assertAnalyzerWarnings(q{
assertAnalyzerWarningsDMD(q{
void foo()
{
if (auto stuff = call())
if (auto stuff = call()) {}
}
}c, sac);

Expand Down
10 changes: 6 additions & 4 deletions src/dscanner/analysis/run.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -843,10 +843,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
checks ~= new FunctionAttributeCheck(args.setSkipTests(
analysisConfig.function_attribute_check == Check.skipTests && !ut));

if (moduleName.shouldRun!IfElseSameCheck(analysisConfig))
checks ~= new IfElseSameCheck(args.setSkipTests(
analysisConfig.if_else_same_check == Check.skipTests&& !ut));

if (moduleName.shouldRun!LabelVarNameCheck(analysisConfig))
checks ~= new LabelVarNameCheck(args.setSkipTests(
analysisConfig.label_var_same_name_check == Check.skipTests && !ut));
Expand Down Expand Up @@ -1347,6 +1343,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN
config.number_style_check == Check.skipTests && !ut
);

if (moduleName.shouldRunDmd!(IfElseSameCheck!ASTCodegen)(config))
visitors ~= new IfElseSameCheck!ASTCodegen(
fileName,
config.if_else_same_check == Check.skipTests && !ut
);

foreach (visitor; visitors)
{
m.accept(visitor);
Expand Down

0 comments on commit a3c4736

Please sign in to comment.