diff --git a/src/analysis/config.d b/src/analysis/config.d index 907dcc5..4a4984e 100644 --- a/src/analysis/config.d +++ b/src/analysis/config.d @@ -80,4 +80,7 @@ struct StaticAnalysisConfig @INI("Checks for local imports that are too broad") bool local_import_check; + + @INI("Checks for variables that could be declared immutable") + bool could_be_immutable_check = false; // disabled by default for now } diff --git a/src/analysis/immutable_finder.d b/src/analysis/immutable_finder.d new file mode 100644 index 0000000..3124f6f --- /dev/null +++ b/src/analysis/immutable_finder.d @@ -0,0 +1,212 @@ +// Copyright Brian Schott (Hackerpilot) 2015. +// Distributed under the Boost Software License, Version 1.0. +// (See accompanying file LICENSE_1_0.txt or copy at +// http://www.boost.org/LICENSE_1_0.txt) +module analysis.immutable_finder; + +import std.container; +import std.d.ast; +import std.d.lexer; +import analysis.base; + +/** + * Checks for variables that could have been declared immutable + */ +class ImmutableFinder:BaseAnalyzer +{ + alias visit = BaseAnalyzer.visit; + + /// + this(string fileName) + { + super(fileName); + } + + override void visit(const Module mod) + { + pushScope(); + mod.accept(this); + popScope(); + } + + override void visit(const BlockStatement blockStatement) + { + pushScope(); + blockStatementDepth++; + blockStatement.accept(this); + blockStatementDepth--; + popScope(); + } + + override void visit(const StructBody structBody) + { + pushScope(); + auto oldBlockStatementDepth = blockStatementDepth; + blockStatementDepth = 0; + structBody.accept(this); + blockStatementDepth = oldBlockStatementDepth; + popScope(); + } + + override void visit(const VariableDeclaration dec) + { + if (dec.autoDeclaration is null && blockStatementDepth > 0 && isImmutable <= 0 && !canFindImmutable(dec)) + { + foreach (d; dec.declarators) + { + tree[$ - 1].insert(new VariableInfo(d.name.text, d.name.line, + d.name.column)); + } + } + dec.accept(this); + } + + override void visit(const AutoDeclaration autoDeclaration) + { + if (blockStatementDepth > 0 && isImmutable <= 0 + && (autoDeclaration.storageClass !is null + && autoDeclaration.storageClass.token != tok!"immutable")) + { + foreach (id; autoDeclaration.identifiers) + tree[$ - 1].insert(new VariableInfo(id.text, id.line, + id.column)); + } + autoDeclaration.accept(this); + } + + override void visit(const AssignExpression assignExpression) + { + if (assignExpression.operator != tok!"") + { + interest++; + assignExpression.ternaryExpression.accept(this); + interest--; + assignExpression.ternaryExpression.accept(this); + } + else + assignExpression.accept(this); + } + + override void visit(const Declaration dec) + { + if (canFindImmutable(dec)) + { + isImmutable++; + dec.accept(this); + isImmutable--; + } + else + dec.accept(this); + } + + override void visit(const IdentifierOrTemplateInstance ioti) + { +// import std.stdio : stderr; +// stderr.writeln(ioti.identifier.text, " ", ioti.identifier.line); + if (ioti.identifier != tok!"" && interest > 0) + { + variableMightBeModified(ioti.identifier.text); + } + ioti.accept(this); + } + + mixin PartsMightModify!IndexExpression; + mixin PartsMightModify!SliceExpression; + mixin PartsMightModify!FunctionCallExpression; + mixin PartsMightModify!IdentifierOrTemplateChain; + mixin PartsMightModify!ReturnStatement; + + override void visit(const UnaryExpression unary) + { + if (unary.prefix == tok!"++" || unary.prefix == tok!"--" + || unary.suffix == tok!"++" || unary.suffix == tok!"--") + { + interest++; + unary.accept(this); + interest--; + } + else + unary.accept(this); + } + +private: + + template PartsMightModify(T) + { + override void visit(const T t) + { + interest++; + t.accept(this); + interest--; + } + } + + void variableMightBeModified(string name) + { +// import std.stdio : stderr; +// stderr.writeln("Marking ", name, " as possibly modified"); + size_t index = tree.length - 1; + auto vi = VariableInfo(name); + while (true) + { + if (tree[index].removeKey(&vi) != 0 || index == 0) + break; + index--; + } + } + + bool canFindImmutable(const Declaration dec) + { + import std.algorithm : canFind, map; + return dec.attributes.map!(a => a.attribute).canFind(cast(IdType) tok!"immutable"); + } + + bool canFindImmutable(const VariableDeclaration dec) + { + import std.algorithm : canFind; + foreach (attr; dec.attributes) + { + if (attr.attribute.type == tok!"immutable") + return true; + } + if (dec.type !is null) + { + if (dec.type.typeConstructors.canFind(cast(IdType) tok!"immutable")) + return true; + } + return false; + } + + static struct VariableInfo + { + string name; + size_t line; + size_t column; + } + + void popScope() + { + foreach (vi; tree[$ - 1]) + { + immutable string errorMessage = "Variable " ~ vi.name + ~ " could have been declared immutable"; + addErrorMessage(vi.line, vi.column, "dscanner.suspicious.could_be_immutable", + errorMessage); + } + tree = tree[0 .. $ - 1]; + } + + void pushScope() + { + tree ~= new RedBlackTree!(VariableInfo*, "a.name < b.name"); + } + + int blockStatementDepth; + + int interest; + + int isImmutable; + + RedBlackTree!(VariableInfo*, "a.name < b.name")[] tree; +} + diff --git a/src/analysis/run.d b/src/analysis/run.d index b297ae5..d4eb11c 100644 --- a/src/analysis/run.d +++ b/src/analysis/run.d @@ -39,6 +39,7 @@ import analysis.undocumented; import analysis.comma_expression; import analysis.function_attributes; import analysis.local_imports; +import analysis.immutable_finder; bool first = true; @@ -185,6 +186,7 @@ MessageSet analyze(string fileName, const Module m, if (analysisConfig.function_attribute_check) checks ~= new FunctionAttributeCheck(fileName); if (analysisConfig.comma_expression_check) checks ~= new CommaExpressionCheck(fileName); if (analysisConfig.local_import_check) checks ~= new LocalImportCheck(fileName); + if (analysisConfig.could_be_immutable_check) checks ~= new ImmutableFinder(fileName); foreach (check; checks) { diff --git a/src/analysis/unused.d b/src/analysis/unused.d index 5f9254a..cb7bf86 100644 --- a/src/analysis/unused.d +++ b/src/analysis/unused.d @@ -224,7 +224,7 @@ class UnusedVariableCheck : BaseAnalyzer { foreach (part; matchAll(primary.primary.text, re)) { - size_t treeIndex = tree.length - 1; + immutable size_t treeIndex = tree.length - 1; auto uu = UnUsed(part.hit); auto r = tree[treeIndex].equalRange(&uu); if (!r.empty) @@ -247,7 +247,7 @@ class UnusedVariableCheck : BaseAnalyzer override void visit(const BlockStatement blockStatement) { - bool sb = inAggregateScope; + immutable bool sb = inAggregateScope; inAggregateScope = false; if (blockStatementIntroducesScope) pushScope(); @@ -281,12 +281,12 @@ class UnusedVariableCheck : BaseAnalyzer override void visit(const Parameter parameter) { - import std.algorithm; - import std.array; + import std.algorithm : canFind; + import std.array : array; if (parameter.name != tok!"") { // stderr.writeln("Adding parameter ", parameter.name.text); - bool isRef = canFind(parameter.parameterAttributes, cast(IdType) tok!"ref") + immutable bool isRef = canFind(parameter.parameterAttributes, cast(IdType) tok!"ref") || canFind(parameter.parameterAttributes, cast(IdType) tok!"in") || canFind(parameter.parameterAttributes, cast(IdType) tok!"out"); variableDeclared(parameter.name.text, parameter.name.line, @@ -302,7 +302,7 @@ class UnusedVariableCheck : BaseAnalyzer override void visit(const StructBody structBody) { - bool sb = inAggregateScope; + immutable bool sb = inAggregateScope; inAggregateScope = true; foreach (dec; structBody.declarations) visit(dec); @@ -311,7 +311,7 @@ class UnusedVariableCheck : BaseAnalyzer override void visit(const ConditionalStatement conditionalStatement) { - bool cs = blockStatementIntroducesScope; + immutable bool cs = blockStatementIntroducesScope; blockStatementIntroducesScope = false; conditionalStatement.accept(this); blockStatementIntroducesScope = cs; @@ -325,6 +325,8 @@ class UnusedVariableCheck : BaseAnalyzer variableUsed(primary.identifierChain.identifiers[0].text); } +private: + void variableDeclared(string name, size_t line, size_t column, bool isParameter, bool isRef) { @@ -353,8 +355,8 @@ class UnusedVariableCheck : BaseAnalyzer { if (!uu.isRef && tree.length > 1) { - string certainty = uu.uncertain ? " might not be used." : " is never used."; - string errorMessage = (uu.isParameter ? "Parameter " : "Variable ") + immutable string certainty = uu.uncertain ? " might not be used." : " is never used."; + immutable string errorMessage = (uu.isParameter ? "Parameter " : "Variable ") ~ uu.name ~ certainty; addErrorMessage(uu.line, uu.column, uu.isParameter ? "dscanner.suspicious.unused_parameter"