Adds a check for too much trusted scope, close #545 (#581)

Adds a check for too much trusted scope, close #545
merged-on-behalf-of: BBasile <BBasile@users.noreply.github.com>

README.md

@@ -141,6 +141,7 @@ Note that the "--skipTests" option is the equivalent of changing each
 * Public declarations without a documented unittest. By default disabled.
 * Asserts without an explanatory message. By default disabled.
 * Indentation of if constraints
+* Check that `@trusted` is not applied to a whole scope. Trusting a whole scope can be a problem when new declarations are added and if they are not verified manually to be trustable.
 
 #### Wishlist
 

src/dscanner/analysis/config.d

@@ -17,41 +17,40 @@ StaticAnalysisConfig defaultStaticAnalysisConfig()
 /// Describes how a check is operated.
 enum Check: string
 {
-    /// Check is disabled.
-    disabled    = "disabled",
-    /// Check is enabled.
-    enabled     = "enabled",
-    /// Check is enabled but not operated in the unittests.
-    skipTests   = "skip-unittest"
+	/// Check is disabled.
+	disabled    = "disabled",
+	/// Check is enabled.
+	enabled     = "enabled",
+	/// Check is enabled but not operated in the unittests.
+	skipTests   = "skip-unittest"
 }
 
 /// Applies the --skipTests switch, allowing to call Dscanner without config
 /// and less noise related to the unittests.
 void enabled2SkipTests(ref StaticAnalysisConfig config)
 {
-    foreach (mem; __traits(allMembers, StaticAnalysisConfig))
-    {
-        static if (is(typeof(__traits(getMember, StaticAnalysisConfig, mem))))
-            static if (is(typeof(__traits(getMember, config, mem)) == string))
-        {
-            if (__traits(getMember, config, mem) == Check.enabled)
-                __traits(getMember, config, mem) = Check.skipTests;
-
-        }
-    }
+	foreach (mem; __traits(allMembers, StaticAnalysisConfig))
+	{
+		static if (is(typeof(__traits(getMember, StaticAnalysisConfig, mem))))
+			static if (is(typeof(__traits(getMember, config, mem)) == string))
+		{
+			if (__traits(getMember, config, mem) == Check.enabled)
+				__traits(getMember, config, mem) = Check.skipTests;
+		}
+	}
 }
 
 /// Returns a config with all the checks disabled.
 StaticAnalysisConfig disabledConfig()
 {
-    StaticAnalysisConfig config;
-    foreach (mem; __traits(allMembers, StaticAnalysisConfig))
-    {
-        static if (is(typeof(__traits(getMember, StaticAnalysisConfig, mem))))
-            static if (is(typeof(__traits(getMember, config, mem)) == string))
-                __traits(getMember, config, mem) = Check.disabled;
-    }
-    return config;
+	StaticAnalysisConfig config;
+	foreach (mem; __traits(allMembers, StaticAnalysisConfig))
+	{
+		static if (is(typeof(__traits(getMember, StaticAnalysisConfig, mem))))
+		static if (is(typeof(__traits(getMember, config, mem)) == string))
+			__traits(getMember, config, mem) = Check.disabled;
+	}
+	return config;
 }
 
 @INI("Configure which static analysis checks are enabled", "analysis.config.StaticAnalysisConfig")
@@ -171,14 +170,14 @@ struct StaticAnalysisConfig
 	@INI("Check for properly documented public functions (Returns, Params)")
 	string properly_documented_public_functions = Check.disabled;
 
-    @INI("Check for useless usage of the final attribute")
-    string final_attribute_check = Check.enabled;
+	@INI("Check for useless usage of the final attribute")
+	string final_attribute_check = Check.enabled;
 
-    @INI("Check for virtual calls in the class constructors")
-    string vcall_in_ctor = Check.enabled;
+	@INI("Check for virtual calls in the class constructors")
+	string vcall_in_ctor = Check.enabled;
 
-    @INI("Check for useless user defined initializers")
-    string useless_initializer = Check.enabled;
+	@INI("Check for useless user defined initializers")
+	string useless_initializer = Check.enabled;
 
 	@INI("Check allman brace style")
 	string allman_braces_check = Check.disabled;
@@ -195,6 +194,9 @@ struct StaticAnalysisConfig
 	@INI("Check indent of if constraints")
 	string if_constraints_indent = Check.disabled;
 
+	@INI("Check for @trusted applied to a bigger scope than a single function")
+	string trust_too_much = Check.enabled;
+
 	@INI("Module-specific filters")
 	ModuleFilters filters;
 }
@@ -207,7 +209,7 @@ private template ModuleFiltersMixin(A)
 			static if (is(typeof(__traits(getMember, StaticAnalysisConfig, mem)) == string))
 				s ~= `@INI("Exclude/Import modules") string[] ` ~ mem ~ ";\n";
 
-    	return s;
+		return s;
 	}();
 }
 

src/dscanner/analysis/run.d

@@ -72,6 +72,7 @@ import dscanner.analysis.redundant_attributes;
 import dscanner.analysis.has_public_example;
 import dscanner.analysis.assert_without_msg;
 import dscanner.analysis.if_constraints_indent;
+import dscanner.analysis.trust_too_much;
 
 import dsymbol.string_interning : internString;
 import dsymbol.scope_;
@@ -511,6 +512,10 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a
 		checks ~= new IfConstraintsIndentCheck(fileName, tokens,
 		analysisConfig.if_constraints_indent == Check.skipTests && !ut);
 
+	if (moduleName.shouldRun!"trust_too_much"(analysisConfig))
+		checks ~= new TrustTooMuchCheck(fileName,
+		analysisConfig.trust_too_much == Check.skipTests && !ut);
+
 	version (none)
 		if (moduleName.shouldRun!"redundant_if_check"(analysisConfig))
 			checks ~= new IfStatementCheck(fileName, moduleScope,

src/dscanner/analysis/trust_too_much.d

@@ -0,0 +1,136 @@
+//          Copyright The dlang community - 2018
+// 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 dscanner.analysis.trust_too_much;
+
+import std.stdio;
+import dparse.ast;
+import dparse.lexer;
+import dscanner.analysis.base;
+import dsymbol.scope_;
+
+/**
+ * Checks that `@trusted` is only applied to a a single function
+ */
+class TrustTooMuchCheck : BaseAnalyzer
+{
+private:
+
+	static immutable MESSAGE = "Trusting a whole scope is a bad idea, " ~
+		"`@trusted` should only be attached to a single function";
+	static immutable string KEY = "dscanner.trust_too_much";
+
+	bool checkAtAttribute = true;
+
+public:
+
+	alias visit = BaseAnalyzer.visit;
+
+	///
+	this(string fileName, bool skipTests = false)
+	{
+		super(fileName, sc, skipTests);
+	}
+
+	override void visit(const AtAttribute d)
+	{
+		if (checkAtAttribute && d.identifier.text == "trusted")
+		{
+			const Token t = d.identifier;
+			addErrorMessage(t.line, t.column, KEY, MESSAGE);
+		}
+		d.accept(this);
+	}
+
+	// always applied to function body, so OK
+	override void visit(const MemberFunctionAttribute d)
+	{
+		const oldCheckAtAttribute = checkAtAttribute;
+		checkAtAttribute = false;
+		d.accept(this);
+		checkAtAttribute = oldCheckAtAttribute;
+	}
+
+	// handles `@trusted{}` and old style, leading, atAttribute for single funcs
+	override void visit(const Declaration d)
+	{
+		const oldCheckAtAttribute = checkAtAttribute;
+		checkAtAttribute = d.functionDeclaration is null;
+		d.accept(this);
+		checkAtAttribute = oldCheckAtAttribute;
+	}
+}
+
+unittest
+{
+	import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig;
+	import dscanner.analysis.helpers : assertAnalyzerWarnings;
+	import std.format : format;
+
+	StaticAnalysisConfig sac = disabledConfig();
+	sac.trust_too_much = Check.enabled;
+	const msg = TrustTooMuchCheck.MESSAGE;
+
+	//--- fail cases ---//
+
+	assertAnalyzerWarnings(q{
+	@trusted: // [warn]: %s
+		void test();
+	}c.format(msg), sac);
+
+	assertAnalyzerWarnings(q{
+	@trusted @nogc: // [warn]: %s
+		void test();
+	}c.format(msg), sac);
+
+	assertAnalyzerWarnings(q{
+	@trusted { // [warn]: %s
+		void test();
+		void test();
+	}
+	}c.format(msg), sac);
+
+	assertAnalyzerWarnings(q{
+	@safe {
+		@trusted @nogc { // [warn]: %s
+		void test();
+		void test();
+	}}
+	}c.format(msg), sac);
+
+	assertAnalyzerWarnings(q{
+	@nogc @trusted { // [warn]: %s
+		void test();
+		void test();
+	}
+	}c.format(msg), sac);
+
+	assertAnalyzerWarnings(q{
+	@trusted template foo(){ // [warn]: %s
+	}
+	}c.format(msg), sac);
+
+	assertAnalyzerWarnings(q{
+	struct foo{
+	@trusted:  // [warn]: %s
+	}
+	}c.format(msg), sac);
+	//--- pass cases ---//
+
+	assertAnalyzerWarnings(q{
+	void test() @trusted {}
+	}c, sac);
+
+	assertAnalyzerWarnings(q{
+	@trusted void test();
+	}c, sac);
+
+	assertAnalyzerWarnings(q{
+	@nogc template foo(){
+	}
+	}c , sac);
+
+	stderr.writeln("Unittest for TrustTooMuchCheck passed.");
+}