Skip to content

Commit

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

* Fix evil segfault bug

* Remove Issue#829 unit test

* Properly detect issue
  • Loading branch information
@Vladiwostok
Vladiwostok authored Nov 8, 2024
1 parent 306c987 commit f6bfa57
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 158 deletions.
267 changes: 113 additions & 154 deletions src/dscanner/analysis/if_constraints_indent.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,194 +4,132 @@

module dscanner.analysis.if_constraints_indent;

import dparse.lexer;
import dparse.ast;
import dscanner.analysis.base;
import dsymbol.scope_ : Scope;

import std.algorithm.iteration : filter;
import std.range;
import dmd.tokens : Token, TOK;

/**
Checks whether all if constraints have the same indention as their declaration.
*/
final class IfConstraintsIndentCheck : BaseAnalyzer
extern (C++) class IfConstraintsIndentCheck(AST) : BaseAnalyzerDmd
{
alias visit = BaseAnalyzerDmd.visit;
mixin AnalyzerInfo!"if_constraints_indent";

///
this(BaseAnalyzerArguments args)
{
super(args);
private enum string KEY = "dscanner.style.if_constraints_indent";
private enum string MSG = "If constraints should have the same indentation as the function";

// convert tokens to a list of token starting positions per line
private Token[] tokens;

// libdparse columns start at 1
foreach (t; tokens)
{
// pad empty positions if we skip empty token-less lines
// t.line (unsigned) may be 0 if the token is uninitialized/broken, so don't subtract from it
// equivalent to: firstSymbolAtLine.length < t.line - 1
while (firstSymbolAtLine.length + 1 < t.line)
firstSymbolAtLine ~= Pos(1, t.index);

// insert a new line with positions if new line is reached
// (previous while pads skipped lines)
if (firstSymbolAtLine.length < t.line)
firstSymbolAtLine ~= Pos(t.column, t.index, t.type == tok!"if");
}
}

override void visit(const FunctionDeclaration decl)
extern (D) this(string fileName, bool skipTests = false)
{
if (decl.constraint !is null)
checkConstraintSpace(decl.constraint, decl.name);
super(fileName, skipTests);
lexFile();
}

override void visit(const InterfaceDeclaration decl)
private void lexFile()
{
if (decl.constraint !is null)
checkConstraintSpace(decl.constraint, decl.name);
}
import dscanner.utils : readFile;
import dmd.errorsink : ErrorSinkNull;
import dmd.globals : global;
import dmd.lexer : Lexer;

auto bytes = readFile(fileName) ~ '\0';

override void visit(const ClassDeclaration decl)
{
if (decl.constraint !is null)
checkConstraintSpace(decl.constraint, decl.name);
}
__gshared ErrorSinkNull errorSinkNull;
if (!errorSinkNull)
errorSinkNull = new ErrorSinkNull;

override void visit(const TemplateDeclaration decl)
{
if (decl.constraint !is null)
checkConstraintSpace(decl.constraint, decl.name);
}

override void visit(const UnionDeclaration decl)
{
if (decl.constraint !is null)
checkConstraintSpace(decl.constraint, decl.name);
}
scope lexer = new Lexer(null, cast(char*) bytes, 0, bytes.length, 0, 0, errorSinkNull, &global.compileEnv);

override void visit(const StructDeclaration decl)
{
if (decl.constraint !is null)
checkConstraintSpace(decl.constraint, decl.name);
}

override void visit(const Constructor decl)
{
if (decl.constraint !is null)
checkConstraintSpace(decl.constraint, decl.line);
}

alias visit = ASTVisitor.visit;

private:

enum string KEY = "dscanner.style.if_constraints_indent";
enum string MESSAGE = "If constraints should have the same indentation as the function";

Pos[] firstSymbolAtLine;
static struct Pos
{
size_t column;
size_t index;
bool isIf;
}

/**
Check indentation of constraints
*/
void checkConstraintSpace(const Constraint constraint, const Token token)
{
checkConstraintSpace(constraint, token.line);
do
{
lexer.nextToken();
tokens ~= lexer.token;
}
while (lexer.token.value != TOK.endOfFile);
}

void checkConstraintSpace(const Constraint constraint, size_t line)
override void visit(AST.TemplateDeclaration templateDecl)
{
import std.algorithm : min;

// dscanner lines start at 1
auto pDecl = firstSymbolAtLine[line - 1];

// search for constraint if (might not be on the same line as the expression)
auto r = firstSymbolAtLine[line .. constraint.expression.line].retro.filter!(s => s.isIf);

auto if_ = constraint.tokens.findTokenForDisplay(tok!"if")[0];

// no hit = constraint is on the same line
if (r.empty)
addErrorMessage(if_, KEY, MESSAGE);
else if (pDecl.column != r.front.column)
addErrorMessage([min(if_.index, pDecl.index), if_.index + 2], if_.line, [min(if_.column, pDecl.column), if_.column + 2], KEY, MESSAGE);
import std.array : array;
import std.algorithm : filter;
import std.range : front, retro;

super.visit(templateDecl);

if (templateDecl.constraint is null || templateDecl.members is null)
return;

auto firstTemplateToken = tokens.filter!(t => t.loc.linnum == templateDecl.loc.linnum)
.filter!(t => t.value != TOK.whitespace)
.front;
uint templateLine = firstTemplateToken.loc.linnum;
uint templateCol = firstTemplateToken.loc.charnum;

auto constraintToken = tokens.filter!(t => t.loc.fileOffset <= templateDecl.constraint.loc.fileOffset)
.array()
.retro()
.filter!(t => t.value == TOK.if_)
.front;
uint constraintLine = constraintToken.loc.linnum;
uint constraintCol = constraintToken.loc.charnum;

if (templateLine == constraintLine || templateCol != constraintCol)
addErrorMessage(cast(ulong) constraintLine, cast(ulong) constraintCol, KEY, MSG);
}
}

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

StaticAnalysisConfig sac = disabledConfig();
sac.if_constraints_indent = Check.enabled;
enum MSG = "If constraints should have the same indentation as the function";

assertAnalyzerWarnings(q{
assertAnalyzerWarningsDMD(q{
void foo(R)(R r)
if (R == null)
{}

// note: since we are using tabs, the ^ look a bit off here. Use 1-wide tab stops to view tests.
void foo(R)(R r)
if (R == null) /+
^^^ [warn]: %s +/
if (R == null) // [warn]: %s
{}
}c.format(
IfConstraintsIndentCheck.MESSAGE,
), sac);
}c.format(MSG), sac);

assertAnalyzerWarnings(q{
assertAnalyzerWarningsDMD(q{
void foo(R)(R r)
if (R == null)
{}

void foo(R)(R r)
if (R == null) /+
^^ [warn]: %s +/
if (R == null) // [warn]: %s
{}

void foo(R)(R r)
if (R == null) /+
^^^ [warn]: %s +/
if (R == null) // [warn]: %s
{}
}c.format(
IfConstraintsIndentCheck.MESSAGE,
IfConstraintsIndentCheck.MESSAGE,
), sac);
}c.format(MSG, MSG), sac);

assertAnalyzerWarnings(q{
assertAnalyzerWarningsDMD(q{
struct Foo(R)
if (R == null)
{}

struct Foo(R)
if (R == null) /+
^^ [warn]: %s +/
if (R == null) // [warn]: %s
{}

struct Foo(R)
if (R == null) /+
^^^ [warn]: %s +/
if (R == null) // [warn]: %s
{}
}c.format(
IfConstraintsIndentCheck.MESSAGE,
IfConstraintsIndentCheck.MESSAGE,
), sac);
}c.format(MSG, MSG), sac);

// test example from Phobos
assertAnalyzerWarnings(q{
assertAnalyzerWarningsDMD(q{
Num abs(Num)(Num x) @safe pure nothrow
if (is(typeof(Num.init >= 0)) && is(typeof(-Num.init)) &&
!(is(Num* : const(ifloat*)) || is(Num* : const(idouble*))
Expand All @@ -205,7 +143,7 @@ if (is(typeof(Num.init >= 0)) && is(typeof(-Num.init)) &&
}, sac);

// weird constraint formatting
assertAnalyzerWarnings(q{
assertAnalyzerWarningsDMD(q{
struct Foo(R)
if
(R == null)
Expand All @@ -217,8 +155,7 @@ if (is(typeof(Num.init >= 0)) && is(typeof(-Num.init)) &&
{}

struct Foo(R)
if /+
^^ [warn]: %s +/
if // [warn]: %s
(R == null)
{}

Expand All @@ -234,39 +171,61 @@ if /+
{}

struct Foo(R)
if ( /+
^^^ [warn]: %s +/
if ( // [warn]: %s
R == null
) {}
}c.format(
IfConstraintsIndentCheck.MESSAGE,
IfConstraintsIndentCheck.MESSAGE,
), sac);
}c.format(MSG, MSG), sac);

// constraint on the same line
assertAnalyzerWarnings(q{
struct CRC(uint N, ulong P) if (N == 32 || N == 64) /+
^^ [warn]: %s +/
assertAnalyzerWarningsDMD(q{
struct CRC(uint N, ulong P) if (N == 32 || N == 64) // [warn]: %s
{}
}c.format(
IfConstraintsIndentCheck.MESSAGE,
), sac);
}c.format(MSG), sac);

stderr.writeln("Unittest for IfConstraintsIndentCheck passed.");
assertAnalyzerWarningsDMD(q{
private template sharedToString(alias field)
if (is(typeof(field) == shared))
{
static immutable sharedToString = typeof(field).stringof;
}
}c, sac);

@("issue #829")
unittest
assertAnalyzerWarningsDMD(q{
private union EndianSwapper(T)
if (canSwapEndianness!T)
{
import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig;
import dscanner.analysis.helpers : assertAnalyzerWarnings;
import std.format : format;
import std.stdio : stderr;
T value;
}
}c, sac);

StaticAnalysisConfig sac = disabledConfig();
sac.if_constraints_indent = Check.enabled;
assertAnalyzerWarningsDMD(q{
void test(alias matchFn)()
{
auto baz(Cap)(Cap m)
if (is(Cap == Captures!(Cap.String)))
{
return toUpper(m.hit);
}
}
}c, sac);

assertAnalyzerWarnings(`void foo() {
f;
}`, sac);
assertAnalyzerWarningsDMD(q{
ElementType!(A) pop (A) (ref A a)
if (isDynamicArray!(A) && !isNarrowString!(A) && isMutable!(A) && !is(A == void[]))
{
auto e = a.back;
a.popBack();
return e;
}
}c, sac);

assertAnalyzerWarningsDMD(q{
template HMAC(H)
if (isDigest!H && hasBlockSize!H)
{
alias HMAC = HMAC!(H, H.blockSize);
}
}, sac);

stderr.writeln("Unittest for IfConstraintsIndentCheck passed.");
}
4 changes: 0 additions & 4 deletions src/dscanner/analysis/run.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -663,10 +663,6 @@ BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
checks ~= new UndocumentedDeclarationCheck(args.setSkipTests(
analysisConfig.undocumented_declaration_check == Check.skipTests && !ut));

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

return checks;
}

Expand Down
7 changes: 7 additions & 0 deletions src/dscanner/analysis/rundmd.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import dscanner.analysis.enumarrayliteral : EnumArrayVisitor;
import dscanner.analysis.explicitly_annotated_unittests : ExplicitlyAnnotatedUnittestCheck;
import dscanner.analysis.final_attribute : FinalAttributeChecker;
import dscanner.analysis.has_public_example : HasPublicExampleCheck;
import dscanner.analysis.if_constraints_indent : IfConstraintsIndentCheck;
import dscanner.analysis.ifelsesame : IfElseSameCheck;
import dscanner.analysis.imports_sortedness : ImportSortednessCheck;
import dscanner.analysis.incorrect_infinite_range : IncorrectInfiniteRangeCheck;
Expand Down Expand Up @@ -332,6 +333,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN
config.mismatched_args_check == Check.skipTests && !ut
);

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

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

0 comments on commit f6bfa57

Please sign in to comment.