From 22104911d44bd618de09fadc1d39eee3066f89eb Mon Sep 17 00:00:00 2001 From: Matthew Brennan Jones Date: Fri, 23 May 2014 20:37:48 -0700 Subject: [PATCH 1/4] Added basic checker for duplicate attributes. --- README.md | 1 + analysis/duplicate_attribute.d | 127 +++++++++++++++++++++++++++++++++ analysis/run.d | 5 +- 3 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 analysis/duplicate_attribute.d diff --git a/README.md b/README.md index 5e2028e..c8f4828 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,7 @@ given source files. * 'if' statements where the 'else' block is the same as the 'if' block. * Unused variables. * Unused parameters (check is skipped if function is marked "override") +* Duplicate attributes #### Wishlish * Assigning to foreach variables that are not "ref". diff --git a/analysis/duplicate_attribute.d b/analysis/duplicate_attribute.d new file mode 100644 index 0000000..c2e7870 --- /dev/null +++ b/analysis/duplicate_attribute.d @@ -0,0 +1,127 @@ +// Copyright (c) 2014, Matthew Brennan Jones +// 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.duplicate_attribute; + +import std.stdio; +import std.string; +import std.d.ast; +import std.d.lexer; +import analysis.base; +import analysis.helpers; + +// FIXME: Make it work with @safe, @trusted, @system +// FIXME: Make it work with pure, nothrow + +/** + * Checks for duplicate attributes such as @property + */ +class DuplicateAttributeCheck : BaseAnalyzer +{ + alias visit = BaseAnalyzer.visit; + + this(string fileName) + { + super(fileName); + } + + override void visit(const Declaration node) + { + checkAttributes(node); + node.accept(this); + } + + void checkAttributes(const Declaration node) + { + bool hasProperty = false; + + // Check the attributes + foreach (attribute; node.attributes) + { + // Just skip if missing child nodes + if (!attribute + || !attribute.storageClass + || !attribute.storageClass.atAttribute + || attribute.storageClass.atAttribute.identifier is Token.init) + continue; + + // Is a property + auto iden = attribute.storageClass.atAttribute.identifier; + checkProperty(iden, hasProperty); + } + + // Just return if missing function nodes + if (!node.functionDeclaration + || !node.functionDeclaration.memberFunctionAttributes) + return; + + // Check the functions + foreach (memberFunctionAttribute; node.functionDeclaration.memberFunctionAttributes) + { + // Just skip if missing child nodes + if (!memberFunctionAttribute + || !memberFunctionAttribute.atAttribute + || memberFunctionAttribute.atAttribute.identifier is Token.init) + continue; + + // Is a property + auto iden = memberFunctionAttribute.atAttribute.identifier; + checkProperty(iden, hasProperty); + } + } + + void checkProperty(const Token iden, ref bool hasProperty) + { + // Just return if not a property + if (!isProperty(iden)) + return; + + // Already has a property + if (hasProperty) + { + string message = "The attribute '%s' is duplicated.".format(iden.text); + addErrorMessage(iden.line, iden.column, message); + } + + // Mark it as a property + hasProperty = true; + } +} + +bool isProperty(const Token token) pure +{ + return token.type == tok!"identifier" && token.text == "property"; +} + +unittest +{ + assertAnalyzerWarnings(q{ + class CAllocator + { + @property bool xxx() // ok + { + return false; + } + + @property @property bool aaa() // [warn]: The attribute 'property' is duplicated. + { + return false; + } + + bool bbb() @property @property // [warn]: The attribute 'property' is duplicated. + { + return false; + } + + @property bool ccc() @property // [warn]: The attribute 'property' is duplicated. + { + return false; + } + } + }c, analysis.run.AnalyzerCheck.duplicate_attribute); + + stderr.writeln("Unittest for DuplicateAttributeCheck passed."); +} + diff --git a/analysis/run.d b/analysis/run.d index 8737cbc..88b1e55 100644 --- a/analysis/run.d +++ b/analysis/run.d @@ -22,6 +22,7 @@ import analysis.range; import analysis.ifelsesame; import analysis.constructors; import analysis.unused; +import analysis.duplicate_attribute; enum AnalyzerCheck : int { @@ -36,7 +37,8 @@ enum AnalyzerCheck : int if_else_same_check = 0x100, constructor_check = 0x200, unused_variable_check = 0x400, - all = 0x7FF + duplicate_attribute = 0x800, + all = 0xFFF } void messageFunction(string fileName, size_t line, size_t column, string message, @@ -104,6 +106,7 @@ string[] analyze(string fileName, ubyte[] code, AnalyzerCheck analyzers, bool st if (analyzers & AnalyzerCheck.if_else_same_check) checks ~= new IfElseSameCheck(fileName); if (analyzers & AnalyzerCheck.constructor_check) checks ~= new ConstructorCheck(fileName); if (analyzers & AnalyzerCheck.unused_variable_check) checks ~= new UnusedVariableCheck(fileName); + if (analyzers & AnalyzerCheck.duplicate_attribute) checks ~= new DuplicateAttributeCheck(fileName); foreach (check; checks) { From be02c872b5734581a1b27958f95e1907e92f492c Mon Sep 17 00:00:00 2001 From: Matthew Brennan Jones Date: Fri, 23 May 2014 20:50:42 -0700 Subject: [PATCH 2/4] Removed duplicate @property attribute. --- std/allocator.d | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/std/allocator.d b/std/allocator.d index be494e9..e4aae8e 100644 --- a/std/allocator.d +++ b/std/allocator.d @@ -4337,7 +4337,7 @@ class CAllocator throw an exception if it does allow setting the alignment but an invalid value is passed. */ - @property bool alignment(uint) pure nothrow @property + @property bool alignment(uint) pure nothrow { return false; } From fad096cdff559c7193201823d3083159e887cd95 Mon Sep 17 00:00:00 2001 From: Matthew Brennan Jones Date: Sat, 24 May 2014 18:24:37 -0700 Subject: [PATCH 3/4] Added more attributes to the duplicate checker. --- analysis/duplicate_attribute.d | 61 +++++++++++++++++++++------------- analysis/helpers.d | 1 + 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/analysis/duplicate_attribute.d b/analysis/duplicate_attribute.d index c2e7870..777e9d2 100644 --- a/analysis/duplicate_attribute.d +++ b/analysis/duplicate_attribute.d @@ -12,11 +12,10 @@ import std.d.lexer; import analysis.base; import analysis.helpers; -// FIXME: Make it work with @safe, @trusted, @system // FIXME: Make it work with pure, nothrow /** - * Checks for duplicate attributes such as @property + * Checks for duplicate attributes such as @property, @safe, @trusted, @system */ class DuplicateAttributeCheck : BaseAnalyzer { @@ -36,6 +35,9 @@ class DuplicateAttributeCheck : BaseAnalyzer void checkAttributes(const Declaration node) { bool hasProperty = false; + bool hasSafe = false; + bool hasTrusted = false; + bool hasSystem = false; // Check the attributes foreach (attribute; node.attributes) @@ -47,9 +49,12 @@ class DuplicateAttributeCheck : BaseAnalyzer || attribute.storageClass.atAttribute.identifier is Token.init) continue; - // Is a property + // Check for the attributes auto iden = attribute.storageClass.atAttribute.identifier; - checkProperty(iden, hasProperty); + checkDuplicateAttribute(iden, "property", hasProperty); + checkDuplicateAttribute(iden, "safe", hasSafe); + checkDuplicateAttribute(iden, "trusted", hasTrusted); + checkDuplicateAttribute(iden, "system", hasSystem); } // Just return if missing function nodes @@ -66,56 +71,64 @@ class DuplicateAttributeCheck : BaseAnalyzer || memberFunctionAttribute.atAttribute.identifier is Token.init) continue; - // Is a property + // Check for the attributes auto iden = memberFunctionAttribute.atAttribute.identifier; - checkProperty(iden, hasProperty); + checkDuplicateAttribute(iden, "property", hasProperty); + checkDuplicateAttribute(iden, "safe", hasSafe); + checkDuplicateAttribute(iden, "trusted", hasTrusted); + checkDuplicateAttribute(iden, "system", hasSystem); } } - void checkProperty(const Token iden, ref bool hasProperty) + void checkDuplicateAttribute(const Token token, const string attributeName, ref bool hasAttribute) { - // Just return if not a property - if (!isProperty(iden)) + // Just return if not an attribute + if (token.type != tok!"identifier" + || token.text != attributeName) return; - // Already has a property - if (hasProperty) + // Already has that attribute + if (hasAttribute) { - string message = "The attribute '%s' is duplicated.".format(iden.text); - addErrorMessage(iden.line, iden.column, message); + string message = "The attribute '%s' is duplicated.".format(token.text); + addErrorMessage(token.line, token.column, message); } - // Mark it as a property - hasProperty = true; + // Mark it as having that attribute + hasAttribute = true; } } -bool isProperty(const Token token) pure -{ - return token.type == tok!"identifier" && token.text == "property"; -} - unittest { assertAnalyzerWarnings(q{ - class CAllocator + class ExampleAttributes { - @property bool xxx() // ok + @property @safe bool xxx() // ok { return false; } + // Duplicate before @property @property bool aaa() // [warn]: The attribute 'property' is duplicated. { return false; } - bool bbb() @property @property // [warn]: The attribute 'property' is duplicated. + // Duplicate after + bool bbb() @safe @safe // [warn]: The attribute 'safe' is duplicated. { return false; } - @property bool ccc() @property // [warn]: The attribute 'property' is duplicated. + // Duplicate before and after + @system bool ccc() @system // [warn]: The attribute 'system' is duplicated. + { + return false; + } + + // Duplicate before and after + @trusted bool ddd() @trusted // [warn]: The attribute 'trusted' is duplicated. { return false; } diff --git a/analysis/helpers.d b/analysis/helpers.d index e4bbc0a..ae081c2 100644 --- a/analysis/helpers.d +++ b/analysis/helpers.d @@ -8,6 +8,7 @@ module analysis.helpers; import std.string; import std.traits; import std.d.ast; +import analysis.run; S between(S)(S value, S before, S after) From 657ef65952639750eb90237e79589d3aad11590d Mon Sep 17 00:00:00 2001 From: Matthew Brennan Jones Date: Sun, 25 May 2014 13:06:38 -0700 Subject: [PATCH 4/4] Added pure and nothrow to the duplicate attribute checker. --- analysis/duplicate_attribute.d | 145 ++++++++++++++++++++++++++------- 1 file changed, 117 insertions(+), 28 deletions(-) diff --git a/analysis/duplicate_attribute.d b/analysis/duplicate_attribute.d index 777e9d2..556d319 100644 --- a/analysis/duplicate_attribute.d +++ b/analysis/duplicate_attribute.d @@ -12,10 +12,10 @@ import std.d.lexer; import analysis.base; import analysis.helpers; -// FIXME: Make it work with pure, nothrow /** - * Checks for duplicate attributes such as @property, @safe, @trusted, @system + * Checks for duplicate attributes such as @property, @safe, + * @trusted, @system, pure, and nothrow */ class DuplicateAttributeCheck : BaseAnalyzer { @@ -38,23 +38,24 @@ class DuplicateAttributeCheck : BaseAnalyzer bool hasSafe = false; bool hasTrusted = false; bool hasSystem = false; + bool hasPure = false; + bool hasNoThrow = false; // Check the attributes foreach (attribute; node.attributes) { - // Just skip if missing child nodes - if (!attribute - || !attribute.storageClass - || !attribute.storageClass.atAttribute - || attribute.storageClass.atAttribute.identifier is Token.init) - continue; + size_t line, column; + string attributeName = getAttributeName(attribute, line, column); + if (!attributeName || line==0 || column==0) + return; // Check for the attributes - auto iden = attribute.storageClass.atAttribute.identifier; - checkDuplicateAttribute(iden, "property", hasProperty); - checkDuplicateAttribute(iden, "safe", hasSafe); - checkDuplicateAttribute(iden, "trusted", hasTrusted); - checkDuplicateAttribute(iden, "system", hasSystem); + checkDuplicateAttribute(attributeName, "property", line, column, hasProperty); + checkDuplicateAttribute(attributeName, "safe", line, column, hasSafe); + checkDuplicateAttribute(attributeName, "trusted", line, column, hasTrusted); + checkDuplicateAttribute(attributeName, "system", line, column, hasSystem); + checkDuplicateAttribute(attributeName, "pure", line, column, hasPure); + checkDuplicateAttribute(attributeName, "nothrow", line, column, hasNoThrow); } // Just return if missing function nodes @@ -65,38 +66,96 @@ class DuplicateAttributeCheck : BaseAnalyzer // Check the functions foreach (memberFunctionAttribute; node.functionDeclaration.memberFunctionAttributes) { - // Just skip if missing child nodes - if (!memberFunctionAttribute - || !memberFunctionAttribute.atAttribute - || memberFunctionAttribute.atAttribute.identifier is Token.init) - continue; + size_t line, column; + string attributeName = getAttributeName(memberFunctionAttribute, line, column); + if (!attributeName || line==0 || column==0) + return; // Check for the attributes - auto iden = memberFunctionAttribute.atAttribute.identifier; - checkDuplicateAttribute(iden, "property", hasProperty); - checkDuplicateAttribute(iden, "safe", hasSafe); - checkDuplicateAttribute(iden, "trusted", hasTrusted); - checkDuplicateAttribute(iden, "system", hasSystem); + checkDuplicateAttribute(attributeName, "property", line, column, hasProperty); + checkDuplicateAttribute(attributeName, "safe", line, column, hasSafe); + checkDuplicateAttribute(attributeName, "trusted", line, column, hasTrusted); + checkDuplicateAttribute(attributeName, "system", line, column, hasSystem); + checkDuplicateAttribute(attributeName, "pure", line, column, hasPure); + checkDuplicateAttribute(attributeName, "nothrow", line, column, hasNoThrow); } } - void checkDuplicateAttribute(const Token token, const string attributeName, ref bool hasAttribute) + void checkDuplicateAttribute(const string attributeName, const string attributeDesired, size_t line, size_t column, ref bool hasAttribute) { // Just return if not an attribute - if (token.type != tok!"identifier" - || token.text != attributeName) + if (attributeName != attributeDesired) return; // Already has that attribute if (hasAttribute) { - string message = "The attribute '%s' is duplicated.".format(token.text); - addErrorMessage(token.line, token.column, message); + string message = "The attribute '%s' is duplicated.".format(attributeName); + addErrorMessage(line, column, message); } // Mark it as having that attribute hasAttribute = true; } + + string getAttributeName(const Attribute attribute, ref size_t line, ref size_t column) + { + // Get the name from the attribute identifier + if (attribute + && attribute.storageClass + && attribute.storageClass.atAttribute + && attribute.storageClass.atAttribute.identifier !is Token.init + && attribute.storageClass.atAttribute.identifier.text + && attribute.storageClass.atAttribute.identifier.text.length) + { + auto token = attribute.storageClass.atAttribute.identifier; + line = token.line; + column = token.column; + return token.text; + } + + // Get the attribute from the storage class token + if (attribute + && attribute.storageClass + && attribute.storageClass.token !is Token.init) + { + auto token = attribute.storageClass.token; + line = token.line; + column = token.column; + return token.type.str; + } + + return null; + } + + string getAttributeName(const MemberFunctionAttribute memberFunctionAttribute, ref size_t line, ref size_t column) + { + // Get the name from the tokenType + if (memberFunctionAttribute + && memberFunctionAttribute.tokenType !is IdType.init + && memberFunctionAttribute.tokenType.str + && memberFunctionAttribute.tokenType.str.length) + { + // FIXME: How do we get the line/column number? + return memberFunctionAttribute.tokenType.str; + } + + // Get the name from the attribute identifier + if (memberFunctionAttribute + && memberFunctionAttribute.atAttribute + && memberFunctionAttribute.atAttribute.identifier !is Token.init + && memberFunctionAttribute.atAttribute.identifier.type == tok!"identifier" + && memberFunctionAttribute.atAttribute.identifier.text + && memberFunctionAttribute.atAttribute.identifier.text.length) + { + auto iden = memberFunctionAttribute.atAttribute.identifier; + line = iden.line; + column = iden.column; + return iden.text; + } + + return null; + } } unittest @@ -133,6 +192,36 @@ unittest return false; } } + + class ExamplePureNoThrow + { + pure nothrow bool aaa() // ok + { + return false; + } + + pure pure bool bbb() // [warn]: The attribute 'pure' is duplicated. + { + return false; + } + + // FIXME: There is no way to get the line/column number of the attribute like this + bool ccc() pure pure // FIXME: [warn]: The attribute 'pure' is duplicated. + { + return false; + } + + nothrow nothrow bool ddd() // [warn]: The attribute 'nothrow' is duplicated. + { + return false; + } + + // FIXME: There is no way to get the line/column number of the attribute like this + bool eee() nothrow nothrow // FIXME: [warn]: The attribute 'nothrow' is duplicated. + { + return false; + } + } }c, analysis.run.AnalyzerCheck.duplicate_attribute); stderr.writeln("Unittest for DuplicateAttributeCheck passed.");