Skip to content

Commit

Permalink
Replace libdparse with DMD in AutoFunctionChecker (#103)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Vladiwostok
Vladiwostok authored Apr 15, 2024
1 parent dc9a243 commit 81526df
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 198 deletions.
294 changes: 100 additions & 194 deletions src/dscanner/analysis/auto_function.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
{
alias visit = BaseAnalyzerDmd.visit;
mixin AnalyzerInfo!"auto_function_check";

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;
private bool foundReturn;
private bool foundFalseAssert;
private bool inMixin;
private bool foundReturnLiteral;
private string[] literalsWithReturn;

mixin AnalyzerInfo!"auto_function_check";
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)
{
if (storageClass.token.type == tok!"auto")
return storageClass.tokens;
else if (storageClass.atAttribute)
lastAtAttribute = storageClass.atAttribute.tokens;
}
return lastAtAttribute;
}
import dmd.astenums : STC, STMT;

override void visit(const(FunctionDeclaration) decl)
{
_returns.length += 1;
scope(exit) _returns.length -= 1;
_returns[$-1] = false;
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 autoTokens = findAutoReturnType(decl);
bool isAtAttribute = autoTokens.length > 1;
auto oldFoundReturn = foundReturn;
auto oldFoundFalseAssert = foundFalseAssert;

decl.accept(this);
foundReturn = false;
foundFalseAssert = false;
super.visitFuncBody(d);

if (decl.functionBody.specifiedFunctionBody && autoTokens.length && !_returns[$-1])
if (!foundReturn && !foundFalseAssert)
{
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 "))]);
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);
}
}

override void visit(const(ReturnStatement) rst)
{
if (_returns.length)
_returns[$-1] = true;
rst.accept(this);
foundReturn = oldFoundReturn;
foundFalseAssert = oldFoundFalseAssert;
}

override void visit(const(AssertArguments) exp)
override void visit(AST.ReturnStatement s)
{
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;
}
foundReturn = true;
}

override void visit(const(MixinExpression) mix)
override void visit(AST.AssertExp assertExpr)
{
++_mixinDepth;
mix.accept(this);
--_mixinDepth;
auto ie = assertExpr.e1.isIntegerExp();
if (ie && ie.getInteger() == 0)
foundFalseAssert = true;
}

override void visit(const(PrimaryExpression) exp)
override void visit(AST.MixinStatement mixinStatement)
{
exp.accept(this);

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;
}
auto oldInMixin = inMixin;
inMixin = true;
super.visit(mixinStatement);
inMixin = oldInMixin;
}

private bool findReturnInLiteral(const(string) value)
override void visit(AST.StringExp stringExpr)
{
import std.algorithm.searching : find;
import std.range : empty;
foundReturnLiteral = foundReturnLiteral || canFind(stringExpr.toStringz(), "return");

return value == "return" || !value.find("return ").empty;
if (inMixin)
foundReturn = foundReturn || foundReturnLiteral;
}

private bool stringliteralHasReturn(const(NonVoidInitializer) nvi)
override void visit(AST.IdentifierExp ie)
{
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;
if (inMixin)
foundReturn = foundReturn || canFind(literalsWithReturn, to!string(ie.ident.toString()));

return result;
super.visit(ie);
}

override void visit(const(AutoDeclaration) decl)
override void visit(AST.VarDeclaration vd)
{
decl.accept(this);

foreach(const(AutoDeclarationPart) p; decl.parts)
if (p.initializer &&
p.initializer.nonVoidInitializer &&
stringliteralHasReturn(p.initializer.nonVoidInitializer))
_literalWithReturn ~= p.identifier.text.idup;
}
auto oldFoundReturnLiteral = foundReturnLiteral;
foundFalseAssert = false;
super.visit(vd);

override void visit(const(VariableDeclaration) decl)
{
decl.accept(this);
if (foundReturnLiteral)
literalsWithReturn ~= to!string(vd.ident.toString());

foreach(const(Declarator) d; decl.declarators)
if (d.initializer &&
d.initializer.nonVoidInitializer &&
stringliteralHasReturn(d.initializer.nonVoidInitializer))
_literalWithReturn ~= d.name.text.idup;
foundReturnLiteral = oldFoundReturnLiteral;
}
}

Expand All @@ -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);

assertAnalyzerWarnings(q{
auto doStuff(){assert(true);} /+
^^^^ [warn]: %s +/
}c.format(MESSAGE, MESSAGE, MESSAGE, MESSAGE), sac);

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);

assertAnalyzerWarnings(q{
@property doStuff(){} /+
^ [warn]: %s +/
@safe doStuff(){} /+
^ [warn]: %s +/
}c.format(MESSAGE), sac);

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
Expand All @@ -291,7 +197,7 @@ unittest
@safe void doStuff(){} // fix
@Custom
void doStuff(){} // fix
}c, sac);
}c, sac);+/

stderr.writeln("Unittest for AutoFunctionChecker passed.");
}
Loading

0 comments on commit 81526df

Please sign in to comment.