From 88cbd2c02d3f6ab4f3cea3e7db7e586e8e304b79 Mon Sep 17 00:00:00 2001 From: Joshua Barr Date: Thu, 24 Oct 2024 14:17:59 -0700 Subject: [PATCH] Various changes in support of system macros * Add a definition of `default` * Use qualified module reference for system macros where relevant * Support use of system macros in TDL * Updating system symbols to match spec as of now * Associate SystemMacro values directly with SystemSymbols_1_1 values * Add SystemSymbol lookup by name --- .../amazon/ion/impl/IonRawTextWriter_1_1.kt | 8 +++ .../impl/IonReaderContinuableCoreBinary.java | 3 +- .../ion/impl/PrivateIonRawWriter_1_1.kt | 5 ++ .../com/amazon/ion/impl/SystemSymbols_1_1.kt | 70 ++++++++++++------- .../ion/impl/bin/IonManagedWriter_1_1.kt | 35 ++++------ .../ion/impl/bin/IonRawBinaryWriter_1_1.kt | 7 ++ .../amazon/ion/impl/macro/MacroCompiler.kt | 16 ++++- .../com/amazon/ion/impl/macro/SystemMacro.kt | 58 +++++++++------ .../ion/impl/bin/IonManagedWriter_1_1_Test.kt | 30 -------- 9 files changed, 131 insertions(+), 101 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/IonRawTextWriter_1_1.kt b/src/main/java/com/amazon/ion/impl/IonRawTextWriter_1_1.kt index 78676cbe0..146d67d0f 100644 --- a/src/main/java/com/amazon/ion/impl/IonRawTextWriter_1_1.kt +++ b/src/main/java/com/amazon/ion/impl/IonRawTextWriter_1_1.kt @@ -458,6 +458,14 @@ class IonRawTextWriter_1_1 internal constructor( isPendingSeparator = true } + override fun stepInTdlSystemMacroInvocation(systemSymbol: SystemSymbols_1_1) { + startSexp { + output.appendAscii("(.\$ion::") + output.appendAscii(systemSymbol.text) + } + isPendingSeparator = true + } + override fun writeTdlVariableExpansion(variableName: String) { writeScalar { output.appendAscii("(%") diff --git a/src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java b/src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java index 293f7c6b2..5865acc25 100644 --- a/src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java +++ b/src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java @@ -2473,7 +2473,8 @@ protected final SymbolToken getSystemSymbolToken(Marker marker) { if (id == 0) { return _Private_Utils.SYMBOL_0; } - return SystemSymbols_1_1.get((int) id).getToken(); + SystemSymbols_1_1 systemSymbol = SystemSymbols_1_1.get((int) id); + return (systemSymbol == null) ? new SymbolTokenImpl(-1) : systemSymbol.getToken(); } @Override diff --git a/src/main/java/com/amazon/ion/impl/PrivateIonRawWriter_1_1.kt b/src/main/java/com/amazon/ion/impl/PrivateIonRawWriter_1_1.kt index 37a2c159d..ae91af202 100644 --- a/src/main/java/com/amazon/ion/impl/PrivateIonRawWriter_1_1.kt +++ b/src/main/java/com/amazon/ion/impl/PrivateIonRawWriter_1_1.kt @@ -27,6 +27,11 @@ internal interface PrivateIonRawWriter_1_1 : IonRawWriter_1_1 { */ fun stepInTdlMacroInvocation(macroRef: String) + /** + * Steps into a TDL System Macro Invocation—an s-expression, followed by `.` and then the qualified macro name + */ + fun stepInTdlSystemMacroInvocation(systemSymbol: SystemSymbols_1_1) + /** * Steps in s-expression, writes `%` symbol, variable name, and then closes the s-expression. */ diff --git a/src/main/java/com/amazon/ion/impl/SystemSymbols_1_1.kt b/src/main/java/com/amazon/ion/impl/SystemSymbols_1_1.kt index 61478e03b..3d07e5417 100644 --- a/src/main/java/com/amazon/ion/impl/SystemSymbols_1_1.kt +++ b/src/main/java/com/amazon/ion/impl/SystemSymbols_1_1.kt @@ -24,9 +24,9 @@ enum class SystemSymbols_1_1(val id: Int, val text: String) { MACRO_TABLE( /* */ 14, "macro_table"), SYMBOL_TABLE( /* */ 15, "symbol_table"), MODULE( /* */ 16, "module"), - RETAIN( /* */ 17, "retain"), + RETAIN( /* */ 17, "retain"), // https://github.com/amazon-ion/ion-docs/issues/345 EXPORT( /* */ 18, "export"), - CATALOG_KEY( /* */ 19, "catalog_key"), + CATALOG_KEY( /* */ 19, "catalog_key"), // https://github.com/amazon-ion/ion-docs/issues/345 IMPORT( /* */ 20, "import"), THE_EMPTY_SYMBOL( /* */ 21, ""), LITERAL( /* */ 22, "literal"), @@ -35,7 +35,7 @@ enum class SystemSymbols_1_1(val id: Int, val text: String) { IF_SINGLE( /* */ 25, "if_single"), IF_MULTI( /* */ 26, "if_multi"), FOR( /* */ 27, "for"), - FAIL( /* */ 28, "fail"), + DEFAULT( /* */ 28, "default"), VALUES( /* */ 29, "values"), ANNOTATE( /* */ 30, "annotate"), MAKE_STRING( /* */ 31, "make_string"), @@ -51,23 +51,28 @@ enum class SystemSymbols_1_1(val id: Int, val text: String) { DELTA( /* */ 41, "delta"), FLATTEN( /* */ 42, "flatten"), SUM( /* */ 43, "sum"), - ADD_SYMBOLS( /* */ 44, "add_symbols"), - ADD_MACROS( /* */ 45, "add_macros"), - COMMENT( /* */ 46, "comment"), - FLEX_SYMBOL( /* */ 47, "flex_symbol"), - FLEX_INT( /* */ 48, "flex_int"), - FLEX_UINT( /* */ 49, "flex_uint"), - UINT8( /* */ 50, "uint8"), - UINT16( /* */ 51, "uint16"), - UINT32( /* */ 52, "uint32"), - UINT64( /* */ 53, "uint64"), - INT8( /* */ 54, "int8"), - INT16( /* */ 55, "int16"), - INT32( /* */ 56, "int32"), - INT64( /* */ 57, "int64"), - FLOAT16( /* */ 58, "float16"), - FLOAT32( /* */ 59, "float32"), - FLOAT64( /* */ 60, "float64"), + SET_SYMBOLS( /* */ 44, "set_symbols"), + ADD_SYMBOLS( /* */ 45, "add_symbols"), + SET_MACROS( /* */ 46, "set_macros"), + ADD_MACROS( /* */ 47, "add_macros"), + USE( /* */ 48, "use"), + META( /* */ 49, "meta"), + FLEX_SYMBOL( /* */ 50, "flex_symbol"), + FLEX_INT( /* */ 51, "flex_int"), + FLEX_UINT( /* */ 52, "flex_uint"), + UINT8( /* */ 53, "uint8"), + UINT16( /* */ 54, "uint16"), + UINT32( /* */ 55, "uint32"), + UINT64( /* */ 56, "uint64"), + INT8( /* */ 57, "int8"), + INT16( /* */ 58, "int16"), + INT32( /* */ 59, "int32"), + INT64( /* */ 60, "int64"), + FLOAT16( /* */ 61, "float16"), + FLOAT32( /* */ 62, "float32"), + FLOAT64( /* */ 63, "float64"), + NONE( /* */ 64, "none"), + MAKE_FIELD( /* */ 65, "make_field"), ; val utf8Bytes = text.encodeToByteArray() @@ -91,6 +96,12 @@ enum class SystemSymbols_1_1(val id: Int, val text: String) { } } + @JvmStatic + private val BY_NAME = ALL_VALUES.fold(HashMap(ALL_VALUES.size)) { map, s -> + check(map.put(s.text, s) == null) { "Duplicate system symbol text: ${s.id}=${s.text}" } + map + } + @JvmStatic fun size() = ALL_VALUES.size @@ -102,19 +113,26 @@ enum class SystemSymbols_1_1(val id: Int, val text: String) { fun allSymbolTexts() = ALL_SYMBOL_TEXTS /** - * Returns true if the [id] is a valid system symbol ID. - */ + * Returns true if the [id] is a valid system symbol ID.*/ @JvmStatic operator fun contains(id: Int): Boolean { return id > 0 && id <= SystemSymbols_1_1.ALL_VALUES.size } /** - * Returns the text of the given system symbol ID, or null if not a valid system symbol ID. - */ + * Returns the system symbol corresponding to the given system symbol ID, + * or `null` if not a valid system symbol ID.*/ + @JvmStatic + operator fun get(id: Int): SystemSymbols_1_1? { + return if (contains(id)) { SystemSymbols_1_1.ALL_VALUES[id - 1] } else { null } + } + + /** + * Returns the system symbol corresponding to the given system symbol text, + * or `null` if not a valid system symbol text.*/ @JvmStatic - operator fun get(id: Int): SystemSymbols_1_1 { - return SystemSymbols_1_1.ALL_VALUES[id - 1] + operator fun get(text: String): SystemSymbols_1_1? { + return BY_NAME[text] } } } diff --git a/src/main/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1.kt b/src/main/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1.kt index 5b9a9df75..0da94f06e 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1.kt +++ b/src/main/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1.kt @@ -163,7 +163,7 @@ internal class IonManagedWriter_1_1( */ private fun addMacroDependencies(macro: Macro) { macro.dependencies.forEach { - if (it !in macroTable && it !in newMacros) { + if (it !is SystemMacro && it !in macroTable && it !in newMacros) { addMacroDependencies(it) assignMacroAddress(it) } @@ -351,7 +351,12 @@ internal class IonManagedWriter_1_1( val name = macroNames[address] when (macro) { is TemplateMacro -> writeMacroDefinition(name, macro) - is SystemMacro -> exportSystemMacro(macro, name) + is SystemMacro -> { + if (name != macro.macroName) { + exportSystemMacro(macro, name) + } + // Else, no need to export the macro since it's already known by the desired name + } } } forceNoNewlines(true) @@ -479,8 +484,12 @@ internal class IonManagedWriter_1_1( numberOfTimesToStepOut[expression.endExclusive]++ } is Expression.MacroInvocation -> { - val invokedAddress = macroTable[expression.macro] - ?: newMacros[expression.macro] + val invokedMacro = expression.macro + if (invokedMacro is SystemMacro) { + stepInTdlSystemMacroInvocation(invokedMacro.systemSymbol) + } + val invokedAddress = macroTable[invokedMacro] + ?: newMacros[invokedMacro] ?: throw IllegalStateException("A macro in the macro table is missing a dependency") val invokedName = macroNames[invokedAddress] if (options.invokeTdlMacrosByName && invokedName != null) { @@ -843,23 +852,7 @@ internal class IonManagedWriter_1_1( } } - private fun startSystemMacro(macro: SystemMacro) { - var macroName = macro.macroName - // TODO: Replace `indexOf` with something that is not O(n) time complexity. - var id = macroNames.indexOf(macroName) - if (id < 0) { - // If the name is not in use, put it into the user macro table - id = getOrAssignMacroAddressAndName(macroName, macro) - } - if (macrosById[id] == macro) { - // The name and id in the local table refers to the system macro we want to invoke, - // so invoke as a local symbol since it's almost always shorter. - startMacro(macroName, id, macro) - } else { - // The name is already being used by something else, so invoke using the system macro syntax. - userData.stepInEExp(macro) - } - } + private fun startSystemMacro(macro: SystemMacro) = userData.stepInEExp(macro) override fun startExpressionGroup() { userData.stepInExpressionGroup(options.writeLengthPrefix(ContainerType.EXPRESSION_GROUP, depth + 1)) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt index 04d56008c..5dd0b0672 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt @@ -960,6 +960,13 @@ class IonRawBinaryWriter_1_1 internal constructor( writeSymbol(macroRef) } + override fun stepInTdlSystemMacroInvocation(systemSymbol: SystemSymbols_1_1) { + stepInSExp(usingLengthPrefix = false) + writeSymbol(".") + writeAnnotations(SystemSymbols_1_1.ION) + writeSymbol(systemSymbol) + } + override fun writeTdlVariableExpansion(variableName: String) { stepInSExp(usingLengthPrefix = false) writeSymbol("%") diff --git a/src/main/java/com/amazon/ion/impl/macro/MacroCompiler.kt b/src/main/java/com/amazon/ion/impl/macro/MacroCompiler.kt index becd56612..d32089bc8 100644 --- a/src/main/java/com/amazon/ion/impl/macro/MacroCompiler.kt +++ b/src/main/java/com/amazon/ion/impl/macro/MacroCompiler.kt @@ -6,7 +6,7 @@ import com.amazon.ion.* import com.amazon.ion.impl.* import com.amazon.ion.impl.bin.Ion_1_1_Constants.* import com.amazon.ion.impl.macro.Expression.* -import com.amazon.ion.util.confirm +import com.amazon.ion.util.* /** * [MacroCompiler] wraps an Ion reader. When directed to do so, it will take over advancing and getting values from the @@ -40,6 +40,7 @@ internal class MacroCompiler( confirm(reader.encodingType() == IonType.SYMBOL && reader.stringValue() == "macro") { "macro compilation expects a sexp starting with the keyword `macro`" } reader.nextAndCheckType(IonType.SYMBOL, IonType.NULL, "macro name") + reader.confirmNoAnnotations("macro name") if (reader.encodingType() != IonType.NULL) { macroName = reader.stringValue().also { confirm(isIdentifierSymbol(it)) { "invalid macro name: '$it'" } } @@ -239,6 +240,10 @@ internal class MacroCompiler( * Must be positioned on the (expected) macro reference. */ private fun ReaderAdapter.readMacroReference(): Macro { + + val annotations = getTypeAnnotationSymbols() + val isSystemMacro = annotations.size == 1 && SystemSymbols_1_1.ION.text == annotations[0].getText() + val macroRef = when (encodingType()) { IonType.SYMBOL -> { val macroName = stringValue() @@ -246,10 +251,15 @@ internal class MacroCompiler( MacroRef.ByName(macroName) } - IonType.INT -> MacroRef.ById(intValue()) + IonType.INT -> { + val sid = intValue() + if (sid < 0) throw IonException("Macro ID must be non-negative: $sid") + MacroRef.ById(intValue()) + } else -> throw IonException("macro invocation must start with an id (int) or identifier (symbol); found ${encodingType() ?: "nothing"}\"") } - return getMacro(macroRef) ?: throw IonException("Unrecognized macro: $macroRef") + val m = if (isSystemMacro) SystemMacro.get(macroRef) else getMacro(macroRef) + return m ?: throw IonException("Unrecognized macro: $macroRef") } private fun ReaderAdapter.compileVariableExpansion(placeholderIndex: Int) { diff --git a/src/main/java/com/amazon/ion/impl/macro/SystemMacro.kt b/src/main/java/com/amazon/ion/impl/macro/SystemMacro.kt index 3af9130be..e43f37fdb 100644 --- a/src/main/java/com/amazon/ion/impl/macro/SystemMacro.kt +++ b/src/main/java/com/amazon/ion/impl/macro/SystemMacro.kt @@ -14,22 +14,27 @@ import com.amazon.ion.impl.macro.ParameterFactory.zeroToManyTagged /** * Macros that are built in, rather than being defined by a template. */ -enum class SystemMacro(val id: Byte, val macroName: String, override val signature: List, override val body: List? = null) : Macro { +enum class SystemMacro( + val id: Byte, + val systemSymbol: SystemSymbols_1_1, + override val signature: List, + override val body: List? = null +) : Macro { // Technically not system macros, but special forms. However, it's easier to model them as if they are macros in TDL. // We give them an ID of -1 to distinguish that they are not addressable outside TDL. - IfNone(-1, "if_none", listOf(zeroToManyTagged("stream"), zeroToManyTagged("true_branch"), zeroToManyTagged("false_branch"))), - IfSome(-1, "if_some", listOf(zeroToManyTagged("stream"), zeroToManyTagged("true_branch"), zeroToManyTagged("false_branch"))), - IfSingle(-1, "if_single", listOf(zeroToManyTagged("stream"), zeroToManyTagged("true_branch"), zeroToManyTagged("false_branch"))), - IfMulti(-1, "if_multi", listOf(zeroToManyTagged("stream"), zeroToManyTagged("true_branch"), zeroToManyTagged("false_branch"))), + IfNone(-1, IF_NONE, listOf(zeroToManyTagged("stream"), zeroToManyTagged("true_branch"), zeroToManyTagged("false_branch"))), + IfSome(-1, IF_SOME, listOf(zeroToManyTagged("stream"), zeroToManyTagged("true_branch"), zeroToManyTagged("false_branch"))), + IfSingle(-1, IF_SINGLE, listOf(zeroToManyTagged("stream"), zeroToManyTagged("true_branch"), zeroToManyTagged("false_branch"))), + IfMulti(-1, IF_MULTI, listOf(zeroToManyTagged("stream"), zeroToManyTagged("true_branch"), zeroToManyTagged("false_branch"))), // The real macros - None(0, "none", emptyList()), - Values(1, "values", listOf(zeroToManyTagged("values"))), - Annotate(2, "annotate", listOf(zeroToManyTagged("ann"), exactlyOneTagged("value"))), - MakeString(3, "make_string", listOf(zeroToManyTagged("text"))), - MakeSymbol(4, "make_symbol", listOf(zeroToManyTagged("text"))), - MakeBlob(5, "make_blob", listOf(zeroToManyTagged("bytes"))), - MakeDecimal(6, "make_decimal", listOf(exactlyOneFlexInt("coefficient"), exactlyOneFlexInt("exponent"))), + None(0, NONE, emptyList()), + Values(1, VALUES, listOf(zeroToManyTagged("values"))), + Annotate(2, ANNOTATE, listOf(zeroToManyTagged("ann"), exactlyOneTagged("value"))), + MakeString(3, MAKE_STRING, listOf(zeroToManyTagged("text"))), + MakeSymbol(4, MAKE_SYMBOL, listOf(zeroToManyTagged("text"))), + MakeBlob(5, MAKE_BLOB, listOf(zeroToManyTagged("bytes"))), + MakeDecimal(6, MAKE_DECIMAL, listOf(exactlyOneFlexInt("coefficient"), exactlyOneFlexInt("exponent"))), /** * ```ion @@ -41,7 +46,7 @@ enum class SystemMacro(val id: Byte, val macroName: String, override val signatu * ``` */ SetSymbols( - 11, "set_symbols", listOf(zeroToManyTagged("symbols")), + 11, SET_SYMBOLS, listOf(zeroToManyTagged("symbols")), templateBody { annotated(ION_ENCODING, ::sexp) { sexp { @@ -66,7 +71,7 @@ enum class SystemMacro(val id: Byte, val macroName: String, override val signatu * ``` */ AddSymbols( - 12, "add_symbols", listOf(zeroToManyTagged("symbols")), + 12, ADD_SYMBOLS, listOf(zeroToManyTagged("symbols")), templateBody { annotated(ION_ENCODING, ::sexp) { sexp { @@ -92,7 +97,7 @@ enum class SystemMacro(val id: Byte, val macroName: String, override val signatu * ``` */ SetMacros( - 13, "set_macros", listOf(zeroToManyTagged("macros")), + 13, SET_MACROS, listOf(zeroToManyTagged("macros")), templateBody { annotated(ION_ENCODING, ::sexp) { sexp { @@ -117,7 +122,7 @@ enum class SystemMacro(val id: Byte, val macroName: String, override val signatu * ``` */ AddMacros( - 14, "add_macros", listOf(zeroToManyTagged("macros")), + 14, ADD_MACROS, listOf(zeroToManyTagged("macros")), templateBody { annotated(ION_ENCODING, ::sexp) { sexp { @@ -144,7 +149,7 @@ enum class SystemMacro(val id: Byte, val macroName: String, override val signatu * ``` */ Use( - 15, "use", listOf(exactlyOneTagged("catalog_key"), zeroOrOneTagged("version")), + 15, USE, listOf(exactlyOneTagged("catalog_key"), zeroOrOneTagged("version")), templateBody { val theModule = _Private_Utils.newSymbolToken("the_module") annotated(ION_ENCODING, ::sexp) { @@ -172,19 +177,32 @@ enum class SystemMacro(val id: Byte, val macroName: String, override val signatu } ), - Repeat(17, "repeat", listOf(exactlyOneTagged("n"), oneToManyTagged("value"))), + Repeat(17, REPEAT, listOf(exactlyOneTagged("n"), oneToManyTagged("value"))), - Comment(21, "comment", listOf(zeroToManyTagged("values")), templateBody { macro(None) {} }), + Comment(21, META, listOf(zeroToManyTagged("values")), templateBody { macro(None) {} }), MakeField( - 22, "make_field", + 22, MAKE_FIELD, listOf( Macro.Parameter("field_name", Macro.ParameterEncoding.CompactSymbol, Macro.ParameterCardinality.ExactlyOne), exactlyOneTagged("value") ) ), + Default( + 23, DEFAULT, listOf(zeroToManyTagged("expr"), zeroToManyTagged("default_expr")), + templateBody { + macro(IfNone) { + variable(0) + variable(1) + variable(0) + } + } + ), + // TODO: Other system macros ; + val macroName: String get() = this.systemSymbol.text + override val dependencies: List get() = body ?.filterIsInstance() diff --git a/src/test/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1_Test.kt b/src/test/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1_Test.kt index d98db272e..e8e29185a 100644 --- a/src/test/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1_Test.kt +++ b/src/test/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1_Test.kt @@ -273,36 +273,6 @@ internal class IonManagedWriter_1_1_Test { assertEquals(expected, actual) } - @Test - fun `when a system macro is invoked, it should be added to the user table if there is no name conflict`() { - val expected = """ - $ion_1_1 - $ion_encoding::((macro_table (export $ion::make_string))) - (:make_string (:: "a" b)) - """.trimIndent() - - var actual = write { - startMacro(SystemMacro.MakeString) - startExpressionGroup() - writeString("a") - writeSymbol("b") - endExpressionGroup() - endMacro() - } - assertEquals(expected, actual) - - // And again, but with using the function that accepts the name of the macro - actual = write { - startMacro("make_string", SystemMacro.MakeString) - startExpressionGroup() - writeString("a") - writeSymbol("b") - endExpressionGroup() - endMacro() - } - assertEquals(expected, actual) - } - @Test fun `it is possible to invoke a system macro using an alias`() { val expected = """