fix case/default scopes, fix #913

src/dscanner/analysis/base.d

@@ -5,6 +5,7 @@ import dparse.lexer : IdType, str, Token;
 import dsymbol.scope_ : Scope;
 import std.array;
 import std.container;
+import std.meta : AliasSeq;
 import std.string;
 import std.sumtype;
 
@@ -498,3 +499,307 @@ const(Token)[] findTokenForDisplay(const Token[] tokens, IdType type, const(Toke
 			return tokens[i .. i + 1];
 	return fallback is null ? tokens : fallback;
 }
+
+abstract class ScopedBaseAnalyzer : BaseAnalyzer
+{
+public:
+	this(string fileName, const Scope* sc, bool skipTests = false)
+	{
+		super(fileName, sc, skipTests);
+	}
+
+
+	template ScopedVisit(NodeType)
+	{
+		override void visit(const NodeType n)
+		{
+			pushScopeImpl();
+			scope (exit)
+				popScopeImpl();
+			n.accept(this);
+		}
+	}
+
+	alias visit = BaseAnalyzer.visit;
+
+	mixin ScopedVisit!BlockStatement;
+	mixin ScopedVisit!ForeachStatement;
+	mixin ScopedVisit!ForStatement;
+	mixin ScopedVisit!IfStatement;
+	mixin ScopedVisit!Module;
+	mixin ScopedVisit!StructBody;
+	mixin ScopedVisit!TemplateDeclaration;
+	mixin ScopedVisit!WithStatement;
+	mixin ScopedVisit!WhileStatement;
+
+	override void visit(const SwitchStatement switchStatement)
+	{
+		switchStack.length++;
+		scope (exit)
+			switchStack.length--;
+		switchStatement.accept(this);
+	}
+
+	static foreach (T; AliasSeq!(CaseStatement, DefaultStatement, CaseRangeStatement))
+		override void visit(const T stmt)
+		{
+			// case and default statements always open new scopes and close
+			// previous case scopes
+			bool close = switchStack.length && switchStack[$ - 1].inCase;
+			bool b = switchStack[$ - 1].inCase;
+			switchStack[$ - 1].inCase = true;
+			scope (exit)
+				switchStack[$ - 1].inCase = b;
+			if (close)
+			{
+				popScope();
+				pushScope();
+				stmt.accept(this);
+			}
+			else
+			{
+				pushScope();
+				stmt.accept(this);
+				popScope();
+			}
+		}
+
+protected:
+	/// Called on new scopes, which includes for example:
+	///
+	/// - `module m; /* here, entire file */`
+	/// - `{ /* here */ }`
+	/// - `if () { /* here */ } else { /* here */ }`
+	/// - `foreach (...) { /* here */ }`
+	/// - `case 1: /* here */ break;`
+	/// - `case 1: /* here, up to next case */ goto case; case 2: /* here 2 */ break;`
+	/// - `default: /* here */ break;`
+	/// - `struct S { /* here */ }`
+	///
+	/// But doesn't include:
+	///
+	/// - `static if (x) { /* not a separate scope */ }` (use `mixin ScopedVisit!ConditionalDeclaration;`)
+	///
+	/// You can `mixin ScopedVisit!NodeType` to automatically call push/popScope
+	/// on occurences of that NodeType.
+	abstract void pushScope();
+	/// ditto
+	abstract void popScope();
+
+	void pushScopeImpl()
+	{
+		if (switchStack.length)
+			switchStack[$ - 1].scopeDepth++;
+		pushScope();
+	}
+
+	void popScopeImpl()
+	{
+		if (switchStack.length)
+			switchStack[$ - 1].scopeDepth--;
+		popScope();
+	}
+
+	struct SwitchStack
+	{
+		int scopeDepth;
+		bool inCase;
+	}
+
+	SwitchStack[] switchStack;
+}
+
+unittest
+{
+	import core.exception : AssertError;
+	import dparse.lexer : getTokensForParser, LexerConfig, StringCache;
+	import dparse.parser : parseModule;
+	import dparse.rollback_allocator : RollbackAllocator;
+	import std.conv : to;
+	import std.exception : assertThrown;
+
+	// test where we can:
+	// call `depth(1);` to check that the scope depth is at 1
+	// if calls are syntactically not valid, define `auto depth = 1;`
+	//
+	// call `isNewScope();` to check that the scope hasn't been checked with isNewScope before
+	// if calls are syntactically not valid, define `auto isNewScope = void;`
+	//
+	// call `isOldScope();` to check that the scope has already been checked with isNewScope
+	// if calls are syntactically not valid, define `auto isOldScope = void;`
+
+	class TestScopedAnalyzer : ScopedBaseAnalyzer
+	{
+		this(size_t codeLine)
+		{
+			super("stdin", null, false);
+
+			this.codeLine = codeLine;
+		}
+
+		override void visit(const FunctionCallExpression f)
+		{
+			int depth = cast(int) stack.length;
+			if (f.unaryExpression && f.unaryExpression.primaryExpression
+				&& f.unaryExpression.primaryExpression.identifierOrTemplateInstance)
+			{
+				auto fname = f.unaryExpression.primaryExpression.identifierOrTemplateInstance.identifier.text;
+				if (fname == "depth")
+				{
+					assert(f.arguments.tokens.length == 3);
+					auto expected = f.arguments.tokens[1].text.to!int;
+					assert(expected == depth, "Expected depth="
+						~ expected.to!string ~ " in line " ~ (codeLine + f.tokens[0].line).to!string
+						~ ", but got depth=" ~ depth.to!string);
+				}
+				else if (fname == "isNewScope")
+				{
+					assert(!stack[$ - 1]);
+					stack[$ - 1] = true;
+				}
+				else if (fname == "isOldScope")
+				{
+					assert(stack[$ - 1]);
+				}
+			}
+		}
+
+		override void visit(const AutoDeclarationPart p)
+		{
+			int depth = cast(int) stack.length;
+
+			if (p.identifier.text == "depth")
+			{
+				assert(p.initializer.tokens.length == 1);
+				auto expected = p.initializer.tokens[0].text.to!int;
+				assert(expected == depth, "Expected depth="
+					~ expected.to!string ~ " in line " ~ (codeLine + p.tokens[0].line).to!string
+					~ ", but got depth=" ~ depth.to!string);
+			}
+			else if (p.identifier.text == "isNewScope")
+			{
+				assert(!stack[$ - 1]);
+				stack[$ - 1] = true;
+			}
+			else if (p.identifier.text == "isOldScope")
+			{
+				assert(stack[$ - 1]);
+			}
+		}
+
+		override void pushScope()
+		{
+			stack.length++;
+		}
+
+		override void popScope()
+		{
+			stack.length--;
+		}
+
+		alias visit = ScopedBaseAnalyzer.visit;
+
+		bool[] stack;
+		size_t codeLine;
+	}
+
+	void testScopes(string code, size_t codeLine = __LINE__ - 1)
+	{
+		StringCache cache = StringCache(4096);
+		LexerConfig config;
+		RollbackAllocator rba;
+		auto tokens = getTokensForParser(code, config, &cache);
+		Module m = parseModule(tokens, "stdin", &rba);
+
+		auto analyzer = new TestScopedAnalyzer(codeLine);
+		analyzer.visit(m);
+	}
+
+	testScopes(q{
+		auto isNewScope = void;
+		auto depth = 1;
+		auto isOldScope = void;
+	});
+
+	assertThrown!AssertError(testScopes(q{
+		auto isNewScope = void;
+		auto isNewScope = void;
+	}));
+
+	assertThrown!AssertError(testScopes(q{
+		auto isOldScope = void;
+	}));
+
+	assertThrown!AssertError(testScopes(q{
+		auto depth = 2;
+	}));
+
+	testScopes(q{
+		auto isNewScope = void;
+		auto depth = 1;
+		
+		void foo() {
+			isNewScope();
+			isOldScope();
+			depth(2);
+			switch (a)
+			{
+			case 1:
+				isNewScope();
+				depth(4);
+				break;
+				depth(4);
+				isOldScope();
+			case 2:
+				isNewScope();
+				depth(4);
+				if (a)
+				{
+					isNewScope();
+					depth(6);
+				default:
+					isNewScope();
+					depth(6); // since cases/default opens new scope
+					break;
+				case 3:
+					isNewScope();
+					depth(6); // since cases/default opens new scope
+					break;
+				default:
+					isNewScope();
+					depth(6); // since cases/default opens new scope
+					break;
+				}
+				break;
+				depth(4);
+			default:
+				isNewScope();
+				depth(4);
+				break;
+				depth(4);
+			}
+
+			isOldScope();
+			depth(2);
+
+			switch (a)
+			{
+				isNewScope();
+				depth(3);
+				isOldScope();
+			default:
+				isNewScope();
+				depth(4);
+				break;
+				isOldScope();
+			case 1:
+				isNewScope();
+				depth(4);
+				break;
+				isOldScope();
+			}
+		}
+
+		auto isOldScope = void;
+	});
+}

src/dscanner/analysis/label_var_same_name_check.d

@@ -13,7 +13,7 @@ import dscanner.analysis.helpers;
 /**
  * Checks for labels and variables that have the same name.
  */
-final class LabelVarNameCheck : BaseAnalyzer
+final class LabelVarNameCheck : ScopedBaseAnalyzer
 {
 	mixin AnalyzerInfo!"label_var_same_name_check";
 
@@ -22,14 +22,6 @@ final class LabelVarNameCheck : BaseAnalyzer
 		super(fileName, sc, skipTests);
 	}
 
-	mixin ScopedVisit!Module;
-	mixin ScopedVisit!BlockStatement;
-	mixin ScopedVisit!StructBody;
-	mixin ScopedVisit!CaseStatement;
-	mixin ScopedVisit!ForStatement;
-	mixin ScopedVisit!IfStatement;
-	mixin ScopedVisit!TemplateDeclaration;
-
 	mixin AggregateVisit!ClassDeclaration;
 	mixin AggregateVisit!StructDeclaration;
 	mixin AggregateVisit!InterfaceDeclaration;
@@ -64,7 +56,7 @@ final class LabelVarNameCheck : BaseAnalyzer
 		--conditionalDepth;
 	}
 
-	alias visit = BaseAnalyzer.visit;
+	alias visit = ScopedBaseAnalyzer.visit;
 
 private:
 
@@ -80,16 +72,6 @@ private:
 		}
 	}
 
-	template ScopedVisit(NodeType)
-	{
-		override void visit(const NodeType n)
-		{
-			pushScope();
-			n.accept(this);
-			popScope();
-		}
-	}
-
 	void duplicateCheck(const Token name, bool fromLabel, bool isConditional)
 	{
 		import std.conv : to;
@@ -128,12 +110,12 @@ private:
 		return stack[$ - 1];
 	}
 
-	void pushScope()
+	protected override void pushScope()
 	{
 		stack.length++;
 	}
 
-	void popScope()
+	protected override void popScope()
 	{
 		stack.length--;
 	}
@@ -278,6 +260,21 @@ unittest
 	struct a { int a; }
 }
 
+unittest
+{
+	switch (1) {
+	case 1:
+		int x, c1;
+		break;
+	case 2:
+		int x, c2;
+		break;
+	default:
+		int x, def;
+		break;
+	}
+}
+
 }c, sac);
 	stderr.writeln("Unittest for LabelVarNameCheck passed.");
 }

src/dscanner/analysis/redundant_attributes.d

@@ -17,7 +17,7 @@ import std.range : empty, front, walkLength;
 /**
  * Checks for redundant attributes. At the moment only visibility attributes.
  */
-final class RedundantAttributesCheck : BaseAnalyzer
+final class RedundantAttributesCheck : ScopedBaseAnalyzer
 {
 	mixin AnalyzerInfo!"redundant_attributes_check";
 
@@ -67,15 +67,8 @@ final class RedundantAttributesCheck : BaseAnalyzer
 		}
 	}
 
-	alias visit = BaseAnalyzer.visit;
+	alias visit = ScopedBaseAnalyzer.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:
@@ -153,22 +146,12 @@ private:
 		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()
+	protected override void pushScope()
 	{
 		stack.length++;
 	}
 
-	void popScope()
+	protected override void popScope()
 	{
 		stack.length--;
 	}