pxf: formatDocument keeps every directive in document order (#188)

CHANGELOG.md

@@ -58,6 +58,7 @@ format changes.
 
 ### Fixed
 
+- **`Format.formatDocument` keeps every directive in document order** (#188; trendvidia/protowire#558). It used to drop `@proto`, and wrote every named directive before every dataset, so `@dataset t.v1.T (id)` / `(1)` followed by `@entry x { a = 1 }` came out with the `@entry` first. That order matters: an anonymous `@proto` binds the typed directive after it. `@type` is still written first, since it binds no `@proto`. The other directives follow in source order, each after its comments, with no blank line between them; a dataset with no type is written `@dataset (id)`. A document whose only directive is `@type` formats exactly as before. The output is byte for byte protowire-go's `FormatDocument` (protowire-go#262).
 - **`Convert.xmlToProto` keeps the length of a `uint8` or `int8` array** (#179). `<type primitiveType="uint8" length="16"/>` imported as one `uint32` with `(sbe.encoding) = "uint8"`, so a 16-byte field was read as 1 byte and every later field in the block was misplaced. It now imports as `bytes` with `(sbe.length) = 16`, the same 16 bytes on the wire.
 - **The SBE decoder rejects a string field that is not UTF-8** (#162; `docs/HARDENING.md` § UTF-8). Both tiers turned the bytes of an `(sbe.length)` `string` into a `String` with `new String(bytes, UTF_8)`, which puts U+FFFD in place of bad bytes and says nothing, so `41 42 FF FE` decoded as `"AB\uFFFD\uFFFD"`. `SbeWireCodec` and `View.stringValue` now decode strictly and throw `IllegalArgumentException` (`sbe: invalid UTF-8 in string field <name>`). A `bytes` field takes any bytes, as before. `check-decode-android` decodes `adversarial.v1.SbeText`, so the lite tier runs the spec's `sbe/utf8-*` rows.
 

pxf-runtime/src/main/java/org/protowire/pxf/Format.java

@@ -6,33 +6,94 @@
 import java.math.MathContext;
 import java.math.RoundingMode;
 import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
 import java.util.Base64;
+import java.util.Comparator;
 import java.util.List;
 import java.util.function.Predicate;
 
 /** Comment-preserving formatter that turns an {@link Ast.Document} back into PXF text. */
 public final class Format {
     private Format() {}
 
+    /**
+     * Pretty-prints a document, preserving comments.
+     *
+     * <p>It keeps every directive, with its dataset rows, in document order
+     * (trendvidia/protowire#558, #188). {@code @type} is written first: it
+     * binds no anonymous {@code @proto}, so moving it changes nothing. The
+     * other directives follow in source order, each after its comments: a
+     * named directive's body and a {@code @proto} body as written, and a
+     * dataset as its header and then one row per line, an absent cell left
+     * empty. A document whose only directive is {@code @type} formats as it
+     * always has. The output is byte for byte protowire-go's
+     * {@code FormatDocument}.
+     */
     public static String formatDocument(Ast.Document doc) {
         StringBuilder sb = new StringBuilder();
+        List<Object> others = topDirectives(doc);
+        if (others.isEmpty()) {
+            if (!doc.typeUrl().isEmpty()) {
+                sb.append("@type ").append(doc.typeUrl()).append("\n\n");
+            }
+            writeComments(sb, doc.leadingComments(), 0);
+            formatEntries(sb, doc.entries(), 0);
+            return sb.toString();
+        }
+
         if (!doc.typeUrl().isEmpty()) {
-            sb.append("@type ").append(doc.typeUrl()).append("\n\n");
+            sb.append("@type ").append(doc.typeUrl()).append('\n');
         }
-        for (Ast.Directive d : doc.directives()) {
-            formatDirective(sb, d);
-            sb.append('\n');
+        writeComments(sb, doc.leadingComments(), 0);
+        for (Object o : others) {
+            switch (o) {
+                case Ast.Directive d -> {
+                    writeComments(sb, d.leadingComments(), 0);
+                    formatNamedDirective(sb, d);
+                }
+                case Ast.DatasetDirective d -> {
+                    writeComments(sb, d.leadingComments(), 0);
+                    formatDataset(sb, d);
+                }
+                case Ast.ProtoDirective d -> {
+                    writeComments(sb, d.leadingComments(), 0);
+                    formatProto(sb, d);
+                }
+                default -> throw new IllegalStateException(
+                        "Format: unexpected directive type " + o.getClass().getSimpleName());
+            }
         }
-        for (Ast.DatasetDirective t : doc.datasets()) {
-            formatTableDirective(sb, t);
+        if (!doc.entries().isEmpty()) {
             sb.append('\n');
+            formatEntries(sb, doc.entries(), 0);
         }
-        writeComments(sb, doc.leadingComments(), 0);
-        formatEntries(sb, doc.entries(), 0);
         return sb.toString();
     }
 
-    private static void formatDirective(StringBuilder sb, Ast.Directive d) {
+    /**
+     * The document's directives other than {@code @type}, which the
+     * {@link Ast.Document} keeps in three lists, merged back into source
+     * order: an anonymous {@code @proto} binds the typed directive after it,
+     * so the order is part of the meaning. Each element is an
+     * {@link Ast.Directive}, {@link Ast.DatasetDirective} or
+     * {@link Ast.ProtoDirective}.
+     */
+    private static List<Object> topDirectives(Ast.Document doc) {
+        record At(int offset, Object d) {}
+        List<At> all = new ArrayList<>();
+        for (Ast.Directive d : doc.directives()) all.add(new At(d.pos().offset(), d));
+        for (Ast.DatasetDirective d : doc.datasets()) all.add(new At(d.pos().offset(), d));
+        for (Ast.ProtoDirective d : doc.protos()) all.add(new At(d.pos().offset(), d));
+        // List.sort is stable, so directives at one offset (built in code,
+        // with no position) keep the order of the lists above.
+        all.sort(Comparator.comparingInt(At::offset));
+        List<Object> out = new ArrayList<>(all.size());
+        for (At a : all) out.add(a.d());
+        return out;
+    }
+
+    /** Writes {@code @<name> prefix* [{body}]}, the body as it was written. */
+    private static void formatNamedDirective(StringBuilder sb, Ast.Directive d) {
         sb.append('@').append(d.name());
         for (String p : d.prefixes()) {
             sb.append(' ').append(p);
@@ -43,25 +104,35 @@ private static void formatDirective(StringBuilder sb, Ast.Directive d) {
         sb.append('\n');
     }
 
-    private static void formatTableDirective(StringBuilder sb, Ast.DatasetDirective t) {
-        sb.append("@dataset ").append(t.type()).append(" (");
-        for (int i = 0; i < t.columns().size(); i++) {
-            if (i > 0) sb.append(", ");
-            sb.append(t.columns().get(i));
-        }
-        sb.append(")\n");
+    /** Writes the header, then one row per line. A null cell is an absent field, left empty; {@code null} is written. */
+    private static void formatDataset(StringBuilder sb, Ast.DatasetDirective t) {
+        sb.append("@dataset");
+        if (!t.type().isEmpty()) sb.append(' ').append(t.type());
+        sb.append(" (").append(String.join(", ", t.columns())).append(")\n");
         for (Ast.DatasetRow row : t.rows()) {
             sb.append('(');
             for (int i = 0; i < row.cells().size(); i++) {
                 if (i > 0) sb.append(", ");
                 Ast.Value cell = row.cells().get(i);
                 if (cell != null) formatValue(sb, cell, 0);
-                // null cell ⇒ empty between commas, leave it blank.
             }
             sb.append(")\n");
         }
     }
 
+    /** Writes a {@code @proto} directive in the shape it was read in. */
+    private static void formatProto(StringBuilder sb, Ast.ProtoDirective d) {
+        sb.append("@proto");
+        switch (d.shape()) {
+            case ANONYMOUS -> sb.append(" {").append(new String(d.body(), StandardCharsets.UTF_8)).append('}');
+            case NAMED -> sb.append(' ').append(d.typeName())
+                    .append(" {").append(new String(d.body(), StandardCharsets.UTF_8)).append('}');
+            case SOURCE -> sb.append(" \"\"\"").append(new String(d.body(), StandardCharsets.UTF_8)).append("\"\"\"");
+            case DESCRIPTOR -> sb.append(" b\"").append(Base64.getEncoder().encodeToString(d.body())).append('"');
+        }
+        sb.append('\n');
+    }
+
     private static void writeIndent(StringBuilder sb, int level) {
         for (int i = 0; i < level; i++) sb.append("  ");
     }

pxf-runtime/src/test/java/org/protowire/pxf/FormatDirectivesTest.java

@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2026 TrendVidia, LLC.
+package org.protowire.pxf;
+
+import org.junit.jupiter.api.Test;
+
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * {@link Format#formatDocument} keeps every directive, with its dataset
+ * rows, in document order (trendvidia/protowire#558, #188). A port of
+ * protowire-go's {@code format_directives_test.go}; the expected texts are
+ * the ones protowire-go's {@code FormatDocument} writes for the same sources.
+ */
+class FormatDirectivesTest {
+
+    /**
+     * Formats {@code src}, checks that the output parses to the same
+     * directives and is a fixed point of formatting, and returns it.
+     */
+    private static String formatRoundTrip(String src) {
+        Ast.Document doc = Parser.parse(src);
+        String out = Format.formatDocument(doc);
+        Ast.Document again = Parser.parse(out);
+        assertEquals(directiveShape(doc), directiveShape(again), out);
+        assertEquals(out, Format.formatDocument(again), "formatting is a fixed point");
+        return out;
+    }
+
+    /** What a document's directives say, without positions or comments. */
+    private static List<Object> directiveShape(Ast.Document doc) {
+        List<Object> out = new ArrayList<>();
+        out.add(doc.typeUrl());
+        for (Ast.Directive d : doc.directives()) {
+            out.add("@" + d.name());
+            out.add(d.prefixes());
+            out.add(d.body() == null ? null : new String(d.body(), StandardCharsets.UTF_8));
+        }
+        for (Ast.DatasetDirective d : doc.datasets()) {
+            out.add("@dataset");
+            out.add(d.type());
+            out.add(d.columns());
+            for (Ast.DatasetRow r : d.rows()) {
+                List<String> cells = new ArrayList<>();
+                for (Ast.Value c : r.cells()) {
+                    if (c == null) {
+                        cells.add("<absent>");
+                    } else {
+                        StringBuilder sb = new StringBuilder();
+                        Format.formatValue(sb, c, 0);
+                        cells.add(sb.toString());
+                    }
+                }
+                out.add(cells);
+            }
+        }
+        for (Ast.ProtoDirective d : doc.protos()) {
+            out.add("@proto");
+            out.add(d.shape());
+            out.add(d.typeName());
+            out.add(new String(d.body(), StandardCharsets.UTF_8));
+        }
+        out.add(doc.entries().size());
+        return out;
+    }
+
+    /** The order the directives come in, @type aside: what the shape above leaves out. */
+    private static List<String> directiveOrder(Ast.Document doc) {
+        List<String> out = new ArrayList<>();
+        for (Ast.Directive d : doc.directives()) out.add(d.pos().offset() + " @" + d.name());
+        for (Ast.DatasetDirective d : doc.datasets()) out.add(d.pos().offset() + " @dataset");
+        for (Ast.ProtoDirective d : doc.protos()) out.add(d.pos().offset() + " @proto");
+        out.sort((a, b) -> Integer.compare(
+                Integer.parseInt(a.substring(0, a.indexOf(' '))), Integer.parseInt(b.substring(0, b.indexOf(' ')))));
+        return out.stream().map(s -> s.substring(s.indexOf(' ') + 1)).toList();
+    }
+
+    @Test
+    void dataset() {
+        String out = formatRoundTrip("@dataset trades.v1.Trade (id, px)\n(1, 2.5)\n(2, 3.5)\n");
+        assertEquals("@dataset trades.v1.Trade (id, px)\n(1, 2.5)\n(2, 3.5)\n", out);
+
+        // An empty cell is an absent field and null a present one; both stay.
+        out = formatRoundTrip("@dataset trades.v1.Trade (id, px, note)\n(1, 2.5, )\n(2, , null)\n");
+        assertEquals("@dataset trades.v1.Trade (id, px, note)\n(1, 2.5, )\n(2, , null)\n", out);
+    }
+
+    @Test
+    void entryAndNamedDirective() {
+        String src = "@type a.v1.M\n@entry x { a = 1 }\n@header h.v1.H\n\nname = \"n\"\n";
+        assertEquals(src, formatRoundTrip(src));
+    }
+
+    @Test
+    void protoShapes() {
+        String[][] cases = {
+            {"@proto { string id = 1; }\n@dataset (id)\n(\"a\")\n", null},
+            {"@proto a.v1.R { string id = 1; }\n@dataset a.v1.R (id)\n(\"a\")\n", null},
+            {"@proto \"\"\"syntax = \"proto3\"; message R { string id = 1; }\"\"\"\n@dataset R (id)\n(\"a\")\n", null},
+            // @type moves to the front: it binds no @proto, so this changes nothing.
+            {"@proto b\"CgA=\"\n@type a.v1.M\n\nx = 1\n", "@type a.v1.M\n@proto b\"CgA=\"\n\nx = 1\n"},
+        };
+        for (String[] c : cases) {
+            String want = c[1] == null ? c[0] : c[1];
+            assertEquals(want, formatRoundTrip(c[0]), c[0]);
+        }
+    }
+
+    /**
+     * An anonymous @proto binds the next typed directive, so a @proto
+     * written after a dataset in the source stays after it.
+     */
+    @Test
+    void directivesKeepSourceOrder() {
+        String src = "@dataset t.v1.T (id)\n(1)\n@proto { string id = 1; }\n@dataset (id)\n(\"a\")\n";
+        assertEquals(src, formatRoundTrip(src));
+    }
+
+    @Test
+    void directiveComments() {
+        String out = formatRoundTrip("# about the rows\n@dataset t.v1.T (id)\n(1)\n# second\n@dataset t.v1.U (id)\n(2)\n");
+        assertTrue(out.contains("# about the rows\n@dataset t.v1.T (id)"), out);
+        assertTrue(out.contains("# second\n@dataset t.v1.U (id)"), out);
+    }
+
+    /** A document whose only directive is @type formats as it always has. */
+    @Test
+    void onlyTypeUnchanged() {
+        Ast.Document doc = Parser.parse("# c\n@type a.v1.M\n\nx = 1\n");
+        assertEquals("@type a.v1.M\n\n# c\nx = 1\n", Format.formatDocument(doc));
+    }
+
+    /** #188, first input: a @proto before an @entry used to be dropped. */
+    @Test
+    void issue188ProtoIsKept() {
+        String src = "@proto a.v1.P { int32 b = 1; }\n@entry x { a = 1 }\n";
+        String out = formatRoundTrip(src);
+        assertEquals(src, out);
+        assertEquals(List.of("@proto", "@entry"), directiveOrder(Parser.parse(out)));
+    }
+
+    /** #188, second input: an @entry after a dataset used to be written before it. */
+    @Test
+    void issue188DatasetBeforeEntryKeepsOrder() {
+        String src = "@dataset t.v1.T (id)\n(1)\n@entry x { a = 1 }\n";
+        String out = formatRoundTrip(src);
+        assertEquals(src, out);
+        assertEquals(List.of("@dataset", "@entry"), directiveOrder(Parser.parse(out)));
+    }
+}