From 4194e6af0c312a7f69faa95b8640c889d66e183c Mon Sep 17 00:00:00 2001 From: WebFreak001 Date: Fri, 7 Jul 2023 17:42:28 +0200 Subject: [PATCH] add `dscanner fix` command --- README.md | 36 +++-- src/dscanner/analysis/base.d | 42 ++++-- src/dscanner/analysis/config.d | 63 ++++++++ src/dscanner/analysis/helpers.d | 22 +-- src/dscanner/analysis/run.d | 200 +++++++++++++++++++++++-- src/dscanner/analysis/static_if_else.d | 3 +- src/dscanner/main.d | 52 +++++-- src/dscanner/utils.d | 13 ++ 8 files changed, 371 insertions(+), 60 deletions(-) diff --git a/README.md b/README.md index f40319a..617a6cc 100644 --- a/README.md +++ b/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. + + +## 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. - - ## Other features ### Token Count diff --git a/src/dscanner/analysis/base.d b/src/dscanner/analysis/base.d index 212dcb0..112b956 100644 --- a/src/dscanner/analysis/base.d +++ b/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; diff --git a/src/dscanner/analysis/config.d b/src/dscanner/analysis/config.d index 886bc0b..7f4a536 100644 --- a/src/dscanner/analysis/config.d +++ b/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) diff --git a/src/dscanner/analysis/helpers.d b/src/dscanner/analysis/helpers.d index c3fc0dc..d9ac658 100644 --- a/src/dscanner/analysis/helpers.d +++ b/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 nl = indent.lastIndexOfAny("\r\n"); - return indent[nl + 1 .. $].idup; + auto indent = tokens[idx].column - 1; + if (formatting.indentation[0] == '\t') + 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!( - (AutoFix.CodeReplacement[] r) => r, - (AutoFix.ResolveContext context) => resolveAutoFix(message, context, - "test", moduleCache, before, tokens, m, config, formattingConfig) - ); + replacements ~= fix.expectReplacements; } replacements.sort!"a.range[0] < b.range[0]"; diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 9d4d9a0..4aa0320 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -7,19 +7,19 @@ module dscanner.analysis.run; import core.memory : GC; -import std.stdio; -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.ast; 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); } } diff --git a/src/dscanner/analysis/static_if_else.d b/src/dscanner/analysis/static_if_else.d index b632837..f8386ea 100644 --- a/src/dscanner/analysis/static_if_else.d +++ b/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; diff --git a/src/dscanner/main.d b/src/dscanner/main.d index ab2eaba..e87950b 100644 --- a/src/dscanner/main.d +++ b/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 .. $]; - styleCheck = true; - if (!errorFormat.length) - errorFormat = "pretty"; + switch (args[1]) + { + case "lint": + 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, - outline, tokenDump, styleCheck, defaultConfig, report, - symbolName !is null, etags, etagsAll, recursiveImports]); + immutable optionCount = count!"a"([sloc, highlight, ctags, tokenCount, + syntaxCheck, ast, imports, outline, tokenDump, styleCheck, + 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 +Interactively fixing issues + %1$s fix [--applySingle] + Parsable outputs: %1$s -S %1$s --report @@ -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); } diff --git a/src/dscanner/utils.d b/src/dscanner/utils.d index 6940faf..4c4c9f2 100644 --- a/src/dscanner/utils.d +++ b/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;