Various changes in support of system macros #981

jobarr-amzn · 2024-10-29T15:00:23Z

Issue #, if available: NA

Description of changes:

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

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* 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

codecov · 2024-10-29T15:06:05Z

Codecov Report

Attention: Patch coverage is 72.91667% with 26 lines in your changes missing coverage. Please review.

Please upload report for BASE (ion-11-encoding@3f92b94). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...n/java/com/amazon/ion/impl/IonRawTextWriter_1_1.kt	0.00%	6 Missing ⚠️
...va/com/amazon/ion/impl/bin/IonManagedWriter_1_1.kt	40.00%	1 Missing and 5 partials ⚠️
.../com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt	0.00%	5 Missing ⚠️
...in/java/com/amazon/ion/impl/macro/MacroCompiler.kt	42.85%	0 Missing and 4 partials ⚠️
...main/java/com/amazon/ion/impl/SystemSymbols_1_1.kt	90.00%	1 Missing and 2 partials ⚠️
...mazon/ion/impl/IonReaderContinuableCoreBinary.java	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##             ion-11-encoding     #981   +/-   ##
==================================================
  Coverage                   ?   70.56%           
  Complexity                 ?     7083           
==================================================
  Files                      ?      201           
  Lines                      ?    28198           
  Branches                   ?     5037           
==================================================
  Hits                       ?    19899           
  Misses                     ?     6710           
  Partials                   ?     1589

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jobarr-amzn · 2024-10-29T15:04:54Z

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

-        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();


🗺️ This needs attention- is this the right thing to do? Formerly, SystemSymbols_1_1.get(int) was documented to return null if the supplied int did not correspond to a system symbol, but in practice it would throw an ArrayIndexOutOfBoundsException.

I added a method to lookup a system symbol by text, where ArrayIndexOutOfBoundsException does not make sense for an unmapped value but yielding null does, so I brought both lookups to the same contract.

Modifying the method to match the documentation exposed the nullability of the return value in a way that triggered ktlint to force me to handle the null case here... is this a sensible response? Should I return null instead? Throw an exception?

It looks to me like throwing is the right thing here. This is only called in places where the encoding indicates a system symbol is present. It would be invalid Ion for that encoding to occur without a valid system symbol mapping.

We probably need to throw an IonException here because if we have something that claims to be a system symbol, but the ID is out of range for the system symbol table, it's invalid Ion data.

jobarr-amzn · 2024-10-29T15:07:02Z

src/main/java/com/amazon/ion/impl/SystemSymbols_1_1.kt

+        @JvmStatic
+        private val BY_NAME = ALL_VALUES.fold(HashMap<String, SystemSymbols_1_1>(ALL_VALUES.size)) { map, s ->
+            check(map.put(s.text, s) == null) { "Duplicate system symbol text: ${s.id}=${s.text}" }
+            map
+        }


🗺️ I added this validation after duplicating system symbol text with a GitHub issue URL caused a small number of tests in EncodingDirectiveCompilationTest to fail in a way that took me a few minutes to track down.

jobarr-amzn · 2024-10-29T15:19:26Z

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

-                    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
+                    }


🗺️ We still don't have support for reading/compiling exported symbols, but that will come later.

jobarr-amzn · 2024-10-29T15:21:50Z

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

-    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)


🗺️ This removes the behavior when a system macro is invoked, it should be added to the user table if there is no name conflict, matching test removed below. We can restore this behavior at need, but it's not certain that the user wants to add every invoked system macro to their macro table.

tgregg · 2024-10-29T17:31:02Z

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

-        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();


It looks to me like throwing is the right thing here. This is only called in places where the encoding indicates a system symbol is present. It would be invalid Ion for that encoding to occur without a valid system symbol mapping.

popematt

Looks pretty good. Thanks!

popematt · 2024-10-29T17:43:32Z

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

-        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();


We probably need to throw an IonException here because if we have something that claims to be a system symbol, but the ID is out of range for the system symbol table, it's invalid Ion data.

src/main/java/com/amazon/ion/impl/SystemSymbols_1_1.kt

Co-authored-by: Matthew Pope <[email protected]>

jobarr-amzn commented Oct 29, 2024

View reviewed changes

jobarr-amzn requested a review from popematt October 29, 2024 15:28

tgregg approved these changes Oct 29, 2024

View reviewed changes

popematt reviewed Oct 29, 2024

View reviewed changes

jobarr-amzn and others added 2 commits October 29, 2024 13:24

Update src/main/java/com/amazon/ion/impl/SystemSymbols_1_1.kt

830d0fe

Co-authored-by: Matthew Pope <[email protected]>

Throw IonException on unrecognized system symbol

140a83e

jobarr-amzn force-pushed the ion-11-system-symbols-and-macros branch from 917bf0a to 140a83e Compare October 29, 2024 21:10

jobarr-amzn merged commit 383066b into amazon-ion:ion-11-encoding Oct 29, 2024
21 of 36 checks passed

jobarr-amzn deleted the ion-11-system-symbols-and-macros branch October 29, 2024 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various changes in support of system macros #981

Various changes in support of system macros #981

jobarr-amzn commented Oct 29, 2024

codecov bot commented Oct 29, 2024 •

edited

Loading

jobarr-amzn Oct 29, 2024

tgregg Oct 29, 2024

popematt Oct 29, 2024

jobarr-amzn Oct 29, 2024

jobarr-amzn Oct 29, 2024

jobarr-amzn Oct 29, 2024

tgregg Oct 29, 2024

popematt left a comment

popematt Oct 29, 2024

Various changes in support of system macros #981

Various changes in support of system macros #981

Conversation

jobarr-amzn commented Oct 29, 2024

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popematt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2024 •

edited

Loading