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

Use DMD in NumberStyleCheck #88

Merged
merged 3 commits into from
Mar 5, 2024
Merged

Use DMD in NumberStyleCheck #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f39b908
Replace libdparse with DMD in NumberStyleCheck
Vladiwostok Feb 20, 2024
49adcc9
Fix re-lexing for windows (CRLF terminators) files
Vladiwostok Mar 2, 2024
62d5807
Improve unit test
Vladiwostok Mar 2, 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
90 changes: 58 additions & 32 deletions src/dscanner/analysis/numbers.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,70 +5,96 @@

module dscanner.analysis.numbers;

import std.stdio;
import std.regex;
import dparse.ast;
import dparse.lexer;
import dscanner.analysis.base;
import dscanner.analysis.helpers;
import dsymbol.scope_ : Scope;
import dmd.tokens : TOK;
import std.conv;
import std.regex;

/**
* Checks for long and hard-to-read number literals
*/
final class NumberStyleCheck : BaseAnalyzer
extern (C++) class NumberStyleCheck(AST) : BaseAnalyzerDmd
{
public:
alias visit = BaseAnalyzer.visit;

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

/**
* Constructs the style checker with the given file name.
*/
this(BaseAnalyzerArguments args)
private enum KEY = "dscanner.style.number_literals";
private enum string MSG = "Use underscores to improve number constant readability.";

private auto badBinaryRegex = ctRegex!(`^0b[01]{9,}`);
private auto badDecimalRegex = ctRegex!(`^\d{5,}`);

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

override void visit(const Token t)
override void visit(AST.IntegerExp intExpr)
{
import std.algorithm : startsWith;
import dscanner.utils : readFile;
import dmd.errorsink : ErrorSinkNull;
import dmd.globals : global;
import dmd.lexer : Lexer;

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

Choose a reason for hiding this comment

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

Is it really necessary to reread the file? Don't we already have the source in memory?

Copy link
Collaborator Author

@Vladiwostok Vladiwostok Mar 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so. I'm thinking a proper solution for this would be a bigger refactor - a common lexer visitor base class, passing down a token stream to the visitors created from the initial d-scanner run, etc.

bytes = bytes[intExpr.loc.fileOffset .. $];

__gshared ErrorSinkNull errorSinkNull;
if (!errorSinkNull)
errorSinkNull = new ErrorSinkNull;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we doing all of these initializations here? Also, why is the file read here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nvm, I understand now. Isn't there any way to reuse a prior used errorSinkNull? For example, the one that was used to lex the initial source?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't that the behaviour of __gshared attribute?


if (isNumberLiteral(t.type) && !t.text.startsWith("0x")
&& ((t.text.startsWith("0b") && !t.text.matchFirst(badBinaryRegex)
.empty) || !t.text.matchFirst(badDecimalRegex).empty))
scope lexer = new Lexer(null, cast(char*) bytes, 0, bytes.length, 0, 0, errorSinkNull, &global.compileEnv);
auto tokenValue = lexer.nextToken();
bool isInt = false;

while (tokenValue != TOK.semicolon && tokenValue != TOK.endOfFile)
{
addErrorMessage(t, "dscanner.style.number_literals",
"Use underscores to improve number constant readability.");
if (isIntegerLiteral(tokenValue))
{
isInt = true;
break;
}

tokenValue = lexer.nextToken();
}

if (!isInt)
return;

auto tokenText = to!string(lexer.token.ptr);

if (!matchFirst(tokenText, badDecimalRegex).empty || !matchFirst(tokenText, badBinaryRegex).empty)
addErrorMessage(intExpr.loc.linnum, intExpr.loc.charnum, KEY, MSG);
}

private:
auto badBinaryRegex = ctRegex!(`^0b[01]{9,}`);
auto badDecimalRegex = ctRegex!(`^\d{5,}`);
private bool isIntegerLiteral(TOK token)
{
return token >= TOK.int32Literal && token <= TOK.uns128Literal;
}
}

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

StaticAnalysisConfig sac = disabledConfig();
sac.number_style_check = Check.enabled;
assertAnalyzerWarnings(q{
assertAnalyzerWarningsDMD(q{
void testNumbers()
{
int a;
a = 1; // ok
a = 10; // ok
a = 100; // ok
a = 1000; // ok
a = 10000; /+
^^^^^ [warn]: Use underscores to improve number constant readability. +/
a = 100000; /+
^^^^^^ [warn]: Use underscores to improve number constant readability. +/
a = 1000000; /+
^^^^^^^ [warn]: Use underscores to improve number constant readability. +/
a = 10_00; // ok
a = 10_000; // ok
a = 100_000; // ok
a = 10000; // [warn]: Use underscores to improve number constant readability.
a = 100000; // [warn]: Use underscores to improve number constant readability.
a = 1000000; // [warn]: Use underscores to improve number constant readability.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also add some numbers with underscores just to make sure that does do not actually output any warnings. E.G: 1_000. Also, should 10_00 output any warnings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add tests for passing cases.
Judging from the existing bad decimal regex (5 or more decimals), 10_00 should not output any warnings

}
}c, sac);

Expand Down
10 changes: 6 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 @@ -860,10 +860,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
checks ~= new MismatchedArgumentCheck(args.setSkipTests(
analysisConfig.mismatched_args_check == Check.skipTests && !ut));

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

if (moduleName.shouldRun!StyleChecker(analysisConfig))
checks ~= new StyleChecker(args.setSkipTests(
analysisConfig.style_check == Check.skipTests && !ut));
Expand Down Expand Up @@ -1350,6 +1346,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN
config.redundant_storage_classes == Check.skipTests && !ut
);

if (moduleName.shouldRunDmd!(NumberStyleCheck!ASTCodegen)(config))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is ASTCodegen passed here since we are only using the lexer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the BaseAnalyzerDmd follows an inheritance chain starting with the parent class ParseTimeVisitor!ASTCodegen . Should ASTBase struct be used instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, it's kind of annoying that BaseAnalyzerDmd is hardcoded to ASTCodegen. Why not just pass AST to BaseAnalyzer?

E.G:

class BaseAnalyzerDmd(AST) : ParseTimeVisitor!AST {...}

I suggest you look into this in a subsequent PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#96

visitors ~= new NumberStyleCheck!ASTCodegen(
fileName,
config.number_style_check == Check.skipTests && !ut
);

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