From e6af6009211dae897ef5af720b108a9255dc70f1 Mon Sep 17 00:00:00 2001 From: lucica28 <57060141+lucica28@users.noreply.github.com> Date: Thu, 1 Dec 2022 15:00:22 +0200 Subject: [PATCH] replace libdpase in assert without msg visitor (#50) --- changelog/dscanner.assert-without-message.dd | 1 + src/dscanner/analysis/assert_without_msg.d | 142 ++++--------------- src/dscanner/analysis/base.d | 11 +- src/dscanner/analysis/run.d | 24 +++- 4 files changed, 52 insertions(+), 126 deletions(-) create mode 100644 changelog/dscanner.assert-without-message.dd diff --git a/changelog/dscanner.assert-without-message.dd b/changelog/dscanner.assert-without-message.dd new file mode 100644 index 0000000..2ea35b6 --- /dev/null +++ b/changelog/dscanner.assert-without-message.dd @@ -0,0 +1 @@ +Avoid checking `enforce` calls as it is phobos specific. \ No newline at end of file diff --git a/src/dscanner/analysis/assert_without_msg.d b/src/dscanner/analysis/assert_without_msg.d index 38246a3..3cf49d9 100644 --- a/src/dscanner/analysis/assert_without_msg.d +++ b/src/dscanner/analysis/assert_without_msg.d @@ -5,163 +5,69 @@ module dscanner.analysis.assert_without_msg; import dscanner.analysis.base; -import dscanner.utils : safeAccess; -import dsymbol.scope_ : Scope; -import dparse.lexer; -import dparse.ast; - import std.stdio; -import std.algorithm; /** * Check that all asserts have an explanatory message. */ -final class AssertWithoutMessageCheck : BaseAnalyzer +extern(C++) class AssertWithoutMessageCheck(AST) : BaseAnalyzerDmd { - enum string KEY = "dscanner.style.assert_without_msg"; - enum string MESSAGE = "An assert should have an explanatory message"; mixin AnalyzerInfo!"assert_without_msg"; + alias visit = BaseAnalyzerDmd.visit; /// - this(BaseAnalyzerArguments args) + extern(D) this(string fileName, bool skipTests = false) { - super(args); + super(fileName, skipTests); } - override void visit(const AssertExpression expr) + // Avoid visiting in/out contracts for this check + override void visitFuncBody(AST.FuncDeclaration f) { - static if (__traits(hasMember, expr.assertArguments, "messageParts")) - { - // libdparse 0.22.0+ - bool hasMessage = expr.assertArguments - && expr.assertArguments.messageParts.length > 0; - } - else - bool hasMessage = expr.assertArguments - && expr.assertArguments.message !is null; - - if (!hasMessage) - addErrorMessage(expr, KEY, MESSAGE); + if (f.fbody) + { + f.fbody.accept(this); + } } - override void visit(const FunctionCallExpression expr) + override void visit(AST.AssertExp ae) { - if (!isStdExceptionImported) - return; - - if (const IdentifierOrTemplateInstance iot = safeAccess(expr) - .unaryExpression.primaryExpression.identifierOrTemplateInstance) - { - auto ident = iot.identifier; - if (ident.text == "enforce" && expr.arguments !is null && expr.arguments.namedArgumentList !is null && - expr.arguments.namedArgumentList.items.length < 2) - addErrorMessage(expr, KEY, MESSAGE); - } + if (!ae.msg) + addErrorMessage(ae.loc.linnum, ae.loc.charnum, KEY, MESSAGE); } - override void visit(const SingleImport sImport) + override void visit(AST.StaticAssert ae) { - static immutable stdException = ["std", "exception"]; - if (sImport.identifierChain.identifiers.map!(a => a.text).equal(stdException)) - isStdExceptionImported = true; + if (!ae.msgs) + addErrorMessage(ae.loc.linnum, ae.loc.charnum, KEY, MESSAGE); } - // revert the stack after new scopes - override void visit(const Declaration decl) - { - // be careful - ImportDeclarations don't introduce a new scope - if (decl.importDeclaration is null) - { - bool tmp = isStdExceptionImported; - scope(exit) isStdExceptionImported = tmp; - decl.accept(this); - } - else - decl.accept(this); - } - - mixin ScopedVisit!IfStatement; - mixin ScopedVisit!BlockStatement; - - alias visit = BaseAnalyzer.visit; private: - bool isStdExceptionImported; - - template ScopedVisit(NodeType) - { - override void visit(const NodeType n) - { - bool tmp = isStdExceptionImported; - scope(exit) isStdExceptionImported = tmp; - n.accept(this); - } - } + enum string KEY = "dscanner.style.assert_without_msg"; + enum string MESSAGE = "An assert should have an explanatory message"; } unittest { import std.stdio : stderr; - import std.format : format; import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; + import dscanner.analysis.helpers : assertAnalyzerWarningsDMD; StaticAnalysisConfig sac = disabledConfig(); sac.assert_without_msg = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ unittest { assert(0, "foo bar"); - assert(0); /+ - ^^^^^^^^^ [warn]: %s +/ + assert(0); // [warn]: An assert should have an explanatory message } - }c.format( - AssertWithoutMessageCheck.MESSAGE, - ), sac); + }c, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ unittest { static assert(0, "foo bar"); - static assert(0); /+ - ^^^^^^^^^ [warn]: %s +/ - } - }c.format( - AssertWithoutMessageCheck.MESSAGE, - ), sac); - - // check for std.exception.enforce - assertAnalyzerWarnings(q{ - unittest { - enforce(0); // std.exception not imported yet - could be a user-defined symbol - import std.exception; - enforce(0, "foo bar"); - enforce(0); /+ - ^^^^^^^^^^ [warn]: %s +/ - } - }c.format( - AssertWithoutMessageCheck.MESSAGE, - ), sac); - - // check for std.exception.enforce - assertAnalyzerWarnings(q{ - unittest { - import exception; - class C { - import std.exception; - } - enforce(0); // std.exception not imported yet - could be a user-defined symbol - struct S { - import std.exception; - } - enforce(0); // std.exception not imported yet - could be a user-defined symbol - if (false) { - import std.exception; - } - enforce(0); // std.exception not imported yet - could be a user-defined symbol - { - import std.exception; - } - enforce(0); // std.exception not imported yet - could be a user-defined symbol + static assert(0); // [warn]: An assert should have an explanatory message } }c, sac); diff --git a/src/dscanner/analysis/base.d b/src/dscanner/analysis/base.d index b4a8664..aa36817 100644 --- a/src/dscanner/analysis/base.d +++ b/src/dscanner/analysis/base.d @@ -913,9 +913,10 @@ extern(C++) class BaseAnalyzerDmd(AST) : ParseTimeTransitiveVisitor!AST { alias visit = ParseTimeTransitiveVisitor!AST.visit; - extern(D) this(string fileName) + extern(D) this(string fileName, bool skipTests = false) { this.fileName = fileName; + this.skipTests = skipTests; _messages = new MessageSet; } @@ -933,6 +934,12 @@ extern(C++) class BaseAnalyzerDmd(AST) : ParseTimeTransitiveVisitor!AST return _messages[].array; } + override void visit(AST.UnitTestDeclaration ud) + { + if (!skipTests) + super.visit(ud); + } + protected: @@ -941,6 +948,8 @@ protected: _messages.insert(Message(fileName, line, column, key, message, getName())); } + extern(D) bool skipTests; + /** * The file name */ diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 3638340..34f815b 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -99,6 +99,11 @@ import dmd.parse : Parser; bool first = true; +version (unittest) + enum ut = true; +else + enum ut = false; + private alias ASTAllocator = CAllocatorImpl!( AllocatorList!(n => Region!Mallocator(1024 * 128), Mallocator)); @@ -801,15 +806,18 @@ unittest assert(test("std.bar.foo", "-barr,+bar")); } -private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName, - const(Token)[] tokens, const Module m, - const StaticAnalysisConfig analysisConfig, const Scope* moduleScope) +private { version (unittest) enum ut = true; else enum ut = false; +} +private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName, + const(Token)[] tokens, const Module m, + const StaticAnalysisConfig analysisConfig, const Scope* moduleScope) +{ BaseAnalyzer[] checks; string moduleName; @@ -957,10 +965,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName, checks ~= new HasPublicExampleCheck(args.setSkipTests( analysisConfig.has_public_example == Check.skipTests && !ut)); - if (moduleName.shouldRun!AssertWithoutMessageCheck(analysisConfig)) - checks ~= new AssertWithoutMessageCheck(args.setSkipTests( - analysisConfig.assert_without_msg == Check.skipTests && !ut)); - if (moduleName.shouldRun!IfConstraintsIndentCheck(analysisConfig)) checks ~= new IfConstraintsIndentCheck(args.setSkipTests( analysisConfig.if_constraints_indent == Check.skipTests && !ut)); @@ -1318,6 +1322,12 @@ MessageSet analyzeDmd(string fileName, ASTBase.Module m, const char[] moduleName if (moduleName.shouldRunDmd!(ConstructorCheck!ASTBase)(config)) visitors ~= new ConstructorCheck!ASTBase(fileName); + + if (moduleName.shouldRunDmd!(AssertWithoutMessageCheck!ASTBase)(config)) + visitors ~= new AssertWithoutMessageCheck!ASTBase( + fileName, + config.assert_without_msg == Check.skipTests && !ut + ); if (moduleName.shouldRunDmd!(LocalImportCheck!ASTBase)(config)) visitors ~= new LocalImportCheck!ASTBase(fileName);