add `dscanner fix` command

README.md

@@ -57,6 +57,32 @@ dscanner lint source/
 
 to view a human readable list of issues.
 
+Diagnostic types can be enabled/disabled using a configuration file, check out
+the `--config` argument / `dscanner.ini` file for more info. Tip: some IDEs that
+integrate D-Scanner may have helpers to configure the diagnostics or help
+generate the dscanner.ini file.
+<!--
+IDE list for overview:
+code-d has an "insert default dscanner.ini content" command + proprietary
+	disabling per-line (we really need to bring that into standard D-Scanner)
+-->
+
+## Auto-Fixing issues
+
+Use
+
+```sh
+dscanner fix source/
+```
+
+to interactively fix all fixable issues within the source directory. Call with
+`--applySingle` to automatically apply fixes that don't have multiple automatic
+solutions.
+
+## Tooling integration
+
+Many D editors already ship with D-Scanner.
+
 For a CLI / tool parsable output use either
 
 ```sh
@@ -75,16 +101,6 @@ dscanner -S -f github source/
 dscanner -S -f '{filepath}({line}:{column})[{type}]: {message}' source/
 ```
 
-Diagnostic types can be enabled/disabled using a configuration file, check out
-the `--config` argument / `dscanner.ini` file for more info. Tip: some IDEs that
-integrate D-Scanner may have helpers to configure the diagnostics or help
-generate the dscanner.ini file.
-<!--
-IDE list for overview:
-code-d has an "insert default dscanner.ini content" command + proprietary
-	disabling per-line (we really need to bring that into standard D-Scanner)
--->
-
 ## Other features
 
 ### Token Count

src/dscanner/analysis/base.d

@@ -41,11 +41,11 @@ struct AutoFix
 	/// `CodeReplacement[]` should be applied to the code in reverse, otherwise
 	/// an offset to the following start indices must be calculated and be kept
 	/// track of.
-	SumType!(CodeReplacement[], ResolveContext) autofix;
+	SumType!(CodeReplacement[], ResolveContext) replacements;
 
 	invariant
 	{
-		autofix.match!(
+		replacements.match!(
 			(const CodeReplacement[] replacement)
 			{
 				import std.algorithm : all, isSorted;
@@ -61,7 +61,7 @@ struct AutoFix
 	{
 		AutoFix ret;
 		ret.name = name;
-		ret.autofix = ResolveContext(params, extraInfo);
+		ret.replacements = ResolveContext(params, extraInfo);
 		return ret;
 	}
 
@@ -94,7 +94,7 @@ struct AutoFix
 	{
 		AutoFix ret;
 		ret.name = name;
-		ret.autofix = [
+		ret.replacements = [
 			AutoFix.CodeReplacement(range, newText)
 		];
 		return ret;
@@ -120,7 +120,7 @@ struct AutoFix
 			: content.strip.length
 				? "Insert `" ~ content.strip ~ "`"
 				: "Insert whitespace";
-		ret.autofix = [
+		ret.replacements = [
 			AutoFix.CodeReplacement([index, index], content)
 		];
 		return ret;
@@ -140,7 +140,7 @@ struct AutoFix
 		}
 		AutoFix ret;
 		ret.name = name;
-		ret.autofix = inserts;
+		ret.replacements = inserts;
 		return ret;
 	}
 
@@ -155,7 +155,7 @@ struct AutoFix
 		CodeReplacement[] concatenated = expectReplacements(errorMsg).dup
 			~ other.expectReplacements(errorMsg);
 		concatenated.sort!"a.range[0] < b.range[0]";
-		ret.autofix = concatenated;
+		ret.replacements = concatenated;
 		return ret;
 	}
 
@@ -163,7 +163,7 @@ struct AutoFix
 		string errorMsg = "Expected available code replacements, not something to resolve later"
 	) @safe pure nothrow @nogc
 	{
-		return autofix.match!(
+		return replacements.match!(
 			(replacement)
 			{
 				if (false) return CodeReplacement[].init;
@@ -179,7 +179,7 @@ struct AutoFix
 		string errorMsg = "Expected available code replacements, not something to resolve later"
 	) const @safe pure nothrow @nogc
 	{
-		return autofix.match!(
+		return replacements.match!(
 			(const replacement)
 			{
 				if (false) return CodeReplacement[].init;
@@ -195,8 +195,12 @@ struct AutoFix
 /// Formatting style for autofix generation (only available for resolve autofix)
 struct AutoFixFormatting
 {
+	enum AutoFixFormatting invalid = AutoFixFormatting(BraceStyle.invalid, null, 0, null);
+
 	enum BraceStyle
 	{
+		/// invalid, shouldn't appear in usable configs
+		invalid,
 		/// $(LINK https://en.wikipedia.org/wiki/Indent_style#Allman_style)
 		allman,
 		/// $(LINK https://en.wikipedia.org/wiki/Indent_style#Variant:_1TBS)
@@ -207,14 +211,30 @@ struct AutoFixFormatting
 		knr,
 	}
 
-	BraceStyle braceStyle;
+	/// Brace style config
+	BraceStyle braceStyle = BraceStyle.allman;
+	/// String to insert on indentations
 	string indentation = "\t";
+	/// For calculating indentation size
+	uint indentationWidth = 4;
+	/// String to insert on line endings
 	string eol = "\n";
 
+	invariant
+	{
+		import std.algorithm : all;
+
+		assert(!indentation.length
+			|| indentation == "\t"
+			|| indentation.all!(c => c == ' '));
+	}
+
 	string getWhitespaceBeforeOpeningBrace(string lastLineIndent, bool isFuncDecl) pure nothrow @safe const
 	{
 		final switch (braceStyle)
 		{
+		case BraceStyle.invalid:
+			assert(false, "invalid formatter config");
 		case BraceStyle.knr:
 			if (isFuncDecl)
 				goto case BraceStyle.allman;
@@ -386,7 +406,6 @@ public:
 
 	AutoFix.CodeReplacement[] resolveAutoFix(
 		const Module mod,
-		scope const(char)[] rawCode,
 		scope const(Token)[] tokens,
 		const Message message,
 		const AutoFix.ResolveContext context,
@@ -394,7 +413,6 @@ public:
 	)
 	{
 		cast(void) mod;
-		cast(void) rawCode;
 		cast(void) tokens;
 		cast(void) message;
 		cast(void) context;

src/dscanner/analysis/config.d

@@ -220,6 +220,69 @@ struct StaticAnalysisConfig
 
 	@INI("Module-specific filters")
 	ModuleFilters filters;
+
+	@INI("Formatting brace style for automatic fixes (allman, otbs, stroustrup, knr)")
+	string brace_style = "allman";
+
+	@INI("Formatting indentation style for automatic fixes (tab, space)")
+	string indentation_style = "tab";
+
+	@INI("Formatting indentation width for automatic fixes (space count, otherwise how wide a tab is)")
+	int indentation_width = 4;
+
+	@INI("Formatting line ending character (lf, cr, crlf)")
+	string eol_style = "lf";
+
+	auto getAutoFixFormattingConfig() const
+	{
+		import dscanner.analysis.base : AutoFixFormatting;
+		import std.array : array;
+		import std.conv : to;
+		import std.range : repeat;
+
+		if (indentation_width < 0)
+			throw new Exception("invalid negative indentation_width");
+
+		AutoFixFormatting ret;
+		ret.braceStyle = brace_style.to!(AutoFixFormatting.BraceStyle);
+		ret.indentationWidth = indentation_width;
+
+		switch (indentation_style)
+		{
+		case "tab":
+			ret.indentation = "\t";
+			break;
+		case "space":
+			static immutable string someSpaces = "                ";
+			if (indentation_width < someSpaces.length)
+				ret.indentation = someSpaces[0 .. indentation_width];
+			else
+				ret.indentation = ' '.repeat(indentation_width).array;
+			break;
+		default:
+			throw new Exception("invalid indentation_style: '" ~ indentation_style ~ "' (expected tab or space)");
+		}
+
+		switch (eol_style)
+		{
+		case "lf":
+		case "LF":
+			ret.eol = "\n";
+			break;
+		case "cr":
+		case "CR":
+			ret.eol = "\r";
+			break;
+		case "crlf":
+		case "CRLF":
+			ret.eol = "\r\n";
+			break;
+		default:
+			throw new Exception("invalid eol_style: '" ~ eol_style ~ "' (expected lf, cr or crlf)");
+		}
+
+		return ret;
+	}
 }
 
 private template ModuleFiltersMixin(A)

src/dscanner/analysis/helpers.d

@@ -41,18 +41,22 @@ 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)
+string getLineIndentation(scope const(Token)[] tokens, size_t line, const AutoFixFormatting formatting)
 {
 	import std.algorithm : countUntil;
+	import std.array : array;
+	import std.range : repeat;
 	import std.string : lastIndexOfAny;
 
 	auto idx = tokens.countUntil!(a => a.line == line);
-	if (idx == -1)
+	if (idx == -1 || tokens[idx].column <= 1 || !formatting.indentation.length)
 		return "";
 
-	auto indent = rawCode[0 .. tokens[idx].index];
+	auto indent = tokens[idx].column - 1;
-	auto nl = indent.lastIndexOfAny("\r\n");
+	if (formatting.indentation[0] == '\t')
-	return indent[nl + 1 .. $].idup;
+		return (cast(immutable)'\t').repeat(indent).array;
+	else
+		return (cast(immutable)' ').repeat(indent).array;
 }
 
 /**
@@ -243,7 +247,7 @@ void assertAutoFix(string before, string after, const StaticAnalysisConfig confi
 	ModuleCache moduleCache;
 
 	// Run the code and get any warnings
-	MessageSet rawWarnings = analyze("test", m, config, moduleCache, tokens);
+	MessageSet rawWarnings = analyze("test", m, config, moduleCache, tokens, true, true, formattingConfig);
 	string[] codeLines = before.splitLines();
 
 	Tuple!(Message, int)[] toApply;
@@ -309,11 +313,7 @@ void assertAutoFix(string before, string after, const StaticAnalysisConfig confi
 	{
 		Message message = pair[0];
 		AutoFix fix = message.autofixes[pair[1]];
-		replacements ~= fix.autofix.match!(
+		replacements ~= fix.expectReplacements;
-			(AutoFix.CodeReplacement[] r) => r,
-			(AutoFix.ResolveContext context) => resolveAutoFix(message, context,
-				"test", moduleCache, before, tokens, m, config, formattingConfig)
-		);
 	}
 
 	replacements.sort!"a.range[0] < b.range[0]";

src/dscanner/analysis/run.d

@@ -7,19 +7,19 @@ module dscanner.analysis.run;
 
 import core.memory : GC;
 
-import std.stdio;
+import dparse.ast;
-import std.array;
-import std.conv;
-import std.algorithm;
-import std.range;
-import std.array;
-import std.functional : toDelegate;
-import std.file : mkdirRecurse;
-import std.path : dirName;
 import dparse.lexer;
 import dparse.parser;
-import dparse.ast;
 import dparse.rollback_allocator;
+import std.algorithm;
+import std.array;
+import std.array;
+import std.conv;
+import std.file : mkdirRecurse;
+import std.functional : toDelegate;
+import std.path : dirName;
+import std.range;
+import std.stdio;
 import std.typecons : scoped;
 
 import std.experimental.allocator : CAllocatorImpl;
@@ -404,6 +404,140 @@ bool analyze(string[] fileNames, const StaticAnalysisConfig config, string error
 	return hasErrors;
 }
 
+/**
+ * Interactive automatic issue fixing for multiple files
+ *
+ * Returns: true if there were parse errors.
+ */
+bool autofix(string[] fileNames, const StaticAnalysisConfig config, string errorFormat,
+		ref StringCache cache, ref ModuleCache moduleCache, bool autoApplySingle,
+		const AutoFixFormatting overrideFormattingConfig = AutoFixFormatting.invalid)
+{
+	import std.format : format;
+
+	bool hasErrors;
+	foreach (fileName; fileNames)
+	{
+		auto code = readFile(fileName);
+		// Skip files that could not be read and continue with the rest
+		if (code.length == 0)
+			continue;
+		RollbackAllocator r;
+		uint errorCount;
+		uint warningCount;
+		const(Token)[] tokens;
+		const Module m = parseModule(fileName, code, &r, errorFormat, cache, false, tokens,
+				null, &errorCount, &warningCount);
+		assert(m);
+		if (errorCount > 0)
+			hasErrors = true;
+		MessageSet results = analyze(fileName, m, config, moduleCache, tokens, true, true, overrideFormattingConfig);
+		if (results is null)
+			continue;
+
+		AutoFix.CodeReplacement[] changes;
+		size_t index;
+		auto numAutofixes = results[].filter!(a => a.autofixes.length > (autoApplySingle ? 1 : 0)).count;
+		foreach (result; results[])
+		{
+			if (autoApplySingle && result.autofixes.length == 1)
+			{
+				changes ~= result.autofixes[0].expectReplacements;
+			}
+			else if (result.autofixes.length)
+			{
+				index++;
+				string fileProgress = format!"[%d / %d] "(index, numAutofixes);
+				messageFunctionFormat(fileProgress ~ errorFormat, result, false, code);
+
+				UserSelect selector;
+				selector.addSpecial(-1, "Skip", "0", "n", "s");
+				auto item = selector.show(result.autofixes.map!"a.name");
+				switch (item)
+				{
+				case -1:
+					break; // skip
+				default:
+					changes ~= result.autofixes[item].expectReplacements;
+					break;
+				}
+			}
+		}
+		if (changes.length)
+		{
+			changes.sort!"a.range[0] < b.range[0]";
+			improveAutoFixWhitespace(cast(const(char)[]) code, changes);
+			foreach_reverse (change; changes)
+				code = code[0 .. change.range[0]]
+					~ cast(const(ubyte)[])change.newText
+					~ code[change.range[1] .. $];
+			writeln("Writing changes to ", fileName);
+			writeFileSafe(fileName, code);
+		}
+	}
+	return hasErrors;
+}
+
+private struct UserSelect
+{
+	import std.string : strip;
+
+	struct SpecialAction
+	{
+		int id;
+		string title;
+		string[] shorthands;
+	}
+
+	SpecialAction[] specialActions;
+
+	void addSpecial(int id, string title, string[] shorthands...)
+	{
+		specialActions ~= SpecialAction(id, title, shorthands.dup);
+	}
+
+	/// Returns an integer in the range 0 - regularItems.length or a
+	/// SpecialAction id or -1 when EOF or empty.
+	int show(R)(R regularItems)
+	{
+		// TODO: implement interactive preview
+		// TODO: implement apply/skip all occurrences (per file or globally)
+		foreach (special; specialActions)
+			writefln("%s) %s", special.shorthands[0], special.title);
+		size_t i;
+		foreach (autofix; regularItems)
+			writefln("%d) %s", ++i, autofix);
+
+		while (true)
+		{
+			try
+			{
+				write(" > ");
+				stdout.flush();
+				string input = readln().strip;
+				if (!input.length)
+				{
+					writeln();
+					return -1;
+				}
+
+				foreach (special; specialActions)
+					if (special.shorthands.canFind(input))
+						return special.id;
+
+				int item = input.to!int;
+				if (item < 0 || item > regularItems.length)
+					throw new Exception("Selected option number out of range.");
+				return item;
+			}
+			catch (ConvException e)
+			{
+				writeln("Invalid selection, try again. ", e.message);
+			}
+		}
+	}
+}
+
 const(Module) parseModule(string fileName, ubyte[] code, RollbackAllocator* p,
 		ref StringCache cache, ref const(Token)[] tokens,
 		MessageDelegate dlgMessage, ulong* linesOfCode = null,
@@ -742,13 +876,20 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
 }
 
 MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig analysisConfig,
-		ref ModuleCache moduleCache, const(Token)[] tokens, bool staticAnalyze = true)
+		ref ModuleCache moduleCache, const(Token)[] tokens, bool staticAnalyze = true,
+		bool resolveAutoFixes = false,
+		const AutoFixFormatting overrideFormattingConfig = AutoFixFormatting.invalid)
 {
 	import dsymbol.symbol : DSymbol;
 
 	if (!staticAnalyze)
 		return null;
 
+	const(AutoFixFormatting) formattingConfig =
+		(resolveAutoFixes && overrideFormattingConfig is AutoFixFormatting.invalid)
+			? analysisConfig.getAutoFixFormattingConfig()
+			: overrideFormattingConfig;
+
 	scope first = new FirstPass(m, internString(fileName), &moduleCache, null);
 	first.run();
 
@@ -763,25 +904,56 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a
 		GC.enable;
 
 	MessageSet set = new MessageSet;
-	foreach (check; getAnalyzersForModuleAndConfig(fileName, tokens, m, analysisConfig, moduleScope))
+	foreach (BaseAnalyzer check; getAnalyzersForModuleAndConfig(fileName, tokens, m, analysisConfig, moduleScope))
 	{
 		check.visit(m);
 		foreach (message; check.messages)
+		{
+			if (resolveAutoFixes)
+				message.resolveMessageFromCheck(check, m, tokens, formattingConfig);
 			set.insert(message);
+		}
 	}
 
 	return set;
 }
 
+private void resolveMessageFromCheck(
+	ref Message message,
+	BaseAnalyzer check,
+	const Module m,
+	scope const(Token)[] tokens,
+	const AutoFixFormatting formattingConfig
+)
+{
+	import std.sumtype : match;
+
+	foreach (ref autofix; message.autofixes)
+	{
+		autofix.replacements.match!(
+			(AutoFix.ResolveContext context) {
+				autofix.replacements = check.resolveAutoFix(m, tokens,
+					message, context, formattingConfig);
+			},
+			(_) {}
+		);
+	}
+}
+
 AutoFix.CodeReplacement[] resolveAutoFix(const Message message,
 	const AutoFix.ResolveContext resolve, string fileName,
 	ref ModuleCache moduleCache, scope const(char)[] rawCode,
 	scope const(Token)[] tokens, const Module m,
 	const StaticAnalysisConfig analysisConfig,
-	const AutoFixFormatting formattingConfig)
+	const AutoFixFormatting overrideFormattingConfig = AutoFixFormatting.invalid)
 {
 	import dsymbol.symbol : DSymbol;
 
+	const(AutoFixFormatting) formattingConfig =
+		overrideFormattingConfig is AutoFixFormatting.invalid
+			? analysisConfig.getAutoFixFormattingConfig()
+			: overrideFormattingConfig;
+
 	scope first = new FirstPass(m, internString(fileName), &moduleCache, null);
 	first.run();
 
@@ -799,7 +971,7 @@ AutoFix.CodeReplacement[] resolveAutoFix(const Message message,
 	{
 		if (check.getName() == message.checkName)
 		{
-			return check.resolveAutoFix(m, rawCode, tokens, message, resolve, formattingConfig);
+			return check.resolveAutoFix(m, tokens, message, resolve, formattingConfig);
 		}
 	}
 

src/dscanner/analysis/static_if_else.d

@@ -62,7 +62,6 @@ final class StaticIfElse : BaseAnalyzer
 
 	override AutoFix.CodeReplacement[] resolveAutoFix(
 		const Module mod,
-		scope const(char)[] rawCode,
 		scope const(Token)[] tokens,
 		const Message message,
 		const AutoFix.ResolveContext context,
@@ -77,7 +76,7 @@ final class StaticIfElse : BaseAnalyzer
 		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);
+		auto indentation = getLineIndentation(tokens, tokens[beforeElse].line, formatting);
 
 		string beforeIf = formatting.getWhitespaceBeforeOpeningBrace(indentation, false)
 			~ "{" ~ formatting.eol ~ indentation;

src/dscanner/main.d

@@ -44,6 +44,7 @@ version (unittest)
 else
 	int main(string[] args)
 {
+	bool autofix;
 	bool sloc;
 	bool highlight;
 	bool ctags;
@@ -62,6 +63,7 @@ else
 	bool defaultConfig;
 	bool report;
 	bool skipTests;
+	bool applySingleFixes;
 	string reportFormat;
 	string reportFile;
 	string symbolName;
@@ -96,6 +98,7 @@ else
 				"report", &report,
 				"reportFormat", &reportFormat,
 				"reportFile", &reportFile,
+				"applySingle", &applySingleFixes,
 				"I", &importPaths,
 				"version", &printVersion,
 				"muffinButton", &muffin,
@@ -165,12 +168,25 @@ else
 		return 0;
 	}
 
-	if (args.length > 1 && args[1] == "lint")
+	if (args.length > 1)
 	{
-		args = args[0] ~ args[2 .. $];
+		switch (args[1])
-		styleCheck = true;
+		{
-		if (!errorFormat.length)
+		case "lint":
-			errorFormat = "pretty";
+			args = args[0] ~ args[2 .. $];
+			styleCheck = true;
+			if (!errorFormat.length)
+				errorFormat = "pretty";
+			break;
+		case "fix":
+			args = args[0] ~ args[2 .. $];
+			autofix = true;
+			if (!errorFormat.length)
+				errorFormat = "pretty";
+			break;
+		default:
+			break;
+		}
 	}
 
 	if (!errorFormat.length)
@@ -195,9 +211,11 @@ else
 	if (reportFormat.length || reportFile.length)
 		report = true;
 
-	immutable optionCount = count!"a"([sloc, highlight, ctags, tokenCount, syntaxCheck, ast, imports,
+	immutable optionCount = count!"a"([sloc, highlight, ctags, tokenCount,
-			outline, tokenDump, styleCheck, defaultConfig, report,
+			syntaxCheck, ast, imports, outline, tokenDump, styleCheck,
-			symbolName !is null, etags, etagsAll, recursiveImports]);
+			defaultConfig, report, autofix,
+			symbolName !is null, etags, etagsAll, recursiveImports,
+	]);
 	if (optionCount > 1)
 	{
 		stderr.writeln("Too many options specified");
@@ -268,7 +286,7 @@ else
 	{
 		stdout.printEtags(etagsAll, expandArgs(args));
 	}
-	else if (styleCheck)
+	else if (styleCheck || autofix)
 	{
 		StaticAnalysisConfig config = defaultStaticAnalysisConfig();
 		string s = configLocation is null ? getConfigurationLocation() : configLocation;
@@ -276,7 +294,12 @@ else
 			readINIFile(config, s);
 		if (skipTests)
 			config.enabled2SkipTests;
-		if (report)
+
+		if (autofix)
+		{
+			return .autofix(expandArgs(args), config, errorFormat, cache, moduleCache, applySingleFixes) ? 1 : 0;
+		}
+		else if (report)
 		{
 			switch (reportFormat)
 			{
@@ -369,6 +392,9 @@ void printHelp(string programName)
 Human-readable output:
     %1$s lint <options> <files...>
 
+Interactively fixing issues
+    %1$s fix [--applySingle] <files...>
+
 Parsable outputs:
     %1$s -S <options> <files...>
     %1$s --report <options> <files...>
@@ -494,7 +520,11 @@ Options:
 
     --skipTests
         Does not analyze code in unittests. Only works if --styleCheck
-        is specified.`,
+        is specified.
+
+    --applySingle
+        when running "dscanner fix", automatically apply all fixes that have
+        only one auto-fix.`,
 
     programName, defaultErrorFormat, errorFormatMap);
 }

src/dscanner/utils.d

@@ -80,6 +80,19 @@ ubyte[] readFile(string fileName)
 	return sourceCode;
 }
 
+void writeFileSafe(string filename, scope const(ubyte)[] content)
+{
+	import std.file : copy, PreserveAttributes, remove, write;
+	import std.path : baseName, buildPath, dirName;
+
+	string tempName = buildPath(filename.dirName, "." ~ filename.baseName ~ "~");
+
+	// FIXME: we are removing the optional BOM here
+	copy(filename, tempName, PreserveAttributes.yes);
+	write(filename, content);
+	remove(tempName);
+}
+
 string[] expandArgs(string[] args)
 {
 	import std.file : isFile, FileException, dirEntries, SpanMode;