From be19b4a8e294edda9a3adc515bbcb4761b79f8aa Mon Sep 17 00:00:00 2001 From: Vladiwostok <55026261+Vladiwostok@users.noreply.github.com> Date: Sun, 10 Nov 2024 13:12:10 +0200 Subject: [PATCH] Replace libdparse with DMD in FunctionAttributeCheck (#156) --- src/dscanner/analysis/function_attributes.d | 348 +++++++++++--------- src/dscanner/analysis/run.d | 7 +- src/dscanner/analysis/rundmd.d | 7 + 3 files changed, 206 insertions(+), 156 deletions(-) diff --git a/src/dscanner/analysis/function_attributes.d b/src/dscanner/analysis/function_attributes.d index 9f106d8..078fa9b 100644 --- a/src/dscanner/analysis/function_attributes.d +++ b/src/dscanner/analysis/function_attributes.d @@ -6,11 +6,9 @@ module dscanner.analysis.function_attributes; import dscanner.analysis.base; -import dscanner.analysis.helpers; -import dparse.ast; -import dparse.lexer; -import std.stdio; -import dsymbol.scope_; +import dmd.astenums : STC, MOD, MODFlags; +import dmd.tokens : Token, TOK; +import std.string : format; /** * Prefer @@ -22,207 +20,251 @@ import dsymbol.scope_; * 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"; - 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; - const t2 = inAggregate; - inInterface = true; - inAggregate = true; - dec.accept(this); - inInterface = t; - inAggregate = t2; + import dscanner.utils : readFile; + import dmd.errorsink : ErrorSinkNull; + import dmd.globals : global; + import dmd.lexer : Lexer; + + auto bytes = readFile(fileName) ~ '\0'; + __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) - { - const t = inInterface; - const t2 = inAggregate; - inInterface = false; - inAggregate = true; - dec.accept(this); - inInterface = t; - inAggregate = t2; - } + mixin visitAggregate!(AST.InterfaceDeclaration, true); + mixin visitAggregate!(AST.ClassDeclaration); + mixin visitAggregate!(AST.StructDeclaration); + mixin visitAggregate!(AST.UnionDeclaration); - override void visit(const StructDeclaration dec) + private template visitAggregate(NodeType, bool isInterface = false) { - const t = inInterface; - const t2 = inAggregate; - inInterface = false; - inAggregate = true; - dec.accept(this); - inInterface = t; - inAggregate = t2; - } - - override void visit(const UnionDeclaration dec) - { - const t = inInterface; - const t2 = inAggregate; - inInterface = false; - inAggregate = true; - dec.accept(this); - inInterface = t; - inAggregate = t2; - } - - override void visit(const AttributeDeclaration dec) - { - if (inInterface && dec.attribute.attribute == tok!"abstract") + override void visit(NodeType node) { - addErrorMessage(dec.attribute, KEY, ABSTRACT_MESSAGE); + immutable bool oldInAggregate = inAggregate; + immutable bool oldInInterface = inInterface; + + inAggregate = !isStaticAggregate(node.loc.linnum, node.loc.charnum); + inInterface = isInterface; + super.visit(node); + + inAggregate = oldInAggregate; + inInterface = oldInInterface; } } - override void visit(const FunctionDeclaration dec) + private bool isStaticAggregate(uint lineNum, uint charNum) { - if (dec.parameters.parameters.length == 0 && inAggregate) - { - bool foundConst; - bool foundProperty; - foreach (attribute; dec.attributes) - 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); + import std.algorithm : any, filter; + + return tokens.filter!(token => token.loc.linnum == lineNum && token.loc.charnum <= charNum) + .filter!(token => token.value >= TOK.struct_ && token.value <= TOK.immutable_) + .any!(token => token.value == TOK.static_); } - override void visit(const Declaration dec) + override void visit(AST.FuncDeclaration fd) { - bool isStatic = false; - if (dec.attributes.length == 0) - goto end; - 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; + import std.algorithm : canFind, find, filter, until; + import std.array : array; + import std.range : retro; - immutable string attrString = str(attr.attribute.type); - AutoFix[] autofixes; - if (dec.functionDeclaration.parameters) - autofixes ~= AutoFix.replacement( - attr.attribute, "", - "Move " ~ str(attr.attribute.type) ~ " after parameter list") - .concat(AutoFix.insertionAfter( - dec.functionDeclaration.parameters.tokens[$ - 1], - " " ~ str(attr.attribute.type))); - if (dec.functionDeclaration.returnType) - autofixes ~= AutoFix.insertionAfter(attr.attribute, "(", "Make return type const") - .concat(AutoFix.insertionAfter(dec.functionDeclaration.returnType.tokens[$ - 1], ")")); - addErrorMessage(attr.attribute, KEY, format( - "'%s' is not an attribute of the return type." - ~ " Place it after the parameter list to clarify.", - attrString), autofixes); + super.visit(fd); + + if (fd.type is null) + return; + + immutable ulong lineNum = cast(ulong) fd.loc.linnum; + immutable ulong charNum = cast(ulong) fd.loc.charnum; + + if (inInterface) + { + immutable bool isAbstract = (fd.storage_class & STC.abstract_) > 0; + if (isAbstract) + { + auto offset = tokens.filter!(t => t.loc.linnum >= fd.loc.linnum) + .until!(t => t.value == TOK.leftCurly) + .array + .retro() + .find!(t => t.value == TOK.abstract_) + .front.loc.fileOffset; + + addErrorMessage( + lineNum, charNum, KEY, ABSTRACT_MSG, + [AutoFix.replacement(offset, offset + 8, "", "Remove `abstract` attribute")] + ); + + return; } } - end: - if (isStatic) { - const t = inAggregate; - inAggregate = false; - dec.accept(this); - inAggregate = t; - } - else { - dec.accept(this); + + auto tf = fd.type.isTypeFunction(); + + if (inAggregate && tf) + { + 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: - bool inInterface; - bool inAggregate; - enum string ABSTRACT_MESSAGE = "'abstract' attribute is redundant in interface declarations"; - enum string KEY = "dscanner.confusing.function_attributes"; + 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 { - 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(); sac.function_attribute_check = Check.enabled; - assertAnalyzerWarnings(q{ + + assertAnalyzerWarningsDMD(q{ int foo() @property { return 0; } class ClassName { - const int confusingConst() { return 0; } /+ - ^^^^^ [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'. +/ + const int confusingConst() { return 0; } // [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'. static int barStatic() @property { return 0; } int barConst() const @property { return 0; } } struct StructName { - 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; } int barConst() const @property { return 0; } } union UnionName { - 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; } int barConst() const @property { return 0; } } interface InterfaceName { - int bar() @property; /+ - ^^^ [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. +/ + int bar() @property; // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. static int barStatic() @property { return 0; } 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); + // 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{ int foo() @property { return 0; } @@ -274,7 +316,7 @@ unittest int method(); // fix } - }c, sac); + }c, sac, true); - stderr.writeln("Unittest for FunctionAttributeCheck passed."); + stderr.writeln("Unittest for ObjectConstCheck passed."); } diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 33fa5dd..c919822 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -655,9 +655,10 @@ BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName, moduleScope ); - if (moduleName.shouldRun!FunctionAttributeCheck(analysisConfig)) - checks ~= new FunctionAttributeCheck(args.setSkipTests( - analysisConfig.function_attribute_check == Check.skipTests && !ut)); + // Add those lines to suppress warnings about unused variables until cleanup is complete + bool ignoreVar = analysisConfig.if_constraints_indent == Check.skipTests; + bool ignoreVar2 = args.skipTests; + ignoreVar = ignoreVar || ignoreVar2; return checks; } diff --git a/src/dscanner/analysis/rundmd.d b/src/dscanner/analysis/rundmd.d index bc20004..8cbe4eb 100644 --- a/src/dscanner/analysis/rundmd.d +++ b/src/dscanner/analysis/rundmd.d @@ -24,6 +24,7 @@ import dscanner.analysis.del : DeleteCheck; import dscanner.analysis.enumarrayliteral : EnumArrayVisitor; import dscanner.analysis.explicitly_annotated_unittests : ExplicitlyAnnotatedUnittestCheck; import dscanner.analysis.final_attribute : FinalAttributeChecker; +import dscanner.analysis.function_attributes : FunctionAttributeCheck; import dscanner.analysis.has_public_example : HasPublicExampleCheck; import dscanner.analysis.if_constraints_indent : IfConstraintsIndentCheck; 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 ); + if (moduleName.shouldRunDmd!(FunctionAttributeCheck!ASTCodegen)(config)) + visitors ~= new FunctionAttributeCheck!ASTCodegen( + fileName, + config.function_attribute_check == Check.skipTests && !ut + ); + foreach (visitor; visitors) { m.accept(visitor);