Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace libdparse with DMD in IfConstraintsIndentCheck #128

Merged
merged 4 commits into from
Nov 8, 2024
Merged

Replace libdparse with DMD in IfConstraintsIndentCheck #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
514ca54
Replace libdparse with DMD in IfConstraintsIndentCheck
Vladiwostok May 1, 2024
37a6f65
Fix evil segfault bug
Vladiwostok Aug 17, 2024
64776e5
Remove Issue#829 unit test
Vladiwostok Oct 25, 2024
f7b5c90
Properly detect issue
Vladiwostok Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
267 changes: 113 additions & 154 deletions src/dscanner/analysis/if_constraints_indent.d
View file
Open in desktop
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()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen this method before. Maybe it should be made public and let all visitors use it.

{
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")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed as this no longer applies with current code

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
Open in desktop
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
Open in desktop
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