Stop BinaryAST.toString() mutating process-global state just to nag about itself

[lagergren]

NOTE: I know that the BAST currently is not supported and that its future is uncertain, but for consistency, and since this is a very simple fix part of the "stop having mutating toStrings because it is impossible to debug stuff" pattern fix. Might as well get it in there since it is simple and done.

One file, +4 −14. First of eight small changes carved out of #569, each one landing independently.

What it does

BinaryAST.toString() returned the node type name, but first called reportUnimplemented():

public String toString() {
    reportUnimplemented("TODO implement toString() for " + this.getClass().getSimpleName());
    return nodeType().name();
}

private static final Set<String> ALREADY_DISPLAYED = new HashSet();

static void reportUnimplemented(String msg) {
    if (ALREADY_DISPLAYED.add(msg)) {
        System.err.println(msg);
    }
}

So merely looking at a node — in a debugger, a log line, a string concatenation — mutated shared
process state and wrote to System.err. Three separate problems in five lines:

• the write. A display method emits to a stream the caller never asked it to write to.
• the shared mutable state. ALREADY_DISPLAYED is process-global, raw (new HashSet(), not
  new HashSet<>()), unsynchronized, and written by any thread that happens to render a node. It
  also grows without bound for the life of the JVM.
• the coupling. Whether rendering prints anything depends on what was rendered earlier in the
  process, so the observable behaviour of toString() is a function of history.

This removes the nag. reportUnimplemented() and ALREADY_DISPLAYED had no other callers, so both
go with their imports.

Why it is safe

The returned text is unchanged by construction, not merely by measurement. Both before and
after, the method body ends return nodeType().name();. The only removal is the side effect. There
is no path by which a caller's string can differ.

I also verified it empirically alongside the other seven slices: rendering all 83,977 constants of
container zero produces a byte-identical digest to master.

Why this matters beyond tidiness

It's the cheapest instance of a pattern worth removing across the display methods: toString() that
does something other than produce a string. The expensive instance is
#639, where rendering a type interns 130 constants into
the pool via isA(). This one is the same class of bug with none of the difficulty, which is why
it's first.

Verification

On 76e5507a6:

• :javatools:test 414 tests / 0 failures / 0 errors; :javatools_utils:test 120 / 0 / 0.
• CI=true ./gradlew spotlessCheck clean.
• Rendered text of the whole constant pool byte-identical to master.

[ggleyzer]

Frankly, I don't understand why you even look at it. Spending ten seconds on that is ten seconds too many

[lagergren]

Frankly, I don't understand why you even look at it. Spending ten seconds on that is ten seconds too many

I simply don't understand how you can't get productivity destroyed by this - aren't you stepping in the debugger and looking at objects at any part of compilation or runtime? This is the thing that forces me to use println debugging instead of just standing with the debugger at a program point, expanding various objects with mouse hover to see what is going on?

[ggleyzer]

I don't know if you noticed, but Cam and I have been looking at objects in the debugger for the last ten years :)