diff --git a/src/dscanner/analysis/base.d b/src/dscanner/analysis/base.d index 2651808..212dcb0 100644 --- a/src/dscanner/analysis/base.d +++ b/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; - autofix.match!( - (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") - ); + CodeReplacement[] concatenated = expectReplacements(errorMsg).dup + ~ other.expectReplacements(errorMsg); 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); } diff --git a/src/dscanner/analysis/helpers.d b/src/dscanner/analysis/helpers.d index 05e0be8..c3fc0dc 100644 --- a/src/dscanner/analysis/helpers.d +++ b/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 std.experimental.allocator.mallocator; +import dsymbol.modulecache : ModuleCache; 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); } } diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index a8eb201..9d4d9a0 100644 --- a/src/dscanner/analysis/run.d +++ b/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, - const StaticAnalysisConfig analysisConfig) + ref ModuleCache moduleCache, scope const(char)[] rawCode, + 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); } } diff --git a/src/dscanner/analysis/static_if_else.d b/src/dscanner/analysis/static_if_else.d index 0823b62..b632837 100644 --- a/src/dscanner/analysis/static_if_else.d +++ b/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.");