From d2753611530029ed7efb9df6c526c620c7f0602b Mon Sep 17 00:00:00 2001 From: WebFreak001 Date: Sun, 9 Jul 2023 15:02:58 +0200 Subject: [PATCH] fix case/default scopes, fix #913 --- src/dscanner/analysis/base.d | 305 ++++++++++++++++++ .../analysis/label_var_same_name_check.d | 41 ++- src/dscanner/analysis/redundant_attributes.d | 25 +- 3 files changed, 328 insertions(+), 43 deletions(-) diff --git a/src/dscanner/analysis/base.d b/src/dscanner/analysis/base.d index dde778e..300ce05 100644 --- a/src/dscanner/analysis/base.d +++ b/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; + }); +} diff --git a/src/dscanner/analysis/label_var_same_name_check.d b/src/dscanner/analysis/label_var_same_name_check.d index 4402f32..542252a 100644 --- a/src/dscanner/analysis/label_var_same_name_check.d +++ b/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."); } diff --git a/src/dscanner/analysis/redundant_attributes.d b/src/dscanner/analysis/redundant_attributes.d index 9929690..f39a76b 100644 --- a/src/dscanner/analysis/redundant_attributes.d +++ b/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--; }