Logic parser improvements for quote/escape handling + unit tests

core/src/mindustry/logic/LAssembler.java

@@ -56,22 +56,19 @@ public class LAssembler{
         return new LParser(text, privileged).parse();
     }
 
-    /** @return a variable by name.
-     * This may be a constant variable referring to a number or object. */
+    /**
+     * @return a variable by name. This may be a constant variable referring to a number or object.
+     * @param symbol the string literal, numeric literal, or variable name. Leading or trailing spaces are not allowed.
+     * */
     public LVar var(String symbol){
         LVar constVar = Vars.logicVars.get(symbol, privileged);
         if(constVar != null) return constVar;
 
-        symbol = symbol.trim();
-
         //string case
-        if(!symbol.isEmpty() && symbol.charAt(0) == '\"' && symbol.charAt(symbol.length() - 1) == '\"'){
-            return putConst("___" + symbol, symbol.substring(1, symbol.length() - 1).replace("\\n", "\n"));
+        if(symbol.length() > 1 && symbol.charAt(0) == '\"' && symbol.charAt(symbol.length() - 1) == '\"'){
+            return putConst("___" + symbol, symbol.substring(1, symbol.length() - 1));
         }
 
-        //remove spaces for non-strings
-        symbol = symbol.replace(' ', '_');
-
         //use a positive invalid number if number might be negative, else use a negative invalid number
         double value = parseDouble(symbol);
 

core/src/mindustry/logic/LParser.java

@@ -25,6 +25,11 @@ public class LParser{
     LParser(String text, boolean privileged){
         this.privileged = privileged;
         this.chars = text.toCharArray();
+
+        //normalize CRLF and lone-CR line endings to LF in place; avoids extra allocations, and an extra \n is harmless
+        for(int i = 0; i < chars.length; i++){
+            if(chars[i] == '\r') chars[i] = '\n';
+        }
     }
 
     void comment(){
@@ -39,15 +44,44 @@ public class LParser{
     String string(){
         int from = pos;
         int utflen = 0;
+        //only allocated if an escape sequence is found
+        StringBuilder escaped = null;
 
         while(++pos < chars.length){
             char c = chars[pos];
+
+            //handle \n and \" escape sequences (also \\ so a literal backslash can be written)
+            if(c == '\\' && pos + 1 < chars.length){
+                char translated = switch(chars[pos + 1]){
+                    case 'n' -> '\n';
+                    case '"' -> '"';
+                    case '\\' -> '\\';
+                    default -> 0;
+                };
+
+                if(translated != 0){
+                    if(escaped == null){
+                        //copy everything read so far (without the surrounding quote)
+                        escaped = new StringBuilder();
+                        escaped.append(chars, from + 1, pos - from - 1);
+                    }
+
+                    escaped.append(translated);
+                    utflen += translated <= 0x7F ? 1 : translated <= 0x7FF ? 2 : 3;
+                    pos ++; //consume the escaped character too
+                    continue;
+                }
+                //not a recognized escape sequence; fall through and treat the backslash as a literal character (TODO: isn't this an invalid escape?)
+            }
+
             if(c == '\n'){
                 error("Missing closing quote \" before end of line.");
             }else if(c == '"'){
                 break;
             }
 
+            if(escaped != null) escaped.append(c);
+
             // See ByteBufferOutput.writeUTF()
             utflen += c != 0 && c <= 0x7F ? 1 : c <= 0x7FF ? 2 : 3;
         }
@@ -55,7 +89,13 @@ public class LParser{
         if(pos >= chars.length || chars[pos] != '"') error("Missing closing quote \" before end of file.");
         if(utflen > 65535) error("String value too long.");
 
-        return new String(chars, from, ++pos - from);
+        pos ++; //move past the closing quote
+
+        if(escaped != null){
+            return "\"" + escaped + "\"";
+        }
+
+        return new String(chars, from, pos - from);
     }
 
     String token(){
@@ -63,7 +103,7 @@ public class LParser{
 
         while(pos < chars.length){
             char c = chars[pos];
-            if(c == '\n' || c == ' ' || c == '#' || c == '\t' || c == ';') break;
+            if(c == '\n' || c == ' ' || c == '#' || c == '\t' || c == ';' || c == '"') break;
             pos ++;
         }
 
@@ -191,7 +231,6 @@ public class LParser{
         while(pos < chars.length && line < LExecutor.maxInstructions){
             switch(chars[pos]){
                 case '\n', ';', ' ' -> pos ++; //skip newlines and spaces
-                case '\r' -> pos += 2; //skip the newline after the \r
                 default -> statement();
             }
         }
@@ -217,4 +256,4 @@ public class LParser{
         }
     }
 
-}
+}

core/src/mindustry/logic/LStatement.java

@@ -83,7 +83,8 @@ public abstract class LStatement{
         tooltip(label, text);
     }
 
-    protected String sanitize(String value){
+    /** Sanitizes variable input strings from text fields into values that will not break logic parsing. */
+    public static String sanitize(String value){
         if(value.length() == 0){
             return "";
         }else if(value.length() == 1){
@@ -94,12 +95,14 @@ public abstract class LStatement{
             StringBuilder res = new StringBuilder(value.length());
             if(value.charAt(0) == '"' && value.charAt(value.length() - 1) == '"'){
                 res.append('\"');
-                //strip out extra quotes
+                //escape characters that would otherwise break out of the string or corrupt it
                 for(int i = 1; i < value.length() - 1; i++){
-                    if(value.charAt(i) == '"'){
-                        res.append('\'');
-                    }else{
-                        res.append(value.charAt(i));
+                    char c = value.charAt(i);
+                    switch(c){
+                        case '"' -> res.append("\\\"");
+                        case '\\' -> res.append("\\\\");
+                        case '\n' -> res.append("\\n");
+                        default -> res.append(c);
                     }
                 }
                 res.append('\"');

core/src/mindustry/logic/LogicDialog.java

@@ -138,7 +138,7 @@ public class LogicDialog extends BaseDialog{
                     t.button("@load.clipboard", Icon.download, style, () -> {
                         dialog.hide();
                         try{
-                            canvas.load(Core.app.getClipboardText().replace("\r\n", "\n"));
+                            canvas.load(Core.app.getClipboardText());
                         }catch(Throwable e){
                             ui.showException(e);
                         }

tests/src/test/java/ApplicationTests.java

@@ -38,7 +38,6 @@ import static org.junit.jupiter.api.DynamicTest.*;
 public class ApplicationTests{
     static Map testMap;
     static boolean initialized;
-    //core/assets
     static final Fi testDataFolder = new Fi("../../tests/build/test_data");
 
     @BeforeAll

tests/src/test/java/LogicTests.java

@@ -0,0 +1,243 @@
+import mindustry.logic.*;
+import mindustry.logic.LExecutor.*;
+import org.junit.jupiter.api.*;
+import org.junit.jupiter.params.*;
+import org.junit.jupiter.params.provider.*;
+
+import java.util.stream.*;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+public class LogicTests{
+
+    @BeforeAll
+    static void init(){
+        ApplicationTests.launchApplication();
+    }
+
+    /** Assembles and loads a small mlog program, mirroring the LogicBlock usage pattern. */
+    static LExecutor load(String code){
+        LExecutor exec = new LExecutor();
+        exec.privileged = true;
+        exec.load(LAssembler.assemble(code, true));
+        return exec;
+    }
+
+    /** Runs a single `set result <value>` line and returns the decoded value assigned to `from`. */
+    static Object setFromValue(String code){
+        LExecutor exec = load(code);
+        assertTrue(exec.instructions.length > 0, "expected at least one instruction to be parsed from: " + code);
+        assertTrue(exec.instructions[0] instanceof SetI, "expected a set instruction from: " + code);
+        return ((SetI)exec.instructions[0]).from.objval;
+    }
+
+    @ParameterizedTest(name = "{0}")
+    @MethodSource("stringEscapeCases")
+    void parsesStringEscapes(String name, String code, String expected){
+        assertEquals(expected, setFromValue(code));
+    }
+
+    static Stream<Arguments> stringEscapeCases(){
+        return Stream.of(
+        Arguments.of(
+        "plain string has no escapes",
+        "set result \"asdf\"",
+        "asdf"
+        ),
+        Arguments.of(
+        "\\n decodes to a real newline character",
+        "set result \"line1\\nline2\"",
+        "line1\nline2"
+        ),
+        Arguments.of(
+        "\\\" decodes to a literal quote without ending the string",
+        "set result \"the entity said \\\"hi\\\"\"",
+        "the entity said \"hi\""
+        ),
+        Arguments.of(
+        "\\\\ decodes to a single literal backslash",
+        "set result \"a\\\\b\"",
+        "a\\b"
+        ),
+        Arguments.of(
+        "an escaped backslash immediately followed by a literal n must NOT collapse into a newline",
+        "set result \"a\\\\nb\"",
+        "a\\nb"
+        ),
+        Arguments.of(
+        "unrecognized escape sequences pass through as a literal backslash + character",
+        "set result \"tab\\ttab\"",
+        "tab\\ttab"
+        ),
+        Arguments.of(
+        "\\n, \\\" and \\\\ combined in a single string",
+        "set result \"start\\nmid\\\"quoted\\\"\\\\end\"",
+        "start\nmid\"quoted\"\\end"
+        ),
+        Arguments.of(
+        "empty quoted string",
+        "set result \"\"",
+        ""
+        ),
+        Arguments.of(
+        "escaped backslash sitting right before the closing quote",
+        "set result \"end\\\\\"",
+        "end\\"
+        )
+        );
+    }
+
+    @Test
+    void plainNumberIsNotTreatedAsAString(){
+        LExecutor exec = load("set result 5");
+        SetI set = (SetI)exec.instructions[0];
+        assertFalse(set.from.isobj, "a bare number should not be stored as an object value");
+        assertEquals(5.0, set.from.numval);
+    }
+
+    @ParameterizedTest(name = "{0}")
+    @MethodSource("sanitizeCases")
+    void sanitizesInput(String name, String input, String expected){
+        assertEquals(expected, LStatement.sanitize(input));
+    }
+
+    static Stream<Arguments> sanitizeCases(){
+        return Stream.of(
+        Arguments.of("a bare single quote character is invalid", "\"", "invalid"),
+        Arguments.of("a bare single semicolon is invalid", ";", "invalid"),
+        Arguments.of("a bare single space is invalid", " ", "invalid"),
+        Arguments.of("a single ordinary character passes through unchanged", "a", "a"),
+        Arguments.of("empty input stays empty", "", ""),
+        Arguments.of("a plain already-quoted value is untouched", "\"hello\"", "\"hello\""),
+        Arguments.of("a unescaped quote at the end gets doubled", "\"hello\\\"", "\"hello\\\\\""),
+        Arguments.of(
+        "a bare, unescaped quote embedded inside a quoted value gets escaped",
+        "\"a\"b\"",
+        "\"a\\\"b\""
+        ),
+        Arguments.of(
+        "a literal backslash inside a quoted value gets escaped so it can't form an accidental escape",
+        "\"C:\\Users\"",
+        "\"C:\\\\Users\""
+        ),
+        Arguments.of(
+        "a real embedded newline inside a quoted value is turned into the \\n escape sequence",
+        "\"line1\nline2\"",
+        "\"line1\\nline2\""
+        ),
+        Arguments.of(
+        "bare quote + literal backslash + real newline combined inside one quoted value",
+        "\"a\\b\"c\nd\"",
+        "\"a\\\\b\\\"c\\nd\""
+        ),
+        Arguments.of(
+        "unquoted values get semicolons/spaces/quotes replaced instead of escaped",
+        "hello world;test\"quote",
+        "hello_worldstest'quote"
+        )
+        );
+    }
+
+    @ParameterizedTest(name = "{0}")
+    @MethodSource("sanitizeRoundTripCases")
+    void sanitizedQuotedValuesRoundTripThroughTheParser(String name, String userInput, String expectedDecoded){
+        String sanitized = LStatement.sanitize(userInput);
+        //sanity check: sanitize should have kept this as a quoted string value
+        assertTrue(sanitized.length() >= 2 && sanitized.charAt(0) == '"' && sanitized.charAt(sanitized.length() - 1) == '"', "expected a quoted value, got: " + sanitized);
+
+        Object decoded = setFromValue("set result " + sanitized);
+        assertEquals(expectedDecoded, decoded);
+    }
+
+    static Stream<Arguments> sanitizeRoundTripCases(){
+        return Stream.of(
+        Arguments.of("plain quoted value", "\"hello\"", "hello"),
+        Arguments.of("bare embedded quote round-trips to a literal quote", "\"a\"b\"", "a\"b"),
+        Arguments.of("literal backslash round-trips to a single backslash", "\"C:\\Users\"", "C:\\Users"),
+        Arguments.of("real embedded newline round-trips to a real newline", "\"line1\nline2\"", "line1\nline2"),
+        Arguments.of(
+        "bare quote + backslash + newline combined round-trip correctly together",
+        "\"a\\b\"c\nd\"",
+        "a\\b\"c\nd"
+        )
+        );
+    }
+
+    //unterminated / malformed string literals: these must fail loudly, never silently misparse
+
+    @ParameterizedTest(name = "{0}")
+    @MethodSource("unterminatedStringCases")
+    void unterminatedStringsThrow(String name, String code){
+        assertThrows(RuntimeException.class, () -> load(code), "expected a parse error from: " + code);
+    }
+
+    static Stream<Arguments> unterminatedStringCases(){
+        return Stream.of(
+        Arguments.of(
+        "a trailing escaped quote is content, not a terminator, so the string is left unterminated",
+        "set result \"asd\\\""
+        ),
+        Arguments.of(
+        "a dangling backslash at the absolute end of input has nothing to escape",
+        "set result \"asd\\"
+        ),
+        Arguments.of(
+        "an escaped-quote-then-semicolon does not let the semicolon act as a statement separator inside the string",
+        "set result \"asd\\\"; set bar 5"
+        ),
+        Arguments.of(
+        "a string missing its closing quote entirely",
+        "set result \"asd"
+        ),
+        Arguments.of(
+        "a string that never closes before the line ends, with trailing content after it",
+        "set result \"asd\nset bar 5"
+        )
+        );
+    }
+
+    @Test
+    void varWithProperlyQuotedEmptyString(){
+        LAssembler asm = new LAssembler();
+        LVar v = asm.var("\"\"");
+        assertTrue(v.isobj);
+        assertEquals("", v.objval);
+    }
+
+    @Test
+    void quoteInVariableNameThrows(){
+        //quotes mid-variable should not be allowed
+        assertThrows(RuntimeException.class, () -> load("set result abc\"def"));
+    }
+
+    @Test
+    void quoteAtEndOfVariableNameThrows(){
+        //ditto
+        assertThrows(RuntimeException.class, () -> load("set result abcdef\""));
+    }
+
+    @Test
+    void crlfAfterUnquotedTokenDoesNotCorruptTheToken(){
+        LExecutor exec = load("set a bar\r\nset b bar\n");
+        SetI first = (SetI)exec.instructions[0];
+        SetI second = (SetI)exec.instructions[1];
+        assertSame(first.from, second.from, "a CRLF-terminated reference to 'bar' must resolve to the same variable as an LF-terminated one");
+    }
+
+    @Test
+    void crlfAfterQuotedStringParsesCleanly(){
+        assertEquals("bar", setFromValue("set result \"bar\"\r\n"));
+    }
+
+    @Test
+    void loneCarriageReturnActsAsALineEnding(){
+        //old-style Mac ('\r'-only) line endings are normalized the same way as CRLF.
+        LExecutor exec = load("set result 1\rset result2 2\r");
+        assertEquals(2, exec.instructions.length, "expected two separate statements, split on the lone '\\r'");
+    }
+
+    @Test
+    void crlfLabelsResolveToTheSameJumpLocationAsLfLabels(){
+        assertDoesNotThrow(() -> load("loop:\r\njump loop always\r\n"));
+    }
+}