From 67119d0be82ae519c2895389b78521c32b931281 Mon Sep 17 00:00:00 2001 From: Tim Claridge Date: Mon, 6 Jul 2026 22:34:20 +1200 Subject: [PATCH] GenStreamableTask flatten: model the POST-regen hierarchy for super-call targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 0f1626ece, found when 85 persisted .board files stopped deserializing: the ancestor walk reflected on classes compiled from the STRIPPED sources, but javac binds the emitted super.readObject to the nearest declaration in the FINAL sources. CargoTank extends Counter extends Prop — Counter regenerates a method covering `count`, so CargoTank's tail computed against Prop streamed `count` twice and desynced the stream. findStreamMethodAncestor now asks willDeclare(): an ancestor counts iff it declares the method in current source (hand-written / substituted frame) OR this run will generate one for it (concrete, in-fileset, nonempty own tail — recursive, memoized; the filesets are pre-scanned so out-of-fileset ancestors are never assumed generatable). Consequence: a subclass whose tail below the NEAREST post-regen ancestor is empty generates nothing (CargoTank inherits Counter's method). The hand-written-custom skip is also per-SIDE now, not per-class: Prop has a custom readObject but no writeObject, and its write side must regenerate both for legacy parity and because subclasses emit super.writeObject against it (the old whole-class skip left those uncompilable). Verified: all 165 persisted .board files load with the regenerated streamers; regen is idempotent (second run converts 0). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01HhfHCXYp2ctWi76Z91y9Ut --- .../presents/tools/GenStreamableTask.java | 131 +++++++++++++----- 1 file changed, 99 insertions(+), 32 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 092dca9d5..ab4c70008 100644 --- a/tools/src/main/java/com/threerings/presents/tools/GenStreamableTask.java +++ b/tools/src/main/java/com/threerings/presents/tools/GenStreamableTask.java @@ -62,14 +62,26 @@ public class GenStreamableTask extends GenTask @Override public void execute () { + // pre-scan: collect every top-level class name covered by the filesets, so the + // post-regen hierarchy model (willDeclare) knows which ancestors this run can generate + // methods for + List sources = Lists.newArrayList(); for (FileSet fs : _filesets) { DirectoryScanner ds = fs.getDirectoryScanner(getProject()); File fromDir = fs.getDir(getProject()); - String[] srcFiles = ds.getIncludedFiles(); - for (String srcFile : srcFiles) { - processClass(new File(fromDir, srcFile)); + for (String srcFile : ds.getIncludedFiles()) { + File source = new File(fromDir, srcFile); + sources.add(source); + try { + _filesetClasses.add(GenUtil.readClassName(source)); + } catch (Exception e) { + System.err.println("Failed to parse " + source + ": " + e.getMessage()); + } } } + for (File source : sources) { + processClass(source); + } } /** @@ -225,16 +237,7 @@ public class GenStreamableTask extends GenTask // 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 - List fields = Lists.newArrayList(); - for (Field field : ClassUtil.getFields(sclass)) { - if (field.getAnnotation(NotStreamable.class) != null) { - continue; - } - if (field.isSynthetic() && field.getName().startsWith("this$")) { - continue; - } - fields.add(field); - } + List fields = streamedFields(sclass); // 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) @@ -253,12 +256,12 @@ public class GenStreamableTask extends GenTask return src; } - // 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 src; - } + // a hand-written custom method (present, but NOT a defaultXObject pass-through) already + // manages that DIRECTION's streaming deterministically — leave it alone. The skip is + // per-side, not per-class: Prop has a custom readObject but no writeObject at all, and + // its write side still needs the generated method (both because legacy reflective writes + // must become explicit, and because willDeclare models sides independently — subclasses + // emit super.writeObject against it) // 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(), @@ -502,29 +505,87 @@ public class GenStreamableTask extends GenTask } /** - * 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. + * Returns the nearest strict ancestor of {@code sclass} that will declare the given streaming + * method AFTER this run's regeneration, or null. This must model the POST-regen hierarchy — + * not the classes being reflected on (which were compiled from stripped sources): javac binds + * the emitted {@code super.readObject(ins)} to the nearest declaration in the FINAL sources, + * so computing the tail against a farther ancestor double-streams every field an intermediate + * generated method covers (found the hard way: CargoTank extends Counter extends Prop — + * Counter regenerates a method covering {@code count}, so a CargoTank tail computed against + * Prop read {@code count} twice and desynced every persisted .board file). + * + * An ancestor declares the method post-regen iff (a) it declares it now (hand-written custom, + * or a pass-through frame that substitution rewrites in place — abstract classes included), or + * (b) it is a concrete in-fileset Streamable whose own post-regen tail is nonempty, i.e. this + * run generates one for it (recursive, memoized). Ancestors OUTSIDE the task's filesets never + * have methods generated, so (b) additionally requires fileset membership. Any such ancestor + * is a safe super-call target: its method streams exactly the ancestor's own flattened prefix. */ protected Class findStreamMethodAncestor (Class sclass, String name, Class arg) { for (Class c = sclass.getSuperclass(); c != null; c = c.getSuperclass()) { - try { - c.getDeclaredMethod(name, arg); + if (willDeclare(c, name, arg)) { return c; - } catch (NoSuchMethodException nsme) { - // keep walking } } return null; } + /** Whether {@code c} will declare the given streaming method once regeneration completes. */ + protected boolean willDeclare (Class c, String name, Class arg) + { + String key = c.getName() + "#" + name; + Boolean cached = _willDeclare.get(key); + if (cached != null) { + return cached; + } + boolean result; + try { + c.getDeclaredMethod(name, arg); + result = true; // declares it in (stripped) source or a dependency jar + } catch (NoSuchMethodException nsme) { + if (!Streamable.class.isAssignableFrom(c) || c.isInterface() || c.isEnum() || + Modifier.isAbstract(c.getModifiers()) || !inFilesets(c)) { + result = false; // never generated for these + } else { + // will get a generated method iff its own tail (below ITS post-regen ancestor, + // recursively) is nonempty + result = !fieldsBelow(streamedFields(c), + findStreamMethodAncestor(c, name, arg)).isEmpty(); + } + } + _willDeclare.put(key, result); + return result; + } + + /** Whether {@code c}'s source is covered by this task's filesets (nested classes count via + * their outermost enclosing class's file). */ + protected boolean inFilesets (Class c) + { + Class outer = c; + while (outer.getEnclosingClass() != null) { + outer = outer.getEnclosingClass(); + } + return _filesetClasses.contains(outer.getName()); + } + + /** Gathers the streamed fields of {@code sclass} in reflective-streamer order (superclass + * first), skipping {@code @NotStreamable} and closure refs. */ + protected static List streamedFields (Class sclass) + { + List fields = Lists.newArrayList(); + for (Field field : ClassUtil.getFields(sclass)) { + if (field.getAnnotation(NotStreamable.class) != null) { + continue; + } + if (field.isSynthetic() && field.getName().startsWith("this$")) { + continue; + } + fields.add(field); + } + return fields; + } + /** 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) @@ -611,6 +672,12 @@ public class GenStreamableTask extends GenTask /** Whether to generate self-contained flattened methods (Android/ART support). */ protected boolean _flatten; + /** Top-level class names covered by the filesets (see {@link #inFilesets}). */ + protected java.util.Set _filesetClasses = new java.util.HashSet(); + + /** Memo for {@link #willDeclare} ("classname#methodname" → will it declare post-regen). */ + protected java.util.Map _willDeclare = new java.util.HashMap(); + protected static final String READ_OPEN = " // from interface Streamable\n" + " public void readObject (ObjectInputStream ins)\n" +