From 0f1626ecef3a68b274882ca95adaeac35fa1a78b Mon Sep 17 00:00:00 2001 From: Tim Claridge Date: Mon, 6 Jul 2026 21:34:05 +1200 Subject: [PATCH] GenStreamableTask flatten: nested classes, region scoping, ancestor-aware supers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three structural fixes to flatten mode, found bringing up games on-device (Epic A1 Phase 5): 1. Nested classes: processFlatten now recurses through getDeclaredClasses() (not gated on the outer class's eligibility), so nested Streamables like BangConfig.Round get deterministic methods too. All detection/substitution/ insertion is scoped to each class's own body region (brace-matched, comment/string-aware, member-class bodies masked during detection) so an instrumented outer class can't hide — or poison — a nested one. 2. Ancestor-aware generation: a class whose ancestor declares read/writeObject now generates `super.readObject(ins)` + only the fields declared below that ancestor, instead of a self-contained full-set method. A self-contained subclass method silently dropped the ancestor's side effects for every subclass that previously inherited it: Prop.init() (server NPE cloning board props at startRound), Item's _nondb identity extras (itemId=0 on every wire-streamed item), InvocationMarshaller's _invdir rebind. Byte layout is unchanged: the ancestor's method streams exactly the flattened prefix. When the subclass adds no fields, nothing is generated — plain inheritance is already complete (the field-free marshaller/unit case). 3. Abstract classes: substitution-only. Their default*Object() pass-throughs are replaced in place with the class's own flattened prefix (making them safe super-call targets — the reflective pass-through reads the full dynamic-class set and would double-read the subclass tail), but whole methods are never generated for them. Requires every Streamable ancestor to be covered by the task's filesets; see findStreamMethodAncestor doc. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01HhfHCXYp2ctWi76Z91y9Ut --- .../presents/tools/GenStreamableTask.java | 364 ++++++++++++++---- 1 file changed, 295 insertions(+), 69 deletions(-) diff --git a/tools/src/main/java/com/threerings/presents/tools/GenStreamableTask.java b/tools/src/main/java/com/threerings/presents/tools/GenStreamableTask.java index bd70c4a8b..092dca9d5 100644 --- a/tools/src/main/java/com/threerings/presents/tools/GenStreamableTask.java +++ b/tools/src/main/java/com/threerings/presents/tools/GenStreamableTask.java @@ -176,17 +176,52 @@ public class GenStreamableTask extends GenTask * "Flatten" mode: emit self-contained declaration-ordered {@code read/writeObject} that stream * the whole {@link ClassUtil#getFields} field set via {@link com.threerings.io.GenStreamUtil}. * Handles {@link DObject}s and replaces pass-through {@code default*Object()} calls in place. + * The top-level class AND every named member class (recursively) are processed — a nested + * Streamable (e.g. {@code BangConfig.Round}) streams reflectively just like a top-level one, + * so it needs the same deterministic methods on ART. All detection/substitution/insertion is + * scoped to each class's own body region so an instrumented outer class never masks (or + * poisons) a nested one and vice versa. */ protected void processFlatten (File source, Class sclass) throws IOException { - // must be a concrete Streamable; never instrument abstract bases (their concrete subclasses - // stream the full flattened field set themselves, so an abstract base carrying a read/write - // method would "poison" any subclass that didn't get its own — silently dropping fields) - if (!Streamable.class.isAssignableFrom(sclass) || sclass.isInterface() || - sclass.isEnum() || Modifier.isAbstract(sclass.getModifiers())) { - return; + String src = new String(Files.readAllBytes(source.toPath())); + String out = processFlattenTree(src, sclass); + if (!out.equals(src)) { + writeFile(source.getAbsolutePath(), out); } + } + + /** + * Processes {@code sclass} then recurses into its named member classes. Recursion is NOT gated + * on the enclosing class's eligibility: an abstract (or non-Streamable) outer class can still + * declare concrete nested Streamables that need instrumenting. + */ + protected String processFlattenTree (String src, Class sclass) + { + src = processFlattenClass(src, sclass); + for (Class nested : sclass.getDeclaredClasses()) { + src = processFlattenTree(src, nested); + } + return src; + } + + /** + * Instruments a single class (top-level or nested) within {@code src}, returning the updated + * source text (or {@code src} unchanged if the class is ineligible or already instrumented). + */ + protected String processFlattenClass (String src, Class sclass) + { + if (!Streamable.class.isAssignableFrom(sclass) || sclass.isInterface() || sclass.isEnum()) { + return src; + } + // ABSTRACT classes get SUBSTITUTION ONLY: an existing default*Object() pass-through is + // replaced with the class's own flattened prefix (so the method becomes a safe super-call + // target for generated subclass methods — reflective defaultReadObject reads the full + // DYNAMIC-class set, which would double-read the subclass tail), but never whole generated + // methods (those would "poison" any concrete subclass that didn't get its own — silently + // dropping its fields from the stream) + boolean isAbstract = Modifier.isAbstract(sclass.getModifiers()); // gather the streamed fields in the exact order the reflective streamer uses (superclass // first, then this class's declared fields), skipping @NotStreamable and closure refs @@ -200,106 +235,248 @@ public class GenStreamableTask extends GenTask } fields.add(field); } - if (fields.isEmpty()) { - return; + + // detection and substitution look only at THIS class's body, with any nested member class + // bodies blanked out (space-padded, so offsets in the masked text map 1:1 onto src) + int[] region = findClassRegion(src, sclass); + String masked = maskNestedClasses(src, region, sclass); + + boolean hasRead = masked.contains("public void readObject"); + boolean hasWrite = masked.contains("public void writeObject"); + boolean hasReadDefault = DEFAULT_READ.matcher(masked).find(); + boolean hasWriteDefault = DEFAULT_WRITE.matcher(masked).find(); + + // a field-less class has nothing to generate, but an existing pass-through must STILL be + // substituted (to an empty read/write — the reflective call would stream the full + // dynamic-class set when reached via a subclass's super-call) + if (fields.isEmpty() && !hasReadDefault && !hasWriteDefault) { + return src; } - String src = new String(Files.readAllBytes(source.toPath())); - - boolean hasRead = src.contains("public void readObject"); - boolean hasWrite = src.contains("public void writeObject"); - boolean hasReadDefault = DEFAULT_READ.matcher(src).find(); - boolean hasWriteDefault = DEFAULT_WRITE.matcher(src).find(); - // if either method is hand-written custom streaming (present, but NOT a defaultXObject // pass-through), the class fully manages its own streaming and is already deterministic; // leave it entirely alone rather than risk a generated/hand-written mismatch if ((hasRead && !hasReadDefault) || (hasWrite && !hasWriteDefault)) { - return; + return src; } - // build whole methods for any streaming direction that has no method at all - StringBuilder methods = new StringBuilder(); - if (!hasRead) { - methods.append(FLAT_READ_OPEN); - for (Field field : fields) { - methods.append(" ").append(flatReadStatement(field, "ins")).append("\n"); - } - methods.append(READ_CLOSE); + // an ancestor that declares its own read/writeObject is the class's streaming "base": a + // generated method here must super-call it (preserving its side effects — Prop.init(), + // InvocationMarshaller's _invdir rebind, Item's _nondb extras) and then stream only the + // fields declared BELOW it. A self-contained full-set method would silently drop those + // side effects for every subclass that previously just inherited the ancestor's method + // (the Phase 5(b) Prop._vscale server NPE / Item._itemId wire bug). Byte layout on the + // stream is unchanged either way: the ancestor's method covers the flattened prefix. + Class readAncestor = findStreamMethodAncestor( + sclass, "readObject", com.threerings.io.ObjectInputStream.class); + Class writeAncestor = findStreamMethodAncestor( + sclass, "writeObject", com.threerings.io.ObjectOutputStream.class); + List readFields = fieldsBelow(fields, readAncestor); + List writeFields = fieldsBelow(fields, writeAncestor); + + // methods are indented one level deeper than the class declaration's nesting depth + int depth = 0; + for (Class c = sclass; c.getEnclosingClass() != null; c = c.getEnclosingClass()) { + depth++; } - if (!hasWrite) { + String ind = indent(depth + 1); + + // build whole methods for any streaming direction that has no method at all — but when an + // ancestor declares the method and this class adds no fields of its own, plain inheritance + // is already complete and deterministic: generate nothing (the field-free-marshaller case) + StringBuilder methods = new StringBuilder(); + if (!hasRead && !isAbstract && !readFields.isEmpty()) { + methods.append(ind).append("// from interface Streamable\n") + .append(ind).append("public void readObject (com.threerings.io.ObjectInputStream ins)\n") + .append(ind).append(" throws java.io.IOException, java.lang.ClassNotFoundException\n") + .append(ind).append("{\n"); + if (readAncestor != null) { + methods.append(ind).append(" super.readObject(ins);\n"); + } + for (Field field : readFields) { + methods.append(ind).append(" ").append(flatReadStatement(field, "ins")).append("\n"); + } + methods.append(ind).append("}\n"); + } + if (!hasWrite && !isAbstract && !writeFields.isEmpty()) { if (methods.length() > 0) { methods.append("\n"); } - methods.append(FLAT_WRITE_OPEN); - for (Field field : fields) { - methods.append(" ").append(flatWriteStatement(field, "out")).append("\n"); + methods.append(ind).append("// from interface Streamable\n") + .append(ind).append("public void writeObject (com.threerings.io.ObjectOutputStream out)\n") + .append(ind).append(" throws java.io.IOException\n") + .append(ind).append("{\n"); + if (writeAncestor != null) { + methods.append(ind).append(" super.writeObject(out);\n"); } - methods.append(WRITE_CLOSE); + for (Field field : writeFields) { + methods.append(ind).append(" ").append(flatWriteStatement(field, "out")).append("\n"); + } + methods.append(ind).append("}\n"); } boolean changed = false; - // replace a pass-through default read/write call in place, preserving surrounding logic - if (hasReadDefault) { - src = substituteDefault(src, DEFAULT_READ, fields, true); + // replace pass-through default read/write calls in place, preserving surrounding logic + // (matched against the masked body so a nested class's pass-through is never rewritten + // with THIS class's fields — it gets its own substitution when its class is processed) + if (hasReadDefault || hasWriteDefault) { + src = substituteDefaults(src, masked, region, fields); changed = true; } - if (hasWriteDefault) { - src = substituteDefault(src, DEFAULT_WRITE, fields, false); - changed = true; - } - // insert whole methods just before the class-closing brace, so we NEVER disturb the + // insert whole methods just before this class's closing brace, so we NEVER disturb the // "// AUTO-GENERATED: METHODS" section (which holds the gendobj/genservice accessors) — // SourceFile.generate() would replace that section wholesale and drop the accessors if (methods.length() > 0) { - src = insertBeforeClassClose(src, methods.toString()); + int closeLineStart = src.lastIndexOf('\n', region[1]) + 1; + src = src.substring(0, closeLineStart) + "\n" + methods + src.substring(closeLineStart); changed = true; } if (changed) { System.err.println("Converting (flatten) " + sclass.getName() + "..."); - writeFile(source.getAbsolutePath(), src); } + return src; } /** - * Inserts {@code methods} (already 4-space indented, newline-terminated) just before the final - * top-level class-closing brace of {@code src}. + * Locates the body of {@code sclass}'s declaration in {@code src}, searching only within the + * enclosing class's own region for nested classes (so same-simple-named classes elsewhere in + * the file can't be confused). Returns {@code {bodyStart, closeBrace}}: the offset just after + * the opening brace and the offset of the matching closing brace. */ - protected String insertBeforeClassClose (String src, String methods) + protected int[] findClassRegion (String src, Class sclass) { - String[] lines = src.split("\n", -1); - int lastBrace = -1; - for (int ii = 0; ii < lines.length; ii++) { - if (lines[ii].trim().equals("}")) { - lastBrace = ii; + int searchStart = 0, searchEnd = src.length(); + Class outer = sclass.getEnclosingClass(); + if (outer != null) { + int[] outerRegion = findClassRegion(src, outer); + searchStart = outerRegion[0]; + searchEnd = outerRegion[1]; + } + Pattern decl = Pattern.compile( + "\\b(?:class|interface|enum)\\s+" + Pattern.quote(sclass.getSimpleName()) + "\\b"); + Matcher m = decl.matcher(src); + m.region(searchStart, searchEnd); + if (!m.find()) { + throw new RuntimeException("Cannot locate declaration of " + sclass.getName()); + } + int open = src.indexOf('{', m.end()); + if (open < 0 || open >= searchEnd) { + throw new RuntimeException("Cannot locate body of " + sclass.getName()); + } + return new int[] { open + 1, matchBrace(src, open) }; + } + + /** + * Returns the offset of the brace closing the one at {@code open}, skipping braces inside + * string/char literals and comments. + */ + protected int matchBrace (String src, int open) + { + int braces = 0; + for (int ii = open; ii < src.length(); ii++) { + char c = src.charAt(ii); + switch (c) { + case '{': + braces++; + break; + case '}': + if (--braces == 0) { + return ii; + } + break; + case '"': + case '\'': + char quote = c; + for (ii++; ii < src.length(); ii++) { + char d = src.charAt(ii); + if (d == '\\') { + ii++; + } else if (d == quote) { + break; + } + } + break; + case '/': + if (ii + 1 < src.length()) { + char d = src.charAt(ii + 1); + if (d == '/') { + int nl = src.indexOf('\n', ii); + ii = (nl < 0) ? src.length() : nl; + } else if (d == '*') { + int end = src.indexOf("*/", ii + 2); + if (end < 0) { + throw new RuntimeException("Unterminated block comment"); + } + ii = end + 1; + } + } + break; } } - if (lastBrace < 0) { - throw new RuntimeException("No class-closing brace found"); + throw new RuntimeException("Unbalanced braces scanning class body"); + } + + /** + * Returns {@code src} with everything outside {@code region} AND every named member class body + * within it replaced by spaces — same length as {@code src}, so match offsets in the masked + * text index directly into the original. + */ + protected String maskNestedClasses (String src, int[] region, Class sclass) + { + StringBuilder sb = new StringBuilder(src); + for (int ii = 0; ii < region[0]; ii++) { + blank(sb, ii); } - StringBuilder sb = new StringBuilder(); - for (int ii = 0; ii < lines.length; ii++) { - if (ii == lastBrace) { - sb.append("\n").append(methods); + for (int ii = region[1]; ii < sb.length(); ii++) { + blank(sb, ii); + } + for (Class nested : sclass.getDeclaredClasses()) { + int[] nregion; + try { + nregion = findClassRegion(src, nested); + } catch (RuntimeException e) { + continue; // e.g. synthetic member with no source declaration } - sb.append(lines[ii]); - if (ii < lines.length - 1) { - sb.append("\n"); + for (int ii = nregion[0]; ii < nregion[1]; ii++) { + blank(sb, ii); } } return sb.toString(); } - /** - * Replaces the single {@code recv.defaultReadObject()} / {@code recv.defaultWriteObject()} - * statement matched by {@code pat} with an explicit, same-indentation block that streams every - * field via {@link com.threerings.io.GenStreamUtil}, reusing the receiver variable name. - */ - protected String substituteDefault (String src, Pattern pat, List fields, boolean read) + protected static void blank (StringBuilder sb, int ii) { - Matcher m = pat.matcher(src); - StringBuffer sb = new StringBuffer(); - while (m.find()) { + if (sb.charAt(ii) != '\n') { + sb.setCharAt(ii, ' '); + } + } + + /** + * Replaces every pass-through {@code recv.defaultReadObject()} / {@code recv.defaultWriteObject()} + * statement found in {@code masked} with an explicit, same-indentation block that streams every + * field via {@link com.threerings.io.GenStreamUtil}, reusing the receiver variable name. + * Matching runs on the masked text (this class's body only); edits apply to the real source. + */ + protected String substituteDefaults (String src, String masked, int[] region, List fields) + { + // collect matches from both patterns, then apply in reverse offset order so earlier + // replacements don't shift later match positions + List matches = Lists.newArrayList(); // {start, end, isRead} + for (Pattern pat : new Pattern[] { DEFAULT_READ, DEFAULT_WRITE }) { + Matcher m = pat.matcher(masked); + while (m.find()) { + matches.add(new int[] { m.start(), m.end(), (pat == DEFAULT_READ) ? 1 : 0 }); + } + } + matches.sort((a, b) -> b[0] - a[0]); + + StringBuilder sb = new StringBuilder(src); + for (int[] match : matches) { + Matcher m = ((match[2] == 1) ? DEFAULT_READ : DEFAULT_WRITE).matcher(masked); + m.region(match[0], match[1]); + if (!m.find()) { + throw new RuntimeException("Lost a default*Object match on re-find"); + } String indent = m.group(1), recv = m.group(2); StringBuilder block = new StringBuilder(); for (int ii = 0; ii < fields.size(); ii++) { @@ -307,15 +484,64 @@ public class GenStreamableTask extends GenTask block.append("\n"); } Field field = fields.get(ii); - block.append(indent).append( - read ? flatReadStatement(field, recv) : flatWriteStatement(field, recv)); + block.append(indent).append((match[2] == 1) + ? flatReadStatement(field, recv) : flatWriteStatement(field, recv)); } - m.appendReplacement(sb, Matcher.quoteReplacement(block.toString())); + sb.replace(match[0], match[1], block.toString()); } - m.appendTail(sb); return sb.toString(); } + protected static String indent (int levels) + { + StringBuilder sb = new StringBuilder(); + for (int ii = 0; ii < levels; ii++) { + sb.append(" "); + } + return sb.toString(); + } + + /** + * Returns the nearest strict ancestor of {@code sclass} that declares the given streaming + * method, or null. Such an ancestor is a safe super-call target because flatten mode + * guarantees its method streams exactly the ancestor's own flattened prefix: hand-written + * methods with a {@code default*Object()} pass-through get it substituted in place (abstract + * classes included — see {@link #processFlattenClass}), and generated methods are explicit by + * construction. REQUIREMENT: every Streamable ancestor's source must be covered by this + * task's filesets (or already be prefix-exact, like narya's own InvocationMarshaller) — an + * out-of-fileset ancestor whose method still calls the reflective pass-through would read the + * full dynamic-class field set and double-read the subclass tail. + */ + protected Class findStreamMethodAncestor (Class sclass, String name, Class arg) + { + for (Class c = sclass.getSuperclass(); c != null; c = c.getSuperclass()) { + try { + c.getDeclaredMethod(name, arg); + return c; + } catch (NoSuchMethodException nsme) { + // keep walking + } + } + return null; + } + + /** Returns the subset of {@code fields} declared strictly below {@code ancestor} (all of + * them when {@code ancestor} is null). */ + protected static List fieldsBelow (List fields, Class ancestor) + { + if (ancestor == null) { + return fields; + } + List below = Lists.newArrayList(); + for (Field field : fields) { + Class declaring = field.getDeclaringClass(); + if (declaring != ancestor && ancestor.isAssignableFrom(declaring)) { + below.add(field); + } + } + return below; + } + protected String flatReadStatement (Field field, String recv) { return "com.threerings.io.GenStreamUtil.readField(" +