forks
/
D-Scanner
mirror of https://github.com/dlang-community/D-Scanner.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Replace libdparse with DMD in FunctionAttributeCheck (#156)

This commit is contained in:
Vladiwostok 2024-11-10 13:12:10 +02:00 committed by Albert24GG
parent 7980ac3a3b
commit f6c367c513
3 changed files with 206 additions and 156 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

350
src/dscanner/analysis/function_attributes.d
View File

@ -6,11 +6,9 @@
module dscanner.analysis.function_attributes; module dscanner.analysis.function_attributes;
import dscanner.analysis.base; import dscanner.analysis.base;
import dscanner.analysis.helpers; import dmd.astenums : STC, MOD, MODFlags;
import dparse.ast; import dmd.tokens : Token, TOK;
import dparse.lexer; import std.string : format;
import std.stdio;
import dsymbol.scope_;
/** /**
* Prefer * Prefer
@ -22,207 +20,251 @@ import dsymbol.scope_;
* const int getStuff() {} * const int getStuff() {}
* --- * ---
*/ */
final class FunctionAttributeCheck : BaseAnalyzer extern (C++) class FunctionAttributeCheck(AST) : BaseAnalyzerDmd
{ {
alias visit = BaseAnalyzer.visit; alias visit = BaseAnalyzerDmd.visit;
mixin AnalyzerInfo!"function_attribute_check"; mixin AnalyzerInfo!"function_attribute_check";
this(BaseAnalyzerArguments args) private enum KEY = "dscanner.confusing.function_attributes";
private enum CONST_MSG = "Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.";
private enum ABSTRACT_MSG = "'abstract' attribute is redundant in interface declarations";
private enum RETURN_MSG = "'%s' is not an attribute of the return type. Place it after the parameter list to clarify.";
private bool inInterface = false;
private bool inAggregate = false;
private Token[] tokens;
extern (D) this(string fileName, bool skipTests = false)
{ {
super(args); super(fileName, skipTests);
getTokens();
} }
override void visit(const InterfaceDeclaration dec) private void getTokens()
{ {
const t = inInterface; import dscanner.utils : readFile;
const t2 = inAggregate; import dmd.errorsink : ErrorSinkNull;
inInterface = true; import dmd.globals : global;
inAggregate = true; import dmd.lexer : Lexer;
dec.accept(this);
inInterface = t; auto bytes = readFile(fileName) ~ '\0';
inAggregate = t2; __gshared ErrorSinkNull errorSinkNull;
if (!errorSinkNull)
errorSinkNull = new ErrorSinkNull;
scope lexer = new Lexer(null, cast(char*) bytes, 0, bytes.length, 0, 0, errorSinkNull, &global.compileEnv);
while (lexer.nextToken() != TOK.endOfFile)
tokens ~= lexer.token;
} }
override void visit(const ClassDeclaration dec) mixin visitAggregate!(AST.InterfaceDeclaration, true);
{ mixin visitAggregate!(AST.ClassDeclaration);
const t = inInterface; mixin visitAggregate!(AST.StructDeclaration);
const t2 = inAggregate; mixin visitAggregate!(AST.UnionDeclaration);
inInterface = false;
inAggregate = true;
dec.accept(this);
inInterface = t;
inAggregate = t2;
}
override void visit(const StructDeclaration dec) private template visitAggregate(NodeType, bool isInterface = false)
{ {
const t = inInterface; override void visit(NodeType node)
const t2 = inAggregate; {
inInterface = false; immutable bool oldInAggregate = inAggregate;
inAggregate = true; immutable bool oldInInterface = inInterface;
dec.accept(this);
inInterface = t;
inAggregate = t2;
}
override void visit(const UnionDeclaration dec) inAggregate = !isStaticAggregate(node.loc.linnum, node.loc.charnum);
{ inInterface = isInterface;
const t = inInterface; super.visit(node);
const t2 = inAggregate;
inInterface = false;
inAggregate = true;
dec.accept(this);
inInterface = t;
inAggregate = t2;
}
override void visit(const AttributeDeclaration dec) inAggregate = oldInAggregate;
{ inInterface = oldInInterface;
if (inInterface && dec.attribute.attribute == tok!"abstract")
{
addErrorMessage(dec.attribute, KEY, ABSTRACT_MESSAGE);
} }
} }
override void visit(const FunctionDeclaration dec) private bool isStaticAggregate(uint lineNum, uint charNum)
{ {
if (dec.parameters.parameters.length == 0 && inAggregate) import std.algorithm : any, filter;
{
bool foundConst; return tokens.filter!(token => token.loc.linnum == lineNum && token.loc.charnum <= charNum)
bool foundProperty; .filter!(token => token.value >= TOK.struct_ && token.value <= TOK.immutable_)
foreach (attribute; dec.attributes) .any!(token => token.value == TOK.static_);
foundConst = foundConst || attribute.attribute.type == tok!"const"
|| attribute.attribute.type == tok!"immutable"
|| attribute.attribute.type == tok!"inout";
foreach (attribute; dec.memberFunctionAttributes)
{
foundProperty = foundProperty || (attribute.atAttribute !is null
&& attribute.atAttribute.identifier.text == "property");
foundConst = foundConst || attribute.tokenType == tok!"const"
|| attribute.tokenType == tok!"immutable" || attribute.tokenType == tok!"inout";
}
if (foundProperty && !foundConst)
{
auto paren = dec.parameters.tokens.length ? dec.parameters.tokens[$ - 1] : Token.init;
auto autofixes = paren is Token.init ? null : [
AutoFix.insertionAfter(paren, " const", "Mark function `const`"),
AutoFix.insertionAfter(paren, " inout", "Mark function `inout`"),
AutoFix.insertionAfter(paren, " immutable", "Mark function `immutable`"),
];
addErrorMessage(dec.name, KEY,
"Zero-parameter '@property' function should be"
~ " marked 'const', 'inout', or 'immutable'.", autofixes);
}
}
dec.accept(this);
} }
override void visit(const Declaration dec) override void visit(AST.FuncDeclaration fd)
{ {
bool isStatic = false; import std.algorithm : canFind, find, filter, until;
if (dec.attributes.length == 0) import std.array : array;
goto end; import std.range : retro;
foreach (attr; dec.attributes)
{
if (attr.attribute.type == tok!"")
continue;
if (attr.attribute == tok!"abstract" && inInterface)
{
addErrorMessage(attr.attribute, KEY, ABSTRACT_MESSAGE,
[AutoFix.replacement(attr.attribute, "")]);
continue;
}
if (attr.attribute == tok!"static")
{
isStatic = true;
}
if (dec.functionDeclaration !is null && (attr.attribute == tok!"const"
|| attr.attribute == tok!"inout" || attr.attribute == tok!"immutable"))
{
import std.string : format;
immutable string attrString = str(attr.attribute.type); super.visit(fd);
AutoFix[] autofixes;
if (dec.functionDeclaration.parameters) if (fd.type is null)
autofixes ~= AutoFix.replacement( return;
attr.attribute, "",
"Move " ~ str(attr.attribute.type) ~ " after parameter list") immutable ulong lineNum = cast(ulong) fd.loc.linnum;
.concat(AutoFix.insertionAfter( immutable ulong charNum = cast(ulong) fd.loc.charnum;
dec.functionDeclaration.parameters.tokens[$ - 1],
" " ~ str(attr.attribute.type))); if (inInterface)
if (dec.functionDeclaration.returnType) {
autofixes ~= AutoFix.insertionAfter(attr.attribute, "(", "Make return type const") immutable bool isAbstract = (fd.storage_class & STC.abstract_) > 0;
.concat(AutoFix.insertionAfter(dec.functionDeclaration.returnType.tokens[$ - 1], ")")); if (isAbstract)
addErrorMessage(attr.attribute, KEY, format( {
"'%s' is not an attribute of the return type." auto offset = tokens.filter!(t => t.loc.linnum >= fd.loc.linnum)
~ " Place it after the parameter list to clarify.", .until!(t => t.value == TOK.leftCurly)
attrString), autofixes); .array
} .retro()
} .find!(t => t.value == TOK.abstract_)
end: .front.loc.fileOffset;
if (isStatic) {
const t = inAggregate; addErrorMessage(
inAggregate = false; lineNum, charNum, KEY, ABSTRACT_MSG,
dec.accept(this); [AutoFix.replacement(offset, offset + 8, "", "Remove `abstract` attribute")]
inAggregate = t; );
}
else { return;
dec.accept(this);
} }
} }
private: auto tf = fd.type.isTypeFunction();
bool inInterface;
bool inAggregate; if (inAggregate && tf)
enum string ABSTRACT_MESSAGE = "'abstract' attribute is redundant in interface declarations"; {
enum string KEY = "dscanner.confusing.function_attributes"; string storageTok = getConstLikeStorage(tf.mod);
auto bodyStartToken = TOK.leftCurly;
if (fd.fbody is null)
bodyStartToken = TOK.semicolon;
Token[] funcTokens = tokens.filter!(t => t.loc.fileOffset > fd.loc.fileOffset)
.until!(t => t.value == TOK.leftCurly || t.value == bodyStartToken)
.array;
if (storageTok is null)
{
bool isStatic = (fd.storage_class & STC.static_) > 0;
bool isZeroParamProperty = tf.isProperty() && tf.parameterList.parameters.length == 0;
auto propertyRange = funcTokens.retro()
.until!(t => t.value == TOK.rightParenthesis)
.find!(t => t.ident.toString() == "property")
.find!(t => t.value == TOK.at);
if (!isStatic && isZeroParamProperty && !propertyRange.empty)
addErrorMessage(
lineNum, charNum, KEY, CONST_MSG,
[
AutoFix.insertionAt(propertyRange.front.loc.fileOffset, "const "),
AutoFix.insertionAt(propertyRange.front.loc.fileOffset, "inout "),
AutoFix.insertionAt(propertyRange.front.loc.fileOffset, "immutable "),
]
);
}
else
{
bool hasConstLikeAttribute = funcTokens.retro()
.canFind!(t => t.value == TOK.const_ || t.value == TOK.immutable_ || t.value == TOK.inout_);
if (!hasConstLikeAttribute)
{
auto funcRange = tokens.filter!(t => t.loc.linnum >= fd.loc.linnum)
.until!(t => t.value == TOK.leftCurly || t.value == TOK.semicolon);
auto parensToken = funcRange.until!(t => t.value == TOK.leftParenthesis)
.array
.retro()
.front;
auto funcEndToken = funcRange.array
.retro()
.find!(t => t.value == TOK.rightParenthesis)
.front;
auto constLikeToken = funcRange
.find!(t => t.value == TOK.const_ || t.value == TOK.inout_ || t.value == TOK.immutable_)
.front;
string modifier;
if (constLikeToken.value == TOK.const_)
modifier = " const";
else if (constLikeToken.value == TOK.inout_)
modifier = " inout";
else
modifier = " immutable";
AutoFix fix1 = AutoFix
.replacement(constLikeToken.loc.fileOffset, constLikeToken.loc.fileOffset + modifier.length,
"", "Move" ~ modifier ~ " after parameter list")
.concat(AutoFix.insertionAt(funcEndToken.loc.fileOffset + 1, modifier));
AutoFix fix2 = AutoFix.replacement(constLikeToken.loc.fileOffset + modifier.length - 1,
constLikeToken.loc.fileOffset + modifier.length, "(", "Make return type" ~ modifier)
.concat(AutoFix.insertionAt(parensToken.loc.fileOffset - 1, ")"));
addErrorMessage(lineNum, charNum, KEY, RETURN_MSG.format(storageTok), [fix1, fix2]);
}
}
}
}
private extern (D) string getConstLikeStorage(MOD mod)
{
if (mod & MODFlags.const_)
return "const";
if (mod & MODFlags.immutable_)
return "immutable";
if (mod & MODFlags.wild)
return "inout";
return null;
}
} }
unittest unittest
{ {
import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; import dscanner.analysis.config : Check, disabledConfig, StaticAnalysisConfig;
import dscanner.analysis.helpers : assertAnalyzerWarningsDMD, assertAutoFix;
import std.stdio : stderr;
StaticAnalysisConfig sac = disabledConfig(); StaticAnalysisConfig sac = disabledConfig();
sac.function_attribute_check = Check.enabled; sac.function_attribute_check = Check.enabled;
assertAnalyzerWarnings(q{
assertAnalyzerWarningsDMD(q{
int foo() @property { return 0; } int foo() @property { return 0; }
class ClassName { class ClassName {
const int confusingConst() { return 0; } /+ const int confusingConst() { return 0; } // [warn]: 'const' is not an attribute of the return type. Place it after the parameter list to clarify.
^^^^^ [warn]: 'const' is not an attribute of the return type. Place it after the parameter list to clarify. +/ int bar() @property { return 0; } // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
int bar() @property { return 0; } /+
^^^ [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. +/
static int barStatic() @property { return 0; } static int barStatic() @property { return 0; }
int barConst() const @property { return 0; } int barConst() const @property { return 0; }
} }
struct StructName { struct StructName {
int bar() @property { return 0; } /+ int bar() @property { return 0; } // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
^^^ [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. +/
static int barStatic() @property { return 0; } static int barStatic() @property { return 0; }
int barConst() const @property { return 0; } int barConst() const @property { return 0; }
} }
union UnionName { union UnionName {
int bar() @property { return 0; } /+ int bar() @property { return 0; } // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
^^^ [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. +/
static int barStatic() @property { return 0; } static int barStatic() @property { return 0; }
int barConst() const @property { return 0; } int barConst() const @property { return 0; }
} }
interface InterfaceName { interface InterfaceName {
int bar() @property; /+ int bar() @property; // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
^^^ [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. +/
static int barStatic() @property { return 0; } static int barStatic() @property { return 0; }
int barConst() const @property; int barConst() const @property;
abstract int method(); // [warn]: 'abstract' attribute is redundant in interface declarations
abstract int method(); /+
^^^^^^^^ [warn]: 'abstract' attribute is redundant in interface declarations +/
} }
}c, sac); }c, sac);
// Test taken from phobos / utf.d, shouldn't warn
assertAnalyzerWarningsDMD(q{
static struct R
{
@safe pure @nogc nothrow:
this(string s) { this.s = s; }
@property bool empty() { return idx == s.length; }
@property char front() { return s[idx]; }
void popFront() { ++idx; }
size_t idx;
string s;
}
}c, sac);
assertAutoFix(q{ assertAutoFix(q{
int foo() @property { return 0; } int foo() @property { return 0; }
@ -274,7 +316,7 @@ unittest
int method(); // fix int method(); // fix
} }
}c, sac); }c, sac, true);
stderr.writeln("Unittest for FunctionAttributeCheck passed."); stderr.writeln("Unittest for ObjectConstCheck passed.");
} }

7
src/dscanner/analysis/run.d
View File

@ -655,9 +655,10 @@ BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
moduleScope moduleScope
); );
if (moduleName.shouldRun!FunctionAttributeCheck(analysisConfig)) // Add those lines to suppress warnings about unused variables until cleanup is complete
checks ~= new FunctionAttributeCheck(args.setSkipTests( bool ignoreVar = analysisConfig.if_constraints_indent == Check.skipTests;
analysisConfig.function_attribute_check == Check.skipTests && !ut)); bool ignoreVar2 = args.skipTests;
ignoreVar = ignoreVar || ignoreVar2;
return checks; return checks;
} }

7
src/dscanner/analysis/rundmd.d
View File

@ -24,6 +24,7 @@ import dscanner.analysis.del : DeleteCheck;
import dscanner.analysis.enumarrayliteral : EnumArrayVisitor; import dscanner.analysis.enumarrayliteral : EnumArrayVisitor;
import dscanner.analysis.explicitly_annotated_unittests : ExplicitlyAnnotatedUnittestCheck; import dscanner.analysis.explicitly_annotated_unittests : ExplicitlyAnnotatedUnittestCheck;
import dscanner.analysis.final_attribute : FinalAttributeChecker; import dscanner.analysis.final_attribute : FinalAttributeChecker;
import dscanner.analysis.function_attributes : FunctionAttributeCheck;
import dscanner.analysis.has_public_example : HasPublicExampleCheck; import dscanner.analysis.has_public_example : HasPublicExampleCheck;
import dscanner.analysis.if_constraints_indent : IfConstraintsIndentCheck; import dscanner.analysis.if_constraints_indent : IfConstraintsIndentCheck;
import dscanner.analysis.ifelsesame : IfElseSameCheck; import dscanner.analysis.ifelsesame : IfElseSameCheck;
@ -346,6 +347,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN
config.undocumented_declaration_check == Check.skipTests && !ut config.undocumented_declaration_check == Check.skipTests && !ut
); );
if (moduleName.shouldRunDmd!(FunctionAttributeCheck!ASTCodegen)(config))
visitors ~= new FunctionAttributeCheck!ASTCodegen(
fileName,
config.function_attribute_check == Check.skipTests && !ut
);
foreach (visitor; visitors) foreach (visitor; visitors)
{ {
m.accept(visitor); m.accept(visitor);