Skip to content

Commit

Permalink
Replace libdparse with DMD in BodyOnDisabledFuncsCheck (#127)
Browse files Browse the repository at this point in the history 
* Replace libdparse with DMD in BodyOnDisabledFuncsCheck

* Address feedback
  • Loading branch information
@Vladiwostok
Vladiwostok authored Aug 8, 2024
1 parent 4bf91f8 commit dffbf3a
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 136 deletions.
184 changes: 52 additions & 132 deletions src/dscanner/analysis/body_on_disabled_funcs.d
View file
Original file line number Diff line number Diff line change
@@ -1,143 +1,76 @@
module dscanner.analysis.body_on_disabled_funcs;

import dscanner.analysis.base;
import dparse.ast;
import dparse.lexer;
import dsymbol.scope_;
import std.meta : AliasSeq;
import dmd.astenums : STC;

final class BodyOnDisabledFuncsCheck : BaseAnalyzer
extern (C++) class BodyOnDisabledFuncsCheck(AST) : BaseAnalyzerDmd
{
alias visit = BaseAnalyzer.visit;

alias visit = BaseAnalyzerDmd.visit;
mixin AnalyzerInfo!"body_on_disabled_func_check";

this(BaseAnalyzerArguments args)
{
super(args);
}
private enum string KEY = "dscanner.confusing.disabled_function_with_body";
private enum FUNC_MSG = "Function marked with '@disabled' should not have a body";
private enum CTOR_MSG = "Constructor marked with '@disabled' should not have a body";
private enum DTOR_MSG = "Destructor marked with '@disabled' should not have a body";

static foreach (AggregateType; AliasSeq!(InterfaceDeclaration, ClassDeclaration,
StructDeclaration, UnionDeclaration, FunctionDeclaration))
override void visit(const AggregateType t)
{
scope wasDisabled = isDisabled;
isDisabled = false;
t.accept(this);
isDisabled = wasDisabled;
}
private bool isDisabled;

override void visit(const Declaration dec)
extern (D) this(string fileName, bool skipTests = false)
{
foreach (attr; dec.attributes)
{
if (attr.atAttribute !is null && attr.atAttribute.identifier.text == "disable") {
// found attr block w. disable: dec.constructor
scope wasDisabled = isDisabled;
isDisabled = true;
visitDeclarationInner(dec);
dec.accept(this);
isDisabled = wasDisabled;
return;
}
}
super(fileName, skipTests);
}

visitDeclarationInner(dec);
scope wasDisabled = isDisabled;
dec.accept(this);
override void visit(AST.StorageClassDeclaration storageClassDecl)
{
bool wasDisabled = isDisabled;
isDisabled = (storageClassDecl.stc & STC.disable) != 0;
super.visit(storageClassDecl);
isDisabled = wasDisabled;
}

private:
bool isDisabled = false;
mixin VisitAggregate!(AST.ClassDeclaration);
mixin VisitAggregate!(AST.InterfaceDeclaration);
mixin VisitAggregate!(AST.StructDeclaration);
mixin VisitAggregate!(AST.UnionDeclaration);

bool isDeclDisabled(T)(const T dec)
private template VisitAggregate(NodeType)
{
// `@disable { ... }`
if (isDisabled)
return true;

static if (__traits(hasMember, T, "storageClasses"))
override void visit(NodeType node)
{
// `@disable doThing() {}`
if (hasDisabledStorageclass(dec.storageClasses))
return true;
}

// `void doThing() @disable {}`
return hasDisabledMemberFunctionAttribute(dec.memberFunctionAttributes);
}
if (isDisabled || (node.storage_class & STC.disable) != 0)
return;

void visitDeclarationInner(const Declaration dec)
{
if (dec.attributeDeclaration !is null
&& dec.attributeDeclaration.attribute
&& dec.attributeDeclaration.attribute.atAttribute
&& dec.attributeDeclaration.attribute.atAttribute.identifier.text == "disable")
{
// found `@disable:`, so all code in this block is now disabled
isDisabled = true;
}
else if (dec.functionDeclaration !is null
&& isDeclDisabled(dec.functionDeclaration)
&& dec.functionDeclaration.functionBody !is null
&& dec.functionDeclaration.functionBody.missingFunctionBody is null)
{
addErrorMessage(dec.functionDeclaration.functionBody,
KEY, "Function marked with '@disabled' should not have a body");
}
else if (dec.constructor !is null
&& isDeclDisabled(dec.constructor)
&& dec.constructor.functionBody !is null
&& dec.constructor.functionBody.missingFunctionBody is null)
{
addErrorMessage(dec.constructor.functionBody,
KEY, "Constructor marked with '@disabled' should not have a body");
}
else if (dec.destructor !is null
&& isDeclDisabled(dec.destructor)
&& dec.destructor.functionBody !is null
&& dec.destructor.functionBody.missingFunctionBody is null)
{
addErrorMessage(dec.destructor.functionBody,
KEY, "Destructor marked with '@disabled' should not have a body");
bool wasDisabled = isDisabled;
isDisabled = false;
super.visit(node);
isDisabled = wasDisabled;
}
}

bool hasDisabledStorageclass(const(StorageClass[]) storageClasses)
{
foreach (sc; storageClasses)
{
if (sc.atAttribute !is null && sc.atAttribute.identifier.text == "disable")
return true;
}
return false;
}
mixin VisitFunction!(AST.FuncDeclaration, FUNC_MSG);
mixin VisitFunction!(AST.CtorDeclaration, CTOR_MSG);
mixin VisitFunction!(AST.DtorDeclaration, DTOR_MSG);

bool hasDisabledMemberFunctionAttribute(const(MemberFunctionAttribute[]) memberFunctionAttributes)
private template VisitFunction(NodeType, string MSG)
{
foreach (attr; memberFunctionAttributes)
override void visit(NodeType node)
{
if (attr.atAttribute !is null && attr.atAttribute.identifier.text == "disable")
return true;
if ((isDisabled || (node.storage_class & STC.disable) != 0) && node.fbody !is null)
addErrorMessage(cast(ulong) node.loc.linnum, cast(ulong) node.loc.charnum, KEY, MSG);
}
return false;
}

enum string KEY = "dscanner.confusing.disabled_function_with_body";
}

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.body_on_disabled_func_check = Check.enabled;

assertAnalyzerWarnings(q{
assertAnalyzerWarningsDMD(q{
class C1
{
this() {}
Expand All @@ -159,12 +92,9 @@ unittest
}
}

this() {} /+
^^ [warn]: Constructor marked with '@disabled' should not have a body +/
void doThing() {} /+
^^ [warn]: Function marked with '@disabled' should not have a body +/
~this() {} /+
^^ [warn]: Destructor marked with '@disabled' should not have a body +/
this() {} // [warn]: Constructor marked with '@disabled' should not have a body
void doThing() {} // [warn]: Function marked with '@disabled' should not have a body
~this() {} // [warn]: Destructor marked with '@disabled' should not have a body

this();
void doThing();
Expand All @@ -173,28 +103,18 @@ unittest

class C2
{
@disable this() {} /+
^^ [warn]: Constructor marked with '@disabled' should not have a body +/
@disable { this() {} } /+
^^ [warn]: Constructor marked with '@disabled' should not have a body +/
this() @disable {} /+
^^ [warn]: Constructor marked with '@disabled' should not have a body +/

@disable void doThing() {} /+
^^ [warn]: Function marked with '@disabled' should not have a body +/
@disable doThing() {} /+
^^ [warn]: Function marked with '@disabled' should not have a body +/
@disable { void doThing() {} } /+
^^ [warn]: Function marked with '@disabled' should not have a body +/
void doThing() @disable {} /+
^^ [warn]: Function marked with '@disabled' should not have a body +/

@disable ~this() {} /+
^^ [warn]: Destructor marked with '@disabled' should not have a body +/
@disable { ~this() {} } /+
^^ [warn]: Destructor marked with '@disabled' should not have a body +/
~this() @disable {} /+
^^ [warn]: Destructor marked with '@disabled' should not have a body +/
@disable this() {} // [warn]: Constructor marked with '@disabled' should not have a body
@disable { this() {} } // [warn]: Constructor marked with '@disabled' should not have a body
this() @disable {} // [warn]: Constructor marked with '@disabled' should not have a body

@disable void doThing() {} // [warn]: Function marked with '@disabled' should not have a body
@disable doThing() {} // [warn]: Function marked with '@disabled' should not have a body
@disable { void doThing() {} } // [warn]: Function marked with '@disabled' should not have a body
void doThing() @disable {} // [warn]: Function marked with '@disabled' should not have a body

@disable ~this() {} // [warn]: Destructor marked with '@disabled' should not have a body
@disable { ~this() {} } // [warn]: Destructor marked with '@disabled' should not have a body
~this() @disable {} // [warn]: Destructor marked with '@disabled' should not have a body

@disable this();
@disable { this(); }
Expand Down
10 changes: 6 additions & 4 deletions src/dscanner/analysis/run.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -870,10 +870,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
checks ~= new UnusedResultChecker(args.setSkipTests(
analysisConfig.unused_result == Check.skipTests && !ut));

if (moduleName.shouldRun!BodyOnDisabledFuncsCheck(analysisConfig))
checks ~= new BodyOnDisabledFuncsCheck(args.setSkipTests(
analysisConfig.body_on_disabled_func_check == Check.skipTests && !ut));

return checks;
}

Expand Down Expand Up @@ -1356,6 +1352,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN
config.could_be_immutable_check == Check.skipTests && !ut
);

if (moduleName.shouldRunDmd!(BodyOnDisabledFuncsCheck!ASTCodegen)(config))
visitors ~= new BodyOnDisabledFuncsCheck!ASTCodegen(
fileName,
config.body_on_disabled_func_check == Check.skipTests && !ut
);

foreach (visitor; visitors)
{
m.accept(visitor);
Expand Down

0 comments on commit dffbf3a

Please sign in to comment.