Remove MacroRef from user-facing APIs (#937)

src/main/java/com/amazon/ion/MacroAwareIonWriter.kt

@@ -8,28 +8,20 @@ import com.amazon.ion.impl.macro.*
  * Extension of the IonWriter interface that supports writing macros.
  *
  * TODO: Consider exposing this as a Facet.
+ *
+ * TODO: See if we can have some sort of safe reference to a macro.
  */
 interface MacroAwareIonWriter : IonWriter {
 
-    /**
-     * Adds a macro to the macro table, returning a MacroRef that can be used to invoke the macro.
-     */
-    fun addMacro(macro: Macro): MacroRef
-
-    /**
-     * Adds a macro to the macro table, returning a MacroRef that can be used to invoke the macro.
-     */
-    fun addMacro(name: String, macro: Macro): MacroRef
-
     /**
      * Starts writing a macro invocation, adding it to the macro table, if needed.
      */
     fun startMacro(macro: Macro)
 
     /**
-     * Starts writing a macro using the given [MacroRef].
+     * Starts writing a macro invocation, adding it to the macro table, if needed.
      */
-    fun startMacro(macro: MacroRef)
+    fun startMacro(name: String, macro: Macro)
 
     /**
      * Ends and steps out of the current macro invocation.

src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java

@@ -117,6 +117,9 @@ class IonReaderContinuableCoreBinary extends IonCursorBinary implements IonReade
     // The core MacroEvaluator that this core reader delegates to when evaluating a macro invocation.
     private MacroEvaluator macroEvaluator = null;
 
+    // The encoding context (macro table) that is currently active.
+    private EncodingContext encodingContext = null;
+
     // Reads encoding directives from the stream.
     private final EncodingDirectiveReader encodingDirectiveReader = new EncodingDirectiveReader();
 
@@ -1147,7 +1150,7 @@ protected void installSymbols(List<String> newSymbols) {
      * @return the {@link EncodingContext} currently active, or {@code null}.
      */
     EncodingContext getEncodingContext() {
-        return macroEvaluator == null ? null : macroEvaluator.getEncodingContext();
+        return encodingContext;
     }
 
     /**
@@ -1159,7 +1162,7 @@ private class EncodingDirectiveReader {
         boolean isSymbolTableAppend = false;
         List<String> newSymbols = new ArrayList<>(8);
         Map<MacroRef, Macro> newMacros = new HashMap<>();
-        MacroCompiler macroCompiler = new MacroCompiler(IonReaderContinuableCoreBinary.this);
+        MacroCompiler macroCompiler = new MacroCompiler(IonReaderContinuableCoreBinary.this, newMacros::get);
 
         private boolean valueUnavailable() {
             Event event = fillValue();
@@ -1201,7 +1204,8 @@ private void stepOutOfSexpWithinEncodingDirective() {
          * Install `newMacros`, initializing a macro evaluator capable of evaluating them.
          */
         private void installMacros() {
-            macroEvaluator = new MacroEvaluator(new EncodingContext(newMacros));
+            encodingContext = new EncodingContext(newMacros);
+            macroEvaluator = new MacroEvaluator();
             macroEvaluatorIonReader = new MacroEvaluatorAsIonReader(macroEvaluator);
         }
 
@@ -1650,7 +1654,7 @@ private void collectEExpressionArgs(List<Expression.EExpressionBodyExpression> e
                 throw new IonException("Macro addresses larger than 2147483647 are not supported by this implementation.");
             }
             MacroRef address = MacroRef.byId((int) id);
-            Macro macro = macroEvaluator.getEncodingContext().getMacroTable().get(address);
+            Macro macro = encodingContext.getMacroTable().get(address);
             if (macro == null) {
                 throw new IonException(String.format("Encountered an unknown macro address: %d.", id));
             }
@@ -1675,7 +1679,7 @@ private void collectEExpressionArgs(List<Expression.EExpressionBodyExpression> e
                 readParameter(signature.get(i), presenceBitmap.get(i), expressions);
             }
             stepOutOfEExpression();
-            expressions.set(invocationStartIndex, new Expression.EExpression(address, invocationStartIndex, expressions.size()));
+            expressions.set(invocationStartIndex, new Expression.EExpression(macro, invocationStartIndex, expressions.size()));
         }
 
         /**

src/main/java/com/amazon/ion/impl/_Private_IonTextWriterBuilder_1_1.java

@@ -149,6 +149,7 @@ public IonWriter build(Appendable out) {
         _Private_IonTextWriterBuilder_1_1 b = fillDefaults();
         ManagedWriterOptions_1_1 options = new ManagedWriterOptions_1_1(
             false,
+            true,
             symbolInliningStrategy,
             LengthPrefixStrategy.NEVER_PREFIXED,
             // This could be made configurable.
@@ -166,6 +167,7 @@ public IonWriter build(OutputStream out) {
         _Private_IonTextWriterBuilder_1_1 b = fillDefaults();
         ManagedWriterOptions_1_1 options = new ManagedWriterOptions_1_1(
             false,
+            true,
             symbolInliningStrategy,
             LengthPrefixStrategy.NEVER_PREFIXED,
             // This could be made configurable.

src/main/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1.kt

@@ -3,9 +3,9 @@
 package com.amazon.ion.impl.bin
 
 import com.amazon.ion.*
-import com.amazon.ion.SymbolTable.UNKNOWN_SYMBOL_ID
+import com.amazon.ion.SymbolTable.*
 import com.amazon.ion.impl.*
-import com.amazon.ion.impl._Private_IonWriter.IntTransformer
+import com.amazon.ion.impl._Private_IonWriter.*
 import com.amazon.ion.impl.bin.LengthPrefixStrategy.*
 import com.amazon.ion.impl.bin.SymbolInliningStrategy.*
 import com.amazon.ion.impl.macro.*
@@ -14,9 +14,6 @@ import java.io.OutputStream
 import java.math.BigDecimal
 import java.math.BigInteger
 import java.util.*
-import kotlin.collections.ArrayList
-import kotlin.collections.HashMap
-import kotlin.collections.LinkedHashMap
 
 /**
  * A managed writer for Ion 1.1 that is generic over whether the raw encoding is text or binary.
@@ -158,26 +155,107 @@ internal class IonManagedWriter_1_1(
         return sid
     }
 
-    private fun internMacro(macro: Macro): Int {
-        // Check the current macro table
-        var id = macroTable[macro]
-        if (id != null) return id
-        // Check the to-be-appended macros
-        id = newMacros[macro]
-        if (id != null) return id
-        // Add to the to-be-appended symbols
-        id = macrosById.size
-        macrosById.add(macro)
-        macroNames.add(null)
-        newMacros[macro] = id
-        return id
+    /**
+     * Checks for macro invocations in the body of a TemplateMacro and ensure that those macros are added to the
+     * macro table.
+     *
+     * This is essentially a recursive, memoized, topological sort. Given a dependency graph, it runs in O(V + E) time.
+     * Memoization is done using the macro table, so the O(V + E) cost is only paid the first time a macro is added to
+     * the macro table.
+     */
+    private fun addMacroDependencies(macro: Macro) {
+        macro.dependencies.forEach {
+            if (it !in macroTable && it !in newMacros) {
+                addMacroDependencies(it)
+                assignMacroAddress(it)
+            }
+        }
     }
 
-    /** Converts a named macro reference to an address */
-    private fun MacroRef.ByName.intoId(): MacroRef.ById = MacroRef.ById(macroNames.indexOf(name))
+    /**
+     * Adds a named macro to the macro table
+     *
+     * Steps:
+     * - If the name is not already in use...
+     *    - And the macro is already in `newMacros`...
+     *      1. Get the address of the macro in `newMacros`
+     *      2. Add the name to `macroNames` for the that address
+     *      3. return the address
+     *    - Else...
+     *      1. Add a new entry for the macro to `newMacros` and get a new address
+     *      2. Add the name to `macroNames` for the new address
+     *      3. Return the new address
+     * - If the name is already in use...
+     *   - And it is associated with the same macro...
+     *      1. Return the address associated with the name
+     *   - And it is associated with a different macro...
+     *      - This is where the managed writer take an opinion. (Or be configurable.)
+     *        - It could mangle the name
+     *        - It could remove the name from a macro in macroTable, but then it would have to immediately flush to
+     *          make sure that any prior e-expressions are still valid. In addition, we would need to re-export all
+     *          the other macros from `$ion_encoding`.
+     *        - For now, we're just throwing an Exception.
+     *
+     * Visible for testing.
+     */
+    internal fun getOrAssignMacroAddressAndName(name: String, macro: Macro): Int {
+        // TODO: This is O(n), but could be O(1).
+        var existingAddress = macroNames.indexOf(name)
+        if (existingAddress < 0) {
+            // Name is not already in use
+            existingAddress = newMacros.getOrDefault(macro, -1)
+
+            val address = if (existingAddress < 0) {
+                // Macro is not in newMacros
+
+                // If it's in macroTable, we can skip adding dependencies
+                if (macro !in macroTable) addMacroDependencies(macro)
+                // Add to newMacros and get a macro address
+                assignMacroAddress(macro)
+            } else {
+                // Macro already exists in newMacros, but doesn't have a name
+                existingAddress
+            }
+            // Set the name of the macro
+            macroNames[address] = name
+            return address
+        } else if (macrosById[existingAddress] == macro) {
+            // Macro already in table, and already using the same name
+            return existingAddress
+        } else {
+            // Name is already in use for a different macro.
+            // This macro may or may not be in the table under a different name, but that's
+            // not particularly relevant unless we want to try to fall back to a different name.
+            TODO("Name shadowing is not supported yet. Call finish() before attempting to shadow an existing macro.")
+        }
+    }
+
+    /**
+     * Steps for adding an anonymous macro to the macro table
+     *    1. Check macroTable, if found, return that address
+     *    2. Check newMacros, if found, return that address
+     *    3. Add to newMacros, return new address
+     *
+     *  Visible for testing
+     */
+    internal fun getOrAssignMacroAddress(macro: Macro): Int {
+        var address = macroTable.getOrDefault(macro, -1)
+        if (address >= 0) return address
+        address = newMacros.getOrDefault(macro, -1)
+        if (address >= 0) return address
 
-    /** Converts a macro address to a macro name. If no name is found, returns the original address. */
-    private fun MacroRef.ById.intoNamed(): MacroRef = macroNames[id]?.let { MacroRef.ByName(it) } ?: this
+        addMacroDependencies(macro)
+        return assignMacroAddress(macro)
+    }
+
+    /** Unconditionally adds a macro to the macro table data structures and returns the new address. */
+    private fun assignMacroAddress(macro: Macro): Int {
+        val address = macrosById.size
+        macrosById.add(macro)
+        macroNames.add(null)
+        newMacros[macro] = address
+        return address
+    }
 
     // Only called by `finish()`
     private fun resetEncodingContext() {
@@ -453,11 +531,20 @@ internal class IonManagedWriter_1_1(
                     }
                     is Expression.MacroInvocation -> {
                         stepInSExp(usingLengthPrefix = false)
-                        when (expression.address) {
-                            is MacroRef.ById -> writeInt(expression.address.id.toLong())
-                            is MacroRef.ByName -> writeSymbol(expression.address.name)
-                        }
                         numberOfTimesToStepOut[expression.endExclusive]++
+
+                        val invokedAddress = macroTable[expression.macro]
+                            ?: newMacros[expression.macro]
+                            ?: throw IllegalStateException("A macro in the macro table is missing a dependency")
+
+                        if (options.invokeTdlMacrosByName) {
+                            val invokedName = macroNames[invokedAddress]
+                            if (invokedName != null) {
+                                writeSymbol(invokedName)
+                                return@forEachIndexed
+                            }
+                        }
+                        writeInt(invokedAddress.toLong())
                     }
                     is Expression.VariableRef -> writeSymbol(macro.signature[expression.signatureIndex].variableName)
                     else -> error("Unreachable")
@@ -482,15 +569,6 @@ internal class IonManagedWriter_1_1(
         TODO("Why do we need to expose this to users in the first place?")
     }
 
-    /**
-     * Extension function for [SymbolInliningStrategy] to accept [SymbolToken].
-     * Indicates whether a particular [SymbolToken] should be written inline (as opposed to writing as a SID).
-     * Symbols with unknown text must always be written as SIDs.
-     */
-    private fun SymbolInliningStrategy.shouldWriteInline(symbolKind: SymbolKind, symbol: SymbolToken): Boolean {
-        return symbol.text?.let { shouldWriteInline(symbolKind, it) } ?: false
-    }
-
     override fun setFieldName(name: String) {
         handleSymbolToken(UNKNOWN_SYMBOL_ID, name, SymbolKind.FIELD_NAME, userData)
     }
@@ -787,44 +865,26 @@ internal class IonManagedWriter_1_1(
         resetEncodingContext()
     }
 
-    override fun addMacro(macro: Macro): MacroRef {
-        val id = internMacro(macro)
-        return if (options.eExpressionIdentifierStrategy == ManagedWriterOptions_1_1.EExpressionIdentifierStrategy.BY_NAME) {
-            macroNames[id]
-                ?.let { MacroRef.ByName(it) }
-                ?: MacroRef.ById(id)
-        } else {
-            MacroRef.ById(id)
-        }
+    override fun startMacro(macro: Macro) {
+        val address = getOrAssignMacroAddress(macro)
+        startMacro(null, address, macro)
     }
 
-    override fun addMacro(name: String, macro: Macro): MacroRef {
-        val id = internMacro(macro)
-        macroNames[id] = name
-        return if (options.eExpressionIdentifierStrategy == ManagedWriterOptions_1_1.EExpressionIdentifierStrategy.BY_NAME) {
-            MacroRef.ByName(name)
-        } else {
-            MacroRef.ById(id)
-        }
+    override fun startMacro(name: String, macro: Macro) {
+        val address = getOrAssignMacroAddressAndName(name, macro)
+        startMacro(name, address, macro)
     }
 
-    override fun startMacro(macroRef: MacroRef) {
+    private fun startMacro(name: String?, address: Int, definition: Macro) {
         val useNames = options.eExpressionIdentifierStrategy == ManagedWriterOptions_1_1.EExpressionIdentifierStrategy.BY_NAME
-        val ref = when (macroRef) {
-            is MacroRef.ById -> if (useNames) macroRef.intoNamed() else macroRef
-            is MacroRef.ByName -> if (useNames) macroRef else macroRef.intoId()
-        }
-        when (ref) {
-            is MacroRef.ById -> userData.stepInEExp(ref.id, options.writeLengthPrefix(ContainerType.EEXP, depth + 1), macrosById[ref.id])
-            is MacroRef.ByName -> userData.stepInEExp(ref.name)
+        if (useNames && name != null) {
+            userData.stepInEExp(name)
+        } else {
+            val includeLengthPrefix = options.writeLengthPrefix(ContainerType.EEXP, depth + 1)
+            userData.stepInEExp(address, includeLengthPrefix, definition)
         }
     }
 
-    override fun startMacro(macro: Macro) {
-        val id = addMacro(macro)
-        startMacro(id)
-    }
-
     override fun startExpressionGroup() {
         userData.stepInExpressionGroup(options.writeLengthPrefix(ContainerType.EXPRESSION_GROUP, depth + 1))
     }

src/main/java/com/amazon/ion/impl/bin/ManagedWriterOptions_1_1.kt

@@ -17,6 +17,7 @@ data class ManagedWriterOptions_1_1(
      * more readable if it's false.
      */
     val internEncodingDirectiveSymbols: Boolean,
+    val invokeTdlMacrosByName: Boolean,
     val symbolInliningStrategy: SymbolInliningStrategy,
     val lengthPrefixStrategy: LengthPrefixStrategy,
     val eExpressionIdentifierStrategy: EExpressionIdentifierStrategy,
@@ -25,6 +26,7 @@ data class ManagedWriterOptions_1_1(
         @JvmField
         val ION_BINARY_DEFAULT = ManagedWriterOptions_1_1(
             internEncodingDirectiveSymbols = true,
+            invokeTdlMacrosByName = false,
             symbolInliningStrategy = SymbolInliningStrategy.NEVER_INLINE,
             lengthPrefixStrategy = LengthPrefixStrategy.ALWAYS_PREFIXED,
             eExpressionIdentifierStrategy = EExpressionIdentifierStrategy.BY_ADDRESS,
@@ -33,6 +35,7 @@ data class ManagedWriterOptions_1_1(
         val ION_TEXT_DEFAULT = ManagedWriterOptions_1_1(
             // Encoding directives are easier to read if we don't intern their keywords.
             internEncodingDirectiveSymbols = false,
+            invokeTdlMacrosByName = true,
             symbolInliningStrategy = SymbolInliningStrategy.ALWAYS_INLINE,
             // This doesn't actually have any effect for Ion Text since there are no length-prefixed containers.
             lengthPrefixStrategy = LengthPrefixStrategy.NEVER_PREFIXED,

src/main/java/com/amazon/ion/impl/macro/Expression.kt

@@ -201,7 +201,7 @@ sealed interface Expression {
      * A macro invocation that needs to be expanded.
      */
     data class MacroInvocation(
-        val address: MacroRef,
+        val macro: Macro,
         override val selfIndex: Int,
         override val endExclusive: Int
     ) : TemplateBodyExpression, HasStartAndEnd
@@ -210,7 +210,7 @@ sealed interface Expression {
      * An e-expression that needs to be expanded.
      */
     data class EExpression(
-        val address: MacroRef,
+        val macro: Macro,
         override val selfIndex: Int,
         override val endExclusive: Int
     ) : EExpressionBodyExpression, HasStartAndEnd