From 9b97b7d929f5ce1219c03fef935d147f3da44ba1 Mon Sep 17 00:00:00 2001 From: Basile Burg Date: Mon, 16 Jan 2017 06:38:55 +0100 Subject: [PATCH 1/5] add a checker for cases where final is a noop --- README.md | 1 + src/analysis/config.d | 3 + src/analysis/final_attribute.d | 294 +++++++++++++++++++++++++++++++++ src/analysis/run.d | 5 + 4 files changed, 303 insertions(+) create mode 100644 src/analysis/final_attribute.d diff --git a/README.md b/README.md index c1d681e..4438442 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,7 @@ Note that the "--skipTests" option is the equivalent of changing each * Incorrect infinite range definitions. * Some assertions that check conditions that will always be true. * Auto functions without return statement. The compiler doesn't see an omission and it infers 'void' as return type. +* `final` attribute is used but in this context it's a noop. #### Wishlist diff --git a/src/analysis/config.d b/src/analysis/config.d index bc86450..32ccc75 100644 --- a/src/analysis/config.d +++ b/src/analysis/config.d @@ -153,4 +153,7 @@ struct StaticAnalysisConfig @INI("Check for explicitly annotated unittests") string explicitly_annotated_unittests = Check.disabled; + + @INI("Check for useless usage of the final attribute") + string final_attribute_check = Check.disabled; } diff --git a/src/analysis/final_attribute.d b/src/analysis/final_attribute.d new file mode 100644 index 0000000..551f223 --- /dev/null +++ b/src/analysis/final_attribute.d @@ -0,0 +1,294 @@ +// Copyright Basile Burg 2017. +// 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.final_attribute; + +import analysis.base; +import analysis.helpers; +import dparse.ast; +import dparse.lexer; + +import std.stdio; + +/** + * Checks for useless usage of the final attribute. + * + * There several cases where the compiler allows them even if it's a noop. + */ +final class FinalAttributeChecker : BaseAnalyzer +{ + +private: + + enum string KEY = "dscanner.useless.final"; + enum string MSGB = "Useless final attribute, %s"; + + static struct MESSAGE + { + static immutable struct_i = "structs inherit of anything"; + static immutable union_i = "unions inherit of anything"; + static immutable class_t = "templatized class member functions are never virtual"; + static immutable class_p = "private class member functions are never virtual"; + static immutable class_f = "final class member functions are never virtual"; + static immutable interface_t = "templatized interface functions are never virtual"; + static immutable struct_f = "struct member functions are never virtual"; + static immutable union_f = "union member functions are never virtual"; + static immutable func_n = "nested functions are never virtual"; + static immutable func_g = "global functions are never virtual"; + } + + enum Parent + { + module_, + struct_, + union_, + class_, + function_, + interface_ + } + + bool[] _private; + bool _finalAggregate; + Parent _parent = Parent.module_; + + void addError(T)(T t, string msg) + { + import std.format : format; + const size_t lne = t.name.line; + const size_t col = t.name.column; + addErrorMessage(lne, col, KEY, MSGB.format(msg)); + } + +public: + + alias visit = BaseAnalyzer.visit; + + /// + this(string fileName, bool skipTests = false) + { + super(fileName, null, skipTests); + _private.length = 1; + } + + override void visit(const(StructDeclaration) sd) + { + const Parent saved = _parent; + _parent = Parent.struct_; + _private.length += 1; + sd.accept(this); + _private.length -= 1; + _parent = saved; + } + + override void visit(const(InterfaceDeclaration) id) + { + const Parent saved = _parent; + _parent = Parent.interface_; + _private.length += 1; + id.accept(this); + _private.length -= 1; + _parent = saved; + } + + override void visit(const(UnionDeclaration) ud) + { + const Parent saved = _parent; + _parent = Parent.union_; + _private.length += 1; + ud.accept(this); + _private.length -= 1; + _parent = saved; + } + + override void visit(const(ClassDeclaration) cd) + { + const Parent saved = _parent; + _parent = Parent.class_; + _private.length += 1; + cd.accept(this); + _private.length -= 1; + _parent = saved; + } + + override void visit(const(Declaration) d) + { + const Parent savedParent = _parent; + bool privatePushed; + _parent = Parent.function_; + + scope(exit) + { + d.accept(this); + _parent = savedParent; + } + + import std.algorithm.searching : find; + import std.algorithm.iteration: filter; + import std.range.primitives : empty; + + if (d.attributeDeclaration && d.attributeDeclaration.attribute) + { + const tp = d.attributeDeclaration.attribute.attribute.type; + _private[$-1] = isProtection(tp) & (tp == tok!"private"); + } + + const bool isFinal = !d.attributes + .find!(a => a.attribute.type == tok!"final") + .empty; + + // determine if private + const bool changeProtectionOnce = !d.attributes + .filter!(a => a.attribute.type.isProtection) + .empty; + + const bool isPrivateOnce = !d.attributes + .find!(a => a.attribute.type == tok!"private") + .empty; + + bool isPrivate; + if (_private[$-1] && isPrivateOnce) + isPrivate = true; + else if (!_private[$-1] && isPrivateOnce) + isPrivate = true; + else if (_private[$-1] && !changeProtectionOnce) + isPrivate = true; + + // check final aggregate type + if (d.classDeclaration || d.structDeclaration || d.unionDeclaration) + { + _finalAggregate = isFinal; + if (savedParent == Parent.module_) + { + if (d.structDeclaration) + addError(d.structDeclaration, MESSAGE.struct_i); + else if (d.unionDeclaration) + addError(d.unionDeclaration, MESSAGE.union_i); + } + } + + if (!d.functionDeclaration) + return; + + // check final functions + const(FunctionDeclaration) fd = d.functionDeclaration; + + if (isFinal) final switch(savedParent) + { + case Parent.class_: + if (fd.templateParameters) + addError(fd, MESSAGE.class_t); + if (isPrivate) + addError(fd, MESSAGE.class_p); + else if (_finalAggregate) + addError(fd, MESSAGE.class_f); + break; + case Parent.interface_: + if (fd.templateParameters) + addError(fd, MESSAGE.interface_t); + break; + case Parent.struct_: + addError(fd, MESSAGE.struct_f); + break; + case Parent.union_: + addError(fd, MESSAGE.union_f); + break; + case Parent.function_: + addError(fd, MESSAGE.func_n); + break; + case Parent.module_: + addError(fd, MESSAGE.func_g); + break; + } + } +} + +@system unittest +{ + import std.stdio : stderr; + import std.format : format; + import analysis.config : StaticAnalysisConfig, Check; + import analysis.helpers : assertAnalyzerWarnings; + + StaticAnalysisConfig sac; + sac.final_attribute_check = Check.enabled; + + // pass + + assertAnalyzerWarnings(q{ + void foo(){} + }, sac); + + assertAnalyzerWarnings(q{ + void foo(){void foo(){}} + }, sac); + + assertAnalyzerWarnings(q{ + class Foo{public final void foo(){}} + }, sac); + + assertAnalyzerWarnings(q{ + final class Foo{static struct Bar{}} + }, sac); + + assertAnalyzerWarnings(q{ + class Foo{private: public final void foo(){}} + }, sac); + + assertAnalyzerWarnings(q{ + class Foo{private: public: final void foo(){}} + }, sac); + + // fail + + assertAnalyzerWarnings(q{ + final void foo(){} // [warn]: %s + }c.format( + FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.func_g) + ), sac); + + assertAnalyzerWarnings(q{ + void foo(){final void foo(){}} // [warn]: %s + }c.format( + FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.func_n) + ), sac); + + assertAnalyzerWarnings(q{ + final struct Foo{} // [warn]: %s + }c.format( + FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.struct_i) + ), sac); + + assertAnalyzerWarnings(q{ + final union Foo{} // [warn]: %s + }c.format( + FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.union_i) + ), sac); + + assertAnalyzerWarnings(q{ + class Foo{private final void foo(){}} // [warn]: %s + }c.format( + FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_p) + ), sac); + + assertAnalyzerWarnings(q{ + class Foo{private: final void foo(){}} // [warn]: %s + }c.format( + FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_p) + ), sac); + + assertAnalyzerWarnings(q{ + interface Foo{final void foo(T)(){}} // [warn]: %s + }c.format( + FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.interface_t) + ), sac); + + assertAnalyzerWarnings(q{ + final class Foo{final void foo(){}} // [warn]: %s + }c.format( + FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_f) + ), sac); + + stderr.writeln("Unittest for FinalAttributeChecker passed."); +} diff --git a/src/analysis/run.d b/src/analysis/run.d index 965ea3b..0bc5ec4 100644 --- a/src/analysis/run.d +++ b/src/analysis/run.d @@ -62,6 +62,7 @@ import analysis.lambda_return_check; import analysis.auto_function; import analysis.imports_sortedness; import analysis.explicitly_annotated_unittests; +import analysis.final_attribute; import dsymbol.string_interning : internString; import dsymbol.scope_; @@ -369,6 +370,10 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new ExplicitlyAnnotatedUnittestCheck(fileName, analysisConfig.explicitly_annotated_unittests == Check.skipTests && !ut); + if (analysisConfig.final_attribute_check != Check.disabled) + checks ~= new FinalAttributeChecker(fileName, + analysisConfig.final_attribute_check == Check.skipTests && !ut); + version (none) if (analysisConfig.redundant_if_check != Check.disabled) checks ~= new IfStatementCheck(fileName, moduleScope, From d9b6828205e55e9f602017858dc05ec09ba25493 Mon Sep 17 00:00:00 2001 From: Basile Burg Date: Mon, 16 Jan 2017 17:07:14 +0100 Subject: [PATCH 2/5] fix false warning on top level struct/union --- src/analysis/final_attribute.d | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/analysis/final_attribute.d b/src/analysis/final_attribute.d index 551f223..bb225ab 100644 --- a/src/analysis/final_attribute.d +++ b/src/analysis/final_attribute.d @@ -159,7 +159,7 @@ public: if (d.classDeclaration || d.structDeclaration || d.unionDeclaration) { _finalAggregate = isFinal; - if (savedParent == Parent.module_) + if (_finalAggregate && savedParent == Parent.module_) { if (d.structDeclaration) addError(d.structDeclaration, MESSAGE.struct_i); @@ -224,6 +224,14 @@ public: void foo(){void foo(){}} }, sac); + assertAnalyzerWarnings(q{ + struct S{} + }, sac); + + assertAnalyzerWarnings(q{ + union U{} + }, sac); + assertAnalyzerWarnings(q{ class Foo{public final void foo(){}} }, sac); From fb2b2182e224ee341802c283aaf15b61fbb93a2c Mon Sep 17 00:00:00 2001 From: Basile Burg Date: Mon, 16 Jan 2017 20:46:01 +0100 Subject: [PATCH 3/5] fix, unhanlded declarations caused wrong results --- src/analysis/final_attribute.d | 35 +++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/analysis/final_attribute.d b/src/analysis/final_attribute.d index bb225ab..821cbfc 100644 --- a/src/analysis/final_attribute.d +++ b/src/analysis/final_attribute.d @@ -116,7 +116,6 @@ public: { const Parent savedParent = _parent; bool privatePushed; - _parent = Parent.function_; scope(exit) { @@ -124,6 +123,14 @@ public: _parent = savedParent; } + if (!d.attributeDeclaration && + !d.classDeclaration && + !d.structDeclaration && + !d.unionDeclaration && + !d.interfaceDeclaration && + !d.functionDeclaration) + return; + import std.algorithm.searching : find; import std.algorithm.iteration: filter; import std.range.primitives : empty; @@ -172,6 +179,7 @@ public: return; // check final functions + _parent = Parent.function_; const(FunctionDeclaration) fd = d.functionDeclaration; if (isFinal) final switch(savedParent) @@ -248,6 +256,21 @@ public: class Foo{private: public: final void foo(){}} }, sac); + assertAnalyzerWarnings(q{ + class Foo{private: public: final void foo(){}} + }, sac); + + assertAnalyzerWarnings(q{ + class Impl + { + private: + static if (true) + { + protected final void _wrap_getSource() {} + } + } + }, sac); + // fail assertAnalyzerWarnings(q{ @@ -262,6 +285,16 @@ public: FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.func_n) ), sac); + assertAnalyzerWarnings(q{ + void foo() + { + static if (true) + final class A{ private: final protected void foo(){}} // [warn]: %s + } + }c.format( + FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_f) + ), sac); + assertAnalyzerWarnings(q{ final struct Foo{} // [warn]: %s }c.format( From 437b8e169d49e2bbeb2049eb2ba26a5e8bb0b4ef Mon Sep 17 00:00:00 2001 From: Basile Burg Date: Tue, 17 Jan 2017 05:54:40 +0100 Subject: [PATCH 4/5] improve error messages, fix mixed tbs/spaces, remove unused variable --- src/analysis/final_attribute.d | 59 ++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/src/analysis/final_attribute.d b/src/analysis/final_attribute.d index 821cbfc..03a749b 100644 --- a/src/analysis/final_attribute.d +++ b/src/analysis/final_attribute.d @@ -10,31 +10,29 @@ import analysis.helpers; import dparse.ast; import dparse.lexer; -import std.stdio; - /** * Checks for useless usage of the final attribute. * - * There several cases where the compiler allows them even if it's a noop. + * There are several cases where the compiler allows them even if it's a noop. */ final class FinalAttributeChecker : BaseAnalyzer { private: - enum string KEY = "dscanner.useless.final"; - enum string MSGB = "Useless final attribute, %s"; + enum string KEY = "dscanner.useless.final"; + enum string MSGB = "Useless final attribute, %s"; static struct MESSAGE { - static immutable struct_i = "structs inherit of anything"; - static immutable union_i = "unions inherit of anything"; - static immutable class_t = "templatized class member functions are never virtual"; - static immutable class_p = "private class member functions are never virtual"; - static immutable class_f = "final class member functions are never virtual"; - static immutable interface_t = "templatized interface functions are never virtual"; - static immutable struct_f = "struct member functions are never virtual"; - static immutable union_f = "union member functions are never virtual"; + static immutable struct_i = "structs cannot be subclassed"; + static immutable union_i = "unions cannot be subclassed"; + static immutable class_t = "templated functions declared within a class are never virtual"; + static immutable class_p = "private functions declared within a class are never virtual"; + static immutable class_f = "functions declared within a final class are never virtual"; + static immutable interface_t = "templated functions declared within an interface are never virtual"; + static immutable struct_f = "functions declared within a struct are never virtual"; + static immutable union_f = "functions declared within an union are never virtual"; static immutable func_n = "nested functions are never virtual"; static immutable func_g = "global functions are never virtual"; } @@ -63,14 +61,14 @@ private: public: - alias visit = BaseAnalyzer.visit; + alias visit = BaseAnalyzer.visit; - /// - this(string fileName, bool skipTests = false) - { - super(fileName, null, skipTests); + /// + this(string fileName, bool skipTests = false) + { + super(fileName, null, skipTests); _private.length = 1; - } + } override void visit(const(StructDeclaration) sd) { @@ -112,10 +110,9 @@ public: _parent = saved; } - override void visit(const(Declaration) d) - { + override void visit(const(Declaration) d) + { const Parent savedParent = _parent; - bool privatePushed; scope(exit) { @@ -131,8 +128,8 @@ public: !d.functionDeclaration) return; - import std.algorithm.searching : find; import std.algorithm.iteration: filter; + import std.algorithm.searching : find; import std.range.primitives : empty; if (d.attributeDeclaration && d.attributeDeclaration.attribute) @@ -214,10 +211,10 @@ public: @system unittest { - import std.stdio : stderr; - import std.format : format; - import analysis.config : StaticAnalysisConfig, Check; - import analysis.helpers : assertAnalyzerWarnings; + import analysis.config : StaticAnalysisConfig, Check; + import analysis.helpers : assertAnalyzerWarnings; + import std.stdio : stderr; + import std.format : format; StaticAnalysisConfig sac; sac.final_attribute_check = Check.enabled; @@ -331,5 +328,11 @@ public: FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_f) ), sac); - stderr.writeln("Unittest for FinalAttributeChecker passed."); + assertAnalyzerWarnings(q{ + private: final class Foo {public: private final void foo(){}} // [warn]: %s + }c.format( + FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_p) + ), sac); + + stderr.writeln("Unittest for FinalAttributeChecker passed."); } From 2b8ba6ffcacc6541b8af1dcc4036021335070fd2 Mon Sep 17 00:00:00 2001 From: Basile Burg Date: Tue, 17 Jan 2017 12:08:35 +0100 Subject: [PATCH 5/5] fix sloppy test for private protection --- src/analysis/final_attribute.d | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/analysis/final_attribute.d b/src/analysis/final_attribute.d index 03a749b..89ae0ec 100644 --- a/src/analysis/final_attribute.d +++ b/src/analysis/final_attribute.d @@ -128,7 +128,7 @@ public: !d.functionDeclaration) return; - import std.algorithm.iteration: filter; + import std.algorithm.iteration : filter; import std.algorithm.searching : find; import std.range.primitives : empty; @@ -152,9 +152,7 @@ public: .empty; bool isPrivate; - if (_private[$-1] && isPrivateOnce) - isPrivate = true; - else if (!_private[$-1] && isPrivateOnce) + if (isPrivateOnce) isPrivate = true; else if (_private[$-1] && !changeProtectionOnce) isPrivate = true;