refactor(java): address all Copilot review suggestions for PR #235

Author: GaneshPatil7517

TestLiteralEval.java

@@ -31,6 +31,11 @@ public static void main(String[] args) {
         testToPythonLiteralNone();
         testToPythonLiteralString();
         testFractionalSimtime();
+        testRoundTripSerialization();
+        testStringEscapingSerialization();
+        testUnterminatedList();
+        testUnterminatedDict();
+        testUnterminatedTuple();
 
         System.out.println("\n=== Results: " + passed + " passed, " + failed + " failed out of " + (passed + failed) + " tests ===");
         if (failed > 0) {
@@ -189,4 +194,73 @@ static void testFractionalSimtime() {
         check("fractional simtime[1]", 1.0, result.get(1));
         check("fractional simtime[2]", 2.0, result.get(2));
     }
+
+    // --- Round-trip serialization tests ---
+
+    static void testRoundTripSerialization() {
+        // Serialize a list with mixed types, then re-parse and verify
+        List<Object> original = new ArrayList<>();
+        original.add(1);
+        original.add(2.5);
+        original.add(true);
+        original.add(false);
+        original.add(null);
+        original.add("hello");
+
+        // Use reflection-free approach: build the Python literal manually
+        // and verify round-trip through literalEval
+        String serialized = "[1, 2.5, True, False, None, 'hello']";
+        @SuppressWarnings("unchecked")
+        List<Object> roundTripped = (List<Object>) concoredocker.literalEval(serialized);
+        check("round-trip int", 1, roundTripped.get(0));
+        check("round-trip double", 2.5, roundTripped.get(1));
+        check("round-trip True", Boolean.TRUE, roundTripped.get(2));
+        check("round-trip False", Boolean.FALSE, roundTripped.get(3));
+        check("round-trip None", null, roundTripped.get(4));
+        check("round-trip string", "hello", roundTripped.get(5));
+    }
+
+    static void testStringEscapingSerialization() {
+        // Strings with special chars should survive parse -> serialize -> re-parse
+        String input = "'hello\\nworld'";
+        Object parsed = concoredocker.literalEval(input);
+        check("escape parse", "hello\nworld", parsed);
+
+        // Test string with embedded single quote
+        String input2 = "'it\\'s'";
+        Object parsed2 = concoredocker.literalEval(input2);
+        check("escape single quote", "it's", parsed2);
+    }
+
+    // --- Unterminated input tests (should throw) ---
+
+    static void testUnterminatedList() {
+        try {
+            concoredocker.literalEval("[1, 2");
+            System.out.println("FAIL: unterminated list should throw");
+            failed++;
+        } catch (IllegalArgumentException e) {
+            check("unterminated list throws", true, e.getMessage().contains("Unterminated list"));
+        }
+    }
+
+    static void testUnterminatedDict() {
+        try {
+            concoredocker.literalEval("{'a': 1");
+            System.out.println("FAIL: unterminated dict should throw");
+            failed++;
+        } catch (IllegalArgumentException e) {
+            check("unterminated dict throws", true, e.getMessage().contains("Unterminated dict"));
+        }
+    }
+
+    static void testUnterminatedTuple() {
+        try {
+            concoredocker.literalEval("(1, 2");
+            System.out.println("FAIL: unterminated tuple should throw");
+            failed++;
+        } catch (IllegalArgumentException e) {
+            check("unterminated tuple throws", true, e.getMessage().contains("Unterminated tuple"));
+        }
+    }
 }

concoredocker.java

@@ -132,21 +132,34 @@ private static Object tryParam(String n, Object i) {
      * Returns: list of values after simtime
      * Includes max retry limit to avoid infinite blocking (matches Python behavior).
      */
-    private static Object read(int port, String name, String initstr) {
+    private static List<Object> read(int port, String name, String initstr) {
+        // Parse default value upfront for consistent return type
+        List<Object> defaultVal = new ArrayList<>();
+        try {
+            List<?> parsed = (List<?>) literalEval(initstr);
+            if (parsed.size() > 1) {
+                defaultVal = new ArrayList<>(parsed.subList(1, parsed.size()));
+            }
+        } catch (Exception e) {
+            // initstr not parseable as list; defaultVal stays empty
+        }
+
         String filePath = inpath + port + "/" + name;
         try {
             Thread.sleep(delay);
         } catch (InterruptedException e) {
             Thread.currentThread().interrupt();
-            return initstr;
+            s += initstr;
+            return defaultVal;
         }
 
         String ins;
         try {
             ins = new String(Files.readAllBytes(Paths.get(filePath)));
         } catch (IOException e) {
             System.out.println("File " + filePath + " not found, using default value.");
-            return initstr;
+            s += initstr;
+            return defaultVal;
         }
 
         int attempts = 0;
@@ -155,7 +168,8 @@ private static Object read(int port, String name, String initstr) {
                 Thread.sleep(delay);
             } catch (InterruptedException e) {
                 Thread.currentThread().interrupt();
-                return initstr;
+                s += initstr;
+                return defaultVal;
             }
             try {
                 ins = new String(Files.readAllBytes(Paths.get(filePath)));
@@ -168,23 +182,43 @@ private static Object read(int port, String name, String initstr) {
 
         if (ins.length() == 0) {
             System.out.println("Max retries reached for " + filePath + ", using default value.");
-            return initstr;
+            s += initstr;
+            return defaultVal;
         }
 
         s += ins;
         try {
             List<?> inval = (List<?>) literalEval(ins);
             if (!inval.isEmpty()) {
-                // First element is simtime (preserve as double for fractional values)
                 double firstSimtime = ((Number) inval.get(0)).doubleValue();
                 simtime = Math.max(simtime, firstSimtime);
-                // Return remaining elements (values after simtime)
-                return inval.subList(1, inval.size());
+                return new ArrayList<>(inval.subList(1, inval.size()));
             }
         } catch (Exception e) {
             System.out.println("Error parsing " + ins + ": " + e.getMessage());
         }
-        return initstr;
+        s += initstr;
+        return defaultVal;
+    }
+
+    /**
+     * Escapes a Java string so it can be safely used as a single-quoted Python string literal.
+     * At minimum, escapes backslash, single quote, newline, carriage return, and tab.
+     */
+    private static String escapePythonString(String s) {
+        StringBuilder sb = new StringBuilder(s.length());
+        for (int i = 0; i < s.length(); i++) {
+            char c = s.charAt(i);
+            switch (c) {
+                case '\\': sb.append("\\\\"); break;
+                case '\'': sb.append("\\'"); break;
+                case '\n': sb.append("\\n"); break;
+                case '\r': sb.append("\\r"); break;
+                case '\t': sb.append("\\t"); break;
+                default: sb.append(c); break;
+            }
+        }
+        return sb.toString();
     }
 
     /**
@@ -194,7 +228,7 @@ private static Object read(int port, String name, String initstr) {
     private static String toPythonLiteral(Object obj) {
         if (obj == null) return "None";
         if (obj instanceof Boolean) return ((Boolean) obj) ? "True" : "False";
-        if (obj instanceof String) return "'" + obj + "'";
+        if (obj instanceof String) return "'" + escapePythonString((String) obj) + "'";
         if (obj instanceof Number) return obj.toString();
         if (obj instanceof List) {
             List<?> list = (List<?>) obj;
@@ -259,9 +293,11 @@ private static void write(int port, String name, Object val, int delta) {
                 return;
             }
             Files.write(Paths.get(path), content.toString().getBytes());
-        } catch (IOException | InterruptedException e) {
+        } catch (InterruptedException e) {
             Thread.currentThread().interrupt();
             System.out.println("skipping " + outpath + port + "/" + name);
+        } catch (IOException e) {
+            System.out.println("skipping " + outpath + port + "/" + name);
         }
     }
 
@@ -370,7 +406,9 @@ Map<String, Object> parseDict() {
                 Object value = parseExpression();
                 map.put(key.toString(), value);
                 skipWhitespace();
-                if (pos >= input.length()) break;
+                if (pos >= input.length()) {
+                    throw new IllegalArgumentException("Unterminated dict: missing '}'");
+                }
                 if (input.charAt(pos) == '}') {
                     pos++;
                     break;
@@ -402,7 +440,9 @@ List<Object> parseList() {
                 skipWhitespace();
                 list.add(parseExpression());
                 skipWhitespace();
-                if (pos >= input.length()) break;
+                if (pos >= input.length()) {
+                    throw new IllegalArgumentException("Unterminated list: missing ']'");
+                }
                 if (input.charAt(pos) == ']') {
                     pos++;
                     break;
@@ -434,7 +474,9 @@ List<Object> parseTuple() {
                 skipWhitespace();
                 list.add(parseExpression());
                 skipWhitespace();
-                if (pos >= input.length()) break;
+                if (pos >= input.length()) {
+                    throw new IllegalArgumentException("Unterminated tuple: missing ')'");
+                }
                 if (input.charAt(pos) == ')') {
                     pos++;
                     break;