Merge remote-tracking branch 'upstream/master' into phobos

.travis.yml

@@ -16,10 +16,10 @@ script: "./.travis.sh"
 jobs:
   include:
     - stage: GitHub Release
-      if: tag IS present
+      #if: tag IS present
-      d: ldc
+      d: ldc-1.8.0
       os: linux
-      script: echo "Deploying to GitHub releases ..." && make release
+      script: echo "Deploying to GitHub releases ..." && ./release.sh
       deploy:
         provider: releases
         api_key: $GH_REPO_TOKEN
@@ -30,10 +30,10 @@ jobs:
           repo: dlang-community/D-Scanner
           tags: true
     - stage: GitHub Release
-      if: tag IS present
+      #if: tag IS present
-      d: ldc
+      d: ldc-1.8.0
       os: osx
-      script: echo "Deploying to GitHub releases ..." && make release
+      script: echo "Deploying to GitHub releases ..." && ./release.sh
       deploy:
         provider: releases
         api_key: $GH_REPO_TOKEN
@@ -43,3 +43,6 @@ jobs:
         on:
           repo: dlang-community/D-Scanner
           tags: true
+stages:
+  - name: test
+    if: type = pull_request or (type = push and branch = master)

README.md

@@ -142,6 +142,7 @@ Note that the "--skipTests" option is the equivalent of changing each
 * 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.
+* Redundant storage class attributes
 
 #### Wishlist
 
@@ -228,7 +229,7 @@ the given source file to standard output in XML format.
 ```sh
 $ dscanner --ast helloworld.d
 ```
-	
+
 ```xml
 <module>
 <declaration>

dsymbol

@@ -1 +1 @@
-Subproject commit 5b90412457ac5f1d67c04e4da01587edfd529ad5
+Subproject commit 239b137b280c06864b73fcc1d00b75e06568d4c2

dub.json

@@ -12,9 +12,9 @@
     "StdLoggerDisableWarning"
   ],
   "dependencies" : {
-    "libdparse": "~>0.8.0-alpha.5",
+    "libdparse": "~>0.8.0",
-    "dsymbol" : "~>0.3.0-alpha.3",
+    "dsymbol" : "~>0.3.0",
-    "inifiled" : "~>1.2.0",
+    "inifiled" : "~>1.3.0",
     "emsi_containers" : "~>0.6.0",
     "libddoc" : "~>0.3.0-beta.1",
     "stdx-allocator" : "~>2.77.0"

inifiled

@@ -1 +1 @@
-Subproject commit 971c5356388a73ebbf69e32f7f5e97cfc06cdcff
+Subproject commit 9d7333ec17f116a05712a24df139ff2f212a9867

libdparse

@@ -1 +1 @@
-Subproject commit ee0fa01ab74b6bf27bed3c7bdb9d6fb789963342
+Subproject commit 970efe34e66fc7b3cb93a6ec59984099908070c5

makefile

@@ -27,6 +27,8 @@ VERSIONS =
 DEBUG_VERSIONS = -version=dparse_verbose
 DMD_FLAGS = -w -inline -release -O -J. -od${OBJ_DIR} -version=StdLoggerDisableWarning -fPIC
 DMD_TEST_FLAGS = -w -g -J. -version=StdLoggerDisableWarning
+override LDC_FLAGS += -O5 -release -oq
+override GDC_FLAGS += -O3 -frelease
 SHELL:=/bin/bash
 
 all: dmdbuild
@@ -46,11 +48,11 @@ dmdbuild: githash $(SRC)
 
 gdcbuild: githash
 	mkdir -p bin
-	${GDC} -O3 -frelease -obin/dscanner ${VERSIONS} ${INCLUDE_PATHS} ${SRC} -J.
+	${GDC} ${GDC_FLAGS} -obin/dscanner ${VERSIONS} ${INCLUDE_PATHS} ${SRC} -J.
 
 ldcbuild: githash
 	mkdir -p bin
-	${LDC} -O5 -release -oq -of=bin/dscanner ${VERSIONS} ${INCLUDE_PATHS} ${SRC} -J.
+	${LDC} ${LDC_FLAGS} -of=bin/dscanner ${VERSIONS} ${INCLUDE_PATHS} ${SRC} -J.
 
 # compile the dependencies separately, s.t. their unittests don't get executed
 bin/dscanner-unittest-lib.a: ${LIB_SRC}
@@ -74,26 +76,5 @@ report: all
 	dscanner --report src > src/dscanner-report.json
 	sonar-runner
 
-.ONESHELL:
 release:
-	@set -eux -o pipefail
+	./release.sh
-	VERSION=$$(git describe --abbrev=0 --tags)
-	ARCH="$${ARCH:-64}"
-	unameOut="$$(uname -s)"
-	case "$$unameOut" in
-	    Linux*) OS=linux; ;;
-	    Darwin*) OS=osx; ;;
-	    *) echo "Unknown OS: $$unameOut"; exit 1
-	esac
-
-	case "$$ARCH" in
-	    64) ARCH_SUFFIX="x86_64";;
-	    32) ARCH_SUFFIX="x86";;
-	    *) echo "Unknown ARCH: $$ARCH"; exit 1
-	esac
-
-	archiveName="dscanner-$$VERSION-$$OS-$$ARCH_SUFFIX.tar.gz"
-
-	echo "Building $$archiveName"
-	${MAKE} ldcbuild
-	tar cvfz "bin/$$archiveName" -C bin dscanner

release.sh

@@ -0,0 +1,23 @@
+#!/usr/bin/env bash
+set -eux -o pipefail
+VERSION=$(git describe --abbrev=0 --tags)
+ARCH="${ARCH:-64}"
+LDC_FLAGS=()
+unameOut="$(uname -s)"
+case "$unameOut" in
+    Linux*) OS=linux; LDC_FLAGS=("-flto=full" "-linker=gold" "-static") ;;
+    Darwin*) OS=osx; LDC_FLAGS+=("-L-macosx_version_min" "-L10.7" "-L-lcrt1.o"); ;;
+    *) echo "Unknown OS: $unameOut"; exit 1
+esac
+
+case "$ARCH" in
+    64) ARCH_SUFFIX="x86_64";;
+    32) ARCH_SUFFIX="x86";;
+    *) echo "Unknown ARCH: $ARCH"; exit 1
+esac
+
+archiveName="dscanner-$VERSION-$OS-$ARCH_SUFFIX.tar.gz"
+
+echo "Building $archiveName"
+${MAKE:-make} ldcbuild LDC_FLAGS="${LDC_FLAGS[*]}"
+tar cvfz "bin/$archiveName" -C bin dscanner

src/dscanner/analysis/config.d

@@ -197,6 +197,9 @@ struct StaticAnalysisConfig
 	@INI("Check for @trusted applied to a bigger scope than a single function")
 	string trust_too_much = Check.enabled;
 
+	@INI("Check for redundant storage classes on variable declarations")
+	string redundant_storage_classes = Check.enabled;
+
 	@INI("Module-specific filters")
 	ModuleFilters filters;
 }

src/dscanner/analysis/opequals_without_tohash.d

@@ -76,12 +76,6 @@ class OpEqualsWithoutToHashCheck : BaseAnalyzer
 			string message = "'" ~ name.text ~ "' has method 'toHash', but not 'opEquals'.";
 			addErrorMessage(name.line, name.column, KEY, message);
 		}
-
-		if (hasOpCmp && !hasOpEquals)
-		{
-			addErrorMessage(name.line, name.column, KEY,
-					"'" ~ name.text ~ "' has method 'opCmp', but not 'opEquals'.");
-		}
 	}
 
 	enum string KEY = "dscanner.suspicious.incomplete_operator_overloading";
@@ -108,6 +102,15 @@ unittest
 			}
 		}
 
+		// AA would use default equal and default toHash
+		struct Bee
+		{
+			int opCmp(Bee) const
+			{
+				return true;
+			}
+		}
+
 		// Fail on class opEquals
 		class Rabbit // [warn]: 'Rabbit' has method 'opEquals', but not 'toHash'.
 		{

src/dscanner/analysis/properly_documented_public_functions.d

@@ -72,12 +72,13 @@ class ProperlyDocumentedPublicFunctions : BaseAnalyzer
 
 		if (islastSeenVisibilityLabelPublic || decl.attributes.map!`a.attribute`.any!(x => x == tokPublic))
 		{
-			if (decl.functionDeclaration !is null ||
+			// Don't complain about non-documented function declarations
-				decl.templateDeclaration !is null ||
+			if ((decl.functionDeclaration !is null && decl.functionDeclaration.comment.ptr !is null) ||
+				(decl.templateDeclaration !is null && decl.templateDeclaration.comment.ptr !is null) ||
 				decl.mixinTemplateDeclaration !is null ||
-				decl.classDeclaration !is null ||
+				(decl.classDeclaration !is null && decl.classDeclaration.comment.ptr !is null) ||
-				decl.structDeclaration !is null)
+				(decl.structDeclaration !is null && decl.structDeclaration.comment.ptr !is null))
-					decl.accept(this);
+				decl.accept(this);
 		}
 	}
 
@@ -112,7 +113,7 @@ class ProperlyDocumentedPublicFunctions : BaseAnalyzer
 
 	override void visit(const FunctionDeclaration decl)
 	{
-		import std.algorithm.searching : any;
+		import std.algorithm.searching : all, any;
 
 		// ignore header declaration for now
 		if (decl.functionBody is null)
@@ -453,8 +454,10 @@ unittest
 		Some text
 		*/
 		private void foo(int k){}
+		///
 		public int bar(){} // [warn]: %s
 	public:
+		///
 		int foobar(){} // [warn]: %s
 	}c.format(
 		ProperlyDocumentedPublicFunctions.MISSING_RETURNS_MESSAGE,
@@ -468,8 +471,10 @@ unittest
 		Some text
 		*/
 		private template foo(int k){}
+		///
 		public template bar(T){} // [warn]: %s
 	public:
+		///
 		template foobar(T){} // [warn]: %s
 	}c.format(
 		ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"),
@@ -483,8 +488,10 @@ unittest
 		Some text
 		*/
 		private struct foo(int k){}
+		///
 		public struct bar(T){} // [warn]: %s
 	public:
+		///
 		struct foobar(T){} // [warn]: %s
 	}c.format(
 		ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"),
@@ -761,9 +768,10 @@ unittest
  * Returns: bar
  */
 template abcde(Args ...) {
-    auto abcde(T, U...)(T t, U varargs) {
+	///
-        /// ....
+	auto abcde(T, U...)(T t, U varargs) {
-    }
+		/// ....
+	}
 }
 	}c, sac);
 }
@@ -801,6 +809,21 @@ string bar(P, R)(R r){}// [warn]: %s
 	), sac);
 }
 
+// https://github.com/dlang-community/D-Scanner/issues/601
+unittest
+{
+	StaticAnalysisConfig sac = disabledConfig;
+	sac.properly_documented_public_functions = Check.enabled;
+
+	assertAnalyzerWarnings(q{
+	void put(Range)(Range items) if (canPutConstRange!Range)
+	{
+		alias p = put!(Unqual!Range);
+		p(items);
+	}
+	}, sac);
+}
+
 // https://github.com/dlang-community/D-Scanner/issues/583
 unittest
 {

src/dscanner/analysis/redundant_storage_class.d

@@ -0,0 +1,102 @@
+// Copyright (c) 2018, dlang-community
+// 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.redundant_storage_class;
+
+import std.stdio;
+import std.string;
+import dparse.ast;
+import dparse.lexer;
+import dscanner.analysis.base;
+import dscanner.analysis.helpers;
+import dsymbol.scope_ : Scope;
+
+/**
+ * Checks for redundant storage classes such immutable and __gshared, static and __gshared
+ */
+class RedundantStorageClassCheck : BaseAnalyzer
+{
+	alias visit = BaseAnalyzer.visit;
+	enum string REDUNDANT_VARIABLE_ATTRIBUTES = "Variable declaration for `%s` has redundant attributes (%-(`%s`%|, %)).";
+
+	this(string fileName, bool skipTests = false)
+	{
+		super(fileName, null, skipTests);
+	}
+
+	override void visit(const Declaration node)
+	{
+		checkAttributes(node);
+		node.accept(this);
+	}
+
+	void checkAttributes(const Declaration node)
+	{
+		if (node.variableDeclaration !is null && node.attributes !is null)
+			checkVariableDeclaration(node.variableDeclaration, node.attributes);
+	}
+
+	void checkVariableDeclaration(const VariableDeclaration vd, const Attribute[] attributes)
+	{
+		import std.algorithm.comparison : among;
+		import std.algorithm.searching: all;
+
+		string[] globalAttributes;
+		foreach (attrib; attributes)
+		{
+			if (attrib.attribute.type.among(tok!"shared", tok!"static", tok!"__gshared", tok!"immutable"))
+				globalAttributes ~= attrib.attribute.type.str;
+		}
+		if (globalAttributes.length > 1)
+		{
+			if (globalAttributes.length == 2 && (
+					globalAttributes.all!(a => a.among("shared", "static")) ||
+					globalAttributes.all!(a => a.among("static", "immutable"))
+			))
+				return;
+			auto t = vd.declarators[0].name;
+			string message = REDUNDANT_VARIABLE_ATTRIBUTES.format(t.text, globalAttributes);
+			addErrorMessage(t.line, t.column, "dscanner.unnecessary.duplicate_attribute", message);
+		}
+	}
+}
+
+unittest
+{
+	import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig;
+
+	StaticAnalysisConfig sac = disabledConfig();
+	sac.redundant_storage_classes = Check.enabled;
+
+	// https://github.com/dlang-community/D-Scanner/issues/438
+	assertAnalyzerWarnings(q{
+		immutable int a;
+
+		immutable shared int a; // [warn]: %s
+		shared immutable int a; // [warn]: %s
+
+		immutable __gshared int a; // [warn]: %s
+		__gshared immutable int a; // [warn]: %s
+
+		__gshared static int a; // [warn]: %s
+
+		shared static int a;
+		static shared int a;
+		static immutable int a;
+		immutable static int a;
+
+		enum int a;
+		extern(C++) immutable int a;
+		immutable int function(immutable int, shared int) a;
+	}c.format(
+		RedundantStorageClassCheck.REDUNDANT_VARIABLE_ATTRIBUTES.format("a", ["immutable", "shared"]),
+		RedundantStorageClassCheck.REDUNDANT_VARIABLE_ATTRIBUTES.format("a", ["shared", "immutable"]),
+		RedundantStorageClassCheck.REDUNDANT_VARIABLE_ATTRIBUTES.format("a", ["immutable", "__gshared"]),
+		RedundantStorageClassCheck.REDUNDANT_VARIABLE_ATTRIBUTES.format("a", ["__gshared", "immutable"]),
+		RedundantStorageClassCheck.REDUNDANT_VARIABLE_ATTRIBUTES.format("a", ["__gshared", "static"]),
+	), sac);
+
+	stderr.writeln("Unittest for RedundantStorageClassCheck passed.");
+}

src/dscanner/analysis/run.d

@@ -73,6 +73,7 @@ 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 dscanner.analysis.redundant_storage_class;
 
 import dsymbol.string_interning : internString;
 import dsymbol.scope_;
@@ -516,6 +517,10 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a
 		checks ~= new TrustTooMuchCheck(fileName,
 		analysisConfig.trust_too_much == Check.skipTests && !ut);
 
+	if (moduleName.shouldRun!"redundant_storage_classes"(analysisConfig))
+		checks ~= new RedundantStorageClassCheck(fileName,
+		analysisConfig.redundant_storage_classes == Check.skipTests && !ut);
+
 	version (none)
 		if (moduleName.shouldRun!"redundant_if_check"(analysisConfig))
 			checks ~= new IfStatementCheck(fileName, moduleScope,

src/dscanner/main.d

@@ -104,6 +104,11 @@ else
 		stderr.writeln(e.msg);
 		return 1;
 	}
+	catch (GetOptException e)
+	{
+		stderr.writeln(e.msg);
+		return 1;
+	}
 
 	if (muffin)
 	{
@@ -143,8 +148,8 @@ else
 		return 0;
 	}
 
-    if (!errorFormat.length)
+	if (!errorFormat.length)
-        errorFormat = defaultErrorFormat;
+		errorFormat = defaultErrorFormat;
 
 	const(string[]) absImportPaths = importPaths.map!(a => a.absolutePath()
 			.buildNormalizedPath()).array();
@@ -234,8 +239,8 @@ else
 		string s = configLocation is null ? getConfigurationLocation() : configLocation;
 		if (s.exists())
 			readINIFile(config, s);
-        if (skipTests)
+		if (skipTests)
-            config.enabled2SkipTests;
+			config.enabled2SkipTests;
 		if (report)
 			generateReport(expandArgs(args), config, cache, moduleCache);
 		else
@@ -422,13 +427,13 @@ string getDefaultConfigurationLocation()
 			configDir = buildPath(configDir, "dscanner", CONFIG_FILE_NAME);
 		return configDir;
 	}
-    else version(Windows)
+	else version(Windows)
-    {
+	{
-        string configDir = environment.get("APPDATA", null);
+		string configDir = environment.get("APPDATA", null);
-        enforce(configDir !is null, "%APPDATA% is unset");
+		enforce(configDir !is null, "%APPDATA% is unset");
-        configDir = buildPath(configDir, "dscanner", CONFIG_FILE_NAME);
+		configDir = buildPath(configDir, "dscanner", CONFIG_FILE_NAME);
-        return configDir;
+		return configDir;
-    }
+	}
 }
 
 /**

src/dscanner/utils.d

@@ -3,7 +3,43 @@ module dscanner.utils;
 import std.array : appender, uninitializedArray;
 import std.stdio : stdin, stderr, File;
 import std.conv : to;
-import std.file : exists;
+import std.encoding : BOM, BOMSeq, EncodingException, getBOM;
+import std.format : format;
+import std.file : exists, read;
+
+private void processBOM(ubyte[] sourceCode, string fname)
+{
+	enum spec = "D-Scanner does not support %s-encoded files (%s)";
+	const BOMSeq bs = sourceCode.getBOM;
+	with(BOM) switch (bs.schema)
+	{
+	case none, utf8:
+		break;
+	default:
+		throw new EncodingException(spec.format(bs.schema, fname));
+	}
+	sourceCode = sourceCode[bs.sequence.length .. $];
+}
+
+unittest
+{
+	import std.exception : assertThrown, assertNotThrown;
+	import std.encoding : bomTable;
+	import std.traits : EnumMembers;
+
+	foreach(m ; EnumMembers!BOM)
+	{
+		auto sc = bomTable[m].sequence.dup;
+		if (m != BOM.none && m != BOM.utf8)
+		{
+			assertThrown!(EncodingException)(processBOM(sc, ""));
+		}
+		else
+		{
+			assertNotThrown!(EncodingException)(processBOM(sc, ""));
+		}
+	}
+}
 
 ubyte[] readStdin()
 {
@@ -16,6 +52,7 @@ ubyte[] readStdin()
 			break;
 		sourceCode.put(b);
 	}
+	sourceCode.data.processBOM("stdin");
 	return sourceCode.data;
 }
 
@@ -29,10 +66,9 @@ ubyte[] readFile(string fileName)
 		return [];
 	}
 	File f = File(fileName);
-	if (f.size == 0)
+	ubyte[] sourceCode;
-		return [];
+	sourceCode = cast(ubyte[]) fileName.read();
-	ubyte[] sourceCode = uninitializedArray!(ubyte[])(to!size_t(f.size));
+	sourceCode.processBOM(fileName);
-	f.rawRead(sourceCode);
 	return sourceCode;
 }