mirror of
https://github.com/dlang-community/D-Scanner.git
synced 2025-04-26 13:20:07 +03:00
Replace libdparse in IfElseSameCheck (#81)
* Fix & extend IfElseSameCheck * Enable debug session * Revert "Enable debug session" This reverts commit e703fbe58db4c2def038b5473b5b127c4a3773d0. * Replace common code with template * Investigate failing workflows * Revert "Investigate failing workflows" This reverts commit 11e1dbf4935e492c18bb837611822df8bbb12efd. * Remove check extension * Trigger assign error only for the ternary operator * Fix assignment error
This commit is contained in:
Vladiwostok
Vladiwostok
parent
8b7612d76a
commit
0ac05511e3
2 changed files with 114 additions and 67 deletions
|
|
@ -5,117 +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,
|
||||
IF_ELSE_SAME_KEY, "'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, SELF_ASSIGNMENT_KEY,
|
||||
"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)
|
||||
{
|
||||
addErrorMessage(andAndExpression.right,
|
||||
LOGIC_OPERATOR_OPERANDS_KEY,
|
||||
"Left side of logical and is identical to right side.");
|
||||
}
|
||||
andAndExpression.accept(this);
|
||||
super.visit(condExp);
|
||||
if (inAssignment)
|
||||
handleBinaryExpression(condExp);
|
||||
}
|
||||
|
||||
override void visit(const OrOrExpression orOrExpression)
|
||||
override void visit(AST.LogicalExp logicalExpr)
|
||||
{
|
||||
if (orOrExpression.left !is null && orOrExpression.right !is null
|
||||
&& orOrExpression.left == orOrExpression.right)
|
||||
{
|
||||
addErrorMessage(orOrExpression.right,
|
||||
LOGIC_OPERATOR_OPERANDS_KEY,
|
||||
"Left side of logical or is identical to right side.");
|
||||
}
|
||||
orOrExpression.accept(this);
|
||||
super.visit(logicalExpr);
|
||||
handleBinaryExpression(logicalExpr);
|
||||
}
|
||||
|
||||
private:
|
||||
private void handleBinaryExpression(AST.BinExp expr)
|
||||
{
|
||||
auto expr1 = to!string(toChars(expr.e1));
|
||||
auto expr2 = to!string(toChars(expr.e2));
|
||||
|
||||
enum string IF_ELSE_SAME_KEY = "dscanner.bugs.if_else_same";
|
||||
enum string SELF_ASSIGNMENT_KEY = "dscanner.bugs.self_assignment";
|
||||
enum string LOGIC_OPERATOR_OPERANDS_KEY = "dscanner.bugs.logic_operator_operands";
|
||||
if (expr1 == expr2)
|
||||
{
|
||||
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]);
|
||||
}
|
||||
}
|
||||
|
||||
private extern (D) Tuple!(string, string) getErrorInfo(EXP op)
|
||||
{
|
||||
switch (op)
|
||||
{
|
||||
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);
|
||||
}
|
||||
|
||||
}
|
||||
}
|
||||
|
||||
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);
|
||||
|
||||
assertAnalyzerWarnings(q{
|
||||
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);
|
||||
|
||||
assertAnalyzerWarningsDMD(q{
|
||||
void foo()
|
||||
{
|
||||
if (auto stuff = call())
|
||||
if (auto stuff = call()) {}
|
||||
}
|
||||
}c, sac);
|
||||
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
|
|
@ -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);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue