diff --git a/dmd b/dmd index ae62618..ac5f925 160000 --- a/dmd +++ b/dmd @@ -1 +1 @@ -Subproject commit ae6261888e10e8072033369a9bce60d7be31ab1c +Subproject commit ac5f925c8b942c2b338f2831c21b11ddfd1aad87 diff --git a/src/dscanner/analysis/helpers.d b/src/dscanner/analysis/helpers.d index d9ac658..bd9e4a3 100644 --- a/src/dscanner/analysis/helpers.d +++ b/src/dscanner/analysis/helpers.d @@ -20,6 +20,9 @@ import dsymbol.modulecache : ModuleCache; import std.experimental.allocator; import std.experimental.allocator.mallocator; +import dmd.parse : Parser; +import dmd.astbase : ASTBase; + S between(S)(S value, S before, S after) if (isSomeString!S) { return value.after(before).before(after); @@ -350,3 +353,121 @@ void assertAutoFix(string before, string after, const StaticAnalysisConfig confi file, line); } } + +void assertAnalyzerWarningsDMD(string code, const StaticAnalysisConfig config, bool semantic = false, + string file = __FILE__, size_t line = __LINE__) +{ + import dmd.globals : global; + import dscanner.utils : getModuleName; + import std.file : remove, exists; + import std.stdio : File; + import std.path : dirName; + import dmd.arraytypes : Strings; + + import std.stdio : File; + import std.file : exists, remove; + + auto deleteme = "test.txt"; + File f = File(deleteme, "w"); + scope(exit) + { + assert(exists(deleteme)); + remove(deleteme); + } + + f.write(code); + f.close(); + + auto dmdParentDir = dirName(dirName(dirName(dirName(__FILE_FULL_PATH__)))); + + global.params.useUnitTests = true; + global.path = new Strings(); + global.path.push((dmdParentDir ~ "/dmd" ~ "\0").ptr); + global.path.push((dmdParentDir ~ "/dmd/druntime/src" ~ "\0").ptr); + + initDMD(); + + auto input = cast(char[]) code; + input ~= '\0'; + auto t = dmd.frontend.parseModule(cast(const(char)[]) file, cast(const (char)[]) input); + if (semantic) + t.module_.fullSemantic(); + + MessageSet rawWarnings = analyzeDmd("test.txt", t.module_, getModuleName(t.module_.md), config); + + string[] codeLines = code.splitLines(); + + // Get the warnings ordered by line + string[size_t] warnings; + foreach (rawWarning; rawWarnings[]) + { + // Skip the warning if it is on line zero + immutable size_t rawLine = rawWarning.line; + if (rawLine == 0) + { + stderr.writefln("!!! Skipping warning because it is on line zero:\n%s", + rawWarning.message); + continue; + } + + size_t warnLine = line - 1 + rawLine; + warnings[warnLine] = format("[warn]: %s", rawWarning.message); + } + + // Get all the messages from the comments in the code + string[size_t] messages; + foreach (i, codeLine; codeLines) + { + // Skip if no [warn] comment + if (codeLine.indexOf("// [warn]:") == -1) + continue; + + // Skip if there is no comment or code + immutable string codePart = codeLine.before("// "); + immutable string commentPart = codeLine.after("// "); + if (!codePart.length || !commentPart.length) + continue; + + // Get the line of this code line + size_t lineNo = i + line; + + // Get the message + messages[lineNo] = commentPart; + } + + // Throw an assert error if any messages are not listed in the warnings + foreach (lineNo, message; messages) + { + // No warning + if (lineNo !in warnings) + { + immutable string errors = "Expected warning:\n%s\nFrom source code at (%s:?):\n%s".format(messages[lineNo], + lineNo, codeLines[lineNo - line]); + throw new AssertError(errors, file, lineNo); + } + // Different warning + else if (warnings[lineNo] != messages[lineNo]) + { + immutable string errors = "Expected warning:\n%s\nBut was:\n%s\nFrom source code at (%s:?):\n%s".format( + messages[lineNo], warnings[lineNo], lineNo, codeLines[lineNo - line]); + throw new AssertError(errors, file, lineNo); + } + } + + // Throw an assert error if there were any warnings that were not expected + string[] unexpectedWarnings; + foreach (lineNo, warning; warnings) + { + // Unexpected warning + if (lineNo !in messages) + { + unexpectedWarnings ~= "%s\nFrom source code at (%s:?):\n%s".format(warning, + lineNo, codeLines[lineNo - line]); + } + } + if (unexpectedWarnings.length) + { + immutable string message = "Unexpected warnings:\n" ~ unexpectedWarnings.join("\n"); + throw new AssertError(message, file, line); + } +} diff --git a/src/dscanner/analysis/objectconst.d b/src/dscanner/analysis/objectconst.d index e000fe5..cc865e6 100644 --- a/src/dscanner/analysis/objectconst.d +++ b/src/dscanner/analysis/objectconst.d @@ -5,102 +5,107 @@ module dscanner.analysis.objectconst; -import std.stdio; -import std.regex; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dsymbol.scope_ : Scope; +import std.stdio; -/** - * Checks that opEquals, opCmp, toHash, 'opCast', and toString are either const, - * immutable, or inout. - */ -final class ObjectConstCheck : BaseAnalyzer +extern(C++) class ObjectConstCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - mixin AnalyzerInfo!"object_const_check"; + alias visit = BaseAnalyzerDmd.visit; - /// - this(BaseAnalyzerArguments args) + extern(D) this(string fileName) { - super(args); + super(fileName); } - mixin visitTemplate!ClassDeclaration; - mixin visitTemplate!InterfaceDeclaration; - mixin visitTemplate!UnionDeclaration; - mixin visitTemplate!StructDeclaration; - - override void visit(const AttributeDeclaration d) + void visitAggregate(AST.AggregateDeclaration ad) { - if (d.attribute.attribute == tok!"const" && inAggregate) - { - constColon = true; - } - d.accept(this); - } + import dmd.astenums : MODFlags, STC; - override void visit(const Declaration d) - { - import std.algorithm : any; - bool setConstBlock; - if (inAggregate && d.attributes && d.attributes.any!(a => a.attribute == tok!"const")) - { - setConstBlock = true; - constBlock = true; - } + if (!ad.members) + return; - bool containsDisable(A)(const A[] attribs) + foreach(member; *ad.members) { - import std.algorithm.searching : canFind; - return attribs.canFind!(a => a.atAttribute !is null && - a.atAttribute.identifier.text == "disable"); - } - - if (const FunctionDeclaration fd = d.functionDeclaration) - { - const isDeclationDisabled = containsDisable(d.attributes) || - containsDisable(fd.memberFunctionAttributes); - - if (inAggregate && !constColon && !constBlock && !isDeclationDisabled - && isInteresting(fd.name.text) && !hasConst(fd.memberFunctionAttributes)) + if (auto fd = member.isFuncDeclaration()) { - addErrorMessage(d.functionDeclaration.name, KEY, - "Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const."); + if (isInteresting(fd.ident.toString()) && !isConstFunc(fd) && + !(fd.storage_class & STC.disable)) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + "Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const."); + + member.accept(this); } + else if (auto scd = member.isStorageClassDeclaration()) + { + foreach (smember; *scd.decl) + { + if (auto fd2 = smember.isFuncDeclaration()) + { + if (isInteresting(fd2.ident.toString()) && !isConstFunc(fd2, scd) && + !(fd2.storage_class & STC.disable)) + addErrorMessage(cast(ulong) fd2.loc.linnum, cast(ulong) fd2.loc.charnum, KEY, + "Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const."); + + smember.accept(this); + } + else + smember.accept(this); + } + } + else + member.accept(this); } - - d.accept(this); - - if (!inAggregate) - constColon = false; - if (setConstBlock) - constBlock = false; } -private: - - enum string KEY = "dscanner.suspicious.object_const"; - - static bool hasConst(const MemberFunctionAttribute[] attributes) + override void visit(AST.ClassDeclaration cd) { - import std.algorithm : any; - - return attributes.any!(a => a.tokenType == tok!"const" - || a.tokenType == tok!"immutable" || a.tokenType == tok!"inout"); + visitAggregate(cd); } - static bool isInteresting(string name) + override void visit(AST.StructDeclaration sd) + { + visitAggregate(sd); + } + + override void visit(AST.InterfaceDeclaration id) + { + visitAggregate(id); + } + + override void visit(AST.UnionDeclaration ud) + { + visitAggregate(ud); + } + + extern(D) private static bool isInteresting(const char[] name) { return name == "opCmp" || name == "toHash" || name == "opEquals" || name == "toString" || name == "opCast"; } - bool constBlock; - bool constColon; + /** + * Checks if a function has either one of attributes `const`, `immutable`, `inout` + */ + private bool isConstFunc(AST.FuncDeclaration fd, AST.StorageClassDeclaration scd = null) + { + import dmd.astenums : MODFlags, STC; + import std.stdio : writeln; + + if (scd && (scd.stc & STC.const_ || scd.stc & STC.immutable_ || scd.stc & STC.wild)) + return true; + + if(fd.type && (fd.type.mod == MODFlags.const_ || + fd.type.mod == MODFlags.immutable_ || fd.type.mod == MODFlags.wild)) + return true; + + return false; + } + + private enum KEY = "dscanner.suspicious.object_const"; + + AST.AggregateDeclaration deleteme; } unittest @@ -109,7 +114,7 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.object_const_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void testConsts() { // Will be ok because all are declared const/immutable @@ -125,7 +130,7 @@ unittest return 1; } - const hash_t toHash() // ok + immutable hash_t toHash() // ok { return 0; } @@ -143,7 +148,7 @@ unittest class Fox { - const{ override string toString() { return "foo"; }} // ok + inout { override string toString() { return "foo"; } } // ok } class Rat @@ -159,26 +164,22 @@ unittest // Will warn, because none are const class Dog { - bool opEquals(Object a, Object b) /+ - ^^^^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/ + bool opEquals(Object a, Object b) // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. { return true; } - int opCmp(Object o) /+ - ^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/ + int opCmp(Object o) // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. { return 1; } - hash_t toHash() /+ - ^^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/ + hash_t toHash() // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. { return 0; } - string toString() /+ - ^^^^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/ + string toString() // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. { return "Dog"; } diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index d253a37..a3ff0fa 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -13,7 +13,6 @@ import dparse.parser; import dparse.rollback_allocator; import std.algorithm; import std.array; -import std.array; import std.conv; import std.file : mkdirRecurse; import std.functional : toDelegate; @@ -96,6 +95,7 @@ import dscanner.utils; import dscanner.reports : DScannerJsonReporter, SonarQubeGenericIssueDataReporter; import dmd.astbase : ASTBase; +import dmd.parse : Parser; bool first = true; @@ -404,9 +404,9 @@ bool analyze(string[] fileNames, const StaticAnalysisConfig config, string error auto ast_m = new ASTBase.Module(fileName.toStringz, id, false, false); auto input = cast(char[]) code; input ~= '\0'; - scope p = new Parser!ASTBase(ast_m, input, false); - p.nextToken(); - ast_m.members = p.parseModule(); + scope astbaseParser = new Parser!ASTBase(ast_m, input, false); + astbaseParser.nextToken(); + ast_m.members = astbaseParser.parseModule(); // Skip files that could not be read and continue with the rest if (code.length == 0) @@ -421,8 +421,8 @@ bool analyze(string[] fileNames, const StaticAnalysisConfig config, string error if (errorCount > 0 || (staticAnalyze && warningCount > 0)) hasErrors = true; MessageSet results = analyze(fileName, m, config, moduleCache, tokens, staticAnalyze); - MessageSet resultsDmd = analyzeDmd(fileName, ast_m); - foreach(result; resultsDmd[]) + MessageSet resultsDmd = analyzeDmd(fileName, ast_m, getModuleName(astbaseParser.md), config); + foreach (result; resultsDmd[]) { results.insert(result); } @@ -726,6 +726,46 @@ bool shouldRun(check : BaseAnalyzer)(string moduleName, const ref StaticAnalysis return true; } +/** + * Checks whether a module is part of a user-specified include/exclude list. + * + * The user can specify a comma-separated list of filters, everyone needs to start with + * either a '+' (inclusion) or '-' (exclusion). + * + * If no includes are specified, all modules are included. +*/ +bool shouldRunDmd(check : BaseAnalyzerDmd!ASTBase)(const char[] moduleName, const ref StaticAnalysisConfig config) +{ + enum string a = check.name; + + if (mixin("config." ~ a) == Check.disabled) + return false; + + // By default, run the check + if (!moduleName.length) + return true; + + auto filters = mixin("config.filters." ~ a); + + // Check if there are filters are defined + // filters starting with a comma are invalid + if (filters.length == 0 || filters[0].length == 0) + return true; + + auto includers = filters.filter!(f => f[0] == '+').map!(f => f[1..$]); + auto excluders = filters.filter!(f => f[0] == '-').map!(f => f[1..$]); + + // exclusion has preference over inclusion + if (!excluders.empty && excluders.any!(s => moduleName.canFind(s))) + return false; + + if (!includers.empty) + return includers.any!(s => moduleName.canFind(s)); + + // by default: include all modules + return true; +} + /// unittest { @@ -856,10 +896,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName, checks ~= new NumberStyleCheck(args.setSkipTests( analysisConfig.number_style_check == Check.skipTests && !ut)); - if (moduleName.shouldRun!ObjectConstCheck(analysisConfig)) - checks ~= new ObjectConstCheck(args.setSkipTests( - analysisConfig.object_const_check == Check.skipTests && !ut)); - if (moduleName.shouldRun!OpEqualsWithoutToHashCheck(analysisConfig)) checks ~= new OpEqualsWithoutToHashCheck(args.setSkipTests( analysisConfig.opequals_tohash_check == Check.skipTests && !ut)); @@ -1285,14 +1321,24 @@ version (unittest) } } -MessageSet analyzeDmd(string fileName, ASTBase.Module m) +MessageSet analyzeDmd(string fileName, ASTBase.Module m, const char[] moduleName, const StaticAnalysisConfig config) { - scope vis = new EnumArrayVisitor!ASTBase(fileName); - m.accept(vis); - MessageSet set = new MessageSet; - foreach(message; vis.messages) - set.insert(message); + BaseAnalyzerDmd!ASTBase[] visitors; + + if (moduleName.shouldRunDmd!(ObjectConstCheck!ASTBase)(config)) + visitors ~= new ObjectConstCheck!ASTBase(fileName); + + if (moduleName.shouldRunDmd!(EnumArrayVisitor!ASTBase)(config)) + visitors ~= new EnumArrayVisitor!ASTBase(fileName); + + foreach (visitor; visitors) + { + m.accept(visitor); + + foreach (message; visitor.messages) + set.insert(message); + } return set; } diff --git a/src/dscanner/utils.d b/src/dscanner/utils.d index 504432e..d08a41e 100644 --- a/src/dscanner/utils.d +++ b/src/dscanner/utils.d @@ -8,6 +8,9 @@ import std.format : format; import std.file : exists, read; import std.path: isValidPath; +import dmd.astbase : ASTBase; +import dmd.parse : Parser; + private void processBOM(ref ubyte[] sourceCode, string fname) { enum spec = "D-Scanner does not support %s-encoded files (%s)"; @@ -309,3 +312,24 @@ auto ref safeAccess(M)(M m) { return SafeAccess!M(m); } + +/** + * Return the module name from a ModuleDeclaration instance with the following format: `foo.bar.module` + */ +const(char[]) getModuleName(ASTBase.ModuleDeclaration *mdptr) +{ + import std.array : array, join; + + if (mdptr !is null) + { + import std.algorithm : map; + ASTBase.ModuleDeclaration md = *mdptr; + + if (md.packages.length != 0) + return join(md.packages.map!(e => e.toString()).array ~ md.id.toString().dup, "."); + else + return md.id.toString(); + } + + return ""; +} \ No newline at end of file