From 2cbc6848037146f6eb2aaa33913d9e50d0f92534 Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Fri, 29 Mar 2024 20:59:14 +0200 Subject: [PATCH 1/2] Replace libdparse with DMD in LabelVarNameCheck --- .../analysis/label_var_same_name_check.d | 207 ++++++++++++------ src/dscanner/analysis/run.d | 10 +- 2 files changed, 149 insertions(+), 68 deletions(-) diff --git a/src/dscanner/analysis/label_var_same_name_check.d b/src/dscanner/analysis/label_var_same_name_check.d index 86fd8a74..009cc4b6 100644 --- a/src/dscanner/analysis/label_var_same_name_check.d +++ b/src/dscanner/analysis/label_var_same_name_check.d @@ -4,172 +4,254 @@ module dscanner.analysis.label_var_same_name_check; -import dparse.ast; -import dparse.lexer; -import dsymbol.scope_ : Scope; import dscanner.analysis.base; -import dscanner.analysis.helpers; +import dmd.cond : Include; +import std.conv : to; /** * Checks for labels and variables that have the same name. */ -final class LabelVarNameCheck : ScopedBaseAnalyzer +extern (C++) class LabelVarNameCheck(AST) : BaseAnalyzerDmd { mixin AnalyzerInfo!"label_var_same_name_check"; - - this(BaseAnalyzerArguments args) + alias visit = BaseAnalyzerDmd.visit; + + mixin ScopedVisit!(AST.Module); + mixin ScopedVisit!(AST.TemplateDeclaration); + mixin ScopedVisit!(AST.IfStatement); + mixin ScopedVisit!(AST.WithStatement); + mixin ScopedVisit!(AST.WhileStatement); + mixin ScopedVisit!(AST.DoStatement); + mixin ScopedVisit!(AST.ForStatement); + mixin ScopedVisit!(AST.CaseStatement); + mixin ScopedVisit!(AST.ForeachStatement); + mixin ScopedVisit!(AST.ForeachRangeStatement); + mixin ScopedVisit!(AST.ScopeStatement); + mixin ScopedVisit!(AST.UnitTestDeclaration); + mixin ScopedVisit!(AST.FuncDeclaration); + mixin ScopedVisit!(AST.FuncLiteralDeclaration); + mixin ScopedVisit!(AST.FuncAliasDeclaration); + mixin ScopedVisit!(AST.CtorDeclaration); + mixin ScopedVisit!(AST.DtorDeclaration); + mixin ScopedVisit!(AST.InvariantDeclaration); + + mixin AggregateVisit!(AST.ClassDeclaration); + mixin AggregateVisit!(AST.StructDeclaration); + mixin AggregateVisit!(AST.InterfaceDeclaration); + mixin AggregateVisit!(AST.UnionDeclaration); + + extern (D) this(string fileName, bool skipTests = false) { - super(args); + super(fileName); } - mixin AggregateVisit!ClassDeclaration; - mixin AggregateVisit!StructDeclaration; - mixin AggregateVisit!InterfaceDeclaration; - mixin AggregateVisit!UnionDeclaration; - - override void visit(const VariableDeclaration var) + override void visit(AST.VarDeclaration vd) { - foreach (dec; var.declarators) - duplicateCheck(dec.name, false, conditionalDepth > 0); + import dmd.astenums : STC; + + if (!(vd.storage_class & STC.scope_)) + { + auto thing = Thing(to!string(vd.ident.toChars()), vd.loc.linnum, vd.loc.charnum); + duplicateCheck(thing, false, conditionalDepth > 0); + } + + super.visit(vd); } - override void visit(const LabeledStatement labeledStatement) + override void visit(AST.LabelStatement ls) { - duplicateCheck(labeledStatement.identifier, true, conditionalDepth > 0); - if (labeledStatement.declarationOrStatement !is null) - labeledStatement.declarationOrStatement.accept(this); + auto thing = Thing(to!string(ls.ident.toChars()), ls.loc.linnum, ls.loc.charnum); + duplicateCheck(thing, true, conditionalDepth > 0); + super.visit(ls); } - override void visit(const ConditionalDeclaration condition) + override void visit(AST.ConditionalDeclaration conditionalDeclaration) { - if (condition.falseDeclarations.length > 0) + import dmd.root.array : peekSlice; + conditionalDeclaration.condition.accept(this); + + if (conditionalDeclaration.condition.isVersionCondition()) ++conditionalDepth; - condition.accept(this); - if (condition.falseDeclarations.length > 0) + + if (conditionalDeclaration.elsedecl || conditionalDeclaration.condition.inc != Include.yes) + pushScope(); + + foreach (decl; conditionalDeclaration.decl.peekSlice()) + decl.accept(this); + + if (conditionalDeclaration.elsedecl || conditionalDeclaration.condition.inc != Include.yes) + popScope(); + + if (conditionalDeclaration.elsedecl) + foreach(decl; conditionalDeclaration.elsedecl.peekSlice()) + decl.accept(this); + + if (conditionalDeclaration.condition.isVersionCondition()) --conditionalDepth; } - override void visit(const VersionCondition condition) + override void visit(AST.ConditionalStatement conditionalStatement) { - ++conditionalDepth; - condition.accept(this); - --conditionalDepth; + conditionalStatement.condition.accept(this); + + if (conditionalStatement.condition.isVersionCondition) + ++conditionalDepth; + + if (conditionalStatement.ifbody) + { + if (conditionalStatement.elsebody) + pushScope(); + + conditionalStatement.ifbody.accept(this); + + if (conditionalStatement.elsebody) + popScope(); + } + + if (conditionalStatement.elsebody) + { + if (conditionalStatement.condition.inc == Include.no) + pushScope(); + + conditionalStatement.elsebody.accept(this); + + if (conditionalStatement.condition.inc == Include.no) + popScope(); + } + + if (conditionalStatement.condition.isVersionCondition) + --conditionalDepth; } - alias visit = ScopedBaseAnalyzer.visit; + override void visit(AST.AnonDeclaration ad) + { + pushScope(); + pushAggregateName("", ad.loc.linnum, ad.loc.charnum); + super.visit(ad); + popScope(); + popAggregateName(); + } private: - - Thing[string][] stack; + extern (D) Thing[string][] stack; template AggregateVisit(NodeType) { - override void visit(const NodeType n) + override void visit(NodeType n) { - pushAggregateName(n.name); - n.accept(this); + pushScope(); + pushAggregateName(to!string(n.ident.toString()), n.loc.linnum, n.loc.charnum); + super.visit(n); + popScope(); popAggregateName(); } } - void duplicateCheck(const Token name, bool fromLabel, bool isConditional) + template ScopedVisit(NodeType) + { + override void visit(NodeType n) + { + pushScope(); + super.visit(n); + popScope(); + } + } + + extern (D) void duplicateCheck(const Thing id, bool fromLabel, bool isConditional) { - import std.conv : to; import std.range : retro; size_t i; foreach (s; retro(stack)) { - string fqn = parentAggregateText ~ name.text; + string fqn = parentAggregateText ~ id.name; const(Thing)* thing = fqn in s; if (thing is null) - currentScope[fqn] = Thing(fqn, name.line, name.column, !fromLabel /+, isConditional+/ ); + currentScope[fqn] = Thing(fqn, id.line, id.column, !fromLabel); else if (i != 0 || !isConditional) { immutable thisKind = fromLabel ? "Label" : "Variable"; immutable otherKind = thing.isVar ? "variable" : "label"; - addErrorMessage(name, "dscanner.suspicious.label_var_same_name", - thisKind ~ " \"" ~ fqn ~ "\" has the same name as a " - ~ otherKind ~ " defined on line " ~ to!string(thing.line) ~ "."); + auto msg = thisKind ~ " \"" ~ fqn ~ "\" has the same name as a " + ~ otherKind ~ " defined on line " ~ to!string(thing.line) ~ "."; + addErrorMessage(id.line, id.column, "dscanner.suspicious.label_var_same_name", msg); } ++i; } } - static struct Thing + extern (D) static struct Thing { string name; size_t line; size_t column; bool isVar; - //bool isConditional; } - ref currentScope() @property + extern (D) ref currentScope() @property { return stack[$ - 1]; } - protected override void pushScope() + extern (D) void pushScope() { stack.length++; } - protected override void popScope() + extern (D) void popScope() { stack.length--; } int conditionalDepth; - void pushAggregateName(Token name) + extern (D) void pushAggregateName(string name, size_t line, size_t column) { - parentAggregates ~= name; + parentAggregates ~= Thing(name, line, column); updateAggregateText(); } - void popAggregateName() + extern (D) void popAggregateName() { parentAggregates.length -= 1; updateAggregateText(); } - void updateAggregateText() + extern (D) void updateAggregateText() { import std.algorithm : map; import std.array : join; if (parentAggregates.length) - parentAggregateText = parentAggregates.map!(a => a.text).join(".") ~ "."; + parentAggregateText = parentAggregates.map!(a => a.name).join(".") ~ "."; else parentAggregateText = ""; } - Token[] parentAggregates; - string parentAggregateText; + extern (D) Thing[] parentAggregates; + extern (D) string parentAggregateText; } unittest { import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import dscanner.analysis.helpers : assertAnalyzerWarningsDMD; import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.label_var_same_name_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ unittest { blah: - int blah; /+ - ^^^^ [warn]: Variable "blah" has the same name as a label defined on line 4. +/ + int blah; // [warn]: Variable "blah" has the same name as a label defined on line 4. } int blah; unittest { static if (stuff) int a; - int a; /+ - ^ [warn]: Variable "a" has the same name as a variable defined on line 12. +/ + int a; // [warn]: Variable "a" has the same name as a variable defined on line 11. } unittest @@ -186,8 +268,7 @@ unittest int a = 10; else int a = 20; - int a; /+ - ^ [warn]: Variable "a" has the same name as a variable defined on line 30. +/ + int a; // [warn]: Variable "a" has the same name as a variable defined on line 28. } template T(stuff) { @@ -210,8 +291,7 @@ unittest int c = 10; else int c = 20; - int c; /+ - ^ [warn]: Variable "c" has the same name as a variable defined on line 54. +/ + int c; // [warn]: Variable "c" has the same name as a variable defined on line 51. } unittest @@ -249,8 +329,7 @@ unittest interface A { int a; - int a; /+ - ^ [warn]: Variable "A.a" has the same name as a variable defined on line 93. +/ + int a; // [warn]: Variable "A.a" has the same name as a variable defined on line 89. } } diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 9483b0d8..f471dae7 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!LabelVarNameCheck(analysisConfig)) - checks ~= new LabelVarNameCheck(args.setSkipTests( - analysisConfig.label_var_same_name_check == Check.skipTests && !ut)); - if (moduleName.shouldRun!MismatchedArgumentCheck(analysisConfig)) checks ~= new MismatchedArgumentCheck(args.setSkipTests( analysisConfig.mismatched_args_check == Check.skipTests && !ut)); @@ -1351,6 +1347,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN config.max_cyclomatic_complexity.to!int ); + if (moduleName.shouldRunDmd!(LabelVarNameCheck!ASTCodegen)(config)) + visitors ~= new LabelVarNameCheck!ASTCodegen( + fileName, + config.label_var_same_name_check == Check.skipTests && !ut + ); + foreach (visitor; visitors) { m.accept(visitor); From 460f3a811aa52836f106e8086a5b70d1e816672c Mon Sep 17 00:00:00 2001 From: Vladiwostok Date: Sun, 7 Apr 2024 12:28:17 +0300 Subject: [PATCH 2/2] Disable check for local functions --- .../analysis/label_var_same_name_check.d | 53 ++++++++++++++----- 1 file changed, 40 insertions(+), 13 deletions(-) diff --git a/src/dscanner/analysis/label_var_same_name_check.d b/src/dscanner/analysis/label_var_same_name_check.d index 009cc4b6..b891d6c7 100644 --- a/src/dscanner/analysis/label_var_same_name_check.d +++ b/src/dscanner/analysis/label_var_same_name_check.d @@ -17,7 +17,6 @@ extern (C++) class LabelVarNameCheck(AST) : BaseAnalyzerDmd alias visit = BaseAnalyzerDmd.visit; mixin ScopedVisit!(AST.Module); - mixin ScopedVisit!(AST.TemplateDeclaration); mixin ScopedVisit!(AST.IfStatement); mixin ScopedVisit!(AST.WithStatement); mixin ScopedVisit!(AST.WhileStatement); @@ -27,14 +26,16 @@ extern (C++) class LabelVarNameCheck(AST) : BaseAnalyzerDmd mixin ScopedVisit!(AST.ForeachStatement); mixin ScopedVisit!(AST.ForeachRangeStatement); mixin ScopedVisit!(AST.ScopeStatement); - mixin ScopedVisit!(AST.UnitTestDeclaration); - mixin ScopedVisit!(AST.FuncDeclaration); - mixin ScopedVisit!(AST.FuncLiteralDeclaration); mixin ScopedVisit!(AST.FuncAliasDeclaration); mixin ScopedVisit!(AST.CtorDeclaration); mixin ScopedVisit!(AST.DtorDeclaration); mixin ScopedVisit!(AST.InvariantDeclaration); + mixin FunctionVisit!(AST.FuncDeclaration); + mixin FunctionVisit!(AST.TemplateDeclaration); + mixin FunctionVisit!(AST.UnitTestDeclaration); + mixin FunctionVisit!(AST.FuncLiteralDeclaration); + mixin AggregateVisit!(AST.ClassDeclaration); mixin AggregateVisit!(AST.StructDeclaration); mixin AggregateVisit!(AST.InterfaceDeclaration); @@ -49,7 +50,7 @@ extern (C++) class LabelVarNameCheck(AST) : BaseAnalyzerDmd { import dmd.astenums : STC; - if (!(vd.storage_class & STC.scope_)) + if (!(vd.storage_class & STC.scope_) && !isInLocalFunction) { auto thing = Thing(to!string(vd.ident.toChars()), vd.loc.linnum, vd.loc.charnum); duplicateCheck(thing, false, conditionalDepth > 0); @@ -68,6 +69,7 @@ extern (C++) class LabelVarNameCheck(AST) : BaseAnalyzerDmd override void visit(AST.ConditionalDeclaration conditionalDeclaration) { import dmd.root.array : peekSlice; + conditionalDeclaration.condition.accept(this); if (conditionalDeclaration.condition.isVersionCondition()) @@ -83,7 +85,7 @@ extern (C++) class LabelVarNameCheck(AST) : BaseAnalyzerDmd popScope(); if (conditionalDeclaration.elsedecl) - foreach(decl; conditionalDeclaration.elsedecl.peekSlice()) + foreach (decl; conditionalDeclaration.elsedecl.peekSlice()) decl.accept(this); if (conditionalDeclaration.condition.isVersionCondition()) @@ -134,6 +136,34 @@ extern (C++) class LabelVarNameCheck(AST) : BaseAnalyzerDmd private: extern (D) Thing[string][] stack; + int conditionalDepth; + extern (D) Thing[] parentAggregates; + extern (D) string parentAggregateText; + bool isInFunction; + bool isInLocalFunction; + enum string KEY = "dscanner.suspicious.label_var_same_name"; + + template FunctionVisit(NodeType) + { + override void visit(NodeType n) + { + auto oldIsInFunction = isInFunction; + auto oldIsInLocalFunction = isInLocalFunction; + + pushScope(); + + if (isInFunction) + isInLocalFunction = true; + else + isInFunction = true; + + super.visit(n); + popScope(); + + isInFunction = oldIsInFunction; + isInLocalFunction = oldIsInLocalFunction; + } + } template AggregateVisit(NodeType) { @@ -167,14 +197,16 @@ private: string fqn = parentAggregateText ~ id.name; const(Thing)* thing = fqn in s; if (thing is null) + { currentScope[fqn] = Thing(fqn, id.line, id.column, !fromLabel); + } else if (i != 0 || !isConditional) { immutable thisKind = fromLabel ? "Label" : "Variable"; immutable otherKind = thing.isVar ? "variable" : "label"; auto msg = thisKind ~ " \"" ~ fqn ~ "\" has the same name as a " ~ otherKind ~ " defined on line " ~ to!string(thing.line) ~ "."; - addErrorMessage(id.line, id.column, "dscanner.suspicious.label_var_same_name", msg); + addErrorMessage(id.line, id.column, KEY, msg); } ++i; } @@ -203,8 +235,6 @@ private: stack.length--; } - int conditionalDepth; - extern (D) void pushAggregateName(string name, size_t line, size_t column) { parentAggregates ~= Thing(name, line, column); @@ -227,9 +257,6 @@ private: else parentAggregateText = ""; } - - extern (D) Thing[] parentAggregates; - extern (D) string parentAggregateText; } unittest @@ -240,6 +267,7 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.label_var_same_name_check = Check.enabled; + assertAnalyzerWarningsDMD(q{ unittest { @@ -353,7 +381,6 @@ unittest break; } } - }c, sac); stderr.writeln("Unittest for LabelVarNameCheck passed."); }