Improves error emission of switch blocks, adds new pragma

Content.Tests/DMProject/Tests/Statements/Switch/weird_when_clause.dm

@@ -0,0 +1,15 @@
+
+//Issue OD#996, kinda: https://github.com/OpenDreamProject/OpenDream/issues/996
+
+/proc/RunTest()
+	var/x = 5
+	switch(x)
+		if(1)
+			CRASH("Strange branch chosen in switch statement")
+		if(4)
+			CRASH("Strange branch chosen in switch statement")
+		else if(x == 3)
+			CRASH("Parser failed to understand 'else if' in switch block")
+		else
+			return
+	CRASH("Parser failed to understand 'else if' in switch block")

Content.Tests/DMProject/Tests/Statements/Switch/weird_when_clause_pragma.dm

@@ -0,0 +1,16 @@
+//COMPILE ERROR
+//test to make sure SuspiciousSwitchCase is working
+
+#pragma SuspiciousSwitchCase error
+
+/proc/RunTest()
+	var/x = 5
+	switch(x)
+		if(1)
+			return
+		if(4)
+			return
+		else if(x == 3)
+			return
+		else
+			return

DMCompiler/Compiler/DM/DMParser.cs

@@ -8,14 +8,9 @@
 
 namespace DMCompiler.Compiler.DM {
     public partial class DMParser : Parser<Token> {
-
         private DreamPath _currentPath = DreamPath.Root;
-
         private bool _allowVarDeclExpression = false;
 
-        public DMParser(DMLexer lexer) : base(lexer) {
-        }
-
         private static readonly TokenType[] AssignTypes = {
             TokenType.DM_Equals,
             TokenType.DM_PlusEquals,
@@ -149,6 +144,9 @@ public DMParser(DMLexer lexer) : base(lexer) {
             TokenType.DM_XorEquals,
         };
 
+        public DMParser(DMLexer lexer) : base(lexer) {
+        }
+
         public DMASTFile File() {
             var loc = Current().Location;
             List<DMASTStatement> statements = new();
@@ -1301,7 +1299,7 @@ DMASTProcBlockInner GetForBody() {
 
                 DMASTProcStatementSwitch.SwitchCase[]? switchCases = SwitchCases();
 
-                if (switchCases == null) Error("Expected switch cases");
+                if (switchCases == null) Error(WarningCode.BadExpression, "Expected switch cases");
                 return new DMASTProcStatementSwitch(loc, value, switchCases);
             }
 
@@ -1352,7 +1350,7 @@ public DMASTProcStatementSwitch.SwitchCase[] SwitchInner() {
             List<DMASTProcStatementSwitch.SwitchCase> switchCases = new();
             DMASTProcStatementSwitch.SwitchCase? switchCase = SwitchCase();
 
-            while (switchCase != null) {
+            while (switchCase is not null) {
                 switchCases.Add(switchCase);
                 Newline();
                 Whitespace();
@@ -1374,19 +1372,20 @@ public DMASTProcStatementSwitch.SwitchCase[] SwitchInner() {
 
                     DMASTExpression? expression = Expression();
                     if (expression == null) {
-                        if (expressions.Count == 0) {
-                            Error("Expected an expression");
-                        } else {
-                            //Eat a trailing comma if there's at least 1 expression
-                            break;
-                        }
+                        if (expressions.Count == 0)
+                            DMCompiler.Emit(WarningCode.BadExpression, Current().Location, "Expected an expression");
+
+                        break;
                     }
 
                     if (Check(TokenType.DM_To)) {
-                        var loc = Current().Location;
                         Whitespace();
+                        var loc = Current().Location;
                         DMASTExpression? rangeEnd = Expression();
-                        if (rangeEnd == null) Error("Expected an upper limit");
+                        if (rangeEnd == null) {
+                            DMCompiler.Emit(WarningCode.BadExpression, loc, "Expected an upper limit");
+                            rangeEnd = new DMASTConstantNull(loc); // Fallback to null
+                        }
 
                         expressions.Add(new DMASTSwitchCaseRange(loc, expression, rangeEnd));
                     } else {
@@ -1402,22 +1401,25 @@ public DMASTProcStatementSwitch.SwitchCase[] SwitchInner() {
 
                 if (body == null) {
                     DMASTProcStatement? statement = ProcStatement();
-                    var loc = Current().Location;
 
                     if (statement != null) {
-                        body = new DMASTProcBlockInner(loc,statement);
+                        body = new DMASTProcBlockInner(statement.Location, statement);
                     } else {
-                        body = new DMASTProcBlockInner(loc);
+                        body = new DMASTProcBlockInner(Current().Location);
                     }
                 }
 
                 return new DMASTProcStatementSwitch.SwitchCaseValues(expressions.ToArray(), body);
             } else if (Check(TokenType.DM_Else)) {
-                var loc = Current().Location;
                 Whitespace();
+                var loc = Current().Location;
                 if (Current().Type == TokenType.DM_If) {
-                    Error("Expected \"if\" or \"else\", \"else if\" is not permitted as a switch case");
+                    //From now on, all ifs/elseifs/elses are actually part of this if's chain, not the switch's.
+                    //Ambiguous, but that is parity behaviour. Ergo, the following emission.
+                    DMCompiler.Emit(WarningCode.SuspiciousSwitchCase, loc,
+                        "Expected \"if\" or \"else\" - \"else if\" is ambiguous as a switch case and may cause unintended flow");
                 }
+
                 DMASTProcBlockInner? body = ProcBlock();
 
                 if (body == null) {

DMCompiler/Compiler/DM/DMParserHelper.cs

@@ -1,14 +1,12 @@
 using OpenDreamShared.Compiler;
-using DMCompiler.Compiler.DMPreprocessor;
 using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Text;
 using DMCompiler.Bytecode;
 
 namespace DMCompiler.Compiler.DM {
-    public partial class DMParser : Parser<Token> {
-
+    public partial class DMParser {
         /// <summary>
         /// A special override of Error() since, for DMParser, we know we are in a compilation context and can make use of error codes.
         /// </summary>
@@ -23,6 +21,12 @@ protected bool Error(WarningCode code, string message) {
             return level == ErrorLevel.Error;
         }
 
+        /// <inheritdoc cref="Parser{SourceType}.Error(string, bool)"/>
+        [Obsolete("This is not a desirable way for DMParser to emit an error, as errors should emit an error code and not cause unnecessary throws. Use DMParser's overrides of this method, instead.")]
+        new protected void Error(string message, bool throwException = true) {
+            base.Error(message, throwException);
+        }
+
         protected bool PeekDelimiter() {
             return Current().Type == TokenType.Newline || Current().Type == TokenType.DM_Semicolon;
         }

DMCompiler/DMStandard/DefaultPragmaConfig.dm

@@ -27,6 +27,7 @@
 #pragma DanglingVarType warning
 #pragma MissingInterpolatedExpression warning
 #pragma AmbiguousResourcePath warning
+#pragma SuspiciousSwitchCase warning
 
 //3000-3999
 #pragma EmptyBlock notice

OpenDreamShared/Compiler/CompilerError.cs

@@ -57,7 +57,8 @@ public enum WarningCode {
         // 3000 - 3999 are reserved for stylistic configuration.
         EmptyBlock = 3100,
         EmptyProc = 3101,
-        UnsafeClientAccess = 3200
+        UnsafeClientAccess = 3200,
+        SuspiciousSwitchCase = 3201, // "else if" cases are actually valid DM, they just spontaneously end the switch context and begin an if-else ladder within the else case of the switch
 
         // 4000 - 4999 are reserved for runtime configuration. (TODO: Runtime doesn't know about configs yet!)
     }