diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 235b8418..fa87e11e 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -853,10 +853,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName, checks ~= new IfConstraintsIndentCheck(args.setSkipTests( analysisConfig.if_constraints_indent == Check.skipTests && !ut)); - if (moduleName.shouldRun!UnusedResultChecker(analysisConfig)) - checks ~= new UnusedResultChecker(args.setSkipTests( - analysisConfig.unused_result == Check.skipTests && !ut)); - return checks; } @@ -1364,6 +1360,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN config.max_line_length ); + if (moduleName.shouldRunDmd!(UnusedResultChecker!ASTCodegen)(config)) + visitors ~= new UnusedResultChecker!ASTCodegen( + fileName, + config.unused_result == Check.skipTests && !ut + ); + foreach (visitor; visitors) { m.accept(visitor); diff --git a/src/dscanner/analysis/unused_result.d b/src/dscanner/analysis/unused_result.d index 83e4c646..b5815062 100644 --- a/src/dscanner/analysis/unused_result.d +++ b/src/dscanner/analysis/unused_result.d @@ -4,15 +4,7 @@ // http://www.boost.org/LICENSE_1_0.txt) module dscanner.analysis.unused_result; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; -import dscanner.analysis.mismatched_args : IdentVisitor, resolveSymbol; -import dscanner.utils; -import dsymbol.scope_; -import dsymbol.symbol; -import std.algorithm : canFind, countUntil; -import std.range : retro; /** * Checks for function call statements which call non-void functions. @@ -24,105 +16,212 @@ import std.range : retro; * When the return value is intentionally discarded, `cast(void)` can * be prepended to silence the check. */ -final class UnusedResultChecker : BaseAnalyzer +extern (C++) class UnusedResultChecker(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; + alias visit = BaseAnalyzerDmd.visit; + mixin AnalyzerInfo!"unused_result"; - mixin AnalyzerInfo!"unused_result"; + private enum KEY = "dscanner.performance.enum_array_literal"; + private enum string MSG = "Function return value is discarded"; -private: + private bool[string] nonVoidFuncs; + private string[] aggregateStack; - enum string KEY = "dscanner.unused_result"; - enum string MSG = "Function return value is discarded"; + extern (D) this(string fileName, bool skipTests = false) + { + super(fileName, skipTests); + } -public: + private template VisitAggregate(NodeType) + { + override void visit(NodeType aggregate) + { + string name = cast(string) aggregate.ident.toString(); + aggregateStack ~= name; + super.visit(aggregate); + aggregateStack.length -= 1; + } + } - const(DSymbol)* void_; - const(DSymbol)* noreturn_; + mixin VisitAggregate!(AST.StructDeclaration); + mixin VisitAggregate!(AST.ClassDeclaration); - /// - this(BaseAnalyzerArguments args) - { - super(args); - void_ = sc.getSymbolsByName(internString("void"))[0]; - auto symbols = sc.getSymbolsByName(internString("noreturn")); - if (symbols.length > 0) - noreturn_ = symbols[0]; - } + override void visit(AST.FuncDeclaration funcDeclaration) + { + import dmd.astenums : TY; - override void visit(const(ExpressionStatement) decl) - { - import std.typecons : scoped; + auto typeFunc = funcDeclaration.type.isTypeFunction(); + if (typeFunc && typeFunc.next && typeFunc.next.ty != TY.Tvoid && typeFunc.next.ty != TY.Tnoreturn) + { + auto typeIdent = typeFunc.next.isTypeIdentifier(); + bool isNoReturn = typeIdent is null ? false : typeIdent.ident.toString() == "noreturn"; - super.visit(decl); - if (!decl.expression) - return; - if (decl.expression.items.length != 1) - return; - auto ue = cast(UnaryExpression) decl.expression.items[0]; - if (!ue) - return; - auto fce = ue.functionCallExpression; - if (!fce) - return; + if (!isNoReturn) + { + string funcName = buildFullyQualifiedName(cast(string) funcDeclaration.ident.toString()); + nonVoidFuncs[funcName] = true; + } + } - auto identVisitor = scoped!IdentVisitor; - if (fce.unaryExpression !is null) - identVisitor.visit(fce.unaryExpression); - else if (fce.type !is null) - identVisitor.visit(fce.type); + super.visit(funcDeclaration); + } - if (!identVisitor.names.length) - return; + override void visit(AST.AliasDeclaration aliasDecl) + { + import std.algorithm : canFind, endsWith; + import std.array : replace; - const(DSymbol)*[] symbols = resolveSymbol(sc, identVisitor.names); + auto typeIdent = aliasDecl.type.isTypeIdentifier(); + if (typeIdent is null) + return; - if (!symbols.length) - return; + string aliasName = cast(string) aliasDecl.ident.toString(); + string targetName = cast(string) typeIdent.ident.toString(); - foreach (sym; symbols) - { - if (!sym) - return; - if (!sym.type) - return; - if (sym.kind != CompletionKind.functionName) - return; - if (sym.type is void_) - return; - if (noreturn_ && sym.type is noreturn_) - return; - } + foreach(func; nonVoidFuncs.byKey()) + { + if (func.endsWith(targetName) || func.canFind(targetName ~ ".")) + { + string newAliasName = func.replace(targetName, aliasName); + nonVoidFuncs[newAliasName] = true; + } + } + } + + private extern (D) string buildFullyQualifiedName(string funcName) + { + import std.algorithm : fold; + + if (aggregateStack.length == 0) + return funcName; + + return aggregateStack.fold!((a, b) => a ~ "." ~ b) ~ "." ~ funcName; + } + + mixin VisitInstructionBlock!(AST.WhileStatement); + mixin VisitInstructionBlock!(AST.ForStatement); + mixin VisitInstructionBlock!(AST.DoStatement); + mixin VisitInstructionBlock!(AST.ForeachRangeStatement); + mixin VisitInstructionBlock!(AST.ForeachStatement); + mixin VisitInstructionBlock!(AST.SwitchStatement); + mixin VisitInstructionBlock!(AST.SynchronizedStatement); + mixin VisitInstructionBlock!(AST.WithStatement); + mixin VisitInstructionBlock!(AST.TryCatchStatement); + mixin VisitInstructionBlock!(AST.TryFinallyStatement); + + override void visit(AST.CompoundStatement compoundStatement) + { + foreach (statement; *compoundStatement.statements) + { + if (hasUnusedResult(statement)) + { + auto lineNum = cast(ulong) statement.loc.linnum; + auto charNum = cast(ulong) statement.loc.charnum; + addErrorMessage(lineNum, charNum, KEY, MSG); + } + + statement.accept(this); + } + } + + override void visit(AST.IfStatement ifStatement) + { + if (hasUnusedResult(ifStatement.ifbody)) + { + auto lineNum = cast(ulong) ifStatement.ifbody.loc.linnum; + auto charNum = cast(ulong) ifStatement.ifbody.loc.charnum; + addErrorMessage(lineNum, charNum, KEY, MSG); + } + + if (ifStatement.elsebody && hasUnusedResult(ifStatement.elsebody)) + { + auto lineNum = cast(ulong) ifStatement.elsebody.loc.linnum; + auto charNum = cast(ulong) ifStatement.elsebody.loc.charnum; + addErrorMessage(lineNum, charNum, KEY, MSG); + } + + super.visit(ifStatement); + } + + private template VisitInstructionBlock(NodeType) + { + override void visit(NodeType statement) + { + if (hasUnusedResult(statement._body)) + { + auto lineNum = cast(ulong) statement._body.loc.linnum; + auto charNum = cast(ulong) statement._body.loc.charnum; + addErrorMessage(lineNum, charNum, KEY, MSG); + } - const(Token)[] tokens = fce.unaryExpression - ? fce.unaryExpression.tokens - : fce.type - ? fce.type.tokens - : fce.tokens; + super.visit(statement); + } + } - addErrorMessage(tokens, KEY, MSG); - } + private bool hasUnusedResult(AST.Statement statement) + { + import dmd.astenums : TY; + + auto exprStatement = statement.isExpStatement(); + if (exprStatement is null) + return false; + + auto callExpr = exprStatement.exp.isCallExp(); + if (callExpr is null) + return false; + + string funcName = ""; + + if (auto identExpr = callExpr.e1.isIdentifierExp()) + funcName = cast(string) identExpr.ident.toString(); + else if (auto dotIdExpr = callExpr.e1.isDotIdExp()) + funcName = buildFullyQualifiedCallName(dotIdExpr); + + return (funcName in nonVoidFuncs) !is null; + } + + private extern (D) string buildFullyQualifiedCallName(AST.DotIdExp dotIdExpr) + { + import std.algorithm : fold, reverse; + + string[] nameStack; + nameStack ~= cast(string) dotIdExpr.ident.toString(); + + auto lastExpr = dotIdExpr.e1; + while (lastExpr.isDotIdExp()) + { + auto current = lastExpr.isDotIdExp(); + nameStack ~= cast(string) current.ident.toString(); + lastExpr = current.e1; + } + + if (auto identExpr = lastExpr.isIdentifierExp()) + nameStack ~= cast(string) identExpr.ident.toString(); + + return nameStack.reverse.fold!((a, b) => a ~ "." ~ b); + } } unittest { - import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; - import std.stdio : stderr; - import std.format : format; + import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import dscanner.analysis.helpers : assertAnalyzerWarningsDMD; + import std.stdio : stderr; + import std.format : format; - StaticAnalysisConfig sac = disabledConfig(); - sac.unused_result = Check.enabled; + enum string MSG = "Function return value is discarded"; + StaticAnalysisConfig sac = disabledConfig(); + sac.unused_result = Check.enabled; - assertAnalyzerWarnings(q{ - void fun() {} + assertAnalyzerWarningsDMD(q{ + void fun() {} void main() { fun(); } }c, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ alias noreturn = typeof(*null); noreturn fun() { while (1) {} } noreturn main() @@ -131,16 +230,15 @@ unittest } }c, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ int fun() { return 1; } void main() { - fun(); /+ - ^^^ [warn]: %s +/ + fun(); // [warn]: %s } - }c.format(UnusedResultChecker.MSG), sac); + }c.format(MSG), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ struct Foo { static bool get() @@ -151,12 +249,12 @@ unittest alias Bar = Foo; void main() { - Bar.get(); /+ - ^^^^^^^ [warn]: %s +/ + Bar.get(); // [warn]: %s + Foo.bar.get(); } - }c.format(UnusedResultChecker.MSG), sac); + }c.format(MSG), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void main() { void fun() {} @@ -164,17 +262,15 @@ unittest } }c, sac); - version (none) // TODO: local functions - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void main() { int fun() { return 1; } - fun(); /+ - ^^^ [warn]: %s +/ + fun(); // [warn]: %s } - }c.format(UnusedResultChecker.MSG), sac); + }c.format(MSG), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ int fun() { return 1; } void main() { @@ -182,7 +278,7 @@ unittest } }c, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void fun() { } alias gun = fun; void main() @@ -191,7 +287,42 @@ unittest } }c, sac); - import std.stdio: writeln; - writeln("Unittest for UnusedResultChecker passed"); -} + assertAnalyzerWarningsDMD(q{ + int fun() { return 1; } + void main() + { + if (true) + fun(); // [warn]: %s + else + fun(); // [warn]: %s + } + }c.format(MSG, MSG), sac); + + assertAnalyzerWarningsDMD(q{ + int fun() { return 1; } + void main() + { + while (true) + fun(); // [warn]: %s + } + }c.format(MSG), sac); + assertAnalyzerWarningsDMD(q{ + int fun() { return 1; } + alias gun = fun; + void main() + { + gun(); // [warn]: %s + } + }c.format(MSG), sac); + + assertAnalyzerWarningsDMD(q{ + void main() + { + void fun() {} + fun(); + } + }c, sac); + + stderr.writeln("Unittest for UnusedResultChecker passed"); +}