Fix writer NPEs when member descriptor or metadata is null (#123)

Also fixes SRG writer omitting fields with null srcDesc
FabricMC · Jan 18, 2025 · 485410e · 485410e
1 parent 5c7eac2
commit 485410e
Show file tree

Hide file tree

Showing 31 changed files with 120 additions and 50 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,10 +4,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+- Added a simplified `MappingNsCompleter` constructor for completing all destination names with the source names
 - Made `OuterClassNamePropagator` configurable
 - Made Enigma writer always output destination names if visited explicitly, establishing consistency across all writers
-- Added a simplified `MappingNsCompleter` constructor for completing all destination names with the source names
 - Adjusted format detection to only return ENIGMA_DIR for non-empty directories with at least one `.mapping` file
+- Fixed writer NPEs when metadata or member source descriptors are null
+- Fixed SRG writer omitting fields with missing source descriptors
 
 ## [0.7.1] - 2025-01-07
 - Restored the ability to read source-namespace-only mapping files, even if not spec-compliant

diff --git a/src/main/java/net/fabricmc/mappingio/MappingVisitor.java b/src/main/java/net/fabricmc/mappingio/MappingVisitor.java
@@ -37,7 +37,7 @@
  * </ul>
  *
  * <p>The elements with a skip-return (Header/Content/Class/Field/Method/Arg/Var/ElementContent) abort processing the
- * remainder of their associated item in the above listing if requested by a {@code true} return value. For example
+ * remainder of their associated item in the above listing if requested by a {@code false} return value. For example
  * skipping in Class does neither DstName nor ElementContent, but continues with another class or End.
  *
  * <p>Returning {@code false} in End requests another complete visitation pass if the flag

diff --git a/src/main/java/net/fabricmc/mappingio/format/enigma/EnigmaWriterBase.java b/src/main/java/net/fabricmc/mappingio/format/enigma/EnigmaWriterBase.java
@@ -59,6 +59,10 @@ public boolean visitClass(String srcName) throws IOException {
 
 	@Override
 	public boolean visitField(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		writeIndent(0);
 		writer.write("FIELD ");
 		writer.write(srcName);
@@ -70,6 +74,10 @@ public boolean visitField(String srcName, @Nullable String srcDesc) throws IOExc
 
 	@Override
 	public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		writeIndent(0);
 		writer.write("METHOD ");
 		writer.write(srcName);

diff --git a/src/main/java/net/fabricmc/mappingio/format/intellij/MigrationMapConstants.java b/src/main/java/net/fabricmc/mappingio/format/intellij/MigrationMapConstants.java
@@ -25,4 +25,5 @@ private MigrationMapConstants() {
 
 	public static final String ORDER_KEY = "migrationmap:order";
 	public static final String MISSING_NAME = "Unnamed migration map";
+	public static final String DEFAULT_ORDER = "0";
 }
diff --git a/src/main/java/net/fabricmc/mappingio/format/intellij/MigrationMapFileWriter.java b/src/main/java/net/fabricmc/mappingio/format/intellij/MigrationMapFileWriter.java
@@ -55,7 +55,7 @@ public void close() throws IOException {
 				if (!wroteOrder) {
 					xmlWriter.writeCharacters("\n\t");
 					xmlWriter.writeEmptyElement("order");
-					xmlWriter.writeAttribute("value", "0");
+					xmlWriter.writeAttribute("value", MigrationMapConstants.DEFAULT_ORDER);
 				}
 
 				xmlWriter.writeCharacters("\n");
@@ -101,17 +101,22 @@ public void visitMetadata(String key, @Nullable String value) throws IOException
 		try {
 			switch (key) {
 			case "name":
+				if (value == null) return;
 				wroteName = true;
 				break;
 			case MigrationMapConstants.ORDER_KEY:
+				if (value == null) return;
 				wroteOrder = true;
 				key = "order";
 				break;
 			}
 
 			xmlWriter.writeCharacters("\n\t");
 			xmlWriter.writeEmptyElement(key);
-			xmlWriter.writeAttribute("value", value);
+
+			if (value != null) {
+				xmlWriter.writeAttribute("value", value);
+			}
 		} catch (XMLStreamException e) {
 			throw new IOException(e);
 		}

diff --git a/src/main/java/net/fabricmc/mappingio/format/jobf/JobfFileWriter.java b/src/main/java/net/fabricmc/mappingio/format/jobf/JobfFileWriter.java
@@ -61,6 +61,10 @@ public boolean visitClass(String srcName) throws IOException {
 
 	@Override
 	public boolean visitField(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 		dstName = null;
@@ -70,6 +74,10 @@ public boolean visitField(String srcName, @Nullable String srcDesc) throws IOExc
 
 	@Override
 	public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 		dstName = null;
@@ -114,7 +122,6 @@ public boolean visitElementContent(MappedElementKind targetKind) throws IOExcept
 			write("c ");
 		} else if ((isField = targetKind == MappedElementKind.FIELD)
 				|| targetKind == MappedElementKind.METHOD) {
-			if (memberSrcDesc == null) return false;
 			write(isField ? "f " : "m ");
 		} else {
 			throw new IllegalStateException("unexpected invocation for "+targetKind);

diff --git a/src/main/java/net/fabricmc/mappingio/format/proguard/ProGuardFileWriter.java b/src/main/java/net/fabricmc/mappingio/format/proguard/ProGuardFileWriter.java
@@ -110,6 +110,10 @@ public boolean visitClass(String srcName) throws IOException {
 
 	@Override
 	public boolean visitField(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 
@@ -118,6 +122,10 @@ public boolean visitField(String srcName, @Nullable String srcDesc) throws IOExc
 
 	@Override
 	public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 

diff --git a/src/main/java/net/fabricmc/mappingio/format/simple/RecafSimpleFileWriter.java b/src/main/java/net/fabricmc/mappingio/format/simple/RecafSimpleFileWriter.java
@@ -66,6 +66,10 @@ public boolean visitField(String srcName, String srcDesc) throws IOException {
 
 	@Override
 	public boolean visitMethod(String srcName, String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 

diff --git a/src/main/java/net/fabricmc/mappingio/format/srg/CsrgFileWriter.java b/src/main/java/net/fabricmc/mappingio/format/srg/CsrgFileWriter.java
@@ -68,6 +68,10 @@ public boolean visitField(String srcName, @Nullable String srcDesc) throws IOExc
 
 	@Override
 	public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		methodSrcDesc = srcDesc;
 

diff --git a/src/main/java/net/fabricmc/mappingio/format/srg/JamFileWriter.java b/src/main/java/net/fabricmc/mappingio/format/srg/JamFileWriter.java
@@ -61,6 +61,10 @@ public boolean visitClass(String srcName) throws IOException {
 
 	@Override
 	public boolean visitField(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 		memberDstName = null;

diff --git a/src/main/java/net/fabricmc/mappingio/format/srg/SrgFileWriter.java b/src/main/java/net/fabricmc/mappingio/format/srg/SrgFileWriter.java
@@ -63,6 +63,10 @@ public boolean visitClass(String srcName) throws IOException {
 
 	@Override
 	public boolean visitField(String srcName, @Nullable String srcDesc) throws IOException {
+		if (xsrg && srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 		memberDstName = null;
@@ -73,6 +77,10 @@ public boolean visitField(String srcName, @Nullable String srcDesc) throws IOExc
 
 	@Override
 	public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 		memberDstName = null;
@@ -123,11 +131,11 @@ public boolean visitElementContent(MappedElementKind targetKind) throws IOExcept
 			write("CL: ");
 			break;
 		case FIELD:
-			if (memberSrcDesc == null || memberDstName == null || (xsrg && memberDstDesc == null)) return false;
+			if (memberDstName == null || xsrg && memberDstDesc == null) return false;
 			write("FD: ");
 			break;
 		case METHOD:
-			if (memberSrcDesc == null || memberDstName == null || memberDstDesc == null) return false;
+			if (memberDstName == null || memberDstDesc == null) return false;
 			write("MD: ");
 			break;
 		default:

diff --git a/src/main/java/net/fabricmc/mappingio/format/srg/TsrgFileWriter.java b/src/main/java/net/fabricmc/mappingio/format/srg/TsrgFileWriter.java
@@ -91,6 +91,10 @@ public boolean visitField(String srcName, @Nullable String srcDesc) throws IOExc
 
 	@Override
 	public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 		hasAnyDstNames = false;
@@ -183,7 +187,7 @@ public boolean visitElementContent(MappedElementKind targetKind) throws IOExcept
 		write(srcName);
 
 		if (targetKind == MappedElementKind.METHOD
-				|| (targetKind == MappedElementKind.FIELD && tsrg2)) {
+				|| (targetKind == MappedElementKind.FIELD && tsrg2 && memberSrcDesc != null)) {
 			writeSpace();
 			write(memberSrcDesc);
 		}

diff --git a/src/main/java/net/fabricmc/mappingio/format/tiny/Tiny1FileWriter.java b/src/main/java/net/fabricmc/mappingio/format/tiny/Tiny1FileWriter.java
@@ -100,6 +100,10 @@ public boolean visitClass(String srcName) throws IOException {
 
 	@Override
 	public boolean visitField(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 
@@ -108,6 +112,10 @@ public boolean visitField(String srcName, @Nullable String srcDesc) throws IOExc
 
 	@Override
 	public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		memberSrcName = srcName;
 		memberSrcDesc = srcDesc;
 

diff --git a/src/main/java/net/fabricmc/mappingio/format/tiny/Tiny2FileWriter.java b/src/main/java/net/fabricmc/mappingio/format/tiny/Tiny2FileWriter.java
@@ -108,6 +108,10 @@ public boolean visitClass(String srcName) throws IOException {
 
 	@Override
 	public boolean visitField(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		write("\tf\t");
 		writeName(srcDesc);
 		writeTab();
@@ -118,6 +122,10 @@ public boolean visitField(String srcName, @Nullable String srcDesc) throws IOExc
 
 	@Override
 	public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+		if (srcDesc == null) {
+			return false;
+		}
+
 		write("\tm\t");
 		writeName(srcDesc);
 		writeTab();

diff --git a/src/test/java/net/fabricmc/mappingio/test/NameGen.java b/src/test/java/net/fabricmc/mappingio/test/NameGen.java
@@ -253,8 +253,8 @@ private String getPrefix(MappedElementKind kind) {
 	private static final String argPrefix = "param";
 	private static final String varPrefix = "var";
 	private static final String comment = "This is a comment";
-	private static final List<String> fldDescs = Collections.unmodifiableList(Arrays.asList("I", "Lcls;", "Lpkg/cls;", "[I"));
-	private static final List<String> mthDescs = Collections.unmodifiableList(Arrays.asList("()I", "(I)V", "(Lcls;)Lcls;", "(ILcls;)Lpkg/cls;", "(Lcls;[I)[[B"));
+	private static final List<String> fldDescs = Collections.unmodifiableList(Arrays.asList("I", "Lcls;", "Lpkg/cls;", "[I", null));
+	private static final List<String> mthDescs = Collections.unmodifiableList(Arrays.asList("()I", "(I)V", "(Lcls;)Lcls;", "(ILcls;)Lpkg/cls;", "(Lcls;[I)[[B", null));
 	private static final MappedElementKind clsKind = MappedElementKind.CLASS;
 	private static final MappedElementKind fldKind = MappedElementKind.FIELD;
 	private static final MappedElementKind mthKind = MappedElementKind.METHOD;

diff --git a/src/test/java/net/fabricmc/mappingio/test/TestMappings.java b/src/test/java/net/fabricmc/mappingio/test/TestMappings.java
@@ -55,7 +55,9 @@ public static <T extends MappingVisitor> T generateValid(T target) throws IOExce
 		if (delegate.visitHeader()) {
 			delegate.visitNamespaces(MappingUtil.NS_SOURCE_FALLBACK, Arrays.asList(MappingUtil.NS_TARGET_FALLBACK, MappingUtil.NS_TARGET_FALLBACK + "2"));
 			delegate.visitMetadata("name", "valid");
-			delegate.visitMetadata(MigrationMapConstants.ORDER_KEY, "0");
+			delegate.visitMetadata(MigrationMapConstants.ORDER_KEY, MigrationMapConstants.DEFAULT_ORDER);
+			delegate.visitMetadata("property-with-empty-value", "");
+			delegate.visitMetadata("property-with-null-value", null);
 		}
 
 		if (delegate.visitContent()) {

diff --git a/src/test/java/net/fabricmc/mappingio/test/visitors/SubsetAssertingVisitor.java b/src/test/java/net/fabricmc/mappingio/test/visitors/SubsetAssertingVisitor.java
@@ -209,6 +209,7 @@ public boolean visitField(String srcClsName, String srcName, @Nullable String sr
 		boolean subHasDstNames = subFeatures.fields().dstNames() != FeaturePresence.ABSENT;
 		boolean supHasDstDescs = supFeatures.fields().dstDescs() != FeaturePresence.ABSENT;
 		boolean subHasDstDescs = subFeatures.fields().dstDescs() != FeaturePresence.ABSENT;
+		boolean supRequiresSrcDescs = supFeatures.fields().srcDescs() == FeaturePresence.REQUIRED;
 
 		if (supFld == null) { // supTree doesn't have this field, ensure the incoming mappings don't have any data for it
 			String[] subDstNames = null;
@@ -217,8 +218,11 @@ public boolean visitField(String srcClsName, String srcName, @Nullable String sr
 			if (supHasDstNames && subHasDstNames) subDstNames = supFeatures.hasNamespaces() || dstNames == null ? dstNames : new String[]{dstNames[subNsIfSupNotNamespaced]};
 			if (supHasDstDescs && subHasDstDescs) subDstDescs = supFeatures.hasNamespaces() || dstDescs == null ? dstDescs : new String[]{dstDescs[subNsIfSupNotNamespaced]};
 
-			assertTrue(isEmpty(subDstNames) && isEmpty(subDstDescs), "Incoming field not contained in supTree: " + subFldId);
-			return true;
+			boolean noData = isEmpty(subDstNames) && isEmpty(subDstDescs);
+			boolean missingRequiredSrcDesc = supRequiresSrcDescs && srcDesc == null;
+
+			assertTrue(noData || missingRequiredSrcDesc, "Incoming field not contained in supTree: " + subFldId);
+			return !missingRequiredSrcDesc;
 		}
 
 		String supFldId = srcClsName + "#" + srcName + ":" + supFld.getSrcDesc();
@@ -289,6 +293,7 @@ public boolean visitMethod(String srcClsName, String srcName, @Nullable String s
 		boolean subHasDstNames = subFeatures.methods().dstNames() != FeaturePresence.ABSENT;
 		boolean supHasDstDescs = supFeatures.methods().dstDescs() != FeaturePresence.ABSENT;
 		boolean subHasDstDescs = subFeatures.methods().dstDescs() != FeaturePresence.ABSENT;
+		boolean supRequiresSrcDescs = supFeatures.methods().srcDescs() == FeaturePresence.REQUIRED;
 
 		if (supMth == null) { // supTree doesn't have this method, ensure the incoming mappings don't have any data for it
 			String[] subDstNames = null;
@@ -297,8 +302,11 @@ public boolean visitMethod(String srcClsName, String srcName, @Nullable String s
 			if (supHasDstNames && subHasDstNames) subDstNames = supFeatures.hasNamespaces() || dstNames == null ? dstNames : new String[]{dstNames[subNsIfSupNotNamespaced]};
 			if (supHasDstDescs && subHasDstDescs) subDstDescs = supFeatures.hasNamespaces() || dstDescs == null ? dstDescs : new String[]{dstDescs[subNsIfSupNotNamespaced]};
 
-			assertTrue(isEmpty(subDstNames) && isEmpty(subDstDescs), "Incoming method not contained in supTree: " + subMthId);
-			return true;
+			boolean noData = isEmpty(subDstNames) && isEmpty(subDstDescs);
+			boolean missingRequiredSrcDesc = supRequiresSrcDescs && srcDesc == null;
+
+			assertTrue(noData || missingRequiredSrcDesc, "Incoming method not contained in supTree: " + subMthId);
+			return !missingRequiredSrcDesc;
 		}
 
 		String supMthId = srcClsName + "#" + srcName + supMth.getSrcDesc();

diff --git a/src/test/resources/reading/holes/enigma-dir/class_35.mapping b/src/test/resources/reading/holes/enigma-dir/class_35.mapping
@@ -4,9 +4,7 @@ CLASS class_35
 	FIELD field_3 Lpkg/cls;
 	FIELD field_4 [I
 		COMMENT This is a comment
-	FIELD field_5 field5Ns0Rename I
-		COMMENT This is a comment
-	FIELD field_6 Lcls;
+	FIELD field_6 I
 		COMMENT This is a comment
 	METHOD method_1 ()I
 	METHOD method_2 method2Ns0Rename (I)V
@@ -15,9 +13,7 @@ CLASS class_35
 		COMMENT This is a comment
 	METHOD method_5 method5Ns0Rename (Lcls;[I)[[B
 		COMMENT This is a comment
-	METHOD method_6 ()I
-		COMMENT This is a comment
-	METHOD method_7 (I)V
+	METHOD method_7 ()I
 		ARG 1
 		ARG 3
 		ARG 5 param3Ns0Rename
@@ -27,4 +23,4 @@ CLASS class_35
 			COMMENT This is a comment
 		ARG 11
 			COMMENT This is a comment
-	METHOD method_8 (Lcls;)Lcls;
+	METHOD method_8 (I)V
diff --git a/src/test/resources/reading/holes/enigma.mappings b/src/test/resources/reading/holes/enigma.mappings
@@ -47,9 +47,7 @@ CLASS class_35
 	FIELD field_3 Lpkg/cls;
 	FIELD field_4 [I
 		COMMENT This is a comment
-	FIELD field_5 field5Ns0Rename I
-		COMMENT This is a comment
-	FIELD field_6 Lcls;
+	FIELD field_6 I
 		COMMENT This is a comment
 	METHOD method_1 ()I
 	METHOD method_2 method2Ns0Rename (I)V
@@ -58,9 +56,7 @@ CLASS class_35
 		COMMENT This is a comment
 	METHOD method_5 method5Ns0Rename (Lcls;[I)[[B
 		COMMENT This is a comment
-	METHOD method_6 ()I
-		COMMENT This is a comment
-	METHOD method_7 (I)V
+	METHOD method_7 ()I
 		ARG 1
 		ARG 3
 		ARG 5 param3Ns0Rename
@@ -70,4 +66,4 @@ CLASS class_35
 			COMMENT This is a comment
 		ARG 11
 			COMMENT This is a comment
-	METHOD method_8 (Lcls;)Lcls;
+	METHOD method_8 (I)V