Fix write() quoting of symbols in proper sexps by DimitriosDalaklidhs · Pull Request #490 · ion-fusion/fusion-java

DimitriosDalaklidhs · 2026-03-23T01:43:11Z

Fix write() quoting of symbols in proper sexps

ImmutablePair.write() was incorrectly quoting operator symbols like + as '+' when inside a sexp. This happened because the manual write path had no context about being inside a sexp, unlike ionize() which delegates to IonWriter and gets correct quoting automatically.

Fix: for proper sexps, delegate write() entirely to ionize() via a temporary IonWriter. Improper sexps fall back to manual rendering with Fusion's {.} dotted-pair notation as before.

Fixes the discrepancy between:
(write (quote (+ 1 2))) => ('+' 1 2) [wrong]
(ionize (quote (+ 1 2))) => (+ 1 2) [correct]

ImmutablePair.write() was incorrectly quoting operator symbols like + as '+' when inside a sexp. This happened because the manual write path had no context about being inside a sexp, unlike ionize() which delegates to IonWriter and gets correct quoting automatically. Fix: for proper sexps, delegate write() entirely to ionize() via a temporary IonWriter. Improper sexps fall back to manual rendering with Fusion's {.} dotted-pair notation as before. Fixes the discrepancy between: (write (quote (+ 1 2))) => ('+' 1 2) [wrong] (ionize (quote (+ 1 2))) => (+ 1 2) [correct]

SharkBaitDLS

It would be good to add tests that assert this behavior is working correctly. runtime/src/test/fusion/scripts/io.test.fusion is an appropriate place to add tests that cover this. There also is one existing failing test according to the CI build.

runtime/src/main/java/dev/ionfusion/fusion/FusionSexp.java

toddjonker

Hi @DimitriosDalaklidhs, thanks for the contribution! You found the correct centrum of the change, but unfortunately the minimal solution has a bit more inherent complexity.

As @SharkBaitDLS notes, we need some test cases as well. The best place for those is in io.test.fusion. While the tests for ionize use ionize_to_string, theres currently no write_to_string so you'll need to use with_output_to_string down at the bottom of that file.

(I needed write_to_string just this weekend, so I just cut #491 and will see if I can add that in the next day or two.)

runtime/src/main/java/dev/ionfusion/fusion/FusionSexp.java

toddjonker · 2026-03-23T20:09:20Z

runtime/src/main/java/dev/ionfusion/fusion/FusionSexp.java

+                // quoting rules, so operators like + are not wrongly quoted.
+                // ionize also handles annotations via setTypeAnnotations.
+                IonWriter iw = WRITER_BUILDER.build(out);
+                ionize(eval, iw);


I think there's a misconception at play: the value being a "proper sexp" doesn't imply that everything inside the sexp, recursively, is ionizable.

So I think this call is incorrect, in that it changes the output style for the entire sexp content, recursively.

It looks like this use:

(write (sexp (quote +) (void)))

...will raise an exception because the void value is not ionizable. But that's exactly the difference between ionize and write, the latter is more lenient.

Instead, that expression should output (+ {{{void}}}).

runtime/src/main/java/dev/ionfusion/fusion/FusionSexp.java

… sexp writing behavior.

toddjonker

Oof, this might turn out to be more complicated than I expected, do to limitations in the ion-java library.

runtime/src/test/fusion/scripts/io.test.fusion

runtime/src/main/java/dev/ionfusion/fusion/FusionSymbol.java

runtime/src/test/fusion/scripts/io.test.fusion

runtime/src/main/java/dev/ionfusion/fusion/FusionIo.java

When writing a sexp, operator symbols such as + and = were being quoted (e.g. '+' '=') rather than emitted bare (e.g. + =). This was caused by dispatchWrite delegating to BaseValue.write() which has no awareness of the parent context, causing ActualSymbol to always call IonTextUtils.printSymbol() which quotes operators. Fix by introducing a quoteOperators context flag threaded through new overloads of FusionIo.dispatchWrite() and BaseValue.write(). ImmutablePair.write() now passes quoteOperators=false to its direct children for both proper and improper sexps. ActualSymbol overrides the new write() overload and uses IonTextUtils.symbolVariant() to suppress quoting only for OPERATOR-variant symbols, leaving all other symbols (identifiers, symbols requiring quotes, etc.) unaffected. AnnotatedSymbol forwards the flag to its inner value while always quoting annotations. The default BaseValue.write(eval, out, quoteOperators) falls through to write(eval, out), so all other value types are unaffected with no call-site changes required. Also fixes a duplicate render bug in ImmutablePair.write() where the improper sexp traversal loop was emitted twice. Add tests covering: operator inside sexp, operator outside sexp, quotes-required symbol inside sexp, operator in improper sexp tail, and quotes-required symbol in improper sexp tail.

toddjonker

This is looking great! My only substantial request is the addition of a few test cases to verify context changes in relevant container combinations.

Plus correction of some spurious whitespace changes affecting indentation.

Thanks so much for following through with this!

toddjonker · 2026-03-24T18:47:43Z

runtime/src/main/java/dev/ionfusion/fusion/FusionSymbol.java

+            throws IOException
+        {
+            if (!quoteOperators
+                && IonTextUtils.symbolVariant(myContent) == IonTextUtils.SymbolVariant.OPERATOR)


[nitpick] Our general style would use static imports for symbolVariant and OPERATOR to avoid the qualified uses.

runtime/src/test/fusion/scripts/io.test.fusion

…stead of pretty in a couple places.

toddjonker · 2026-03-24T23:52:39Z

runtime/src/main/java/dev/ionfusion/fusion/FusionSymbol.java

This file got a ton of superfluous formatting changes. Please revert all except for the functional changes relating to the PR intent.

Looks like the whole thing got indented several spaces?

toddjonker · 2026-03-24T23:52:44Z

runtime/src/main/java/dev/ionfusion/fusion/FusionIo.java

        {
            myCurrentOutputPort = (DynamicParameter) currentOutputPort;
-            myBuilder = IonTextWriterBuilder.pretty().immutable();
+            myBuilder = IonTextWriterBuilder.minimal().immutable();


Ditto.

Eventually this will be controlled by a dynamic parameter or something, but for now the human-oriented pretty printing is best.

Also, I don't think that this would have any impact on the write paths we're concerned with here. Did you see otherwise?

toddjonker · 2026-03-24T23:52:48Z

fusioncli/src/main/java/dev/ionfusion/fusioncli/Version.java

            throws IOException
        {
-            IonTextWriterBuilder b = IonTextWriterBuilder.pretty();
+            IonTextWriterBuilder b = IonTextWriterBuilder.minimal();


Please revert this; it will change the CLI output and I'd like to keep that in the more readable pretty-printed format.

toddjonker · 2026-03-25T00:07:44Z

runtime/src/test/fusion/scripts/io.test.fusion

+// List context: nested sexp correctly unquotes its direct children but contains void.
+(check === "['+', ['+'], {'+':'+'}, (+ {{{void}}})]"
+  (with_output_to_string
+    (write (quote ['+', ['+'], {'+':'+'}, (+ (void))]))))


This input quote won't produce the desired output, since the (void) is treated literally, not evaluated to produce the void-value.

Please be sure to run ./gradlew build from the repo root to make sure all the tests pass. On the latest commit two test files fail: https://github.com/ion-fusion/fusion-java/actions/runs/23517778238/job/68456272579?pr=490#step:5:83

toddjonker · 2026-03-25T00:10:11Z

runtime/src/test/fusion/scripts/io.test.fusion

+// Voids verify that write mode is maintained and does not escalate to ionize.
+
+// List context: nested sexp correctly unquotes its direct children but contains void.
+(check === "['+', ['+'], {'+':'+'}, (+ {{{void}}})]"


OK, I see why you did the formatting change. Theses cases would be quite annoying to write without that.

And the struct one is just grrrrrrrr. You came up with a clever solution though!

It's not worth holding up the whole PR on this, though, so you can drop a big (when false around this whole section, and I can come back to it later. (And probably build some features to make it easier to test.)

DimitriosDalaklidhs requested review from SharkBaitDLS and toddjonker as code owners March 23, 2026 01:43

SharkBaitDLS requested changes Mar 23, 2026

View reviewed changes

runtime/src/main/java/dev/ionfusion/fusion/FusionSexp.java Outdated Show resolved Hide resolved

toddjonker requested changes Mar 23, 2026

View reviewed changes

toddjonker mentioned this pull request Mar 23, 2026

Prevent write and display from quoting operators in sexps #488

Open

Add diagnostic I/O tests to verify with_output_to_string, symbol, and…

182bc86

… sexp writing behavior.

toddjonker requested changes Mar 23, 2026

View reviewed changes

toddjonker reviewed Mar 23, 2026

View reviewed changes

runtime/src/main/java/dev/ionfusion/fusion/FusionIo.java Outdated Show resolved Hide resolved

toddjonker requested changes Mar 24, 2026

View reviewed changes

ddalaklidhs added 2 commits March 25, 2026 01:38

Fixed indentation manually, refined symbol class and added minimal in…

e0cf6e7

…stead of pretty in a couple places.

Fixed indentation manually, refined symbol class and added minimal in…

e95881e

…stead of pretty in a couple places.

toddjonker reviewed Mar 25, 2026

View reviewed changes

Conversation

DimitriosDalaklidhs commented Mar 23, 2026

Uh oh!

SharkBaitDLS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

toddjonker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

toddjonker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddjonker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants