Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Includes various small fixes for tagless parameters, expression groups, and system macro invocations. #975

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Includes various small fixes for tagless parameters, expression groups, and system macro invocations. #975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/main/java/com/amazon/ion/impl/IonCursorBinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3361,6 +3361,10 @@ private void setCheckpointAfterValueHeader() {
case START_CONTAINER:
setCheckpoint(CheckpointLocation.AFTER_CONTAINER_HEADER);
break;
case NEEDS_INSTRUCTION:
// A macro invocation header has just been read.
setCheckpoint(CheckpointLocation.BEFORE_UNANNOTATED_TYPE_ID);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This moves the cursor past the macro invocation header, positioning it on the first argument (if any). This method is called when parsing expression groups.

break;
default:
throw new IllegalStateException();
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1449,7 +1449,7 @@ private void readGroupExpression(Macro.Parameter parameter, List<Expression.EExp
int startIndex = expressions.size();
expressions.add(Expression.Placeholder.INSTANCE);
boolean isSingleton = true;
while (nextGroupedValue() != Event.NEEDS_INSTRUCTION) {
while (nextGroupedValue() != Event.NEEDS_INSTRUCTION || isMacroInvocation()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NEEDS_INSTRUCTION is used either when the end of the group has been reached, or when a macro invocation header has been encountered. When it's the latter, we need to read the invocation as an expression to continue progressing through the group.

readValueAsExpression(false, expressions);
isSingleton = false;
}
Expand Down Expand Up @@ -1560,7 +1560,7 @@ protected PresenceBitmap loadPresenceBitmapIfNecessary(List<Macro.Parameter> sig

@Override
protected boolean isMacroInvocation() {
return valueTid.isMacroInvocation;
return valueTid != null && valueTid.isMacroInvocation;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is now called when the cursor may be at the end of the group, at which point valueTid will be null.

}

@Override
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/com/amazon/ion/impl/IonReaderTextSystemX.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,6 +1211,8 @@ protected Macro loadMacro() {
if (IonReaderTextSystemX.this.nextRaw() == null) {
throw new IonException("Macro invocation missing address.");
}
List<SymbolToken> annotations = getAnnotations();
boolean isSystemMacro = !annotations.isEmpty() && SystemSymbols_1_1.ION.getText().equals(annotations.get(0).getText());
Comment on lines +1214 to +1215
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added macroInvocationInTaggedExpressionGroup test exercises this because it invokes the values macro from a context that does not export the Ion 1.1 system macros. The invocation looks like (:$ion::values ...)

MacroRef address;
if (_value_type == IonType.SYMBOL) {
String name = stringValue();
Expand All @@ -1228,7 +1230,7 @@ protected Macro loadMacro() {
throw new IonException("E-expressions must begin with an address.");
}
Macro macro;
if (encodingContext == null) {
if (encodingContext == null || isSystemMacro) {
macro = SystemMacro.get(address);
if (macro == null) {
throw new IonException(String.format("Encountered an unknown macro address: %s.", address));
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,12 +859,12 @@ class IonRawBinaryWriter_1_1 internal constructor(
thisContainerTotalLength++
} else if (isTagless) {
// End tagless group -- write the number of expressions, end with FlexUInt 0
thisContainerTotalLength += writeCurrentContainerLength(lengthPrefixPreallocation)
thisContainerTotalLength += writeCurrentContainerLength(maxOf(1, lengthPrefixPreallocation))
Copy link
Contributor Author

@tgregg tgregg Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now symmetrical with stepInExpressionGroup for this case, which always reserves at least one byte for the length. Without this fix, the byte is reserved in the buffer and a patch point is added, resulting in an erroneous extra byte in the final output. The same can be said for the other change in this file.

This change is verified by the added taglessExpressionGroup test.

buffer.writeByte(FlexInt.ZERO)
thisContainerTotalLength++
} else if (currentContainer.isLengthPrefixed) {
// Length-prefixed, tagged -- write the number of bytes
thisContainerTotalLength += writeCurrentContainerLength(lengthPrefixPreallocation)
thisContainerTotalLength += writeCurrentContainerLength(maxOf(1, lengthPrefixPreallocation))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exercised by the added macroInvocationInTaggedExpressionGroup test.

} else {
// Delimited, tagged -- start with `01` end with `F0`
buffer.writeByte(OpCodes.DELIMITED_END_MARKER)
Expand Down
17 changes: 15 additions & 2 deletions src/main/java/com/amazon/ion/impl/macro/MacroCompiler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,23 @@ internal class MacroCompiler(

// Read the next parameter name
val annotations = getTypeAnnotationSymbols()
confirm(annotations.isEmptyOr(Macro.ParameterEncoding.Tagged.ionTextName)) { "unsupported parameter encoding $annotations" }
val parameterEncoding = when (annotations.size) {
0 -> Macro.ParameterEncoding.Tagged
1 -> {
val encodingText = annotations[0].text
val encoding = Macro.ParameterEncoding.entries.singleOrNull { it.ionTextName == encodingText }
if (encoding == null) {
// TODO: Check for macro-shaped parameter encodings, and only if it's still null, we throw.
throw IonException("unsupported parameter encoding $annotations")
}
encoding
}
2 -> TODO("Qualified references for macro-shaped parameters")
else -> throw IonException("unsupported parameter encoding $annotations")
}
Comment on lines +88 to +101
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch is contributed by @popematt, and is exercised by the added taglessExpressionGroup test.

confirm(isIdentifierSymbol(symbolText)) { "invalid parameter name: '$symbolText'" }
confirm(signature.none { it.variableName == symbolText }) { "redeclaration of parameter '$symbolText'" }
pendingParameter = Macro.Parameter(symbolText, Macro.ParameterEncoding.Tagged, Macro.ParameterCardinality.ExactlyOne)
pendingParameter = Macro.Parameter(symbolText, parameterEncoding, Macro.ParameterCardinality.ExactlyOne)
}
// If we have a pending parameter than hasn't been added to the signature, add it here.
if (pendingParameter != null) signature.add(pendingParameter!!)
Expand Down
111 changes: 110 additions & 1 deletion src/test/java/com/amazon/ion/impl/EncodingDirectiveCompilationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@

import com.amazon.ion.FakeSymbolToken;
import com.amazon.ion.IntegerSize;
import com.amazon.ion.IonDatagram;
import com.amazon.ion.IonEncodingVersion;
import com.amazon.ion.IonLoader;
import com.amazon.ion.IonReader;
import com.amazon.ion.IonText;
import com.amazon.ion.IonType;
import com.amazon.ion.SystemSymbols;
import com.amazon.ion.impl.bin.IonRawBinaryWriter_1_1;
Expand All @@ -14,8 +17,10 @@
import com.amazon.ion.impl.macro.Macro;
import com.amazon.ion.impl.macro.MacroRef;
import com.amazon.ion.impl.macro.ParameterFactory;
import com.amazon.ion.impl.macro.SystemMacro;
import com.amazon.ion.impl.macro.TemplateMacro;
import com.amazon.ion.system.IonReaderBuilder;
import com.amazon.ion.system.IonSystemBuilder;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
Expand Down Expand Up @@ -136,6 +141,30 @@ private static void writeMacroSignature(IonRawWriter_1_1 writer, Map<String, Int
writer.stepOut();
}

private static void writeMacroSignatureFromDatagram(IonRawWriter_1_1 writer, Map<String, Integer> symbols, IonDatagram... signature) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows us to specify tagless encodings. The other variant just takes a String, without a way to add an encoding annotation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fascinating use of IonDatagram for "unmodeled sequence of values". I did a double-take.

writer.stepInSExp(false);
for (IonDatagram parameter : signature) {
if (parameter.size() > 2) {
throw new IllegalStateException("Parameters can only have two components: a name and a cardinality.");
}
Comment on lines +147 to +149
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (parameter.size() > 2) {
throw new IllegalStateException("Parameters can only have two components: a name and a cardinality.");
}
if (parameter.size() > 2) {
throw new IllegalStateException("Parameters can only have two components: a name and a cardinality.");
}
if (parameter.size() < 1) {
throw new IllegalStateException("Parameters must have at least one component: a name.");
}

IonText name = (IonText) parameter.get(0);
String[] encoding = name.getTypeAnnotations();
if (encoding.length == 1) {
// The encoding, e.g. uint8
writer.writeAnnotations(encoding);
} else if (encoding.length > 1) {
throw new IllegalStateException("Only one encoding annotation is allowed.");
}
// The name
writeSymbol(writer, symbols, name.stringValue());
if (parameter.size() == 2) {
// The cardinality, e.g. *
writeSymbol(writer, symbols, ((IonText) parameter.get(1)).stringValue());
}
}
writer.stepOut();
}

private static void writeVariableExpansion(IonRawWriter_1_1 writer, Map<String, Integer> symbols, String variableName) {
writer.stepInSExp(false);
writer.writeSymbol("%");
Expand Down Expand Up @@ -1031,8 +1060,88 @@ public void blobsAndClobs(InputType inputType, StreamType streamType) throws Exc
}
}

@ParameterizedTest(name = "{0},{1}")
@MethodSource("allCombinations")
public void macroInvocationInTaggedExpressionGroup(InputType inputType, StreamType streamType) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
IonRawWriter_1_1 writer = streamType.newWriter(out);

writer.writeIVM();
Map<String, Integer> symbols = initializeSymbolTable(writer, "foo", "value");
startEncodingDirective(writer);
startMacroTable(writer);
startMacro(writer, symbols, "foo");
writeMacroSignature(writer, symbols, "value", "*");
writeVariableExpansion(writer, symbols, "value");
endMacro(writer);
endMacroTable(writer);
endEncodingDirective(writer);

Macro expectedMacro = new TemplateMacro(
Collections.singletonList(new Macro.Parameter("value", Macro.ParameterEncoding.Tagged, Macro.ParameterCardinality.ZeroOrMore)),
Collections.singletonList(new Expression.VariableRef(0))
);

writer.stepInEExp(0, false, expectedMacro); {
writer.stepInExpressionGroup(true); {
writer.stepInEExp(SystemMacro.Values); {
writer.writeString("bar");
} writer.stepOut();
} writer.stepOut();
} writer.stepOut();
Comment on lines +1085 to +1091
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you're also using the stepIn/Out idiom from old IonJava tests :D


byte[] data = getBytes(writer, out);

try (IonReader reader = inputType.newReader(data)) {
assertEquals(IonType.STRING, reader.next());
assertEquals("bar", reader.stringValue());
assertNull(reader.next());
}
}

private static final IonLoader LOADER = IonSystemBuilder.standard().build().getLoader();

@ParameterizedTest(name = "{0},{1}")
@MethodSource("allCombinations")
public void taglessExpressionGroup(InputType inputType, StreamType streamType) throws Exception {
ByteArrayOutputStream out = new ByteArrayOutputStream();
IonRawWriter_1_1 writer = streamType.newWriter(out);

writer.writeIVM();
Map<String, Integer> symbols = initializeSymbolTable(writer, "foo", "value");
startEncodingDirective(writer);
startMacroTable(writer);
startMacro(writer, symbols, "foo");
writeMacroSignatureFromDatagram(writer, symbols, LOADER.load("uint8::value '*'"));
writeVariableExpansion(writer, symbols, "value");
endMacro(writer);
endMacroTable(writer);
endEncodingDirective(writer);

Macro expectedMacro = new TemplateMacro(
Collections.singletonList(new Macro.Parameter("value", Macro.ParameterEncoding.Uint8, Macro.ParameterCardinality.ZeroOrMore)),
Collections.singletonList(new Expression.VariableRef(0))
);

writer.stepInEExp(0, false, expectedMacro); {
writer.stepInExpressionGroup(true); {
writer.writeInt(1);
writer.writeInt(2);
} writer.stepOut();
} writer.stepOut();

byte[] data = getBytes(writer, out);

try (IonReader reader = inputType.newReader(data)) {
assertEquals(IonType.INT, reader.next());
assertEquals(1, reader.intValue());
assertEquals(IonType.INT, reader.next());
assertEquals(2, reader.intValue());
assertNull(reader.next());
}
}

// TODO cover every Ion type
// TODO tagless values and tagless argument groups
// TODO annotations in macro definition (using 'annotate' system macro)
// TODO macro invocation that expands to a system value
// TODO test error conditions
Expand Down
Loading