mirror of
https://github.com/dlang-community/D-Scanner.git
synced 2025-04-26 13:20:07 +03:00
Replace libdparse with DMD in AutoFunctionChecker (#103)
This commit is contained in:
Vladiwostok
Vladiwostok
parent
232cf54e73
commit
511bdce4b3
2 changed files with 111 additions and 203 deletions
|
|
@ -6,12 +6,8 @@
|
|||
module dscanner.analysis.auto_function;
|
||||
|
||||
import dscanner.analysis.base;
|
||||
import dscanner.analysis.helpers;
|
||||
import dparse.ast;
|
||||
import dparse.lexer;
|
||||
|
||||
import std.stdio;
|
||||
import std.algorithm : map, filter;
|
||||
import std.conv : to;
|
||||
import std.algorithm.searching : canFind;
|
||||
|
||||
/**
|
||||
* Checks for auto functions without return statement.
|
||||
|
|
@ -20,166 +16,104 @@ import std.algorithm : map, filter;
|
|||
* detected by the compiler. However sometimes they can be used as a trick
|
||||
* to infer attributes.
|
||||
*/
|
||||
final class AutoFunctionChecker : BaseAnalyzer
|
||||
// TODO: Fix Autofix
|
||||
extern (C++) class AutoFunctionChecker(AST) : BaseAnalyzerDmd
|
||||
{
|
||||
|
||||
private:
|
||||
|
||||
enum string KEY = "dscanner.suspicious.missing_return";
|
||||
enum string MESSAGE = "Auto function without return statement, prefer replacing auto with void";
|
||||
enum string MESSAGE_INSERT = "Auto function without return statement, prefer inserting void to be explicit";
|
||||
|
||||
bool[] _returns;
|
||||
size_t _mixinDepth;
|
||||
string[] _literalWithReturn;
|
||||
|
||||
public:
|
||||
|
||||
alias visit = BaseAnalyzer.visit;
|
||||
|
||||
alias visit = BaseAnalyzerDmd.visit;
|
||||
mixin AnalyzerInfo!"auto_function_check";
|
||||
|
||||
private bool foundReturn;
|
||||
private bool foundFalseAssert;
|
||||
private bool inMixin;
|
||||
private bool foundReturnLiteral;
|
||||
private string[] literalsWithReturn;
|
||||
|
||||
private enum string KEY = "dscanner.suspicious.missing_return";
|
||||
private enum string MESSAGE = "Auto function without return statement, prefer replacing auto with void";
|
||||
private enum string MESSAGE_INSERT = "Auto function without return statement, prefer inserting void to be explicit";
|
||||
|
||||
///
|
||||
this(BaseAnalyzerArguments args)
|
||||
extern (D) this(string fileName, bool skipTests = false)
|
||||
{
|
||||
super(args);
|
||||
super(fileName, skipTests);
|
||||
}
|
||||
|
||||
package static const(Token)[] findAutoReturnType(const(FunctionDeclaration) decl)
|
||||
override void visit(AST.FuncDeclaration d)
|
||||
{
|
||||
const(Token)[] lastAtAttribute;
|
||||
foreach (storageClass; decl.storageClasses)
|
||||
import dmd.astenums : STC, STMT;
|
||||
|
||||
if (d.storage_class & STC.disable || d.fbody is null || (d.fbody && d.fbody.isReturnStatement()))
|
||||
return;
|
||||
|
||||
ulong lineNum = cast(ulong) d.loc.linnum;
|
||||
ulong charNum = cast(ulong) d.loc.charnum;
|
||||
|
||||
auto oldFoundReturn = foundReturn;
|
||||
auto oldFoundFalseAssert = foundFalseAssert;
|
||||
|
||||
foundReturn = false;
|
||||
foundFalseAssert = false;
|
||||
super.visitFuncBody(d);
|
||||
|
||||
if (!foundReturn && !foundFalseAssert)
|
||||
{
|
||||
if (storageClass.token.type == tok!"auto")
|
||||
return storageClass.tokens;
|
||||
else if (storageClass.atAttribute)
|
||||
lastAtAttribute = storageClass.atAttribute.tokens;
|
||||
if (d.storage_class & STC.auto_)
|
||||
addErrorMessage(lineNum, charNum, KEY, MESSAGE);
|
||||
else if (auto returnType = cast(AST.TypeFunction) d.type)
|
||||
if (returnType.next is null)
|
||||
addErrorMessage(lineNum, charNum, KEY, MESSAGE_INSERT);
|
||||
}
|
||||
return lastAtAttribute;
|
||||
|
||||
foundReturn = oldFoundReturn;
|
||||
foundFalseAssert = oldFoundFalseAssert;
|
||||
}
|
||||
|
||||
override void visit(const(FunctionDeclaration) decl)
|
||||
override void visit(AST.ReturnStatement s)
|
||||
{
|
||||
_returns.length += 1;
|
||||
scope(exit) _returns.length -= 1;
|
||||
_returns[$-1] = false;
|
||||
|
||||
auto autoTokens = findAutoReturnType(decl);
|
||||
bool isAtAttribute = autoTokens.length > 1;
|
||||
|
||||
decl.accept(this);
|
||||
|
||||
if (decl.functionBody.specifiedFunctionBody && autoTokens.length && !_returns[$-1])
|
||||
{
|
||||
if (isAtAttribute)
|
||||
{
|
||||
// highlight on the whitespace between attribute and function name
|
||||
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,
|
||||
[AutoFix.insertionAt(whitespaceIndex + 1, "void ")]);
|
||||
}
|
||||
else
|
||||
addErrorMessage(autoTokens, KEY, MESSAGE,
|
||||
[AutoFix.replacement(autoTokens[0], "", "Replace `auto` with `void`")
|
||||
.concat(AutoFix.insertionAt(decl.name.index, "void "))]);
|
||||
}
|
||||
foundReturn = true;
|
||||
}
|
||||
|
||||
override void visit(const(ReturnStatement) rst)
|
||||
override void visit(AST.AssertExp assertExpr)
|
||||
{
|
||||
if (_returns.length)
|
||||
_returns[$-1] = true;
|
||||
rst.accept(this);
|
||||
auto ie = assertExpr.e1.isIntegerExp();
|
||||
if (ie && ie.getInteger() == 0)
|
||||
foundFalseAssert = true;
|
||||
}
|
||||
|
||||
override void visit(const(AssertArguments) exp)
|
||||
override void visit(AST.MixinStatement mixinStatement)
|
||||
{
|
||||
exp.accept(this);
|
||||
if (_returns.length)
|
||||
{
|
||||
const UnaryExpression u = cast(UnaryExpression) exp.assertion;
|
||||
if (!u)
|
||||
return;
|
||||
const PrimaryExpression p = u.primaryExpression;
|
||||
if (!p)
|
||||
return;
|
||||
|
||||
immutable token = p.primary;
|
||||
if (token.type == tok!"false")
|
||||
_returns[$-1] = true;
|
||||
else if (token.text == "0")
|
||||
_returns[$-1] = true;
|
||||
}
|
||||
auto oldInMixin = inMixin;
|
||||
inMixin = true;
|
||||
super.visit(mixinStatement);
|
||||
inMixin = oldInMixin;
|
||||
}
|
||||
|
||||
override void visit(const(MixinExpression) mix)
|
||||
override void visit(AST.StringExp stringExpr)
|
||||
{
|
||||
++_mixinDepth;
|
||||
mix.accept(this);
|
||||
--_mixinDepth;
|
||||
foundReturnLiteral = foundReturnLiteral || canFind(stringExpr.toStringz(), "return");
|
||||
|
||||
if (inMixin)
|
||||
foundReturn = foundReturn || foundReturnLiteral;
|
||||
}
|
||||
|
||||
override void visit(const(PrimaryExpression) exp)
|
||||
override void visit(AST.IdentifierExp ie)
|
||||
{
|
||||
exp.accept(this);
|
||||
if (inMixin)
|
||||
foundReturn = foundReturn || canFind(literalsWithReturn, to!string(ie.ident.toString()));
|
||||
|
||||
import std.algorithm.searching : canFind;
|
||||
|
||||
if (_returns.length && _mixinDepth)
|
||||
{
|
||||
if (findReturnInLiteral(exp.primary.text))
|
||||
_returns[$-1] = true;
|
||||
else if (exp.identifierOrTemplateInstance &&
|
||||
_literalWithReturn.canFind(exp.identifierOrTemplateInstance.identifier.text))
|
||||
_returns[$-1] = true;
|
||||
}
|
||||
super.visit(ie);
|
||||
}
|
||||
|
||||
private bool findReturnInLiteral(const(string) value)
|
||||
override void visit(AST.VarDeclaration vd)
|
||||
{
|
||||
import std.algorithm.searching : find;
|
||||
import std.range : empty;
|
||||
auto oldFoundReturnLiteral = foundReturnLiteral;
|
||||
foundFalseAssert = false;
|
||||
super.visit(vd);
|
||||
|
||||
return value == "return" || !value.find("return ").empty;
|
||||
}
|
||||
if (foundReturnLiteral)
|
||||
literalsWithReturn ~= to!string(vd.ident.toString());
|
||||
|
||||
private bool stringliteralHasReturn(const(NonVoidInitializer) nvi)
|
||||
{
|
||||
bool result;
|
||||
if (!nvi.assignExpression || (cast(UnaryExpression) nvi.assignExpression) is null)
|
||||
return result;
|
||||
|
||||
const(UnaryExpression) u = cast(UnaryExpression) nvi.assignExpression;
|
||||
if (u.primaryExpression &&
|
||||
u.primaryExpression.primary.type.isStringLiteral &&
|
||||
findReturnInLiteral(u.primaryExpression.primary.text))
|
||||
result = true;
|
||||
|
||||
return result;
|
||||
}
|
||||
|
||||
override void visit(const(AutoDeclaration) decl)
|
||||
{
|
||||
decl.accept(this);
|
||||
|
||||
foreach(const(AutoDeclarationPart) p; decl.parts)
|
||||
if (p.initializer &&
|
||||
p.initializer.nonVoidInitializer &&
|
||||
stringliteralHasReturn(p.initializer.nonVoidInitializer))
|
||||
_literalWithReturn ~= p.identifier.text.idup;
|
||||
}
|
||||
|
||||
override void visit(const(VariableDeclaration) decl)
|
||||
{
|
||||
decl.accept(this);
|
||||
|
||||
foreach(const(Declarator) d; decl.declarators)
|
||||
if (d.initializer &&
|
||||
d.initializer.nonVoidInitializer &&
|
||||
stringliteralHasReturn(d.initializer.nonVoidInitializer))
|
||||
_literalWithReturn ~= d.name.text.idup;
|
||||
foundReturnLiteral = oldFoundReturnLiteral;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -188,95 +122,67 @@ unittest
|
|||
import std.stdio : stderr;
|
||||
import std.format : format;
|
||||
import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig;
|
||||
import dscanner.analysis.helpers : assertAnalyzerWarnings;
|
||||
import dscanner.analysis.helpers : assertAnalyzerWarningsDMD;
|
||||
|
||||
StaticAnalysisConfig sac = disabledConfig();
|
||||
sac.auto_function_check = Check.enabled;
|
||||
assertAnalyzerWarnings(q{
|
||||
auto ref doStuff(){} /+
|
||||
^^^^ [warn]: %s +/
|
||||
auto doStuff(){} /+
|
||||
^^^^ [warn]: %s +/
|
||||
|
||||
string MESSAGE = "Auto function without return statement, prefer replacing auto with void";
|
||||
string MESSAGE_INSERT = "Auto function without return statement, prefer inserting void to be explicit";
|
||||
|
||||
assertAnalyzerWarningsDMD(q{
|
||||
auto ref doStuff(){} // [warn]: %s
|
||||
auto doStuff(){} // [warn]: %s
|
||||
@Custom
|
||||
auto doStuff(){} /+
|
||||
^^^^ [warn]: %s +/
|
||||
int doStuff(){auto doStuff(){}} /+
|
||||
^^^^ [warn]: %s +/
|
||||
auto doStuff(){} // [warn]: %s
|
||||
int doStuff(){auto doStuff(){}} // [warn]: %s
|
||||
auto doStuff(){return 0;}
|
||||
int doStuff(){/*error but not the aim*/}
|
||||
}c.format(
|
||||
AutoFunctionChecker.MESSAGE,
|
||||
AutoFunctionChecker.MESSAGE,
|
||||
AutoFunctionChecker.MESSAGE,
|
||||
AutoFunctionChecker.MESSAGE,
|
||||
), sac);
|
||||
}c.format(MESSAGE, MESSAGE, MESSAGE, MESSAGE), sac);
|
||||
|
||||
assertAnalyzerWarnings(q{
|
||||
auto doStuff(){assert(true);} /+
|
||||
^^^^ [warn]: %s +/
|
||||
assertAnalyzerWarningsDMD(q{
|
||||
auto doStuff(){assert(true);} // [warn]: %s
|
||||
auto doStuff(){assert(false);}
|
||||
}c.format(
|
||||
AutoFunctionChecker.MESSAGE,
|
||||
), sac);
|
||||
}c.format(MESSAGE), sac);
|
||||
|
||||
assertAnalyzerWarnings(q{
|
||||
auto doStuff(){assert(1);} /+
|
||||
^^^^ [warn]: %s +/
|
||||
assertAnalyzerWarningsDMD(q{
|
||||
auto doStuff(){assert(1);} // [warn]: %s
|
||||
auto doStuff(){assert(0);}
|
||||
}c.format(
|
||||
AutoFunctionChecker.MESSAGE,
|
||||
), sac);
|
||||
}c.format(MESSAGE), sac);
|
||||
|
||||
assertAnalyzerWarnings(q{
|
||||
auto doStuff(){mixin("0+0");} /+
|
||||
^^^^ [warn]: %s +/
|
||||
assertAnalyzerWarningsDMD(q{
|
||||
auto doStuff(){mixin("0+0");} // [warn]: %s
|
||||
auto doStuff(){mixin("return 0;");}
|
||||
}c.format(
|
||||
AutoFunctionChecker.MESSAGE,
|
||||
), sac);
|
||||
}c.format(MESSAGE), sac);
|
||||
|
||||
assertAnalyzerWarnings(q{
|
||||
auto doStuff(){mixin("0+0");} /+
|
||||
^^^^ [warn]: %s +/
|
||||
assertAnalyzerWarningsDMD(q{
|
||||
auto doStuff(){mixin("0+0");} // [warn]: %s
|
||||
auto doStuff(){mixin("static if (true)" ~ " return " ~ 0.stringof ~ ";");}
|
||||
}c.format(
|
||||
AutoFunctionChecker.MESSAGE,
|
||||
), sac);
|
||||
}c.format(MESSAGE), sac);
|
||||
|
||||
assertAnalyzerWarnings(q{
|
||||
auto doStuff(){} /+
|
||||
^^^^ [warn]: %s +/
|
||||
assertAnalyzerWarningsDMD(q{
|
||||
auto doStuff(){} // [warn]: %s
|
||||
extern(C) auto doStuff();
|
||||
}c.format(
|
||||
AutoFunctionChecker.MESSAGE,
|
||||
), sac);
|
||||
}c.format(MESSAGE), sac);
|
||||
|
||||
assertAnalyzerWarnings(q{
|
||||
auto doStuff(){} /+
|
||||
^^^^ [warn]: %s +/
|
||||
assertAnalyzerWarningsDMD(q{
|
||||
auto doStuff(){} // [warn]: %s
|
||||
@disable auto doStuff();
|
||||
}c.format(
|
||||
AutoFunctionChecker.MESSAGE,
|
||||
), sac);
|
||||
}c.format(MESSAGE), sac);
|
||||
|
||||
assertAnalyzerWarnings(q{
|
||||
@property doStuff(){} /+
|
||||
^ [warn]: %s +/
|
||||
@safe doStuff(){} /+
|
||||
^ [warn]: %s +/
|
||||
assertAnalyzerWarningsDMD(q{
|
||||
@property doStuff(){} // [warn]: %s
|
||||
@safe doStuff(){} // [warn]: %s
|
||||
@disable doStuff();
|
||||
@safe void doStuff();
|
||||
}c.format(
|
||||
AutoFunctionChecker.MESSAGE_INSERT,
|
||||
AutoFunctionChecker.MESSAGE_INSERT,
|
||||
), sac);
|
||||
}c.format(MESSAGE_INSERT, MESSAGE_INSERT), sac);
|
||||
|
||||
assertAnalyzerWarnings(q{
|
||||
assertAnalyzerWarningsDMD(q{
|
||||
enum _genSave = "return true;";
|
||||
auto doStuff(){ mixin(_genSave);}
|
||||
}, sac);
|
||||
|
||||
|
||||
/+ TODO: Fix Autofix
|
||||
assertAutoFix(q{
|
||||
auto ref doStuff(){} // fix
|
||||
auto doStuff(){} // fix
|
||||
|
|
@ -291,7 +197,7 @@ unittest
|
|||
@safe void doStuff(){} // fix
|
||||
@Custom
|
||||
void doStuff(){} // fix
|
||||
}c, sac);
|
||||
}c, sac);+/
|
||||
|
||||
stderr.writeln("Unittest for AutoFunctionChecker passed.");
|
||||
}
|
||||
|
|
|
|||
|
|
@ -858,10 +858,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
|
|||
analysisConfig.long_line_check == Check.skipTests && !ut),
|
||||
analysisConfig.max_line_length);
|
||||
|
||||
if (moduleName.shouldRun!AutoFunctionChecker(analysisConfig))
|
||||
checks ~= new AutoFunctionChecker(args.setSkipTests(
|
||||
analysisConfig.auto_function_check == Check.skipTests && !ut));
|
||||
|
||||
if (moduleName.shouldRun!VcallCtorChecker(analysisConfig))
|
||||
checks ~= new VcallCtorChecker(args.setSkipTests(
|
||||
analysisConfig.vcall_in_ctor == Check.skipTests && !ut));
|
||||
|
|
@ -1348,6 +1344,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN
|
|||
config.style_check == Check.skipTests && !ut
|
||||
);
|
||||
|
||||
if (moduleName.shouldRunDmd!(AutoFunctionChecker!ASTCodegen)(config))
|
||||
visitors ~= new AutoFunctionChecker!ASTCodegen(
|
||||
fileName,
|
||||
config.auto_function_check == Check.skipTests && !ut
|
||||
);
|
||||
|
||||
foreach (visitor; visitors)
|
||||
{
|
||||
m.accept(visitor);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue