From ffabc780ff4097547e79578061addead8506f942 Mon Sep 17 00:00:00 2001 From: NebelNidas Date: Mon, 23 Dec 2024 12:10:15 +0100 Subject: [PATCH 1/2] Improve tree-API namespace setters' documentation and robustness --- .../fabricmc/mappingio/tree/MappingTree.java | 23 +++- .../mappingio/tree/MemoryMappingTree.java | 41 ++++++- .../mappingio/tree/SetNamespacesTest.java | 108 ++++++++++++++++++ 3 files changed, 168 insertions(+), 4 deletions(-) create mode 100644 src/test/java/net/fabricmc/mappingio/tree/SetNamespacesTest.java diff --git a/src/main/java/net/fabricmc/mappingio/tree/MappingTree.java b/src/main/java/net/fabricmc/mappingio/tree/MappingTree.java index de5703c8..2e20e66a 100644 --- a/src/main/java/net/fabricmc/mappingio/tree/MappingTree.java +++ b/src/main/java/net/fabricmc/mappingio/tree/MappingTree.java @@ -21,6 +21,9 @@ import org.jetbrains.annotations.Nullable; +import net.fabricmc.mappingio.adapter.MappingDstNsReorder; +import net.fabricmc.mappingio.adapter.MappingSourceNsSwitch; + /** * Mutable mapping tree. * @@ -28,15 +31,31 @@ */ public interface MappingTree extends MappingTreeView { /** - * Sets the tree's and all of its contained elements' source namespace. + * Sets the tree's and all of its contained elements' source namespace name. + * + *

If the passed namespace name equals an existing destination namespace's name, + * implementors may choose to switch the two namespaces around, analogous to {@link MappingSourceNsSwitch}. + * This has to be made clear in the implementation's documentation. * - * @return The previous source namespace, if present. + * @implSpec If switching with an existing destination namespace is requested, but not supported, an {@link UnsupportedOperationException} must be thrown. + * + * @return The previous source namespace name, if present. */ @Nullable String setSrcNamespace(String namespace); /** + * Sets the tree's and all of its contained elements' destination namespace names. + * + *

Can be used to reorder and/or drop destination namespaces, analogous to {@link MappingDstNsReorder}. + * + *

Implementors may allow switching with the source namespace as well, analogous to {@link MappingSourceNsSwitch}. + * This has to be made clear in the implementation's documentation. + * + * @implSpec If switching with the source namespace is requested, but not supported, an {@link UnsupportedOperationException} must be thrown. + * * @return The previous destination namespaces. + * @throws IllegalArgumentException If the passed namespace names contain duplicates. */ List setDstNamespaces(List namespaces); diff --git a/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java b/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java index c5fa15ec..ebcbb2f9 100644 --- a/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java +++ b/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java @@ -39,9 +39,13 @@ import net.fabricmc.mappingio.MappedElementKind; import net.fabricmc.mappingio.MappingFlag; import net.fabricmc.mappingio.MappingVisitor; +import net.fabricmc.mappingio.adapter.MappingSourceNsSwitch; /** * {@link VisitableMappingTree} implementation that stores all data in memory. + * + *

Switching the source namespace with an existing destination namespace via + * {@link #setSrcNamespace(String)} or {@link #setDstNamespaces(List)} is not supported yet. */ public final class MemoryMappingTree implements VisitableMappingTree { public MemoryMappingTree() { @@ -118,13 +122,25 @@ public String getSrcNamespace() { return srcNamespace; } + /** + * {@inheritDoc} + * + * @throws UnsupportedOperationException If the passed namespace name is already in use by one of the destination namespaces. This may change in a future release. + */ @Override @Nullable public String setSrcNamespace(String namespace) { assertNotInVisitPass(); + + if (dstNamespaces.contains(namespace)) { + throw new UnsupportedOperationException(String.format( + "Can't use name \"%s\" for the source namespace, as it's already in use by one of the destination namespaces %s.\n" + + "If a source namespace shuffle was the desired outcome, please resort to a %s instead; %s doesn't support this operation natively yet.", + namespace, dstNamespaces, MappingSourceNsSwitch.class.getSimpleName(), getClass().getSimpleName())); + } + String ret = srcNamespace; srcNamespace = namespace; - return ret; } @@ -133,6 +149,12 @@ public List getDstNamespaces() { return dstNamespaces; } + /** + * {@inheritDoc} + * + * @throws IllegalArgumentException If the passed namespace names contain duplicates. + * @throws UnsupportedOperationException If the passed namespace names contain the source namespace's name. This may change in a future release. + */ @Override public List setDstNamespaces(List namespaces) { assertNotInVisitPass(); @@ -140,16 +162,31 @@ public List setDstNamespaces(List namespaces) { if (!classesBySrcName.isEmpty()) { // classes present, update existing dstNames int newSize = namespaces.size(); int[] nameMap = new int[newSize]; + Set processedNamespaces = new HashSet<>(newSize); + Set duplicateNamespaces = new HashSet<>(newSize); for (int i = 0; i < newSize; i++) { String newNs = namespaces.get(i); if (newNs.equals(srcNamespace)) { - throw new IllegalArgumentException("can't use the same namespace for src and dst"); + throw new UnsupportedOperationException(String.format( + "Can't use name \"%s\" for destination namespace %s, as it's already in use by the source namespace.\n" + + "If a source namespace shuffle was the desired outcome, please resort to a %s instead; %s doesn't support this operation natively yet.", + newNs, i, MappingSourceNsSwitch.class.getSimpleName(), getClass().getSimpleName())); } else { int oldNsIdx = dstNamespaces.indexOf(newNs); nameMap[i] = oldNsIdx; } + + if (processedNamespaces.contains(newNs)) { + duplicateNamespaces.add(newNs); + } + + processedNamespaces.add(newNs); + } + + if (!duplicateNamespaces.isEmpty()) { + throw new IllegalArgumentException("Duplicate destination namespace names: " + duplicateNamespaces); } boolean useResize = true; diff --git a/src/test/java/net/fabricmc/mappingio/tree/SetNamespacesTest.java b/src/test/java/net/fabricmc/mappingio/tree/SetNamespacesTest.java new file mode 100644 index 00000000..2a521677 --- /dev/null +++ b/src/test/java/net/fabricmc/mappingio/tree/SetNamespacesTest.java @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2024 FabricMC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package net.fabricmc.mappingio.tree; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.util.Arrays; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import net.fabricmc.mappingio.TestHelper; + +public class SetNamespacesTest { + private MemoryMappingTree tree; + private String srcNs; + private String dstNs0; + private String dstNs1; + + @BeforeEach + public void setup() throws IOException { + tree = new MemoryMappingTree(); + TestHelper.acceptTestMappings(tree); + + srcNs = tree.getSrcNamespace(); + dstNs0 = tree.getDstNamespaces().get(0); + dstNs1 = tree.getDstNamespaces().get(1); + } + + @Test + public void srcToNewSrc() { + String srcNsNew = srcNs + "New"; + + tree.setSrcNamespace(srcNsNew); + assertSrc(srcNsNew); + assertDst(dstNs0, dstNs1); + + tree.setSrcNamespace(srcNs); + assertSrc(srcNs); + assertDst(dstNs0, dstNs1); + } + + @Test + public void dstToNewDst() { + String dstNs0New = dstNs0 + "New"; + String dstNs1New = dstNs1 + "New"; + + tree.setDstNamespaces(Arrays.asList(dstNs0New, dstNs1New)); + assertSrc(srcNs); + assertDst(dstNs0New, dstNs1New); + + tree.setDstNamespaces(Arrays.asList(dstNs0, dstNs1)); + assertSrc(srcNs); + assertDst(dstNs0, dstNs1); + } + + @Test + public void srcToDst() { + assertThrows(UnsupportedOperationException.class, () -> tree.setSrcNamespace(dstNs0)); + } + + @Test + public void dstToSrc() { + assertThrows(UnsupportedOperationException.class, () -> tree.setDstNamespaces(Arrays.asList(srcNs, dstNs1))); + } + + @Test + public void dstToDst() { + tree.setDstNamespaces(Arrays.asList(dstNs1, dstNs0)); + assertSrc(srcNs); + assertDst(dstNs1, dstNs0); + + tree.setDstNamespaces(Arrays.asList(dstNs0, dstNs1)); + assertSrc(srcNs); + assertDst(dstNs0, dstNs1); + } + + @Test + public void duplicateDst() { + assertThrows(IllegalArgumentException.class, () -> tree.setDstNamespaces(Arrays.asList(dstNs0, dstNs0))); + assertThrows(IllegalArgumentException.class, () -> tree.setDstNamespaces(Arrays.asList(dstNs1, dstNs1))); + assertThrows(IllegalArgumentException.class, () -> tree.setDstNamespaces(Arrays.asList(dstNs1 + "New", dstNs1 + "New"))); + } + + private void assertSrc(String srcNs) { + assertTrue(tree.getSrcNamespace().equals(srcNs)); + } + + private void assertDst(String... dstNs) { + assertTrue(tree.getDstNamespaces().equals(Arrays.asList(dstNs))); + } +} From eaaf250603a6a4af97207c4711b08fabd8709aae Mon Sep 17 00:00:00 2001 From: NebelNidas Date: Mon, 23 Dec 2024 12:18:29 +0100 Subject: [PATCH 2/2] Don't use line breaks in exception messages --- .../net/fabricmc/mappingio/tree/MemoryMappingTree.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java b/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java index ebcbb2f9..c7083807 100644 --- a/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java +++ b/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java @@ -134,8 +134,8 @@ public String setSrcNamespace(String namespace) { if (dstNamespaces.contains(namespace)) { throw new UnsupportedOperationException(String.format( - "Can't use name \"%s\" for the source namespace, as it's already in use by one of the destination namespaces %s.\n" - + "If a source namespace shuffle was the desired outcome, please resort to a %s instead; %s doesn't support this operation natively yet.", + "Can't use name \"%s\" for the source namespace, as it's already in use by one of the destination namespaces %s." + + " If a source namespace shuffle was the desired outcome, please resort to a %s instead; %s doesn't support this operation natively yet.", namespace, dstNamespaces, MappingSourceNsSwitch.class.getSimpleName(), getClass().getSimpleName())); } @@ -170,8 +170,8 @@ public List setDstNamespaces(List namespaces) { if (newNs.equals(srcNamespace)) { throw new UnsupportedOperationException(String.format( - "Can't use name \"%s\" for destination namespace %s, as it's already in use by the source namespace.\n" - + "If a source namespace shuffle was the desired outcome, please resort to a %s instead; %s doesn't support this operation natively yet.", + "Can't use name \"%s\" for destination namespace %s, as it's already in use by the source namespace." + + " If a source namespace shuffle was the desired outcome, please resort to a %s instead; %s doesn't support this operation natively yet.", newNs, i, MappingSourceNsSwitch.class.getSimpleName(), getClass().getSimpleName())); } else { int oldNsIdx = dstNamespaces.indexOf(newNs);