From d84108e3f6e3d02f85b27b7b891fdc643df30178 Mon Sep 17 00:00:00 2001 From: NebelNidas <48808497+NebelNidas@users.noreply.github.com> Date: Mon, 26 Aug 2024 15:28:06 +0200 Subject: [PATCH] Make `ColumnFileReader` consume trailing column separators instead of leading ones (#108) This restores the pre-https://github.com/FabricMC/mapping-io/commit/36c52a537a06b812b8855c6d5cf176dd58c14540 behavior, fixing `\tc` (a comment) sometimes being interpreted as `c` (a class) in Tiny v2. --- .../mappingio/format/ColumnFileReader.java | 67 ++++++++++++------- .../net/fabricmc/mappingio/TestHelper.java | 1 + .../mappingio/read/ValidContentReadTest.java | 41 ++++++++---- .../valid/enigma-dir/class1Ns0Rename.mapping | 1 + src/test/resources/read/valid/enigma.mappings | 1 + src/test/resources/read/valid/tinyV2.tiny | 1 + 6 files changed, 73 insertions(+), 39 deletions(-) diff --git a/src/main/java/net/fabricmc/mappingio/format/ColumnFileReader.java b/src/main/java/net/fabricmc/mappingio/format/ColumnFileReader.java index 4d26aa0b..f6e009aa 100644 --- a/src/main/java/net/fabricmc/mappingio/format/ColumnFileReader.java +++ b/src/main/java/net/fabricmc/mappingio/format/ColumnFileReader.java @@ -32,6 +32,11 @@ @ApiStatus.Internal public final class ColumnFileReader implements Closeable { public ColumnFileReader(Reader reader, char indentationChar, char columnSeparator) { + assert indentationChar != '\r'; + assert indentationChar != '\n'; + assert columnSeparator != '\r'; + assert columnSeparator != '\n'; + this.reader = reader; this.indentationChar = indentationChar; this.columnSeparator = columnSeparator; @@ -51,7 +56,7 @@ public void close() throws IOException { * @return {@code true} if the column was read and had the expected content, {@code false} otherwise. */ public boolean nextCol(String expected) throws IOException { - return read(false, false, true, expected) != noMatch; + return read(false, false, true, expected) != NO_MATCH; } /** @@ -89,20 +94,20 @@ public String peekCol(boolean unescape) throws IOException { * @param unescape Whether to unescape the read string. * @param consume Whether to advance the bufferPos. * @param stopAtNextCol Whether to only read one column. - * @param expected If not {@code null}, the read string must match this exactly, otherwise we early-exit with {@link #noMatch}. Always consumes if matched. + * @param expected If not {@code null}, the read string must match this exactly, otherwise we early-exit with {@link #NO_MATCH}. Always consumes if matched. * * @return {@code null} if nothing has been read (first char was EOL), otherwise the read string (may be empty). - * If {@code expected} is not {@code null}, it will be returned if matched, otherwise {@link #noMatch}. + * If {@code expected} is not {@code null}, it will be returned if matched, otherwise {@link #NO_MATCH}. */ @Nullable private String read(boolean unescape, boolean consume, boolean stopAtNextCol, @Nullable String expected) throws IOException { - if (eol) return expected == null ? null : noMatch; + if (eol) return expected == null ? null : NO_MATCH; int expectedLength = expected != null ? expected.length() : -1; // Check if the buffer needs to be filled and if we hit EOF while doing so if (expectedLength > 0 && bufferPos + expectedLength >= bufferLimit) { - if (!fillBuffer(expectedLength, !consume, false)) return noMatch; + if (!fillBuffer(expectedLength, !consume, false)) return NO_MATCH; } int start; @@ -110,31 +115,24 @@ private String read(boolean unescape, boolean consume, boolean stopAtNextCol, @N int firstEscaped = -1; int contentCharsRead = 0; int modifiedBufferPos = -1; - int startOffset = 0; boolean readAnything = false; boolean filled = true; + boolean isColumnSeparator = false; readLoop: for (;;) { while (end < bufferLimit) { char c = buffer[end]; - boolean isColumnSeparator = (c == columnSeparator); - - // skip leading column separator - if (isColumnSeparator && !readAnything) { - startOffset = 1; - contentCharsRead = -1; - } - + isColumnSeparator = (c == columnSeparator); readAnything = true; - if (expected != null && contentCharsRead > -1) { + if (expected != null) { if ((contentCharsRead < expectedLength && c != expected.charAt(contentCharsRead)) || contentCharsRead > expectedLength) { - return noMatch; + return NO_MATCH; } } - if (c == '\n' || c == '\r' || (isColumnSeparator && stopAtNextCol && contentCharsRead > -1)) { // stop reading + if (c == '\n' || c == '\r' || (isColumnSeparator && stopAtNextCol)) { // stop reading start = bufferPos; modifiedBufferPos = end; @@ -166,28 +164,36 @@ private String read(boolean unescape, boolean consume, boolean stopAtNextCol, @N } } - start += startOffset; String ret; if (expected != null) { consume = true; ret = expected; } else { - int len = end - start; + int contentLength = end - start; - if (len == 0) { + if (contentLength == 0) { ret = readAnything ? "" : null; } else if (firstEscaped >= 0) { - ret = Tiny2Util.unescape(String.valueOf(buffer, start, len)); + ret = Tiny2Util.unescape(String.valueOf(buffer, start, contentLength)); } else { - ret = String.valueOf(buffer, start, len); + ret = String.valueOf(buffer, start, contentLength); } } if (consume) { if (readAnything) bof = false; + + if (modifiedBufferPos != -1) { + bufferPos = modifiedBufferPos; + + // consume trailing column separator if present + if (isColumnSeparator && filled && fillBuffer(1, false, false)) { + bufferPos++; + } + } + if (!filled) eof = eol = true; - if (modifiedBufferPos != -1) bufferPos = modifiedBufferPos; if (eol && !eof) { // manually check for EOF int charsToRead = buffer[bufferPos] == '\r' ? 2 : 1; // 2 for \r\n, 1 for just \n @@ -237,6 +243,15 @@ public int nextIntCol() throws IOException { } } + /** + * Read and consume until the the start of the next line is reached, and return whether the + * following {@code indent} characters match {@link #indentationChar}. + * + *

Empty lines are skipped if {@code indent} is 0. + * + * @param indent The number of characters to check for indentation. + * @return {@code true} if the next line has the specified indentation or higher, {@code false} otherwise. + */ public boolean nextLine(int indent) throws IOException { fillLoop: do { while (bufferPos < bufferLimit) { @@ -246,9 +261,9 @@ public boolean nextLine(int indent) throws IOException { if (indent == 0) { // skip empty lines if indent is 0 if (!fillBuffer(2, false, true)) break fillLoop; - c = buffer[bufferPos + 1]; + char next = buffer[bufferPos + 1]; - if (c == '\n' || c == '\r') { // 2+ consecutive new lines, consume first nl and retry + if (next == '\n' || next == '\r') { // 2+ consecutive new lines, consume first nl and retry bufferPos++; lineNumber++; bof = false; @@ -428,7 +443,7 @@ private boolean fillBuffer(int count, boolean preventCompaction, boolean markEof return true; } - private static final String noMatch = new String(); + private static final String NO_MATCH = new String(); private final Reader reader; private final char indentationChar; private final char columnSeparator; diff --git a/src/test/java/net/fabricmc/mappingio/TestHelper.java b/src/test/java/net/fabricmc/mappingio/TestHelper.java index 4df90857..7d227b94 100644 --- a/src/test/java/net/fabricmc/mappingio/TestHelper.java +++ b/src/test/java/net/fabricmc/mappingio/TestHelper.java @@ -93,6 +93,7 @@ public static MemoryMappingTree createTestTree() { visitMethodArg(tree, dstNs); visitMethodVar(tree, dstNs); visitInnerClass(tree, 1, dstNs); + visitComment(tree); visitField(tree, dstNs); visitClass(tree, dstNs); diff --git a/src/test/java/net/fabricmc/mappingio/read/ValidContentReadTest.java b/src/test/java/net/fabricmc/mappingio/read/ValidContentReadTest.java index 530dbaee..90539995 100644 --- a/src/test/java/net/fabricmc/mappingio/read/ValidContentReadTest.java +++ b/src/test/java/net/fabricmc/mappingio/read/ValidContentReadTest.java @@ -16,6 +16,8 @@ package net.fabricmc.mappingio.read; +import java.nio.file.Path; + import org.jetbrains.annotations.Nullable; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -25,8 +27,10 @@ import net.fabricmc.mappingio.TestHelper; import net.fabricmc.mappingio.VisitOrderVerifyingVisitor; import net.fabricmc.mappingio.adapter.FlatAsRegularMappingVisitor; +import net.fabricmc.mappingio.adapter.MappingSourceNsSwitch; import net.fabricmc.mappingio.format.MappingFormat; import net.fabricmc.mappingio.tree.MappingTree; +import net.fabricmc.mappingio.tree.MappingTreeView; import net.fabricmc.mappingio.tree.MemoryMappingTree; import net.fabricmc.mappingio.tree.VisitableMappingTree; @@ -138,27 +142,38 @@ public void jobfFile() throws Exception { checkHoles(format); } - private VisitableMappingTree checkDefault(MappingFormat format) throws Exception { - VisitableMappingTree tree = new MemoryMappingTree(); - MappingReader.read(TestHelper.MappingDirs.VALID.resolve(TestHelper.getFileName(format)), format, new VisitOrderVerifyingVisitor(tree)); + private void checkDefault(MappingFormat format) throws Exception { + Path path = TestHelper.MappingDirs.VALID.resolve(TestHelper.getFileName(format)); - assertSubset(tree, format, testTree, null); - assertSubset(testTree, null, tree, format); + VisitableMappingTree tree = new MemoryMappingTree(); + MappingReader.read(path, format, new VisitOrderVerifyingVisitor(tree)); + assertEqual(format, tree, testTree); - return tree; + tree = new MemoryMappingTree(); + MappingReader.read(path, format, + new MappingSourceNsSwitch( + new VisitOrderVerifyingVisitor( + new MappingSourceNsSwitch( + new VisitOrderVerifyingVisitor(tree), + testTree.getSrcNamespace())), + testTree.getDstNamespaces().get(0))); + assertEqual(format, tree, testTree); } - private VisitableMappingTree checkHoles(MappingFormat format) throws Exception { - VisitableMappingTree tree = new MemoryMappingTree(); - MappingReader.read(TestHelper.MappingDirs.VALID_WITH_HOLES.resolve(TestHelper.getFileName(format)), format, new VisitOrderVerifyingVisitor(tree)); + private void checkHoles(MappingFormat format) throws Exception { + Path path = TestHelper.MappingDirs.VALID_WITH_HOLES.resolve(TestHelper.getFileName(format)); - assertSubset(tree, format, testTreeWithHoles, null); - assertSubset(testTreeWithHoles, null, tree, format); + VisitableMappingTree tree = new MemoryMappingTree(); + MappingReader.read(path, format, new VisitOrderVerifyingVisitor(tree)); + assertEqual(format, tree, testTreeWithHoles); + } - return tree; + private void assertEqual(MappingFormat format, MappingTreeView tree, MappingTreeView referenceTree) throws Exception { + assertSubset(tree, format, referenceTree, null); + assertSubset(referenceTree, null, tree, format); } - private void assertSubset(MappingTree subTree, @Nullable MappingFormat subFormat, MappingTree supTree, @Nullable MappingFormat supFormat) throws Exception { + private void assertSubset(MappingTreeView subTree, @Nullable MappingFormat subFormat, MappingTreeView supTree, @Nullable MappingFormat supFormat) throws Exception { subTree.accept( new VisitOrderVerifyingVisitor( new FlatAsRegularMappingVisitor( diff --git a/src/test/resources/read/valid/enigma-dir/class1Ns0Rename.mapping b/src/test/resources/read/valid/enigma-dir/class1Ns0Rename.mapping index a328505e..8a313665 100644 --- a/src/test/resources/read/valid/enigma-dir/class1Ns0Rename.mapping +++ b/src/test/resources/read/valid/enigma-dir/class1Ns0Rename.mapping @@ -3,4 +3,5 @@ CLASS class_1 class1Ns0Rename METHOD method_1 method1Ns0Rename ()I ARG 1 param1Ns0Rename CLASS class_2 class2Ns0Rename + COMMENT This is a comment FIELD field_2 field2Ns0Rename I diff --git a/src/test/resources/read/valid/enigma.mappings b/src/test/resources/read/valid/enigma.mappings index 426d0ef5..05963c4a 100644 --- a/src/test/resources/read/valid/enigma.mappings +++ b/src/test/resources/read/valid/enigma.mappings @@ -3,5 +3,6 @@ CLASS class_1 class1Ns0Rename METHOD method_1 method1Ns0Rename ()I ARG 1 param1Ns0Rename CLASS class_2 class2Ns0Rename + COMMENT This is a comment FIELD field_2 field2Ns0Rename I CLASS class_3 class3Ns0Rename diff --git a/src/test/resources/read/valid/tinyV2.tiny b/src/test/resources/read/valid/tinyV2.tiny index 84237945..b8470282 100644 --- a/src/test/resources/read/valid/tinyV2.tiny +++ b/src/test/resources/read/valid/tinyV2.tiny @@ -5,5 +5,6 @@ c class_1 class1Ns0Rename class1Ns1Rename p 1 param_1 param1Ns0Rename param1Ns1Rename v 2 2 2 var_1 var1Ns0Rename var1Ns1Rename c class_1$class_2 class1Ns0Rename$class2Ns0Rename class1Ns1Rename$class2Ns1Rename + c This is a comment f I field_2 field2Ns0Rename field2Ns1Rename c class_3 class3Ns0Rename class3Ns1Rename