From e681e59db7ef1643e83571d7f0f83ad4b4c5fa57 Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Sun, 3 Mar 2024 14:26:33 +0200 Subject: [PATCH 1/9] Fix & extend IfElseSameCheck --- src/dscanner/analysis/ifelsesame.d | 223 ++++++++++++++++++++++------- src/dscanner/analysis/run.d | 10 +- 2 files changed, 176 insertions(+), 57 deletions(-) diff --git a/src/dscanner/analysis/ifelsesame.d b/src/dscanner/analysis/ifelsesame.d index 13dcbe8f..c6c588b0 100644 --- a/src/dscanner/analysis/ifelsesame.d +++ b/src/dscanner/analysis/ifelsesame.d @@ -5,111 +5,228 @@ 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; /** * 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) * ) */ -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 CMP_KEY = "dscanner.bugs.comparison_operator_operands"; + private enum CMP_MESSAGE = "Left side of %s operator is identical to right side."; + + private enum ASSIGN_KEY = "dscanner.bugs.self_assignment"; + private enum ASSIGN_MESSAGE = "Left side of assignment operatior is identical to the right side."; + + 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.LogicalExp logicalExpr) { - auto e = cast(const AssignExpression) assignExpression.expression; - if (e !is null && assignExpression.operator == tok!"=" - && e.ternaryExpression == assignExpression.ternaryExpression) + super.visit(logicalExpr); + + auto expr1 = to!string(toChars(logicalExpr.e1)); + auto expr2 = to!string(toChars(logicalExpr.e2)); + + if (expr1 == expr2) { - addErrorMessage(assignExpression, "dscanner.bugs.self_assignment", - "Left side of assignment operatior is identical to the right side."); + auto lineNum = cast(ulong) logicalExpr.loc.linnum; + auto charNum = cast(ulong) logicalExpr.loc.charnum; + auto errorMsg = LOGICAL_EXP_MESSAGE.format(exprOpToString(logicalExpr.op)); + addErrorMessage(lineNum, charNum, LOGICAL_EXP_KEY, errorMsg); } - assignExpression.accept(this); } - override void visit(const AndAndExpression andAndExpression) + override void visit(AST.EqualExp equalExp) { - if (andAndExpression.left !is null && andAndExpression.right !is null - && andAndExpression.left == andAndExpression.right) + super.visit(equalExp); + + auto expr1 = to!string(toChars(equalExp.e1)); + auto expr2 = to!string(toChars(equalExp.e2)); + + if (expr1 == expr2) + { + auto lineNum = cast(ulong) equalExp.loc.linnum; + auto charNum = cast(ulong) equalExp.loc.charnum; + auto errorMsg = CMP_MESSAGE.format(exprOpToString(equalExp.op)); + addErrorMessage(lineNum, charNum, CMP_KEY, errorMsg); + } + } + + override void visit(AST.CmpExp cmpExp) + { + super.visit(cmpExp); + + auto expr1 = to!string(toChars(cmpExp.e1)); + auto expr2 = to!string(toChars(cmpExp.e2)); + + if (expr1 == expr2) + { + auto lineNum = cast(ulong) cmpExp.loc.linnum; + auto charNum = cast(ulong) cmpExp.loc.charnum; + auto errorMsg = CMP_MESSAGE.format(exprOpToString(cmpExp.op)); + addErrorMessage(lineNum, charNum, CMP_KEY, errorMsg); + } + } + + override void visit(AST.AssignExp assignExp) + { + super.visit(assignExp); + + auto expr1 = to!string(toChars(assignExp.e1)); + auto expr2 = to!string(toChars(assignExp.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) assignExp.loc.linnum; + auto charNum = cast(ulong) assignExp.loc.charnum; + addErrorMessage(lineNum, charNum, ASSIGN_KEY, ASSIGN_MESSAGE); } - andAndExpression.accept(this); } - override void visit(const OrOrExpression orOrExpression) + private extern (D) string exprOpToString(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 "or"; + case EXP.andAnd: + return "and"; + case EXP.equal: + return "'=='"; + case EXP.notEqual: + return "'!='"; + case EXP.lessThan: + return "'<'"; + case EXP.lessOrEqual: + return "'<='"; + case EXP.greaterThan: + return "'>'"; + case EXP.greaterOrEqual: + return "'>='"; + default: + return "unknown"; } - 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 testLogicalExprSame() + { + int a = 1, b = 2; + + if (a == 1 && b == 1) {} + if (a == 1 && a == 1) {} // [warn]: Left side of logical and is identical to right side. + + if (a == 1 || b == 1) {} + if (a == 1 || a == 1) {} // [warn]: Left side of logical or is identical to right side. + } + }c, sac); + + assertAnalyzerWarningsDMD(q{ + void testCmpExprSame() + { + int a = 1, b = 2; + + if (a == b) {} + if (a == a) {} // [warn]: Left side of '==' operator is identical to right side. + + if (a != b) {} + if (a != a) {} // [warn]: Left side of '!=' operator is identical to right side. + + b = a == a ? 1 : 2; // [warn]: Left side of '==' operator is identical to right side. + + if (a > b) {} + if (a > a) {} // [warn]: Left side of '>' operator is identical to right side. + + if (a < b) {} + if (a < a) {} // [warn]: Left side of '<' operator is identical to right side. + + if (a >= b) {} + if (a >= a) {} // [warn]: Left side of '>=' operator is identical to right side. + + if (a <= b) {} + if (a <= a) {} // [warn]: Left side of '<=' operator is identical to right side. + } + }c, sac); + + assertAnalyzerWarningsDMD(q{ + void testAssignSame() + { + int a = 1; + a = 5; + a = a; // [warn]: Left side of assignment operatior is identical to the right side. } }c, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void foo() { - if (auto stuff = call()) + if (auto stuff = call()) {} } }c, sac); diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index e3fc866e..58db6d02 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -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)); @@ -1345,6 +1341,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN config.redundant_storage_classes == 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); From e703fbe58db4c2def038b5473b5b127c4a3773d0 Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Sun, 3 Mar 2024 16:01:54 +0200 Subject: [PATCH 2/9] Enable debug session --- .github/workflows/default.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/default.yml b/.github/workflows/default.yml index 9e11eab2..6c8d2c90 100644 --- a/.github/workflows/default.yml +++ b/.github/workflows/default.yml @@ -98,7 +98,7 @@ jobs: # - name: Setup upterm session # if: ${{ matrix.build.type == 'make' && matrix.host == 'macos-latest'}} # uses: lhotari/action-upterm@v1 - + # Compile D-Scanner and execute all tests without dub - name: Build and test without dub if: ${{ matrix.build.type == 'make' }} @@ -174,6 +174,10 @@ jobs: repository: dlang/phobos path: phobos + - name: Setup upterm session + if: ${{ matrix.build.type == 'make' && matrix.host == 'ubuntu-22.04'}} + uses: lhotari/action-upterm@v1 + - name: Apply D-Scanner to Phobos if: ${{ matrix.build.version != 'min libdparse'}} # Older versions crash with "Invalid UTF..." working-directory: phobos From c103fee82d8600a3fcbc10a54c50adf06d0aafd3 Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Sun, 3 Mar 2024 16:28:14 +0200 Subject: [PATCH 3/9] Revert "Enable debug session" This reverts commit e703fbe58db4c2def038b5473b5b127c4a3773d0. --- .github/workflows/default.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/.github/workflows/default.yml b/.github/workflows/default.yml index 6c8d2c90..9e11eab2 100644 --- a/.github/workflows/default.yml +++ b/.github/workflows/default.yml @@ -98,7 +98,7 @@ jobs: # - name: Setup upterm session # if: ${{ matrix.build.type == 'make' && matrix.host == 'macos-latest'}} # uses: lhotari/action-upterm@v1 - + # Compile D-Scanner and execute all tests without dub - name: Build and test without dub if: ${{ matrix.build.type == 'make' }} @@ -174,10 +174,6 @@ jobs: repository: dlang/phobos path: phobos - - name: Setup upterm session - if: ${{ matrix.build.type == 'make' && matrix.host == 'ubuntu-22.04'}} - uses: lhotari/action-upterm@v1 - - name: Apply D-Scanner to Phobos if: ${{ matrix.build.version != 'min libdparse'}} # Older versions crash with "Invalid UTF..." working-directory: phobos From 3c12dd7b8e7d6acb5d2d6c6615d60bf076643983 Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Sun, 3 Mar 2024 16:49:22 +0200 Subject: [PATCH 4/9] Replace common code with template --- src/dscanner/analysis/ifelsesame.d | 104 ++++++++++------------------- 1 file changed, 35 insertions(+), 69 deletions(-) diff --git a/src/dscanner/analysis/ifelsesame.d b/src/dscanner/analysis/ifelsesame.d index c6c588b0..69682e5b 100644 --- a/src/dscanner/analysis/ifelsesame.d +++ b/src/dscanner/analysis/ifelsesame.d @@ -10,6 +10,7 @@ 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. @@ -18,6 +19,7 @@ import std.string : format; * $(LI || and && 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) * ) */ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd @@ -35,7 +37,7 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd private enum CMP_MESSAGE = "Left side of %s operator is identical to right side."; private enum ASSIGN_KEY = "dscanner.bugs.self_assignment"; - private enum ASSIGN_MESSAGE = "Left side of assignment operatior is identical to the right side."; + private enum ASSIGN_MESSAGE = "Left side of assignment operation is identical to the right side."; extern (D) this(string fileName, bool skipTests = false) { @@ -60,92 +62,56 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd } } - override void visit(AST.LogicalExp logicalExpr) - { - super.visit(logicalExpr); - - auto expr1 = to!string(toChars(logicalExpr.e1)); - auto expr2 = to!string(toChars(logicalExpr.e2)); - - if (expr1 == expr2) - { - auto lineNum = cast(ulong) logicalExpr.loc.linnum; - auto charNum = cast(ulong) logicalExpr.loc.charnum; - auto errorMsg = LOGICAL_EXP_MESSAGE.format(exprOpToString(logicalExpr.op)); - addErrorMessage(lineNum, charNum, LOGICAL_EXP_KEY, errorMsg); - } - } + mixin VisitBinaryExpression!(AST.LogicalExp); + mixin VisitBinaryExpression!(AST.EqualExp); + mixin VisitBinaryExpression!(AST.CmpExp); + mixin VisitBinaryExpression!(AST.AssignExp); - override void visit(AST.EqualExp equalExp) + private template VisitBinaryExpression(NodeType) { - super.visit(equalExp); - - auto expr1 = to!string(toChars(equalExp.e1)); - auto expr2 = to!string(toChars(equalExp.e2)); - - if (expr1 == expr2) + override void visit(NodeType node) { - auto lineNum = cast(ulong) equalExp.loc.linnum; - auto charNum = cast(ulong) equalExp.loc.charnum; - auto errorMsg = CMP_MESSAGE.format(exprOpToString(equalExp.op)); - addErrorMessage(lineNum, charNum, CMP_KEY, errorMsg); + super.visit(node); + + auto expr1 = to!string(toChars(node.e1)); + auto expr2 = to!string(toChars(node.e2)); + + if (expr1 == expr2) + { + auto lineNum = cast(ulong) node.loc.linnum; + auto charNum = cast(ulong) node.loc.charnum; + auto errorInfo = getErrorInfo(node.op); + addErrorMessage(lineNum, charNum, errorInfo[0], errorInfo[1]); + } } } - override void visit(AST.CmpExp cmpExp) - { - super.visit(cmpExp); - - auto expr1 = to!string(toChars(cmpExp.e1)); - auto expr2 = to!string(toChars(cmpExp.e2)); - - if (expr1 == expr2) - { - auto lineNum = cast(ulong) cmpExp.loc.linnum; - auto charNum = cast(ulong) cmpExp.loc.charnum; - auto errorMsg = CMP_MESSAGE.format(exprOpToString(cmpExp.op)); - addErrorMessage(lineNum, charNum, CMP_KEY, errorMsg); - } - } - - override void visit(AST.AssignExp assignExp) - { - super.visit(assignExp); - - auto expr1 = to!string(toChars(assignExp.e1)); - auto expr2 = to!string(toChars(assignExp.e2)); - - if (expr1 == expr2) - { - auto lineNum = cast(ulong) assignExp.loc.linnum; - auto charNum = cast(ulong) assignExp.loc.charnum; - addErrorMessage(lineNum, charNum, ASSIGN_KEY, ASSIGN_MESSAGE); - } - } - - private extern (D) string exprOpToString(EXP op) + private extern (D) Tuple!(string, string) getErrorInfo(EXP op) { switch (op) { case EXP.orOr: - return "or"; + return tuple(LOGICAL_EXP_KEY, LOGICAL_EXP_MESSAGE.format("or")); case EXP.andAnd: - return "and"; + return tuple(LOGICAL_EXP_KEY, LOGICAL_EXP_MESSAGE.format("and")); case EXP.equal: - return "'=='"; + return tuple(CMP_KEY, CMP_MESSAGE.format("'=='")); case EXP.notEqual: - return "'!='"; + return tuple(CMP_KEY, CMP_MESSAGE.format("'!='")); case EXP.lessThan: - return "'<'"; + return tuple(CMP_KEY, CMP_MESSAGE.format("'<'")); case EXP.lessOrEqual: - return "'<='"; + return tuple(CMP_KEY, CMP_MESSAGE.format("'<='")); case EXP.greaterThan: - return "'>'"; + return tuple(CMP_KEY, CMP_MESSAGE.format("'>'")); case EXP.greaterOrEqual: - return "'>='"; + return tuple(CMP_KEY, CMP_MESSAGE.format("'>='")); + case EXP.assign: + return tuple(ASSIGN_KEY, ASSIGN_MESSAGE); default: - return "unknown"; + assert(0); } + } } @@ -219,7 +185,7 @@ unittest { int a = 1; a = 5; - a = a; // [warn]: Left side of assignment operatior is identical to the right side. + a = a; // [warn]: Left side of assignment operation is identical to the right side. } }c, sac); From 11e1dbf4935e492c18bb837611822df8bbb12efd Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Sun, 3 Mar 2024 17:00:38 +0200 Subject: [PATCH 5/9] Investigate failing workflows --- src/dscanner/analysis/ifelsesame.d | 76 +++++++++++++++--------------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/src/dscanner/analysis/ifelsesame.d b/src/dscanner/analysis/ifelsesame.d index 69682e5b..a0940a08 100644 --- a/src/dscanner/analysis/ifelsesame.d +++ b/src/dscanner/analysis/ifelsesame.d @@ -63,9 +63,9 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd } mixin VisitBinaryExpression!(AST.LogicalExp); - mixin VisitBinaryExpression!(AST.EqualExp); - mixin VisitBinaryExpression!(AST.CmpExp); - mixin VisitBinaryExpression!(AST.AssignExp); + //mixin VisitBinaryExpression!(AST.EqualExp); + //mixin VisitBinaryExpression!(AST.CmpExp); + //mixin VisitBinaryExpression!(AST.AssignExp); private template VisitBinaryExpression(NodeType) { @@ -153,41 +153,41 @@ unittest } }c, sac); - assertAnalyzerWarningsDMD(q{ - void testCmpExprSame() - { - int a = 1, b = 2; - - if (a == b) {} - if (a == a) {} // [warn]: Left side of '==' operator is identical to right side. - - if (a != b) {} - if (a != a) {} // [warn]: Left side of '!=' operator is identical to right side. - - b = a == a ? 1 : 2; // [warn]: Left side of '==' operator is identical to right side. - - if (a > b) {} - if (a > a) {} // [warn]: Left side of '>' operator is identical to right side. - - if (a < b) {} - if (a < a) {} // [warn]: Left side of '<' operator is identical to right side. - - if (a >= b) {} - if (a >= a) {} // [warn]: Left side of '>=' operator is identical to right side. - - if (a <= b) {} - if (a <= a) {} // [warn]: Left side of '<=' operator is identical to right side. - } - }c, sac); - - assertAnalyzerWarningsDMD(q{ - void testAssignSame() - { - int a = 1; - a = 5; - a = a; // [warn]: Left side of assignment operation is identical to the right side. - } - }c, sac); + //assertAnalyzerWarningsDMD(q{ + // void testCmpExprSame() + // { + // int a = 1, b = 2; + // + // if (a == b) {} + // if (a == a) {} // [warn]: Left side of '==' operator is identical to right side. + // + // if (a != b) {} + // if (a != a) {} // [warn]: Left side of '!=' operator is identical to right side. + // + // b = a == a ? 1 : 2; // [warn]: Left side of '==' operator is identical to right side. + // + // if (a > b) {} + // if (a > a) {} // [warn]: Left side of '>' operator is identical to right side. + // + // if (a < b) {} + // if (a < a) {} // [warn]: Left side of '<' operator is identical to right side. + // + // if (a >= b) {} + // if (a >= a) {} // [warn]: Left side of '>=' operator is identical to right side. + // + // if (a <= b) {} + // if (a <= a) {} // [warn]: Left side of '<=' operator is identical to right side. + // } + //}c, sac); + + //assertAnalyzerWarningsDMD(q{ + // void testAssignSame() + // { + // int a = 1; + // a = 5; + // a = a; // [warn]: Left side of assignment operation is identical to the right side. + // } + //}c, sac); assertAnalyzerWarningsDMD(q{ void foo() From fd91794430197dc169cfd6007565cf7c44d00e99 Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Thu, 7 Mar 2024 18:49:03 +0200 Subject: [PATCH 6/9] Revert "Investigate failing workflows" This reverts commit 11e1dbf4935e492c18bb837611822df8bbb12efd. --- src/dscanner/analysis/ifelsesame.d | 76 +++++++++++++++--------------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/src/dscanner/analysis/ifelsesame.d b/src/dscanner/analysis/ifelsesame.d index a0940a08..69682e5b 100644 --- a/src/dscanner/analysis/ifelsesame.d +++ b/src/dscanner/analysis/ifelsesame.d @@ -63,9 +63,9 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd } mixin VisitBinaryExpression!(AST.LogicalExp); - //mixin VisitBinaryExpression!(AST.EqualExp); - //mixin VisitBinaryExpression!(AST.CmpExp); - //mixin VisitBinaryExpression!(AST.AssignExp); + mixin VisitBinaryExpression!(AST.EqualExp); + mixin VisitBinaryExpression!(AST.CmpExp); + mixin VisitBinaryExpression!(AST.AssignExp); private template VisitBinaryExpression(NodeType) { @@ -153,41 +153,41 @@ unittest } }c, sac); - //assertAnalyzerWarningsDMD(q{ - // void testCmpExprSame() - // { - // int a = 1, b = 2; - // - // if (a == b) {} - // if (a == a) {} // [warn]: Left side of '==' operator is identical to right side. - // - // if (a != b) {} - // if (a != a) {} // [warn]: Left side of '!=' operator is identical to right side. - // - // b = a == a ? 1 : 2; // [warn]: Left side of '==' operator is identical to right side. - // - // if (a > b) {} - // if (a > a) {} // [warn]: Left side of '>' operator is identical to right side. - // - // if (a < b) {} - // if (a < a) {} // [warn]: Left side of '<' operator is identical to right side. - // - // if (a >= b) {} - // if (a >= a) {} // [warn]: Left side of '>=' operator is identical to right side. - // - // if (a <= b) {} - // if (a <= a) {} // [warn]: Left side of '<=' operator is identical to right side. - // } - //}c, sac); - - //assertAnalyzerWarningsDMD(q{ - // void testAssignSame() - // { - // int a = 1; - // a = 5; - // a = a; // [warn]: Left side of assignment operation is identical to the right side. - // } - //}c, sac); + assertAnalyzerWarningsDMD(q{ + void testCmpExprSame() + { + int a = 1, b = 2; + + if (a == b) {} + if (a == a) {} // [warn]: Left side of '==' operator is identical to right side. + + if (a != b) {} + if (a != a) {} // [warn]: Left side of '!=' operator is identical to right side. + + b = a == a ? 1 : 2; // [warn]: Left side of '==' operator is identical to right side. + + if (a > b) {} + if (a > a) {} // [warn]: Left side of '>' operator is identical to right side. + + if (a < b) {} + if (a < a) {} // [warn]: Left side of '<' operator is identical to right side. + + if (a >= b) {} + if (a >= a) {} // [warn]: Left side of '>=' operator is identical to right side. + + if (a <= b) {} + if (a <= a) {} // [warn]: Left side of '<=' operator is identical to right side. + } + }c, sac); + + assertAnalyzerWarningsDMD(q{ + void testAssignSame() + { + int a = 1; + a = 5; + a = a; // [warn]: Left side of assignment operation is identical to the right side. + } + }c, sac); assertAnalyzerWarningsDMD(q{ void foo() From 8229d32ba83d79a78ba561df5b198d0d8ba39753 Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Sun, 17 Mar 2024 17:05:24 +0200 Subject: [PATCH 7/9] Remove check extension --- src/dscanner/analysis/ifelsesame.d | 63 ++++-------------------------- 1 file changed, 8 insertions(+), 55 deletions(-) diff --git a/src/dscanner/analysis/ifelsesame.d b/src/dscanner/analysis/ifelsesame.d index 69682e5b..0bbf7341 100644 --- a/src/dscanner/analysis/ifelsesame.d +++ b/src/dscanner/analysis/ifelsesame.d @@ -33,9 +33,6 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd 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 CMP_KEY = "dscanner.bugs.comparison_operator_operands"; - private enum CMP_MESSAGE = "Left side of %s operator 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."; @@ -63,8 +60,6 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd } mixin VisitBinaryExpression!(AST.LogicalExp); - mixin VisitBinaryExpression!(AST.EqualExp); - mixin VisitBinaryExpression!(AST.CmpExp); mixin VisitBinaryExpression!(AST.AssignExp); private template VisitBinaryExpression(NodeType) @@ -94,18 +89,6 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd 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.equal: - return tuple(CMP_KEY, CMP_MESSAGE.format("'=='")); - case EXP.notEqual: - return tuple(CMP_KEY, CMP_MESSAGE.format("'!='")); - case EXP.lessThan: - return tuple(CMP_KEY, CMP_MESSAGE.format("'<'")); - case EXP.lessOrEqual: - return tuple(CMP_KEY, CMP_MESSAGE.format("'<='")); - case EXP.greaterThan: - return tuple(CMP_KEY, CMP_MESSAGE.format("'>'")); - case EXP.greaterOrEqual: - return tuple(CMP_KEY, CMP_MESSAGE.format("'>='")); case EXP.assign: return tuple(ASSIGN_KEY, ASSIGN_MESSAGE); default: @@ -141,50 +124,20 @@ unittest }c, sac); assertAnalyzerWarningsDMD(q{ - void testLogicalExprSame() - { - int a = 1, b = 2; - - if (a == 1 && b == 1) {} - if (a == 1 && a == 1) {} // [warn]: Left side of logical and is identical to right side. - - if (a == 1 || b == 1) {} - if (a == 1 || a == 1) {} // [warn]: Left side of logical or is identical to right side. - } - }c, sac); - - assertAnalyzerWarningsDMD(q{ - void testCmpExprSame() + void testLogicalExp() { - int a = 1, b = 2; - - if (a == b) {} - if (a == a) {} // [warn]: Left side of '==' operator is identical to right side. - - if (a != b) {} - if (a != a) {} // [warn]: Left side of '!=' operator is identical to right side. - - b = a == a ? 1 : 2; // [warn]: Left side of '==' operator is identical to right side. - - if (a > b) {} - if (a > a) {} // [warn]: Left side of '>' operator is identical to right side. - - if (a < b) {} - if (a < a) {} // [warn]: Left side of '<' operator is identical to right side. - - if (a >= b) {} - if (a >= a) {} // [warn]: Left side of '>=' operator is identical to right side. - - if (a <= b) {} - if (a <= a) {} // [warn]: Left side of '<=' operator is identical to right side. + 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 testAssignSame() + void testAssignExp() { - int a = 1; - a = 5; + int a = 5; a = a; // [warn]: Left side of assignment operation is identical to the right side. } }c, sac); From 04376c0e90935623c1392e034f1b5ac159996c70 Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Sun, 17 Mar 2024 17:19:45 +0200 Subject: [PATCH 8/9] Trigger assign error only for the ternary operator --- src/dscanner/analysis/ifelsesame.d | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dscanner/analysis/ifelsesame.d b/src/dscanner/analysis/ifelsesame.d index 0bbf7341..90737ab5 100644 --- a/src/dscanner/analysis/ifelsesame.d +++ b/src/dscanner/analysis/ifelsesame.d @@ -60,7 +60,7 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd } mixin VisitBinaryExpression!(AST.LogicalExp); - mixin VisitBinaryExpression!(AST.AssignExp); + mixin VisitBinaryExpression!(AST.CondExp); private template VisitBinaryExpression(NodeType) { @@ -89,7 +89,7 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd 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.assign: + case EXP.question: return tuple(ASSIGN_KEY, ASSIGN_MESSAGE); default: assert(0); @@ -137,8 +137,8 @@ unittest assertAnalyzerWarningsDMD(q{ void testAssignExp() { - int a = 5; - a = a; // [warn]: Left side of assignment operation is identical to the right side. + int a = 5, b = 5; + a = b > 5 ? b : b; // [warn]: Left side of assignment operation is identical to the right side. } }c, sac); From 09415fb5b78c1cfc184a511bad3d925c482331b6 Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Sun, 17 Mar 2024 17:42:13 +0200 Subject: [PATCH 9/9] Fix assignment error --- src/dscanner/analysis/ifelsesame.d | 49 +++++++++++++++++++----------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/src/dscanner/analysis/ifelsesame.d b/src/dscanner/analysis/ifelsesame.d index 90737ab5..ae9a6e7b 100644 --- a/src/dscanner/analysis/ifelsesame.d +++ b/src/dscanner/analysis/ifelsesame.d @@ -36,6 +36,8 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd 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(fileName, skipTests); @@ -59,25 +61,38 @@ extern (C++) class IfElseSameCheck(AST) : BaseAnalyzerDmd } } - mixin VisitBinaryExpression!(AST.LogicalExp); - mixin VisitBinaryExpression!(AST.CondExp); + override void visit(AST.AssignExp assignExp) + { + bool oldInAssignment = inAssignment; + inAssignment = true; + super.visit(assignExp); + inAssignment = oldInAssignment; + } + + override void visit(AST.CondExp condExp) + { + super.visit(condExp); + if (inAssignment) + handleBinaryExpression(condExp); + } + + override void visit(AST.LogicalExp logicalExpr) + { + super.visit(logicalExpr); + handleBinaryExpression(logicalExpr); + } - private template VisitBinaryExpression(NodeType) + private void handleBinaryExpression(AST.BinExp expr) { - override void visit(NodeType node) + auto expr1 = to!string(toChars(expr.e1)); + auto expr2 = to!string(toChars(expr.e2)); + + if (expr1 == expr2) { - super.visit(node); - - auto expr1 = to!string(toChars(node.e1)); - auto expr2 = to!string(toChars(node.e2)); - - if (expr1 == expr2) - { - auto lineNum = cast(ulong) node.loc.linnum; - auto charNum = cast(ulong) node.loc.charnum; - auto errorInfo = getErrorInfo(node.op); - addErrorMessage(lineNum, charNum, errorInfo[0], errorInfo[1]); - } + 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]); } } @@ -138,7 +153,7 @@ unittest void testAssignExp() { int a = 5, b = 5; - a = b > 5 ? b : b; // [warn]: Left side of assignment operation is identical to the right side. + a = b > 5 ? b : b; // [warn]: Left side of assignment operation is identical to the right side. } }c, sac);