From 93aae57469365c8a967faaa3a700e7459957b957 Mon Sep 17 00:00:00 2001 From: WebFreak001 Date: Sat, 8 Jul 2023 22:21:43 +0200 Subject: [PATCH] add autofix testing API --- src/dscanner/analysis/auto_function.d | 11 ++ src/dscanner/analysis/base.d | 2 +- src/dscanner/analysis/del.d | 24 +++- src/dscanner/analysis/duplicate_attribute.d | 35 +++++ src/dscanner/analysis/enumarrayliteral.d | 22 ++++ .../analysis/explicitly_annotated_unittests.d | 28 +++- src/dscanner/analysis/final_attribute.d | 63 ++++++++- src/dscanner/analysis/function_attributes.d | 53 ++++++++ src/dscanner/analysis/helpers.d | 123 +++++++++++++++++- src/dscanner/analysis/lambda_return_check.d | 36 ++++- src/dscanner/analysis/length_subtraction.d | 14 ++ src/dscanner/analysis/run.d | 84 ++++++++---- src/dscanner/analysis/static_if_else.d | 20 ++- 13 files changed, 476 insertions(+), 39 deletions(-) diff --git a/src/dscanner/analysis/auto_function.d b/src/dscanner/analysis/auto_function.d index 00384e2..8bfce40 100644 --- a/src/dscanner/analysis/auto_function.d +++ b/src/dscanner/analysis/auto_function.d @@ -270,5 +270,16 @@ unittest auto doStuff(){ mixin(_genSave);} }, sac); + + assertAutoFix(q{ + auto doStuff(){} // fix + @property doStuff(){} // fix + @safe doStuff(){} // fix + }c, q{ + void doStuff(){} // fix + @property void doStuff(){} // fix + @safe void doStuff(){} // fix + }c, sac); + stderr.writeln("Unittest for AutoFunctionChecker passed."); } diff --git a/src/dscanner/analysis/base.d b/src/dscanner/analysis/base.d index 1cf910d..2651808 100644 --- a/src/dscanner/analysis/base.d +++ b/src/dscanner/analysis/base.d @@ -276,7 +276,7 @@ public: _messages = new MessageSet; } - protected string getName() + string getName() { assert(0); } diff --git a/src/dscanner/analysis/del.d b/src/dscanner/analysis/del.d index 9067ad9..ab79a7a 100644 --- a/src/dscanner/analysis/del.d +++ b/src/dscanner/analysis/del.d @@ -37,8 +37,8 @@ final class DeleteCheck : BaseAnalyzer unittest { - import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; + import dscanner.analysis.config : Check, disabledConfig, StaticAnalysisConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings, assertAutoFix; StaticAnalysisConfig sac = disabledConfig(); sac.delete_check = Check.enabled; @@ -55,5 +55,25 @@ unittest } }c, sac); + assertAutoFix(q{ + void testDelete() + { + int[int] data = [1 : 2]; + delete data[1]; // fix + + auto a = new Class(); + delete a; // fix + } + }c, q{ + void testDelete() + { + int[int] data = [1 : 2]; + destroy(data[1]); // fix + + auto a = new Class(); + destroy(a); // fix + } + }c, sac); + stderr.writeln("Unittest for DeleteCheck passed."); } diff --git a/src/dscanner/analysis/duplicate_attribute.d b/src/dscanner/analysis/duplicate_attribute.d index 9016a7e..7340cca 100644 --- a/src/dscanner/analysis/duplicate_attribute.d +++ b/src/dscanner/analysis/duplicate_attribute.d @@ -227,5 +227,40 @@ unittest } }c, sac); + + assertAutoFix(q{ + class ExampleAttributes + { + @property @property bool aaa() {} // fix + bool bbb() @safe @safe {} // fix + @system bool ccc() @system {} // fix + @trusted bool ddd() @trusted {} // fix + } + + class ExamplePureNoThrow + { + pure pure bool bbb() {} // fix + bool ccc() pure pure {} // fix + nothrow nothrow bool ddd() {} // fix + bool eee() nothrow nothrow {} // fix + } + }c, q{ + class ExampleAttributes + { + @property bool aaa() {} // fix + bool bbb() @safe {} // fix + @system bool ccc() {} // fix + @trusted bool ddd() {} // fix + } + + class ExamplePureNoThrow + { + pure bool bbb() {} // fix + bool ccc() pure {} // fix + nothrow bool ddd() {} // fix + bool eee() nothrow {} // fix + } + }c, sac); + stderr.writeln("Unittest for DuplicateAttributeCheck passed."); } diff --git a/src/dscanner/analysis/enumarrayliteral.d b/src/dscanner/analysis/enumarrayliteral.d index 8552f59..ec0ddca 100644 --- a/src/dscanner/analysis/enumarrayliteral.d +++ b/src/dscanner/analysis/enumarrayliteral.d @@ -59,3 +59,25 @@ final class EnumArrayLiteralCheck : BaseAnalyzer autoDec.accept(this); } } + +unittest +{ + import dscanner.analysis.config : Check, disabledConfig, StaticAnalysisConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings, assertAutoFix; + import std.stdio : stderr; + + StaticAnalysisConfig sac = disabledConfig(); + sac.enum_array_literal_check = Check.enabled; + assertAnalyzerWarnings(q{ + enum x = [1, 2, 3]; /+ + ^^^^^^^^^ [warn]: This enum may lead to unnecessary allocation at run-time. Use 'static immutable x = [ ...' instead. +/ + }c, sac); + + assertAutoFix(q{ + enum x = [1, 2, 3]; // fix + }c, q{ + static immutable x = [1, 2, 3]; // fix + }c, sac); + + stderr.writeln("Unittest for EnumArrayLiteralCheck passed."); +} diff --git a/src/dscanner/analysis/explicitly_annotated_unittests.d b/src/dscanner/analysis/explicitly_annotated_unittests.d index 10f14cb..3a32f0c 100644 --- a/src/dscanner/analysis/explicitly_annotated_unittests.d +++ b/src/dscanner/analysis/explicitly_annotated_unittests.d @@ -62,10 +62,10 @@ final class ExplicitlyAnnotatedUnittestCheck : BaseAnalyzer unittest { - import std.stdio : stderr; + import dscanner.analysis.config : Check, disabledConfig, StaticAnalysisConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings, assertAutoFix; import std.format : format; - import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; + import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.explicitly_annotated_unittests = Check.enabled; @@ -101,5 +101,27 @@ unittest ExplicitlyAnnotatedUnittestCheck.MESSAGE, ), sac); + + // nested + assertAutoFix(q{ + unittest {} // fix:0 + pure nothrow @nogc unittest {} // fix:0 + + struct Foo + { + unittest {} // fix:1 + pure nothrow @nogc unittest {} // fix:1 + } + }c, q{ + @safe unittest {} // fix:0 + pure nothrow @nogc @safe unittest {} // fix:0 + + struct Foo + { + @system unittest {} // fix:1 + pure nothrow @nogc @system unittest {} // fix:1 + } + }c, sac); + stderr.writeln("Unittest for ExplicitlyAnnotatedUnittestCheck passed."); } diff --git a/src/dscanner/analysis/final_attribute.d b/src/dscanner/analysis/final_attribute.d index 0ecaaf2..1832950 100644 --- a/src/dscanner/analysis/final_attribute.d +++ b/src/dscanner/analysis/final_attribute.d @@ -254,10 +254,10 @@ public: @system unittest { - import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; - import std.stdio : stderr; + import dscanner.analysis.config : Check, disabledConfig, StaticAnalysisConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings, assertAutoFix; import std.format : format; + import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.final_attribute_check = Check.enabled; @@ -433,5 +433,62 @@ public: } }, sac); + + assertAutoFix(q{ + final void foo(){} // fix + void foo(){final void foo(){}} // fix + void foo() + { + static if (true) + final class A{ private: final protected void foo(){}} // fix + } + final struct Foo{} // fix + final union Foo{} // fix + class Foo{private final void foo(){}} // fix + class Foo{private: final void foo(){}} // fix + interface Foo{final void foo(T)(){}} // fix + final class Foo{final void foo(){}} // fix + private: final class Foo {public: private final void foo(){}} // fix + class Foo {final static void foo(){}} // fix + class Foo + { + void foo(){} + static: final void foo(){} // fix + } + class Foo + { + void foo(){} + static{ final void foo(){}} // fix + void foo(){} + } + }, q{ + void foo(){} // fix + void foo(){ void foo(){}} // fix + void foo() + { + static if (true) + final class A{ private: protected void foo(){}} // fix + } + struct Foo{} // fix + union Foo{} // fix + class Foo{private void foo(){}} // fix + class Foo{private: void foo(){}} // fix + interface Foo{ void foo(T)(){}} // fix + final class Foo{ void foo(){}} // fix + private: final class Foo {public: private void foo(){}} // fix + class Foo { static void foo(){}} // fix + class Foo + { + void foo(){} + static: void foo(){} // fix + } + class Foo + { + void foo(){} + static{ void foo(){}} // fix + void foo(){} + } + }, sac); + stderr.writeln("Unittest for FinalAttributeChecker passed."); } diff --git a/src/dscanner/analysis/function_attributes.d b/src/dscanner/analysis/function_attributes.d index f8159e3..419acfc 100644 --- a/src/dscanner/analysis/function_attributes.d +++ b/src/dscanner/analysis/function_attributes.d @@ -223,5 +223,58 @@ unittest } }c, sac); + + assertAutoFix(q{ + int foo() @property { return 0; } + + class ClassName { + const int confusingConst() { return 0; } // fix:0 + const int confusingConst() { return 0; } // fix:1 + + int bar() @property { return 0; } // fix:0 + int bar() @property { return 0; } // fix:1 + int bar() @property { return 0; } // fix:2 + } + + struct StructName { + int bar() @property { return 0; } // fix:0 + } + + union UnionName { + int bar() @property { return 0; } // fix:0 + } + + interface InterfaceName { + int bar() @property; // fix:0 + + abstract int method(); // fix + } + }c, q{ + int foo() @property { return 0; } + + class ClassName { + int confusingConst() const { return 0; } // fix:0 + const(int) confusingConst() { return 0; } // fix:1 + + int bar() const @property { return 0; } // fix:0 + int bar() inout @property { return 0; } // fix:1 + int bar() immutable @property { return 0; } // fix:2 + } + + struct StructName { + int bar() const @property { return 0; } // fix:0 + } + + union UnionName { + int bar() const @property { return 0; } // fix:0 + } + + interface InterfaceName { + int bar() const @property; // fix:0 + + int method(); // fix + } + }c, sac); + stderr.writeln("Unittest for FunctionAttributeCheck passed."); } diff --git a/src/dscanner/analysis/helpers.d b/src/dscanner/analysis/helpers.d index 00e0ba2..05e0be8 100644 --- a/src/dscanner/analysis/helpers.d +++ b/src/dscanner/analysis/helpers.d @@ -6,9 +6,9 @@ module dscanner.analysis.helpers; import core.exception : AssertError; +import std.stdio; import std.string; import std.traits; -import std.stdio; import dparse.ast; import dparse.rollback_allocator; @@ -194,3 +194,124 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, throw new AssertError(message, file, line); } } + +/** + * This assert function will analyze the passed in code, get the warnings, and + * apply all specified autofixes all at once. + * + * Indicate which autofix to apply by adding a line comment at the end of the + * line with the following content: `// fix:0`, where 0 is the index which + * autofix to apply. There may only be one diagnostic on a line with this fix + * comment. Alternatively you can also just write `// fix` to apply the only + * available suggestion. + */ +void assertAutoFix(string before, string after, const StaticAnalysisConfig config, + string file = __FILE__, size_t line = __LINE__) +{ + import dparse.lexer : StringCache, Token; + import dscanner.analysis.run : parseModule; + import std.algorithm : canFind, findSplit, map, sort; + import std.conv : to; + import std.sumtype : match; + import std.typecons : tuple, Tuple; + + StringCache cache = StringCache(StringCache.defaultBucketCount); + RollbackAllocator r; + const(Token)[] tokens; + const(Module) m = parseModule(file, cast(ubyte[]) before, &r, defaultErrorFormat, cache, false, tokens); + + ModuleCache moduleCache; + + // Run the code and get any warnings + MessageSet rawWarnings = analyze("test", m, config, moduleCache, tokens); + string[] codeLines = before.splitLines(); + + Tuple!(Message, int)[] toApply; + int[] applyLines; + + scope (failure) + { + if (toApply.length) + stderr.writefln("Would have applied these fixes:%(\n- %s%)", + toApply.map!"a[0].autofixes[a[1]].name"); + else + stderr.writeln("Did not find any fixes at all up to this point."); + stderr.writeln("Found warnings on lines: ", rawWarnings[].map!(a + => a.endLine == 0 ? 0 : a.endLine - 1 + line)); + } + + foreach (rawWarning; rawWarnings[]) + { + // Skip the warning if it is on line zero + immutable size_t rawLine = rawWarning.endLine; + if (rawLine == 0) + { + stderr.writefln("!!! Skipping warning because it is on line zero:\n%s", + rawWarning.message); + continue; + } + + auto fixComment = codeLines[rawLine - 1].findSplit("// fix"); + if (fixComment[1].length) + { + applyLines ~= cast(int)rawLine - 1; + if (fixComment[2].startsWith(":")) + { + auto i = fixComment[2][1 .. $].to!int; + assert(i >= 0, "can't use negative autofix indices"); + if (i >= rawWarning.autofixes.length) + throw new AssertError("autofix index out of range, diagnostic only has %s autofixes (%s)." + .format(rawWarning.autofixes.length, rawWarning.autofixes.map!"a.name"), + file, rawLine + line); + toApply ~= tuple(rawWarning, i); + } + else + { + if (rawWarning.autofixes.length != 1) + throw new AssertError("diagnostic has %s autofixes (%s), but expected exactly one." + .format(rawWarning.autofixes.length, rawWarning.autofixes.map!"a.name"), + file, rawLine + line); + toApply ~= tuple(rawWarning, 0); + } + } + } + + foreach (i, codeLine; codeLines) + { + if (!applyLines.canFind(i) && codeLine.canFind("// fix")) + throw new AssertError("Missing expected warning for autofix on line %s" + .format(i + line), file, i + line); + } + + AutoFix.CodeReplacement[] replacements; + + foreach_reverse (pair; toApply) + { + 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, tokens, m, config) + ); + } + + replacements.sort!"a.range[0] < b.range[0]"; + + improveAutoFixWhitespace(before, replacements); + + string newCode = before; + foreach_reverse (replacement; replacements) + { + newCode = newCode[0 .. replacement.range[0]] ~ replacement.newText + ~ newCode[replacement.range[1] .. $]; + } + + if (newCode != after) + { + throw new AssertError("Applying autofix didn't yield expected results. Expected:\n" + ~ after.lineSplitter!(KeepTerminator.yes).map!(a => "\t" ~ a).join + ~ "\n\nActual:\n" + ~ newCode.lineSplitter!(KeepTerminator.yes).map!(a => "\t" ~ a).join, + file, line); + } +} diff --git a/src/dscanner/analysis/lambda_return_check.d b/src/dscanner/analysis/lambda_return_check.d index 2ed9e34..c0a1ab0 100644 --- a/src/dscanner/analysis/lambda_return_check.d +++ b/src/dscanner/analysis/lambda_return_check.d @@ -62,14 +62,14 @@ private: version(Windows) {/*because of newline in code*/} else unittest { - import dscanner.analysis.helpers : assertAnalyzerWarnings; - import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import dscanner.analysis.config : Check, disabledConfig, StaticAnalysisConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings, assertAutoFix; import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.lambda_return_check = Check.enabled; - auto code = ` + assertAnalyzerWarnings(q{ void main() { int[] b; @@ -81,7 +81,33 @@ unittest ^^^^^^^^ [warn]: This lambda returns a lambda. Add parenthesis to clarify. +/ pragma(msg, typeof({ return a; })); pragma(msg, typeof(a => () { return a; })); - }`c; - assertAnalyzerWarnings(code, sac); + } + }c, sac); + + + assertAutoFix(q{ + void main() + { + int[] b; + auto a = b.map!(a => { return a * a + 2; }).array(); // fix:0 + auto a = b.map!(a => { return a * a + 2; }).array(); // fix:1 + pragma(msg, typeof(a => { return a; })); // fix:0 + pragma(msg, typeof(a => { return a; })); // fix:1 + pragma(msg, typeof((a) => { return a; })); // fix:0 + pragma(msg, typeof((a) => { return a; })); // fix:1 + } + }c, q{ + void main() + { + int[] b; + auto a = b.map!((a) { return a * a + 2; }).array(); // fix:0 + auto a = b.map!(a => ({ return a * a + 2; })).array(); // fix:1 + pragma(msg, typeof((a) { return a; })); // fix:0 + pragma(msg, typeof(a => ({ return a; }))); // fix:1 + pragma(msg, typeof((a) { return a; })); // fix:0 + pragma(msg, typeof((a) => ({ return a; }))); // fix:1 + } + }c, sac); + stderr.writeln("Unittest for LambdaReturnCheck passed."); } diff --git a/src/dscanner/analysis/length_subtraction.d b/src/dscanner/analysis/length_subtraction.d index 06bc428..bc89183 100644 --- a/src/dscanner/analysis/length_subtraction.d +++ b/src/dscanner/analysis/length_subtraction.d @@ -65,5 +65,19 @@ unittest writeln("something"); } }c, sac); + + assertAutoFix(q{ + void testSizeT() + { + if (i < a.length - 1) // fix + writeln("something"); + } + }c, q{ + void testSizeT() + { + if (i < cast(ptrdiff_t) a.length - 1) // fix + writeln("something"); + } + }c, sac); stderr.writeln("Unittest for IfElseSameCheck passed."); } diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 3bc0eea..a8eb201 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -510,37 +510,23 @@ unittest assert(test("std.bar.foo", "-barr,+bar")); } -MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig analysisConfig, - ref ModuleCache moduleCache, const(Token)[] tokens, bool staticAnalyze = true) +private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName, + const(Token)[] tokens, const Module m, + const StaticAnalysisConfig analysisConfig, const Scope* moduleScope) { - import dsymbol.symbol : DSymbol; - - if (!staticAnalyze) - return null; - version (unittest) enum ut = true; else enum ut = false; + BaseAnalyzer[] checks; + string moduleName; if (m !is null && m.moduleDeclaration !is null && m.moduleDeclaration.moduleName !is null && m.moduleDeclaration.moduleName.identifiers !is null) moduleName = m.moduleDeclaration.moduleName.identifiers.map!(e => e.text).join("."); - scope first = new FirstPass(m, internString(fileName), &moduleCache, null); - first.run(); - - secondPass(first.rootSymbol, first.moduleScope, moduleCache); - auto moduleScope = first.moduleScope; - scope(exit) typeid(DSymbol).destroy(first.rootSymbol.acSymbol); - scope(exit) typeid(SemanticSymbol).destroy(first.rootSymbol); - scope(exit) typeid(Scope).destroy(first.moduleScope); - BaseAnalyzer[] checks; - - GC.disable; - if (moduleName.shouldRun!AsmStyleCheck(analysisConfig)) checks ~= new AsmStyleCheck(fileName, moduleScope, analysisConfig.asm_style_check == Check.skipTests && !ut); @@ -752,19 +738,73 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new IfStatementCheck(fileName, moduleScope, analysisConfig.redundant_if_check == Check.skipTests && !ut); + return checks; +} + +MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig analysisConfig, + ref ModuleCache moduleCache, const(Token)[] tokens, bool staticAnalyze = true) +{ + import dsymbol.symbol : DSymbol; + + if (!staticAnalyze) + return null; + + scope first = new FirstPass(m, internString(fileName), &moduleCache, null); + first.run(); + + secondPass(first.rootSymbol, first.moduleScope, moduleCache); + auto moduleScope = first.moduleScope; + scope(exit) typeid(DSymbol).destroy(first.rootSymbol.acSymbol); + scope(exit) typeid(SemanticSymbol).destroy(first.rootSymbol); + scope(exit) typeid(Scope).destroy(first.moduleScope); + + GC.disable; + scope (exit) + GC.enable; + MessageSet set = new MessageSet; - foreach (check; checks) + foreach (check; getAnalyzersForModuleAndConfig(fileName, tokens, m, analysisConfig, moduleScope)) { check.visit(m); foreach (message; check.messages) set.insert(message); } - GC.enable; - return set; } +AutoFix.CodeReplacement[] resolveAutoFix(const Message message, + const AutoFix.ResolveContext resolve, string fileName, + ref ModuleCache moduleCache, const(Token)[] tokens, const Module m, + const StaticAnalysisConfig analysisConfig) +{ + import dsymbol.symbol : DSymbol; + + scope first = new FirstPass(m, internString(fileName), &moduleCache, null); + first.run(); + + secondPass(first.rootSymbol, first.moduleScope, moduleCache); + auto moduleScope = first.moduleScope; + scope(exit) typeid(DSymbol).destroy(first.rootSymbol.acSymbol); + scope(exit) typeid(SemanticSymbol).destroy(first.rootSymbol); + scope(exit) typeid(Scope).destroy(first.moduleScope); + + GC.disable; + scope (exit) + GC.enable; + + foreach (BaseAnalyzer check; getAnalyzersForModuleAndConfig(fileName, tokens, m, analysisConfig, moduleScope)) + { + if (check.getName() == message.checkName) + { + return check.resolveAutoFix(m, tokens, message, resolve); + } + } + + throw new Exception("Cannot find analyzer " ~ message.checkName + ~ " to resolve autofix with."); +} + void improveAutoFixWhitespace(scope const(char)[] code, AutoFix.CodeReplacement[] replacements) { import std.ascii : isWhite; diff --git a/src/dscanner/analysis/static_if_else.d b/src/dscanner/analysis/static_if_else.d index 17efce1..0823b62 100644 --- a/src/dscanner/analysis/static_if_else.d +++ b/src/dscanner/analysis/static_if_else.d @@ -65,8 +65,8 @@ final class StaticIfElse : BaseAnalyzer unittest { - import dscanner.analysis.helpers : assertAnalyzerWarnings; - import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import dscanner.analysis.config : Check, disabledConfig, StaticAnalysisConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings, assertAutoFix; import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); @@ -92,5 +92,21 @@ unittest } }c, sac); + assertAutoFix(q{ + void foo() { + static if (false) + auto a = 0; + else if (true) // fix + auto b = 1; + } + }c, q{ + void foo() { + static if (false) + auto a = 0; + else static if (true) // fix + auto b = 1; + } + }c, sac); + stderr.writeln("Unittest for StaticIfElse passed."); }