From 513b7dafc314bc9ddaaeae83972cda150ed0b5b5 Mon Sep 17 00:00:00 2001
From: WebFreak001 <gh@webfreak.org>
Date: Thu, 6 Jul 2023 01:55:27 +0200
Subject: [PATCH] add auto-fix API
---
src/dscanner/analysis/auto_function.d | 6 +-
src/dscanner/analysis/base.d | 207 ++++++++++++++++--
src/dscanner/analysis/del.d | 4 +-
src/dscanner/analysis/duplicate_attribute.d | 3 +-
src/dscanner/analysis/enumarrayliteral.d | 10 +-
.../analysis/explicitly_annotated_unittests.d | 9 +-
src/dscanner/analysis/final_attribute.d | 3 +-
src/dscanner/analysis/function_attributes.d | 27 ++-
src/dscanner/analysis/lambda_return_check.d | 19 +-
src/dscanner/analysis/length_subtraction.d | 5 +-
src/dscanner/analysis/static_if_else.d | 6 +-
11 files changed, 261 insertions(+), 38 deletions(-)
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)