implement indentation aware autofixes

src/dscanner/analysis/base.d

@@ -57,6 +57,14 @@ struct AutoFix
 		);
 	}
 
+	static AutoFix resolveLater(string name, ulong[3] params, string extraInfo = null)
+	{
+		AutoFix ret;
+		ret.name = name;
+		ret.autofix = ResolveContext(params, extraInfo);
+		return ret;
+	}
+
 	static AutoFix replacement(const Token token, string newText, string name = null)
 	{
 		if (!name.length)
@@ -118,31 +126,107 @@ struct AutoFix
 		return ret;
 	}
 
+	static AutoFix indentLines(scope const(Token)[] tokens, const AutoFixFormatting formatting, string name = "Indent code")
+	{
+		CodeReplacement[] inserts;
+		size_t line = -1;
+		foreach (token; tokens)
+		{
+			if (line != token.line)
+			{
+				line = token.line;
+				inserts ~= CodeReplacement([token.index, token.index], formatting.indentation);
+			}
+		}
+		AutoFix ret;
+		ret.name = name;
+		ret.autofix = inserts;
+		return ret;
+	}
+
 	AutoFix concat(AutoFix other) const
 	{
 		import std.algorithm : sort;
 
+		static immutable string errorMsg = "Cannot concatenate code replacement with late-resolve";
+
 		AutoFix ret;
 		ret.name = name;
-		CodeReplacement[] concatenated;
+		CodeReplacement[] concatenated = expectReplacements(errorMsg).dup
-		autofix.match!(
+			~ other.expectReplacements(errorMsg);
-			(const CodeReplacement[] replacement)
-			{
-				concatenated = replacement.dup;
-			},
-			_ => assert(false, "Cannot concatenate code replacement with late-resolve")
-		);
-		other.autofix.match!(
-			(const CodeReplacement[] concat)
-			{
-				concatenated ~= concat.dup;
-			},
-			_ => assert(false, "Cannot concatenate code replacement with late-resolve")
-		);
 		concatenated.sort!"a.range[0] < b.range[0]";
 		ret.autofix = concatenated;
 		return ret;
 	}
+
+	CodeReplacement[] expectReplacements(
+		string errorMsg = "Expected available code replacements, not something to resolve later"
+	) @safe pure nothrow @nogc
+	{
+		return autofix.match!(
+			(replacement)
+			{
+				if (false) return CodeReplacement[].init;
+				static if (is(immutable typeof(replacement) == immutable CodeReplacement[]))
+					return replacement;
+				else
+					assert(false, errorMsg);
+			}
+		);
+	}
+
+	const(CodeReplacement[]) expectReplacements(
+		string errorMsg = "Expected available code replacements, not something to resolve later"
+	) const @safe pure nothrow @nogc
+	{
+		return autofix.match!(
+			(const replacement)
+			{
+				if (false) return CodeReplacement[].init;
+				static if (is(immutable typeof(replacement) == immutable CodeReplacement[]))
+					return replacement;
+				else
+					assert(false, errorMsg);
+			}
+		);
+	}
+}
+
+/// Formatting style for autofix generation (only available for resolve autofix)
+struct AutoFixFormatting
+{
+	enum BraceStyle
+	{
+		/// $(LINK https://en.wikipedia.org/wiki/Indent_style#Allman_style)
+		allman,
+		/// $(LINK https://en.wikipedia.org/wiki/Indent_style#Variant:_1TBS)
+		otbs,
+		/// $(LINK https://en.wikipedia.org/wiki/Indent_style#Variant:_Stroustrup)
+		stroustrup,
+		/// $(LINK https://en.wikipedia.org/wiki/Indentation_style#K&R_style)
+		knr,
+	}
+
+	BraceStyle braceStyle;
+	string indentation = "\t";
+	string eol = "\n";
+
+	string getWhitespaceBeforeOpeningBrace(string lastLineIndent, bool isFuncDecl) pure nothrow @safe const
+	{
+		final switch (braceStyle)
+		{
+		case BraceStyle.knr:
+			if (isFuncDecl)
+				goto case BraceStyle.allman;
+			else
+				goto case BraceStyle.otbs;
+		case BraceStyle.otbs:
+		case BraceStyle.stroustrup:
+			return " ";
+		case BraceStyle.allman:
+			return eol ~ lastLineIndent;
+		}
+	}
 }
 
 /// A diagnostic message. Each message defines one issue in the file, which
@@ -302,15 +386,19 @@ public:
 
 	AutoFix.CodeReplacement[] resolveAutoFix(
 		const Module mod,
-		const(Token)[] tokens,
+		scope const(char)[] rawCode,
+		scope const(Token)[] tokens,
 		const Message message,
-		const AutoFix.ResolveContext context
+		const AutoFix.ResolveContext context,
+		const AutoFixFormatting formatting,
 	)
 	{
 		cast(void) mod;
+		cast(void) rawCode;
 		cast(void) tokens;
 		cast(void) message;
 		cast(void) context;
+		cast(void) formatting;
 		assert(0);
 	}
 

src/dscanner/analysis/helpers.d

@@ -11,13 +11,14 @@ import std.string;
 import std.traits;
 
 import dparse.ast;
+import dparse.lexer : tok, Token;
 import dparse.rollback_allocator;
-import dsymbol.modulecache : ModuleCache;
+import dscanner.analysis.base;
 import dscanner.analysis.config;
 import dscanner.analysis.run;
-import dscanner.analysis.base;
+import dsymbol.modulecache : ModuleCache;
-import std.experimental.allocator.mallocator;
 import std.experimental.allocator;
+import std.experimental.allocator.mallocator;
 
 S between(S)(S value, S before, S after) if (isSomeString!S)
 {
@@ -40,6 +41,20 @@ S after(S)(S value, S separator) if (isSomeString!S)
 	return value[i + separator.length .. $];
 }
 
+string getLineIndentation(scope const(char)[] rawCode, scope const(Token)[] tokens, size_t line)
+{
+	import std.algorithm : countUntil;
+	import std.string : lastIndexOfAny;
+
+	auto idx = tokens.countUntil!(a => a.line == line);
+	if (idx == -1)
+		return "";
+
+	auto indent = rawCode[0 .. tokens[idx].index];
+	auto nl = indent.lastIndexOfAny("\r\n");
+	return indent[nl + 1 .. $].idup;
+}
+
 /**
  * This assert function will analyze the passed in code, get the warnings,
  * and make sure they match the warnings in the comments. Warnings are
@@ -195,6 +210,10 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config,
 	}
 }
 
+/// EOL inside this project, for tests
+private static immutable fileEol = q{
+};
+
 /**
  * This assert function will analyze the passed in code, get the warnings, and
  * apply all specified autofixes all at once.
@@ -206,6 +225,7 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config,
  * available suggestion.
  */
 void assertAutoFix(string before, string after, const StaticAnalysisConfig config,
+		const AutoFixFormatting formattingConfig = AutoFixFormatting(AutoFixFormatting.BraceStyle.otbs, "\t", 4, fileEol),
 		string file = __FILE__, size_t line = __LINE__)
 {
 	import dparse.lexer : StringCache, Token;
@@ -291,7 +311,8 @@ void assertAutoFix(string before, string after, const StaticAnalysisConfig confi
 		AutoFix fix = message.autofixes[pair[1]];
 		replacements ~= fix.autofix.match!(
 			(AutoFix.CodeReplacement[] r) => r,
-			(AutoFix.ResolveContext context) => resolveAutoFix(message, context, "test", moduleCache, tokens, m, config)
+			(AutoFix.ResolveContext context) => resolveAutoFix(message, context,
+				"test", moduleCache, before, tokens, m, config, formattingConfig)
 		);
 	}
 
@@ -308,10 +329,24 @@ void assertAutoFix(string before, string after, const StaticAnalysisConfig confi
 
 	if (newCode != after)
 	{
+		bool onlyWhitespaceDiffers = newCode.replace("\t", "").replace(" ", "")
+			== after.replace("\t", "").replace(" ", "").replace("\r", "");
+
+		string formatDisplay(string code)
+		{
+			string ret = code.lineSplitter!(KeepTerminator.yes).map!(a => "\t" ~ a).join;
+			if (onlyWhitespaceDiffers)
+				ret = ret
+					.replace("\r", "\x1B[2m\\r\x1B[m")
+					.replace("\t", "\x1B[2m→   \x1B[m")
+					.replace(" ", "\x1B[2m⸱\x1B[m");
+			return ret;
+		}
+
 		throw new AssertError("Applying autofix didn't yield expected results. Expected:\n"
-			~ after.lineSplitter!(KeepTerminator.yes).map!(a => "\t" ~ a).join
+			~ formatDisplay(after)
 			~ "\n\nActual:\n"
-			~ newCode.lineSplitter!(KeepTerminator.yes).map!(a => "\t" ~ a).join,
+			~ formatDisplay(newCode),
 			file, line);
 	}
 }

src/dscanner/analysis/run.d

@@ -775,8 +775,10 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a
 
 AutoFix.CodeReplacement[] resolveAutoFix(const Message message,
 	const AutoFix.ResolveContext resolve, string fileName,
-	ref ModuleCache moduleCache, const(Token)[] tokens, const Module m,
+	ref ModuleCache moduleCache, scope const(char)[] rawCode,
-	const StaticAnalysisConfig analysisConfig)
+	scope const(Token)[] tokens, const Module m,
+	const StaticAnalysisConfig analysisConfig,
+	const AutoFixFormatting formattingConfig)
 {
 	import dsymbol.symbol : DSymbol;
 
@@ -797,7 +799,7 @@ AutoFix.CodeReplacement[] resolveAutoFix(const Message message,
 	{
 		if (check.getName() == message.checkName)
 		{
-			return check.resolveAutoFix(m, tokens, message, resolve);
+			return check.resolveAutoFix(m, rawCode, tokens, message, resolve, formattingConfig);
 		}
 	}
 

src/dscanner/analysis/static_if_else.d

@@ -51,7 +51,7 @@ final class StaticIfElse : BaseAnalyzer
 		addErrorMessage(tokens, KEY, "Mismatched static if. Use 'else static if' here.",
 			[
 				AutoFix.insertionBefore(tokens[$ - 1], "static "),
-				// TODO: make if explicit with block {}, using correct indentation
+				AutoFix.resolveLater("Wrap '{}' block around 'if'", [tokens[0].index, ifStmt.tokens[$ - 1].index, 0])
 			]);
 	}
 
@@ -60,6 +60,35 @@ final class StaticIfElse : BaseAnalyzer
 		return safeAccess(cc).falseStatement.statement.statementNoCaseNoDefault.ifStatement;
 	}
 
+	override AutoFix.CodeReplacement[] resolveAutoFix(
+		const Module mod,
+		scope const(char)[] rawCode,
+		scope const(Token)[] tokens,
+		const Message message,
+		const AutoFix.ResolveContext context,
+		const AutoFixFormatting formatting,
+	)
+	{
+		import dscanner.analysis.helpers : getLineIndentation;
+		import std.algorithm : countUntil;
+
+		auto beforeElse = tokens.countUntil!(a => a.index == context.params[0]);
+		auto lastToken = tokens.countUntil!(a => a.index == context.params[1]);
+		if (beforeElse == -1 || lastToken == -1)
+			throw new Exception("got different tokens than what was used to generate this autofix");
+
+		auto indentation = getLineIndentation(rawCode, tokens, tokens[beforeElse].line);
+
+		string beforeIf = formatting.getWhitespaceBeforeOpeningBrace(indentation, false)
+			~ "{" ~ formatting.eol ~ indentation;
+		string afterIf = formatting.eol ~ indentation ~ "}";
+
+		return AutoFix.replacement([tokens[beforeElse].index + 4, tokens[beforeElse + 1].index], beforeIf, "")
+			.concat(AutoFix.indentLines(tokens[beforeElse + 1 .. lastToken + 1], formatting))
+			.concat(AutoFix.insertionAfter(tokens[lastToken], afterIf))
+			.expectReplacements;
+	}
+
 	enum KEY = "dscanner.suspicious.static_if_else";
 }
 
@@ -96,16 +125,46 @@ unittest
 		void foo() {
 			static if (false)
 				auto a = 0;
-			else if (true) // fix
+			else if (true) // fix:0
 				auto b = 1;
 		}
+		void bar() {
+			static if (false)
+				auto a = 0;
+			else if (true) // fix:1
+				auto b = 1;
+		}
+		void baz() {
+			static if (false)
+				auto a = 0;
+			else if (true) { // fix:1
+				auto b = 1;
+			}
+		}
 	}c, q{
 		void foo() {
 			static if (false)
 				auto a = 0;
-			else static if (true) // fix
+			else static if (true) // fix:0
 				auto b = 1;
 		}
+		void bar() {
+			static if (false)
+				auto a = 0;
+			else {
+				if (true) // fix:1
+					auto b = 1;
+			}
+		}
+		void baz() {
+			static if (false)
+				auto a = 0;
+			else {
+				if (true) { // fix:1
+					auto b = 1;
+				}
+			}
+		}
 	}c, sac);
 
 	stderr.writeln("Unittest for StaticIfElse passed.");