diff --git a/README.md b/README.md index 4540910..93b6dc7 100644 --- a/README.md +++ b/README.md @@ -130,6 +130,7 @@ Note that the "--skipTests" option is the equivalent of changing each * Virtual calls inside classes constructors. * Useless initializers. * Allman brace style +* Redundant visibility attributes #### Wishlist diff --git a/src/analysis/config.d b/src/analysis/config.d index 4c801ab..0a2ca20 100644 --- a/src/analysis/config.d +++ b/src/analysis/config.d @@ -182,4 +182,7 @@ struct StaticAnalysisConfig @INI("Check allman brace style") string allman_braces_check = Check.disabled; + + @INI("Check for redundant attributes") + string redundant_attributes_check = Check.enabled; } diff --git a/src/analysis/redundant_attributes.d b/src/analysis/redundant_attributes.d new file mode 100644 index 0000000..3ceaaad --- /dev/null +++ b/src/analysis/redundant_attributes.d @@ -0,0 +1,290 @@ +// Distributed under the Boost Software License, Version 1.0. +// (See accompanying file LICENSE_1_0.txt or copy at +// http://www.boost.org/LICENSE_1_0.txt) + +module analysis.redundant_attributes; + +import dparse.ast; +import dparse.lexer; +import dsymbol.scope_ : Scope; +import analysis.base; +import analysis.helpers; + +import std.algorithm; +import std.conv : to, text; +import std.range : empty, front, walkLength; + +/** + * Checks for redundant attributes. At the moment only visibility attributes. + */ +class RedundantAttributesCheck : BaseAnalyzer +{ + this(string fileName, const(Scope)* sc, bool skipTests = false) + { + super(fileName, sc, skipTests); + stack.length = 0; + } + + override void visit(const Declaration decl) + { + + // labels, e.g. private: + if (auto attr = decl.attributeDeclaration) + { + if (filterAttributes(attr.attribute)) + { + addAttribute(attr.attribute); + } + } + + auto attributes = decl.attributes.filter!(a => filterAttributes(a)); + if (attributes.walkLength > 0) { + + // new scope: private { } + if (decl.declarations.length > 0) + { + auto prev = currentAttributes[]; + // append to current scope and reset once block is left + foreach (attr; attributes) + addAttribute(attr); + + scope(exit) currentAttributes = prev; + decl.accept(this); + } // declarations, e.g. private int ... + else + { + foreach (attr; attributes) + checkAttribute(attr); + + decl.accept(this); + } + } + else + { + decl.accept(this); + } + } + + alias visit = BaseAnalyzer.visit; + + mixin ScopedVisit!Module; + mixin ScopedVisit!BlockStatement; + mixin ScopedVisit!StructBody; + mixin ScopedVisit!CaseStatement; + mixin ScopedVisit!ForStatement; + mixin ScopedVisit!IfStatement; + mixin ScopedVisit!TemplateDeclaration; + mixin ScopedVisit!ConditionalDeclaration; + +private: + + alias ConstAttribute = const Attribute; + alias CurrentScope = ConstAttribute[]; + ref CurrentScope currentAttributes() + { + return stack[$ - 1]; + } + + CurrentScope[] stack; + + void addAttribute(const Attribute attr) + { + removeOverwrite(attr); + if (checkAttribute(attr)) + { + currentAttributes ~= attr; + } + } + + bool checkAttribute(const Attribute attr) + { + auto match = currentAttributes.find!(a => a.attribute.type == attr.attribute.type); + if (!match.empty) + { + auto token = attr.attribute; + addErrorMessage(token.line, token.column, KEY, + text("same visibility attribute used as defined on line ", + match.front.attribute.line.to!string, ".")); + return false; + } + return true; + } + + void removeOverwrite(const Attribute attr) + { + import std.array : array; + auto group = getAttributeGroup(attr); + if (currentAttributes.filter!(a => getAttributeGroup(a) == group + && !isIdenticalAttribute(a, attr)).walkLength > 0) + { + currentAttributes = currentAttributes.filter!(a => getAttributeGroup(a) != group + || isIdenticalAttribute(a, attr)).array; + } + } + + bool filterAttributes(const Attribute attr) + { + return isAccessSpecifier(attr); + } + + static int getAttributeGroup(const Attribute attr) + { + if (isAccessSpecifier(attr)) + return 1; + + // TODO: not implemented + return attr.attribute.type; + } + + static bool isAccessSpecifier(const Attribute attr) + { + auto type = attr.attribute.type; + return type.among(tok!"private", tok!"protected", tok!"public", tok!"package", tok!"export") > 0; + } + + static bool isIdenticalAttribute(const Attribute a, const Attribute b) + { + return a.attribute.type == b.attribute.type; + } + + auto attributesString() + { + return currentAttributes.map!(a => a.attribute.type.str).joiner(",").to!string; + } + + template ScopedVisit(NodeType) + { + override void visit(const NodeType n) + { + pushScope(); + n.accept(this); + popScope(); + } + } + + void pushScope() + { + stack.length++; + } + + void popScope() + { + stack.length--; + } + +private: + enum string KEY = "dscanner.suspicious.redundant_attributes"; +} + + +version(unittest) +{ + import analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import std.stdio : stderr; +} + +unittest +{ + StaticAnalysisConfig sac = disabledConfig(); + sac.redundant_attributes_check = Check.enabled; + + // test labels vs. block attributes + assertAnalyzerWarnings(q{ +unittest +{ +private: + private int blah; // [warn]: same visibility attribute used as defined on line 4. +protected +{ + protected int blah; // [warn]: same visibility attribute used as defined on line 6. +} + private int blah; // [warn]: same visibility attribute used as defined on line 4. +}}c, sac); + + // test labels vs. block attributes + assertAnalyzerWarnings(q{ +unittest +{ + private: + private: // [warn]: same visibility attribute used as defined on line 4. + public: + private int a; + public int b; // [warn]: same visibility attribute used as defined on line 6. + public // [warn]: same visibility attribute used as defined on line 6. + { + int c; + } +}}c, sac); + + // test scopes + assertAnalyzerWarnings(q{ +unittest +{ +private: + private int foo2; // [warn]: same visibility attribute used as defined on line 4. + private void foo() // [warn]: same visibility attribute used as defined on line 4. + { + private int blah; + } +}}c, sac); + + // check duplicated visibility attributes + assertAnalyzerWarnings(q{ +unittest +{ +private: + public int a; +private: // [warn]: same visibility attribute used as defined on line 4. +}}c, sac); + + // test conditional compilation + assertAnalyzerWarnings(q{ +unittest +{ +version(unittest) +{ + private: + private int foo; // [warn]: same visibility attribute used as defined on line 6. +} +private int foo2; +}}c, sac); + +// test scopes + assertAnalyzerWarnings(q{ +unittest +{ +public: + if (1 == 1) + { + private int b; + } + else + { + public int b; + } + public int b; // [warn]: same visibility attribute used as defined on line 4. +}}c, sac); +} + +// test other attribute (not yet implemented, thus shouldn't trigger warnings) +unittest +{ + StaticAnalysisConfig sac = disabledConfig(); + sac.redundant_attributes_check = Check.enabled; + + // test labels vs. block attributes + assertAnalyzerWarnings(q{ +unittest +{ +@safe: + @safe void foo(); +@system +{ + @system void foo(); +} + @safe void foo(); +}}c, sac); + + + stderr.writeln("Unittest for RedundantAttributesCheck passed."); +} diff --git a/src/analysis/run.d b/src/analysis/run.d index b200b9d..3e3a1dd 100644 --- a/src/analysis/run.d +++ b/src/analysis/run.d @@ -67,6 +67,7 @@ import analysis.final_attribute; import analysis.vcall_in_ctor; import analysis.useless_initializer; import analysis.allman; +import analysis.redundant_attributes; import dsymbol.string_interning : internString; import dsymbol.scope_; @@ -394,6 +395,10 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new AllManCheck(fileName, tokens, analysisConfig.allman_braces_check == Check.skipTests && !ut); + if (analysisConfig.redundant_attributes_check != Check.disabled) + checks ~= new RedundantAttributesCheck(fileName, moduleScope, + analysisConfig.redundant_attributes_check == Check.skipTests && !ut); + version (none) if (analysisConfig.redundant_if_check != Check.disabled) checks ~= new IfStatementCheck(fileName, moduleScope,