diff --git a/src/dscanner/analysis/auto_function.d b/src/dscanner/analysis/auto_function.d index cf9b14c..00384e2 100644 --- a/src/dscanner/analysis/auto_function.d +++ b/src/dscanner/analysis/auto_function.d @@ -76,10 +76,12 @@ public: auto tok = autoTokens[$ - 1]; auto whitespace = tok.column + (tok.text.length ? tok.text.length : str(tok.type).length); auto whitespaceIndex = tok.index + (tok.text.length ? tok.text.length : str(tok.type).length); - addErrorMessage([whitespaceIndex, whitespaceIndex + 1], tok.line, [whitespace, whitespace + 1], KEY, MESSAGE_INSERT); + addErrorMessage([whitespaceIndex, whitespaceIndex + 1], tok.line, [whitespace, whitespace + 1], KEY, MESSAGE_INSERT, + [AutoFix.insertionAt(whitespaceIndex + 1, "void ")]); } else - addErrorMessage(autoTokens, KEY, MESSAGE); + addErrorMessage(autoTokens, KEY, MESSAGE, + [AutoFix.replacement(autoTokens[0], "void")]); } } diff --git a/src/dscanner/analysis/base.d b/src/dscanner/analysis/base.d index a853d60..1cf910d 100644 --- a/src/dscanner/analysis/base.d +++ b/src/dscanner/analysis/base.d @@ -1,14 +1,159 @@ module dscanner.analysis.base; +import dparse.ast; +import dparse.lexer : IdType, str, Token; +import dsymbol.scope_ : Scope; +import std.array; import std.container; import std.string; -import dparse.ast; -import std.array; -import dsymbol.scope_ : Scope; -import dparse.lexer : Token, str, IdType; +import std.sumtype; +/// +struct AutoFix +{ + /// + struct CodeReplacement + { + /// Byte index `[start, end)` within the file what text to replace. + /// `start == end` if text is only getting inserted. + size_t[2] range; + /// The new text to put inside the range. (empty to delete text) + string newText; + } + + /// Context that the analyzer resolve method can use to generate the + /// resolved `CodeReplacement` with. + struct ResolveContext + { + /// Arbitrary analyzer-defined parameters. May grow in the future with + /// more items. + ulong[3] params; + /// For dynamically sized data, may contain binary data. + string extraInfo; + } + + /// Display name for the UI. + string name; + /// Either code replacements, sorted by range start, never overlapping, or a + /// context that can be passed to `BaseAnalyzer.resolveAutoFix` along with + /// the message key from the parent `Message` object. + /// + /// `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; + + invariant + { + autofix.match!( + (const CodeReplacement[] replacement) + { + import std.algorithm : all, isSorted; + + assert(replacement.all!"a.range[0] <= a.range[1]"); + assert(replacement.isSorted!"a.range[0] < b.range[0]"); + }, + (_) {} + ); + } + + static AutoFix replacement(const Token token, string newText, string name = null) + { + if (!name.length) + { + auto text = token.text.length ? token.text : str(token.type); + if (newText.length) + name = "Replace `" ~ text ~ "` with `" ~ newText ~ "`"; + else + name = "Remove `" ~ text ~ "`"; + } + return replacement([token], newText, name); + } + + static AutoFix replacement(const BaseNode node, string newText, string name) + { + return replacement(node.tokens, newText, name); + } + + static AutoFix replacement(const Token[] tokens, string newText, string name) + in(tokens.length > 0, "must provide at least one token") + { + auto end = tokens[$ - 1].text.length ? tokens[$ - 1].text : str(tokens[$ - 1].type); + return replacement([tokens[0].index, tokens[$ - 1].index + end.length], newText, name); + } + + static AutoFix replacement(size_t[2] range, string newText, string name) + { + AutoFix ret; + ret.name = name; + ret.autofix = [ + AutoFix.CodeReplacement(range, newText) + ]; + return ret; + } + + static AutoFix insertionBefore(const Token token, string content, string name = null) + { + return insertionAt(token.index, content, name); + } + + static AutoFix insertionAfter(const Token token, string content, string name = null) + { + auto tokenText = token.text.length ? token.text : str(token.type); + return insertionAt(token.index + tokenText.length, content, name); + } + + static AutoFix insertionAt(size_t index, string content, string name = null) + { + assert(content.length > 0, "generated auto fix inserting text without content"); + AutoFix ret; + ret.name = name.length + ? name + : content.strip.length + ? "Insert `" ~ content.strip ~ "`" + : "Insert whitespace"; + ret.autofix = [ + AutoFix.CodeReplacement([index, index], content) + ]; + return ret; + } + + AutoFix concat(AutoFix other) const + { + import std.algorithm : sort; + + 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") + ); + concatenated.sort!"a.range[0] < b.range[0]"; + ret.autofix = concatenated; + return ret; + } +} + +/// A diagnostic message. Each message defines one issue in the file, which +/// consists of one or more squiggly line ranges within the code, as well as +/// human readable descriptions and optionally also one or more automatic code +/// fixes that can be applied. struct Message { + /// A squiggly line range within the code. May be the issue itself if it's + /// the `diagnostic` member or supplemental information that can aid the + /// user in resolving the issue. struct Diagnostic { /// Name of the file where the warning was triggered. @@ -22,8 +167,6 @@ struct Message /// Warning message, may be null for supplemental diagnostics. string message; - // TODO: add auto-fix suggestion API here - deprecated("Use startLine instead") alias line = startLine; deprecated("Use startColumn instead") alias column = startColumn; @@ -74,6 +217,10 @@ struct Message /// Check name string checkName; + /// Either immediate code changes that can be applied or context to call + /// the `BaseAnalyzer.resolveAutoFix` method with. + AutoFix[] autofixes; + deprecated this(string fileName, size_t line, size_t column, string key = null, string message = null, string checkName = null) { diagnostic.fileName = fileName; @@ -84,19 +231,21 @@ struct Message this.checkName = checkName; } - this(Diagnostic diagnostic, string key = null, string checkName = null) + this(Diagnostic diagnostic, string key = null, string checkName = null, AutoFix[] autofixes = null) { this.diagnostic = diagnostic; this.key = key; this.checkName = checkName; + this.autofixes = autofixes; } - this(Diagnostic diagnostic, Diagnostic[] supplemental, string key = null, string checkName = null) + this(Diagnostic diagnostic, Diagnostic[] supplemental, string key = null, string checkName = null, AutoFix[] autofixes = null) { this.diagnostic = diagnostic; this.supplemental = supplemental; this.key = key; this.checkName = checkName; + this.autofixes = autofixes; } alias diagnostic this; @@ -151,6 +300,20 @@ public: unittest_.accept(this); } + AutoFix.CodeReplacement[] resolveAutoFix( + const Module mod, + const(Token)[] tokens, + const Message message, + const AutoFix.ResolveContext context + ) + { + cast(void) mod; + cast(void) tokens; + cast(void) message; + cast(void) context; + assert(0); + } + protected: bool inAggregate; @@ -172,40 +335,40 @@ protected: _messages.insert(Message(fileName, line, column, key, message, getName())); } - void addErrorMessage(const BaseNode node, string key, string message) + void addErrorMessage(const BaseNode node, string key, string message, AutoFix[] autofixes = null) { - addErrorMessage(Message.Diagnostic.from(fileName, node, message), key); + addErrorMessage(Message.Diagnostic.from(fileName, node, message), key, autofixes); } - void addErrorMessage(const Token token, string key, string message) + void addErrorMessage(const Token token, string key, string message, AutoFix[] autofixes = null) { - addErrorMessage(Message.Diagnostic.from(fileName, token, message), key); + addErrorMessage(Message.Diagnostic.from(fileName, token, message), key, autofixes); } - void addErrorMessage(const Token[] tokens, string key, string message) + void addErrorMessage(const Token[] tokens, string key, string message, AutoFix[] autofixes = null) { - addErrorMessage(Message.Diagnostic.from(fileName, tokens, message), key); + addErrorMessage(Message.Diagnostic.from(fileName, tokens, message), key, autofixes); } - void addErrorMessage(size_t[2] index, size_t line, size_t[2] columns, string key, string message) + void addErrorMessage(size_t[2] index, size_t line, size_t[2] columns, string key, string message, AutoFix[] autofixes = null) { - addErrorMessage(index, [line, line], columns, key, message); + addErrorMessage(index, [line, line], columns, key, message, autofixes); } - void addErrorMessage(size_t[2] index, size_t[2] lines, size_t[2] columns, string key, string message) + void addErrorMessage(size_t[2] index, size_t[2] lines, size_t[2] columns, string key, string message, AutoFix[] autofixes = null) { auto d = Message.Diagnostic.from(fileName, index, lines, columns, message); - _messages.insert(Message(d, key, getName())); + _messages.insert(Message(d, key, getName(), autofixes)); } - void addErrorMessage(Message.Diagnostic diagnostic, string key) + void addErrorMessage(Message.Diagnostic diagnostic, string key, AutoFix[] autofixes = null) { - _messages.insert(Message(diagnostic, key, getName())); + _messages.insert(Message(diagnostic, key, getName(), autofixes)); } - void addErrorMessage(Message.Diagnostic diagnostic, Message.Diagnostic[] supplemental, string key) + void addErrorMessage(Message.Diagnostic diagnostic, Message.Diagnostic[] supplemental, string key, AutoFix[] autofixes = null) { - _messages.insert(Message(diagnostic, supplemental, key, getName())); + _messages.insert(Message(diagnostic, supplemental, key, getName(), autofixes)); } /** diff --git a/src/dscanner/analysis/del.d b/src/dscanner/analysis/del.d index e32aba3..9067ad9 100644 --- a/src/dscanner/analysis/del.d +++ b/src/dscanner/analysis/del.d @@ -28,7 +28,9 @@ final class DeleteCheck : BaseAnalyzer override void visit(const DeleteExpression d) { addErrorMessage(d.tokens[0], "dscanner.deprecated.delete_keyword", - "Avoid using the 'delete' keyword."); + "Avoid using the 'delete' keyword.", + [AutoFix.replacement(d.tokens[0], `destroy(`, "Replace delete with destroy()") + .concat(AutoFix.insertionAfter(d.tokens[$ - 1], ")"))]); d.accept(this); } } diff --git a/src/dscanner/analysis/duplicate_attribute.d b/src/dscanner/analysis/duplicate_attribute.d index 59b5afe..9016a7e 100644 --- a/src/dscanner/analysis/duplicate_attribute.d +++ b/src/dscanner/analysis/duplicate_attribute.d @@ -93,7 +93,8 @@ final class DuplicateAttributeCheck : BaseAnalyzer if (hasAttribute) { string message = "Attribute '%s' is duplicated.".format(attributeName); - addErrorMessage(tokens, "dscanner.unnecessary.duplicate_attribute", message); + addErrorMessage(tokens, "dscanner.unnecessary.duplicate_attribute", message, + [AutoFix.replacement(tokens, "", "Remove second attribute " ~ attributeName)]); } // Mark it as having that attribute diff --git a/src/dscanner/analysis/enumarrayliteral.d b/src/dscanner/analysis/enumarrayliteral.d index 68f973f..8552f59 100644 --- a/src/dscanner/analysis/enumarrayliteral.d +++ b/src/dscanner/analysis/enumarrayliteral.d @@ -8,7 +8,7 @@ module dscanner.analysis.enumarrayliteral; import dparse.ast; import dparse.lexer; import dscanner.analysis.base; -import std.algorithm : canFind, map; +import std.algorithm : find, map; import dsymbol.scope_ : Scope; void doNothing(string, size_t, size_t, string, bool) @@ -35,7 +35,8 @@ final class EnumArrayLiteralCheck : BaseAnalyzer override void visit(const AutoDeclaration autoDec) { - if (autoDec.storageClasses.canFind!(a => a.token == tok!"enum")) + auto enumToken = autoDec.storageClasses.find!(a => a.token == tok!"enum"); + if (enumToken.length) { foreach (part; autoDec.parts) { @@ -49,7 +50,10 @@ final class EnumArrayLiteralCheck : BaseAnalyzer "dscanner.performance.enum_array_literal", "This enum may lead to unnecessary allocation at run-time." ~ " Use 'static immutable " - ~ part.identifier.text ~ " = [ ...' instead."); + ~ part.identifier.text ~ " = [ ...' instead.", + [ + AutoFix.replacement(enumToken[0].token, "static immutable") + ]); } } autoDec.accept(this); diff --git a/src/dscanner/analysis/explicitly_annotated_unittests.d b/src/dscanner/analysis/explicitly_annotated_unittests.d index 680c959..10f14cb 100644 --- a/src/dscanner/analysis/explicitly_annotated_unittests.d +++ b/src/dscanner/analysis/explicitly_annotated_unittests.d @@ -44,7 +44,14 @@ final class ExplicitlyAnnotatedUnittestCheck : BaseAnalyzer } } if (!isSafeOrSystem) - addErrorMessage(decl.unittest_.findTokenForDisplay(tok!"unittest"), KEY, MESSAGE); + { + auto token = decl.unittest_.findTokenForDisplay(tok!"unittest"); + addErrorMessage(token, KEY, MESSAGE, + [ + AutoFix.insertionBefore(token[0], "@safe ", "Mark unittest @safe"), + AutoFix.insertionBefore(token[0], "@system ", "Mark unittest @system") + ]); + } } decl.accept(this); } diff --git a/src/dscanner/analysis/final_attribute.d b/src/dscanner/analysis/final_attribute.d index 66cc284..0ecaaf2 100644 --- a/src/dscanner/analysis/final_attribute.d +++ b/src/dscanner/analysis/final_attribute.d @@ -57,7 +57,8 @@ private: void addError(T)(const Token finalToken, T t, string msg) { import std.format : format; - addErrorMessage(finalToken.type ? finalToken : t.name, KEY, MSGB.format(msg)); + addErrorMessage(finalToken.type ? finalToken : t.name, KEY, MSGB.format(msg), + [AutoFix.replacement(finalToken, "")]); } public: diff --git a/src/dscanner/analysis/function_attributes.d b/src/dscanner/analysis/function_attributes.d index 1560850..f8159e3 100644 --- a/src/dscanner/analysis/function_attributes.d +++ b/src/dscanner/analysis/function_attributes.d @@ -104,9 +104,15 @@ final class FunctionAttributeCheck : BaseAnalyzer } if (foundProperty && !foundConst) { + auto paren = dec.parameters.tokens.length ? dec.parameters.tokens[$ - 1] : Token.init; + auto autofixes = paren is Token.init ? null : [ + AutoFix.insertionAfter(paren, " const", "Mark function `const`"), + AutoFix.insertionAfter(paren, " inout", "Mark function `inout`"), + AutoFix.insertionAfter(paren, " immutable", "Mark function `immutable`"), + ]; addErrorMessage(dec.name, KEY, "Zero-parameter '@property' function should be" - ~ " marked 'const', 'inout', or 'immutable'."); + ~ " marked 'const', 'inout', or 'immutable'.", autofixes); } } dec.accept(this); @@ -123,7 +129,8 @@ final class FunctionAttributeCheck : BaseAnalyzer continue; if (attr.attribute == tok!"abstract" && inInterface) { - addErrorMessage(attr.attribute, KEY, ABSTRACT_MESSAGE); + addErrorMessage(attr.attribute, KEY, ABSTRACT_MESSAGE, + [AutoFix.replacement(attr.attribute, "")]); continue; } if (attr.attribute == tok!"static") @@ -136,9 +143,21 @@ final class FunctionAttributeCheck : BaseAnalyzer import std.string : format; immutable string attrString = str(attr.attribute.type); + AutoFix[] autofixes; + if (dec.functionDeclaration.parameters) + autofixes ~= AutoFix.replacement( + attr.attribute, "", + "Move " ~ str(attr.attribute.type) ~ " after parameter list") + .concat(AutoFix.insertionAfter( + dec.functionDeclaration.parameters.tokens[$ - 1], + " " ~ str(attr.attribute.type))); + if (dec.functionDeclaration.returnType) + autofixes ~= AutoFix.insertionAfter(attr.attribute, "(", "Make return type const") + .concat(AutoFix.insertionAfter(dec.functionDeclaration.returnType.tokens[$ - 1], ")")); addErrorMessage(attr.attribute, KEY, format( - "'%s' is not an attribute of the return type." ~ " Place it after the parameter list to clarify.", - attrString)); + "'%s' is not an attribute of the return type." + ~ " Place it after the parameter list to clarify.", + attrString), autofixes); } } end: diff --git a/src/dscanner/analysis/lambda_return_check.d b/src/dscanner/analysis/lambda_return_check.d index 876653f..2ed9e34 100644 --- a/src/dscanner/analysis/lambda_return_check.d +++ b/src/dscanner/analysis/lambda_return_check.d @@ -23,6 +23,8 @@ final class LambdaReturnCheck : BaseAnalyzer override void visit(const FunctionLiteralExpression fLit) { + import std.algorithm : find; + auto fe = safeAccess(fLit).assignExpression.as!UnaryExpression .primaryExpression.functionLiteralExpression.unwrap; @@ -35,7 +37,22 @@ final class LambdaReturnCheck : BaseAnalyzer auto endIncl = &fe.specifiedFunctionBody.tokens[0]; assert(endIncl >= start); auto tokens = start[0 .. endIncl - start + 1]; - addErrorMessage(tokens, KEY, "This lambda returns a lambda. Add parenthesis to clarify."); + auto arrow = tokens.find!(a => a.type == tok!"=>"); + + AutoFix[] autofixes; + if (arrow.length) + { + if (fLit.tokens[0] == tok!"(") + autofixes ~= AutoFix.replacement(arrow[0], "", "Remove arrow (use function body)"); + else + autofixes ~= AutoFix.insertionBefore(fLit.tokens[0], "(", "Remove arrow (use function body)") + .concat(AutoFix.insertionAfter(fLit.tokens[0], ")")) + .concat(AutoFix.replacement(arrow[0], "")); + } + autofixes ~= AutoFix.insertionBefore(*endIncl, "(", "Add parenthesis (return delegate)") + .concat(AutoFix.insertionAfter(fe.specifiedFunctionBody.tokens[$ - 1], ")")); + addErrorMessage(tokens, KEY, "This lambda returns a lambda. Add parenthesis to clarify.", + autofixes); } private: diff --git a/src/dscanner/analysis/length_subtraction.d b/src/dscanner/analysis/length_subtraction.d index 764dcaf..06bc428 100644 --- a/src/dscanner/analysis/length_subtraction.d +++ b/src/dscanner/analysis/length_subtraction.d @@ -41,7 +41,10 @@ final class LengthSubtractionCheck : BaseAnalyzer || l.identifierOrTemplateInstance.identifier.text != "length") goto end; addErrorMessage(addExpression, "dscanner.suspicious.length_subtraction", - "Avoid subtracting from '.length' as it may be unsigned."); + "Avoid subtracting from '.length' as it may be unsigned.", + [ + AutoFix.insertionBefore(l.tokens[0], "cast(ptrdiff_t) ", "Cast to ptrdiff_t") + ]); } end: addExpression.accept(this); diff --git a/src/dscanner/analysis/static_if_else.d b/src/dscanner/analysis/static_if_else.d index e9c2d87..17efce1 100644 --- a/src/dscanner/analysis/static_if_else.d +++ b/src/dscanner/analysis/static_if_else.d @@ -48,7 +48,11 @@ final class StaticIfElse : BaseAnalyzer auto tokens = ifStmt.tokens[0 .. 1]; // extend one token to include `else` before this if tokens = (tokens.ptr - 1)[0 .. 2]; - addErrorMessage(tokens, KEY, "Mismatched static if. Use 'else static if' here."); + 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 + ]); } const(IfStatement) getIfStatement(const ConditionalStatement cc)