Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improves error emission of switch blocks, adds new pragma #1131

Merged
merged 5 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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")
Original file line number Diff line number Diff line change
@@ -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
12 changes: 7 additions & 5 deletions DMCompiler/Compiler/DM/DMParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,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);
}

Expand Down Expand Up @@ -1341,7 +1341,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();
Expand All @@ -1364,7 +1364,7 @@ public DMASTProcStatementSwitch.SwitchCase[] SwitchInner() {
DMASTExpression? expression = Expression();
if (expression == null) {
if (expressions.Count == 0) {
Error("Expected an expression");
Error(WarningCode.BadExpression, "Expected an expression");
} else {
//Eat a trailing comma if there's at least 1 expression
break;
Expand All @@ -1375,7 +1375,7 @@ public DMASTProcStatementSwitch.SwitchCase[] SwitchInner() {
var loc = Current().Location;
Whitespace();
DMASTExpression? rangeEnd = Expression();
if (rangeEnd == null) Error("Expected an upper limit");
if (rangeEnd == null) Error(WarningCode.BadExpression, "Expected an upper limit");

expressions.Add(new DMASTSwitchCaseRange(loc, expression, rangeEnd));
} else {
Expand Down Expand Up @@ -1405,7 +1405,9 @@ public DMASTProcStatementSwitch.SwitchCase[] SwitchInner() {
var loc = Current().Location;
Whitespace();
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.
Error(WarningCode.SuspiciousSwitchCase, "Expected \"if\" or \"else\" - \"else if\" is ambiguous as a switch case and may cause unintended flow");
}
DMASTProcBlockInner? body = ProcBlock();

Expand Down
6 changes: 6 additions & 0 deletions DMCompiler/Compiler/DM/DMParserHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,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;
}
Expand Down
1 change: 1 addition & 0 deletions DMCompiler/DMStandard/DefaultPragmaConfig.dm
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,4 @@
//3000-3999
#pragma EmptyBlock notice
#pragma EmptyProc disabled // NOTE: If you enable this in OD's default pragma config file, it will emit for OD's DMStandard. Put it in your codebase's pragma config file.
#pragma SuspiciousSwitchCase warning
3 changes: 2 additions & 1 deletion OpenDreamShared/Compiler/CompilerError.cs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ public enum WarningCode {

// 3000 - 3999 are reserved for stylistic configuration.
EmptyBlock = 3100,
EmptyProc = 3101
EmptyProc = 3101,
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!)
}
Expand Down