Skip to content

Commit

Permalink
replace libparse in opequals without tohash visitor (#53)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lucica28 @edi33416
lucica28 authored and edi33416 committed Jan 29, 2024
1 parent 82bc26b commit 710c184
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 82 deletions.
135 changes: 57 additions & 78 deletions src/dscanner/analysis/opequals_without_tohash.d
View file
Expand Up @@ -5,118 +5,101 @@

module dscanner.analysis.opequals_without_tohash;

import dparse.ast;
import dparse.lexer;
import dscanner.analysis.base;
import dscanner.analysis.helpers;
import dsymbol.scope_ : Scope;
import std.stdio;
import std.typecons : Rebindable;

/**
* Checks for when a class/struct has the method opEquals without toHash, or
* toHash without opEquals.
*/
final class OpEqualsWithoutToHashCheck : BaseAnalyzer
extern(C++) class OpEqualsWithoutToHashCheck(AST) : BaseAnalyzerDmd
{
alias visit = BaseAnalyzer.visit;

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

this(BaseAnalyzerArguments args)
extern(D) this(string fileName, bool skipTests = false)
{
super(args);
super(fileName, skipTests);
}

override void visit(const ClassDeclaration node)
override void visit(AST.ClassDeclaration cd)
{
actualCheck(node.name, node.structBody);
node.accept(this);
visitBaseClasses(cd);
visitAggregate(cd);
}

override void visit(const StructDeclaration node)
override void visit(AST.StructDeclaration sd)
{
actualCheck(node.name, node.structBody);
node.accept(this);
visitAggregate(sd);
}

private void actualCheck(const Token name, const StructBody structBody)
private void isInteresting(AST.FuncDeclaration fd, ref bool hasOpEquals, ref bool hasToHash)
{
Rebindable!(const Declaration) hasOpEquals;
Rebindable!(const Declaration) hasToHash;
Rebindable!(const Declaration) hasOpCmp;
import dmd.astenums : STC;

// Just return if missing children
if (!structBody || !structBody.declarations || name is Token.init)
return;
if (!(fd.storage_class & STC.disable) && fd.ident.toString() == "opEquals")
hasOpEquals = true;

// Check all the function declarations
foreach (declaration; structBody.declarations)
{
// Skip if not a function declaration
if (!declaration || !declaration.functionDeclaration)
continue;
if (!(fd.storage_class & STC.disable) && fd.ident.toString() == "toHash")
hasToHash = true;
}

bool containsDisable(A)(const A[] attribs)
private void visitAggregate(AST.AggregateDeclaration ad)
{
bool hasOpEquals, hasToHash;

if (!ad.members)
return;

foreach(member; *ad.members)
{
if (auto fd = member.isFuncDeclaration())
{
import std.algorithm.searching : canFind;
return attribs.canFind!(a => a.atAttribute !is null &&
a.atAttribute.identifier.text == "disable");
isInteresting(fd, hasOpEquals, hasToHash);
member.accept(this);
}

const isDeclationDisabled = containsDisable(declaration.attributes) ||
containsDisable(declaration.functionDeclaration.memberFunctionAttributes);

if (isDeclationDisabled)
continue;

// Check if opEquals or toHash
immutable string methodName = declaration.functionDeclaration.name.text;
if (methodName == "opEquals")
hasOpEquals = declaration;
else if (methodName == "toHash")
hasToHash = declaration;
else if (methodName == "opCmp")
hasOpCmp = declaration;
else if (auto scd = member.isStorageClassDeclaration())
{
foreach (smember; *scd.decl)
{
if (auto fd2 = smember.isFuncDeclaration())
{
isInteresting(fd2, hasOpEquals, hasToHash);
smember.accept(this);
}
else
smember.accept(this);
}
}
else
member.accept(this);
}

// Warn if has opEquals, but not toHash
if (hasOpEquals && !hasToHash)
{
string message = "'" ~ name.text ~ "' has method 'opEquals', but not 'toHash'.";
addErrorMessage(
Message.Diagnostic.from(fileName, name, message),
[
Message.Diagnostic.from(fileName, hasOpEquals.get, "'opEquals' defined here")
],
KEY
);
string message = ad.ident.toString().dup;
message = "'" ~ message ~ "' has method 'opEquals', but not 'toHash'.";
addErrorMessage(cast(ulong) ad.loc.linnum, cast(ulong) ad.loc.charnum, KEY, message);
}
// Warn if has toHash, but not opEquals
else if (!hasOpEquals && hasToHash)
{
string message = "'" ~ name.text ~ "' has method 'toHash', but not 'opEquals'.";
addErrorMessage(
Message.Diagnostic.from(fileName, name, message),
[
Message.Diagnostic.from(fileName, hasToHash.get, "'toHash' defined here")
],
KEY
);
string message = ad.ident.toString().dup;
message = "'" ~ message ~ "' has method 'toHash', but not 'opEquals'.";
addErrorMessage(cast(ulong) ad.loc.linnum, cast(ulong) ad.loc.charnum, KEY, message);
}
}

enum string KEY = "dscanner.suspicious.incomplete_operator_overloading";
private enum KEY = "dscanner.suspicious.incomplete_operator_overloading";
}

unittest
{
import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig;
import std.stdio : stderr;

StaticAnalysisConfig sac = disabledConfig();
sac.opequals_tohash_check = Check.enabled;
// TODO: test supplemental diagnostics
assertAnalyzerWarnings(q{
assertAnalyzerWarningsDMD(q{
// Success because it has opEquals and toHash
class Chimp
{
Expand All @@ -141,8 +124,7 @@ unittest
}

// Fail on class opEquals
class Rabbit /+
^^^^^^ [warn]: 'Rabbit' has method 'opEquals', but not 'toHash'. +/
class Rabbit // [warn]: 'Rabbit' has method 'opEquals', but not 'toHash'.
{
const bool opEquals(Object a, Object b)
{
Expand All @@ -151,8 +133,7 @@ unittest
}

// Fail on class toHash
class Kangaroo /+
^^^^^^^^ [warn]: 'Kangaroo' has method 'toHash', but not 'opEquals'. +/
class Kangaroo // [warn]: 'Kangaroo' has method 'toHash', but not 'opEquals'.
{
override const hash_t toHash()
{
Expand All @@ -161,8 +142,7 @@ unittest
}

// Fail on struct opEquals
struct Tarantula /+
^^^^^^^^^ [warn]: 'Tarantula' has method 'opEquals', but not 'toHash'. +/
struct Tarantula // [warn]: 'Tarantula' has method 'opEquals', but not 'toHash'.
{
const bool opEquals(Object a, Object b)
{
Expand All @@ -171,8 +151,7 @@ unittest
}

// Fail on struct toHash
struct Puma /+
^^^^ [warn]: 'Puma' has method 'toHash', but not 'opEquals'. +/
struct Puma // [warn]: 'Puma' has method 'toHash', but not 'opEquals'.
{
const nothrow @safe hash_t toHash()
{
Expand All @@ -189,4 +168,4 @@ unittest
}c, sac);

stderr.writeln("Unittest for OpEqualsWithoutToHashCheck passed.");
}
}
10 changes: 6 additions & 4 deletions src/dscanner/analysis/run.d
View file
Expand Up @@ -888,10 +888,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
checks ~= new NumberStyleCheck(args.setSkipTests(
analysisConfig.number_style_check == Check.skipTests && !ut));

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

if (moduleName.shouldRun!RedundantParenCheck(analysisConfig))
checks ~= new RedundantParenCheck(args.setSkipTests(
analysisConfig.redundant_parens_check == Check.skipTests && !ut));
Expand Down Expand Up @@ -1332,6 +1328,12 @@ MessageSet analyzeDmd(string fileName, ASTBase.Module m, const char[] moduleName
if (moduleName.shouldRunDmd!(LocalImportCheck!ASTBase)(config))
visitors ~= new LocalImportCheck!ASTBase(fileName);

if (moduleName.shouldRunDmd!(OpEqualsWithoutToHashCheck!ASTBase)(config))
visitors ~= new OpEqualsWithoutToHashCheck!ASTBase(
fileName,
config.opequals_tohash_check == Check.skipTests && !ut
);

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

0 comments on commit 710c184

Please sign in to comment.