Remove srgsAsFallback = false 🎉

CHANGELOG.md

@@ -4,21 +4,24 @@ Running changelog document, will be added to as I commit things.
 
 ## Critical bugfixes
 
-* **Very important** fix: Fix reobfuscation of method calls that mention a class from Minecraft in their parameter list or return type
-  * Which is, frankly, "most of them"
-  * Bug only cropped up if you didn't have `srgsAsFallback = true`
-* Fix the Forge-added class `amq$1`/`net/minecraft/src/Block$1` getting put in the wrong spot for like, the millionth time
-  * fixes `canSustainPlant`-related runtime crashes in dev on 1.4.7, and other assorted crashes
+* **Very important** fix: Fix reobfuscation of method calls that mention a class from Minecraft in their parameter list or return type.
+  * Which is, frankly, "most of them".
+  * Bug only cropped up if you didn't have `srgsAsFallback = true`. This option has been removed and is now effectively always `true`.
+* Fix the Forge-added class `amq$1`/`net/minecraft/src/Block$1` getting put in the wrong spot (for like, the millionth time)
+  * Fixes several `IllegalAccessError` crashes in the dev workspace, e.g. a `canSustainPlant`-related crash on 1.4.7
 
 ## Breaking changes
 
-* SRG field/method names now (correctly) no longer show through if `srgsAsFallback` is not set.
-  * Either switch your code to use the proguarded names (look for things with 1 or 2 char names)
-  * Or set `volde { forgeCapabilities { srgsAsFallback = true } }`
-* The run dir is now resolved against the current subproject project directory, instead of the *root* project directory.
-  * This makes more sense, matches what other Minecraft dev tools use, etc. 
+* `srgsAsFallback` as a configurable option is removed. It is now effectively set to `true`.
+  * Proguarded names can now only show up in your dev workspace if they are not mentioned in your `joined.srg` (which'd probably mean you're using mappings for the wrong version).
+  * Supporting both `true` and `false` turned out to be very complicated and buggy for little benefit. `false` mode wasn't working properly anyway; it still dumped SRG names into your workspace - and did you know that enum values weren't given MCP names? A correct implementation of `srgsAsFallback = false` stripped all enum value names. That's no good.
+  * **If your mod was calling methods or accessing fields with their proguarded names,** you'll need to update it to use the `func_` or `field_`-prefixed names.
+* The run dir is now correctly resolved against the current subproject project directory, instead of the *root* project directory.
   * Sorry about that. If you were using voldeloom in a subproject, you can probably remove your custom runDir now.
-* Bumped mappings version (expect a longer first-run setup time)
+
+## Changes
+
+* Bumped internal mappings version. Expect a slightly longer import time after doing this update.
 
 ## Roadmap
 

README.md

@@ -11,7 +11,7 @@ Gradle plugin for ~~Fabric~~ ancient versions of Forge.
 * [Forked](https://github.com/TwilightFlower/fabric-loom/) by [TwilightFlower](https://github.com/TwilightFlower/) May 2020 for the release of [Retro Tater](https://github.com/TwilightFlower/retro-tater); she did lots of the initial architecture work.
 * [Tweaked, maintained, and additional version support](https://github.com/unascribed/voldeloom/) by [unascribed](https://github.com/unascribed/) May 2021 - May 2022 for the release of [Ears](https://git.sleeping.town/unascribed/Ears/src/branch/trunk/platform-forge-1.4) and other Forge mods.
 * Minor tweak by [quaternary](https://github.com/quat1024) Jul 2022 for the release of [Hopper](https://github.com/quat1024/hoppers).
-* `disaster-time` rewrite branch started by quaternary Dec 2022 - Apr 2023.
+* `disaster-time` rewrite branch started by quaternary Dec 2022 - Jun 2023.
 
 This branch is my playground and my domain. Here be dragons (it's me. I'm the dragon)
 

doc/forge-capabilities.md

@@ -25,12 +25,12 @@ They are:
 
 *`distributionNamingScheme`* - string - the way references to fields and variables are encoded in reobfuscated release mod jars. This is `"intermediary"` since 1.5 and `"official"` before it.
 
-*`srgsAsFallback`* - bool - when a field or method is missing an MCP name, if `true` (1.5+) the proguarded name will show through, and if `false` (1.4-) the SRG will show through. This is relevant if you need to use reflection or a coremod to access a field. **TODO: I need to research this more**
-
 *`requiresLaunchwrapper`* - bool - Minecraft 1.6 changed its launch procedure to require LegacyLaunch (aka Launchwrapper); a different method of setting up run configurations is required.
 
 *`supportsAssetsDir`* - bool - If `true` (1.6+), the game accepts an `--assetsDir` argument to set the path of the asset directory. If `false` (1.5-), the game uses an assets directory in a hardcoded path inside the game folder, and Voldeloom must copy assets into that folder before starting a client run configuration.
 
 *`mappedAccessTransformers`* - bool - If `true` (1.7+), access transformer files have SRG-named access transformers. If `false` (otherwise), they're proguarded.
 
+~~*`srgsAsFallback`* - bool - when a field or method is missing an MCP name, if `true` (1.5+) the proguarded name will show through, and if `false` (1.4-) the SRG will show through. This is relevant if you need to use reflection or a coremod to access a field.~~ Removed in Voldeloom 2.4; effectively always evaluates to "true". If you'd like to do reflection, you'll need to look up the proguarded name yourself.
+
 (All options also have a form suffixed with `Supplier` (like `srgsAsFallbackSupplier`) that let you provide a `Supplier<T>` instead of a T. See also: `net.fabricmc.loom.util.Suppliers.memoize`.)

src/main/java/net/fabricmc/loom/ForgeCapabilities.java

@@ -67,38 +67,6 @@ public ForgeCapabilities setDistributionNamingSchemeSupplier(Supplier<String> di
 		return this;
 	}
 
-	/**
-	 * If a field/method is missing a mapping, if 'true' the proguarded name will show through, and if 'false' the MCP name will.
-	 * <p>
-	 * By default, this evaluates to 'true' since 1.5, and 'false' before it.
-	 * TODO: is this even a good idea?
-	 */
-	public Supplier<Boolean> srgsAsFallback = Suppliers.memoize(this::guessSrgsAsFallback);
-
-	public boolean guessSrgsAsFallback() {
-		checkConfigured("guessSrgsAsFallback");
-
-		if(guessMinecraftMinorVersion() >= 5) {
-			log.info("|-> [ForgeCapabilities guess] I think this Forge version uses SRGs as the fallback when no mapping is defined?");
-			return true;
-		} else {
-			log.info("|-> [ForgeCapabilities guess] I think this Forge version uses proguarded names as the fallback when no mapping is defined?");
-			return false;
-		}
-	}
-
-	@SuppressWarnings("unused") //gradle api
-	public ForgeCapabilities setSrgsAsFallback(boolean srgsAsFallback) {
-		this.srgsAsFallback = () -> srgsAsFallback;
-		return this;
-	}
-
-	@SuppressWarnings("unused") //gradle api
-	public ForgeCapabilities setSrgsAsFallbackSupplier(Supplier<Boolean> srgsAsFallback) {
-		this.srgsAsFallback = srgsAsFallback;
-		return this;
-	}
-
 	/**
 	 * Minecraft used to contain shaded copies of libraries that MCP tried to map back to reality.
 	 * The Forge installation procedure would sometimes delete these libraries after deobfuscating the game.
@@ -315,4 +283,30 @@ public ForgeCapabilities minecraftRealPathSupplier(Supplier<Function<Path, Path>
 		this.minecraftRealPath = minecraftRealPath;
 		return this;
 	}
+
+	/// --- ///
+
+	/*
+	 * "If a field/method is missing a mapping, if 'true' the proguarded name will show through, and if 'false' the MCP name will.
+	 * By default, this evaluates to 'true' since 1.5, and 'false' before it."
+	 * ---
+	 * that was old documentation. This item was removed and effectively always evaluates to `true`.
+	 * It was buggy, had undesirable side effects (enum values were not given MCP names), and the benefit is questionable.
+	 * It's really only important if you needed to do reflection on something unmapped. You'll need to look up its proguarded name.
+	 */
+
+	@SuppressWarnings("unused") //gradle api
+	@Deprecated
+	public ForgeCapabilities setSrgsAsFallback(boolean hmm) {
+		log.error("[voldeloom] `srgsAsFallback = false` mode was removed, and does not need to be configured anymore");
+		if(!hmm) throw new IllegalArgumentException("[voldeloom] `srgsAsFallback = false` mode was removed");
+
+		return this;
+	}
+
+	@SuppressWarnings("unused") //gradle api
+	@Deprecated
+	public ForgeCapabilities setSrgsAsFallbackSupplier(Supplier<Boolean> hmm) {
+		return setSrgsAsFallback(hmm.get());
+	}
 }

src/main/java/net/fabricmc/loom/ProviderGraph.java

@@ -131,7 +131,7 @@ private void setupSide(
 		ResolvedConfigElementWrapper forgeWrapper
 	) throws Exception {
 		log.lifecycle("# ({}) Fetching Forge dependencies...", side);
-		ForgeDependencyFetcher forgeDeps = new ForgeDependencyFetcher(project, extension)
+		new ForgeDependencyFetcher(project, extension)
 			.forgeJar(forgeWrapper.getPath())
 			.fmlLibrariesBaseUrl(extension.fmlLibrariesBaseUrl)
 			.libDownloaderDir(forgeWrapper.getFilenameSafeDepString())
@@ -250,13 +250,12 @@ private void setupSide(
 
 		//TODO: oops all leaky abstraction again
 		if(side.equals("joined")) {
-			boolean srgFieldsMethodsAsFallback = extension.forgeCapabilities.srgsAsFallback.get();
 			boolean reobfToSrg = extension.forgeCapabilities.distributionNamingScheme.get().equals(Constants.INTERMEDIATE_NAMING_SCHEME);
 
 			log.lifecycle("# ({}) Initializing reobf mappings ({} -> {})...", side, Constants.MAPPED_NAMING_SCHEME,
 				reobfToSrg ? Constants.INTERMEDIATE_NAMING_SCHEME : Constants.PROGUARDED_NAMING_SCHEME);
 
-			reobfSrg = mappings.chooseSrg(side).reobf(mappings.fields, mappings.methods, srgFieldsMethodsAsFallback, reobfToSrg);
+			reobfSrg = mappings.chooseSrg(side).reobf(mappings.fields, mappings.methods, reobfToSrg);
 
 			if(project.hasProperty("voldeloom.reobf-debug")) {
 				Path dbgOut = project.getBuildDir().toPath().resolve("voldeloom-reobf-mappings-debug.srg");

src/main/java/net/fabricmc/loom/mcp/McpMappingsBuilder.java

@@ -20,15 +20,12 @@ public class McpMappingsBuilder {
 	public Members fields = new Members();
 	public Members methods = new Members();
 
-	public McpMappings build(boolean srgFieldsMethodsAsFallback) {
+	public McpMappings build() {
 		//Only apply packaging transformation to joined srg. Packaging transformations theoretically make sense for client/server
 		//srgs too, but this never happened in practice because packages.csv was invented in 1.4, after the client/server srg merge.
 		//todo: might be nice to come up with an anachronistic way to do it?
 		Srg joined2 = !packages.isEmpty() && !joined.isEmpty() ? joined.repackage(packages) : joined;
 
-		//If requested, "re-proguard" fields/methods that don't have a corresponding MCP name
-		if(!srgFieldsMethodsAsFallback) joined2 = joined2.reproguardUnnamed(fields, methods);
-
 		return new McpMappings(joined2, client, server, fields, methods);
 	}
 

src/main/java/net/fabricmc/loom/mcp/Srg.java

@@ -25,12 +25,6 @@ public Srg() {
 		methodMappingsByOwningClass = new LinkedHashMap<>();
 	}
 
-	public Srg(Map<String, String> classMappings, Map<String, Map<String, String>> fieldMappingsByOwningClass, Map<String, Map<MethodEntry, MethodEntry>> methodMappingsByOwningClass) {
-		this.classMappings = classMappings;
-		this.fieldMappingsByOwningClass = fieldMappingsByOwningClass;
-		this.methodMappingsByOwningClass = methodMappingsByOwningClass;
-	}
-
 	public final Map<String, String> classMappings;
 	public final Map<String, Map<String, String>> fieldMappingsByOwningClass;
 	public final Map<String, Map<MethodEntry, MethodEntry>> methodMappingsByOwningClass;
@@ -260,12 +254,8 @@ public Srg repackage(Packages packages) {
 	/**
 	 * emulates the "rename the SRG using find-and-replace" step of official tools.
 	 * Anywhere an SRG name could appear, it is fed through the fields/methods csvs.
-	 *
-	 * @param srgAsFallback if {@code true} and an MCP field name doesn't exist, the srg name will be used,
-	 *                      if not, the proguarded name will be used. same for method names.
-	 *                      mainly because this method is for reobf, so it should match what's in the dev workspace
 	 */
-	public Srg named(Members fields, Members methods, boolean srgAsFallback) {
+	public Srg named(Members fields, Members methods) {
 		//StringInterner mem = new StringInterner(); //Not necessary since i only make selections from existing String objects
 		Srg named = new Srg();
 
@@ -278,7 +268,7 @@ public Srg named(Members fields, Members methods, boolean srgAsFallback) {
 				named.putFieldMapping(
 					owningClass,
 					proguard,
-					namedEntry != null ? namedEntry.remappedName : (srgAsFallback ? srg : proguard)
+					namedEntry != null ? namedEntry.remappedName : srg
 				);
 			}));
 
@@ -290,45 +280,22 @@ public Srg named(Members fields, Members methods, boolean srgAsFallback) {
 					owningClass,
 					proguardEntry.name,
 					proguardEntry.descriptor,
-					namedEntry != null ? namedEntry.remappedName : (srgAsFallback ? srgEntry.name : proguardEntry.name),
+					namedEntry != null ? namedEntry.remappedName : srgEntry.name,
 					srgEntry.descriptor
 				);
 			}));
 
 		return named;
 	}
 
-	public Srg reproguardUnnamed(Members fields, Members methods) {
-		Srg reproguarded = new Srg();
-		reproguarded.putAllClassMappings(classMappings);
-
-		fieldMappingsByOwningClass.forEach((owningClass, fieldMappings) -> {
-			Map<String, String> newFieldMappings = new LinkedHashMap<>();
-			fieldMappings.forEach((prg, srg) -> newFieldMappings.put(prg, fields.remapSrg(srg) == null ? prg : srg));
-			reproguarded.putAllFieldMappings(owningClass, newFieldMappings);
-		});
-
-		methodMappingsByOwningClass.forEach((owningClass, methodMappings) -> {
-			Map<MethodEntry, MethodEntry> newMethodMappings = new LinkedHashMap<>();
-			methodMappings.forEach((prg, srg) -> {
-				newMethodMappings.put(prg, methods.remapSrg(srg.name) != null ? srg : new MethodEntry(prg.name, srg.descriptor));
-			});
-			reproguarded.putAllMethodMappings(owningClass, newMethodMappings);
-		});
-
-		return reproguarded;
-	}
-
-	public Srg reobf(Members fields, Members methods, boolean srgFieldsMethodsAsFallback, boolean reobfToSrg) {
+	public Srg reobf(Members fields, Members methods, boolean reobfToSrg) {
 		Srg reobf = new Srg();
 
 		if(reobfToSrg) {
 			//don't reproguard class names, use passthrough class name mappings
 			classMappings.values().forEach(c -> reobf.putClassMapping(c, c));
 		} else {
-			Map<String, String> inverted = new LinkedHashMap<>();
-			classMappings.forEach((key, value) -> inverted.put(value, key));
-			reobf.putAllClassMappings(inverted);
+			classMappings.forEach((key, value) -> reobf.putClassMapping(value, key));
 		}
 
 		fieldMappingsByOwningClass.forEach((owningClass, fieldMappings) -> {
@@ -337,7 +304,7 @@ public Srg reobf(Members fields, Members methods, boolean srgFieldsMethodsAsFall
 			fieldMappings.forEach((proguard, srg) -> {
 				Members.Entry namedEntry = fields.remapSrg(srg);
 
-				String sourceName = namedEntry != null ? namedEntry.remappedName : (srgFieldsMethodsAsFallback ? srg : proguard);
+				String sourceName = namedEntry != null ? namedEntry.remappedName : srg;
 				String targetName = reobfToSrg ? srg : proguard;
 
 				reobf.putFieldMapping(sourceOwningClass, sourceName, targetName);
@@ -350,7 +317,7 @@ public Srg reobf(Members fields, Members methods, boolean srgFieldsMethodsAsFall
 			methodMappings.forEach((proguardEntry, srgEntry) -> {
 				Members.Entry namedEntry = methods.remapSrg(srgEntry.name);
 
-				String sourceName = namedEntry != null ? namedEntry.remappedName : (srgFieldsMethodsAsFallback ? srgEntry.name : proguardEntry.name);
+				String sourceName = namedEntry != null ? namedEntry.remappedName : srgEntry.name;
 				String sourceDesc = srgEntry.descriptor;
 				String targetName = reobfToSrg ? srgEntry.name : proguardEntry.name;
 				String targetDesc = reobfToSrg ? srgEntry.descriptor : proguardEntry.descriptor;
@@ -464,6 +431,7 @@ public boolean equals(Object o) {
 			return name.equals(that.name) && descriptor.equals(that.descriptor);
 		}
 
+		@SuppressWarnings("ObjectInstantiationInEqualsHashCode")
 		@Override
 		public int hashCode() {
 			return Objects.hash(name, descriptor);

src/main/java/net/fabricmc/loom/mcp/layer/LayeredMcpMappings.java

@@ -203,7 +203,7 @@ public Dependency createDependency() {
 				for(Layer layer : layers) layer.visit(project.getLogger(), builder, mem);
 
 				project.getLogger().lifecycle("|-> Building...");
-				McpMappings mappings = builder.build(ext.forgeCapabilities.srgsAsFallback.get());
+				McpMappings mappings = builder.build();
 
 				//and create the file. everything goes into the root of the zip
 				project.getLogger().lifecycle("|-> Writing...");

src/main/java/net/fabricmc/loom/newprovider/MappingsWrapper.java

@@ -37,13 +37,13 @@ public MappingsWrapper(Project project, LoomGradleExtension ext, Configuration c
 		mappingsBuilder.augment(new JarScanData().scan(scanJar));
 
 		log.info("|-> Building...");
-		mappings = mappingsBuilder.build(ext.forgeCapabilities.srgsAsFallback.get());
+		mappings = mappingsBuilder.build();
 
 		MessageDigest sha = Checksum.SHA256.get();
 		Checksum.feedFileToHasher(getPath(), sha);
 		this.props = new Props()
 			.put("mappings-hash", Checksum.toHexString(sha.digest()))
-			.put("srgsAsFallback", Boolean.toString(ext.forgeCapabilities.srgsAsFallback.get()));
+			.put("v", "2");
 	}
 
 	public final McpMappings mappings;

src/main/java/net/fabricmc/loom/task/fernflower/McpJavadocProvider.java

@@ -28,7 +28,7 @@ public McpJavadocProvider(Path mcpZip) throws IOException {
 			builder.mergeFromFieldsCsv(scan, mem);
 			builder.mergeFromMethodsCsv(scan, mem);
 		}
-		McpMappings mappings = builder.build(true);
+		McpMappings mappings = builder.build();
 
 		mappings.fields.members.values().forEach(entry -> {
 			if(entry.comment != null && !entry.comment.trim().isEmpty()) fieldComments.put(entry.remappedName, entry.comment);