Improvements to the onmodified variable checker

src/analysis/builtin_property_names.d

@@ -29,8 +29,6 @@ class BuiltinPropertyNameCheck : BaseAnalyzer
 {
 	alias visit = BaseAnalyzer.visit;
 
-	enum string KEY = "dscanner.confusing.builtin_property_names";
-
 	this(string fileName)
 	{
 		super(fileName);
@@ -47,7 +45,7 @@ class BuiltinPropertyNameCheck : BaseAnalyzer
 
 	override void visit(const FunctionBody functionBody)
 	{
-		int d = depth;
+		immutable int d = depth;
 		scope(exit) depth = d;
 		depth = 0;
 		functionBody.accept(this);
@@ -77,16 +75,18 @@ class BuiltinPropertyNameCheck : BaseAnalyzer
 
 private:
 
+	enum string KEY = "dscanner.confusing.builtin_property_names";
+
 	string generateErrorMessage(string name)
 	{
-		import std.string;
+		import std.string : format;
 		return format("Avoid naming members '%s'. This can"
 		~ " confuse code that depends on the '.%s' property of a type.", name, name);
 	}
 
 	bool isBuiltinProperty(string name)
 	{
-		import std.algorithm;
+		import std.algorithm : canFind;
 		return builtinProperties.canFind(name);
 	}
 
@@ -98,7 +98,7 @@ private:
 
 unittest
 {
-	import analysis.config;
+	import analysis.config : StaticAnalysisConfig;
 	StaticAnalysisConfig sac;
 	sac.builtin_property_names_check = true;
 	assertAnalyzerWarnings(q{

src/analysis/constructors.d

@@ -18,11 +18,11 @@ class ConstructorCheck : BaseAnalyzer
 
 	override void visit(const ClassDeclaration classDeclaration)
 	{
-		bool oldHasDefault = hasDefaultArgConstructor;
+		immutable bool oldHasDefault = hasDefaultArgConstructor;
-		bool oldHasNoArg = hasNoArgConstructor;
+		immutable bool oldHasNoArg = hasNoArgConstructor;
 		hasNoArgConstructor = false;
 		hasDefaultArgConstructor = false;
-		State prev = state;
+		immutable State prev = state;
 		state = State.inClass;
 		classDeclaration.accept(this);
 		if (hasNoArgConstructor && hasDefaultArgConstructor)
@@ -39,7 +39,7 @@ class ConstructorCheck : BaseAnalyzer
 
 	override void visit(const StructDeclaration structDeclaration)
 	{
-		State prev = state;
+		immutable State prev = state;
 		state = State.inStruct;
 		structDeclaration.accept(this);
 		state = prev;

src/analysis/helpers.d

@@ -69,7 +69,7 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, stri
 	foreach (rawWarning; rawWarnings[])
 	{
 		// Skip the warning if it is on line zero
-		size_t rawLine = rawWarning.line;
+		immutable size_t rawLine = rawWarning.line;
 		if (rawLine == 0)
 		{
 			stderr.writefln("!!! Skipping warning because it is on line zero:\n%s", rawWarning.message);
@@ -89,8 +89,8 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, stri
 			continue;
 
 		// Skip if there is no comment or code
-		string codePart = codeLine.before("// ");
+		immutable string codePart = codeLine.before("// ");
-		string commentPart = codeLine.after("// ");
+		immutable string commentPart = codeLine.after("// ");
 		if (!codePart.length || !commentPart.length)
 			continue;
 
@@ -107,7 +107,7 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, stri
 		// No warning
 		if (lineNo !in warnings)
 		{
-			string errors = "Expected warning:\n%s\nFrom source code at (%s:?):\n%s".format(
+			immutable string errors = "Expected warning:\n%s\nFrom source code at (%s:?):\n%s".format(
 				messages[lineNo],
 				lineNo,
 				codeLines[lineNo - line]
@@ -117,7 +117,7 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, stri
 		// Different warning
 		else if (warnings[lineNo] != messages[lineNo])
 		{
-			string errors = "Expected warning:\n%s\nBut was:\n%s\nFrom source code at (%s:?):\n%s".format(
+			immutable string errors = "Expected warning:\n%s\nBut was:\n%s\nFrom source code at (%s:?):\n%s".format(
 				messages[lineNo],
 				warnings[lineNo],
 				lineNo,
@@ -143,7 +143,7 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, stri
 	}
 	if (unexpectedWarnings.length)
 	{
-		string message = "Unexpected warnings:\n" ~ unexpectedWarnings.join("\n");
+		immutable string message = "Unexpected warnings:\n" ~ unexpectedWarnings.join("\n");
 		throw new core.exception.AssertError(message, file, line);
 	}
 }

src/analysis/immutable_finder.d

@@ -54,6 +54,8 @@ class ImmutableFinder:BaseAnalyzer
 		{
 			foreach (d; dec.declarators)
 			{
+				if (initializedFromCast(d.initializer))
+					continue;
 				tree[$ - 1].insert(new VariableInfo(d.name.text, d.name.line,
 					d.name.column));
 			}
@@ -68,9 +70,13 @@ class ImmutableFinder:BaseAnalyzer
 				&& autoDeclaration.storageClass.token != tok!"enum"
 				&& autoDeclaration.storageClass.token != tok!"immutable"))
 		{
-			foreach (id; autoDeclaration.identifiers)
+			foreach (size_t i, id; autoDeclaration.identifiers)
+			{
+				if (initializedFromCast(autoDeclaration.initializers[i]))
+					continue;
 				tree[$ - 1].insert(new VariableInfo(id.text, id.line,
 					id.column));
+			}
 		}
 		autoDeclaration.accept(this);
 	}
@@ -130,6 +136,17 @@ class ImmutableFinder:BaseAnalyzer
 			unary.accept(this);
 	}
 
+	override void visit(const ForeachStatement foreachStatement)
+	{
+		if (foreachStatement.low !is null)
+		{
+			interest++;
+			foreachStatement.low.accept(this);
+			interest--;
+		}
+		foreachStatement.declarationOrStatement.accept(this);
+	}
+
 private:
 
 	template PartsMightModify(T)
@@ -156,6 +173,28 @@ private:
 		}
 	}
 
+	bool initializedFromCast(const Initializer initializer)
+	{
+		import std.typecons : scoped;
+
+		static class CastFinder : ASTVisitor
+		{
+			alias visit = ASTVisitor.visit;
+			override void visit(const CastExpression castExpression)
+			{
+				foundCast = true;
+				castExpression.accept(this);
+			}
+			bool foundCast = false;
+		}
+
+		if (initializer is null)
+			return false;
+		auto finder = scoped!CastFinder();
+		finder.visit(initializer);
+		return finder.foundCast;
+	}
+
 	bool canFindImmutable(const Declaration dec)
 	{
 		import std.algorithm : canFind, map;
@@ -195,7 +234,7 @@ private:
 		foreach (vi; tree[$ - 1])
 		{
 			immutable string errorMessage = "Variable " ~ vi.name
-				~ " could have been declared immutable";
+				~ " is never modified and could have been declared immutable.";
 			addErrorMessage(vi.line, vi.column, "dscanner.suspicious.could_be_immutable",
 				errorMessage);
 		}

src/analysis/length_subtraction.d

@@ -29,8 +29,8 @@ class LengthSubtractionCheck : BaseAnalyzer
 	{
 		if (addExpression.operator == tok!"-")
 		{
-			UnaryExpression l = cast(UnaryExpression) addExpression.left;
+			const UnaryExpression l = cast(const UnaryExpression) addExpression.left;
-			UnaryExpression r = cast(UnaryExpression) addExpression.right;
+			const UnaryExpression r = cast(const UnaryExpression) addExpression.right;
 			if (l is null || r is null)
 			{
 //				stderr.writeln(__FILE__, " ", __LINE__);

src/analysis/opequals_without_tohash.d

@@ -57,7 +57,7 @@ class OpEqualsWithoutToHashCheck : BaseAnalyzer
 				continue;
 
 			// Check if opEquals or toHash
-			string methodName = declaration.functionDeclaration.name.text;
+			immutable string methodName = declaration.functionDeclaration.name.text;
 			if (methodName == "opEquals")
 				hasOpEquals = true;
 			else if (methodName == "toHash")

src/analysis/unused.d

@@ -44,7 +44,7 @@ class UnusedVariableCheck : BaseAnalyzer
 		pushScope();
 		if (functionDec.functionBody !is null)
 		{
-			bool ias = inAggregateScope;
+			immutable bool ias = inAggregateScope;
 			inAggregateScope = false;
 			if (!isOverride)
 				functionDec.parameters.accept(this);

src/main.d

@@ -133,7 +133,7 @@ int run(string[] args)
 	}
 	else if (tokenDump || highlight)
 	{
-		bool usingStdin = args.length == 1;
+		immutable bool usingStdin = args.length == 1;
 		ubyte[] bytes = usingStdin ? readStdin() : readFile(args[1]);
 		LexerConfig config;
 		config.stringBehavior = StringBehavior.source;
@@ -211,7 +211,7 @@ int run(string[] args)
 		}
 		else if (imports)
 		{
-			string[] fileNames = usingStdin ? ["stdin"] : args[1 .. $];
+			const string[] fileNames = usingStdin ? ["stdin"] : args[1 .. $];
 			LexerConfig config;
 			config.stringBehavior = StringBehavior.source;
 			auto visitor = new ImportPrinter;
@@ -236,12 +236,6 @@ int run(string[] args)
 			auto tokens = getTokensForParser(
 				usingStdin ? readStdin() : readFile(args[1]),
 				config, &cache);
-//			writeln("text                    blank\tindex\tline\tcolumn\ttype\tcomment");
-//			foreach (token; tokens)
-//			{
-//				writefln("<<%20s>>%b\t%d\t%d\t%d\t%d\t%s", token.text is null ? str(token.type) : token.text,
-//					token.text !is null, token.index, token.line, token.column, token.type, token.comment);
-//			}
 			auto mod = parseModule(tokens, fileName, null, &doNothing);
 
 			if (ast)
@@ -387,6 +381,7 @@ string getConfigurationLocation()
 {
 	version (useXDG)
 	{
+		int x;
 		import std.process;
 		string configDir = environment.get("XDG_CONFIG_HOME", null);
 		if (configDir is null)