Improve mod loading errors (#9870)

fmlcore/src/main/java/net/minecraftforge/fml/ModList.java

@@ -47,7 +47,7 @@ import java.util.stream.Stream;
  */
 public class ModList
 {
-    private static Logger LOGGER = LogManager.getLogger();
+    //private static final Logger LOGGER = LogManager.getLogger();
     private static ModList INSTANCE;
     private final List<IModFileInfo> modFiles;
     private final List<IModInfo> sortedList;
@@ -120,14 +120,14 @@ public class ModList
         return executor -> gather(
                 this.mods.stream()
                 .map(mod -> ModContainer.buildTransitionHandler(mod, eventGenerator, progressBar, stateChange, executor))
-                .collect(Collectors.toList()))
-            .thenComposeAsync(ModList::completableFutureFromExceptionList, executor);
+                .toList()
+        ).thenComposeAsync(ModList::completableFutureFromExceptionList, executor);
     }
     static CompletionStage<Void> completableFutureFromExceptionList(List<? extends Map.Entry<?, Throwable>> t) {
         if (t.stream().noneMatch(e->e.getValue()!=null)) {
             return CompletableFuture.completedFuture(null);
         } else {
-            final List<Throwable> throwables = t.stream().filter(e -> e.getValue() != null).map(Map.Entry::getValue).collect(Collectors.toList());
+            final List<Throwable> throwables = t.stream().filter(e -> e.getValue() != null).map(Map.Entry::getValue).toList();
             CompletableFuture<Void> cf = new CompletableFuture<>();
             final RuntimeException accumulator = new RuntimeException();
             cf.completeExceptionally(accumulator);

fmlcore/src/main/java/net/minecraftforge/fml/ModLoader.java

@@ -6,6 +6,7 @@
 package net.minecraftforge.fml;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Streams;
 import net.minecraftforge.eventbus.api.Event;
 import net.minecraftforge.fml.event.IModBusEvent;
 import net.minecraftforge.fml.loading.FMLEnvironment;
@@ -32,6 +33,7 @@ import java.util.function.BiConsumer;
 import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static net.minecraftforge.fml.Logging.CORE;
 import static net.minecraftforge.fml.Logging.LOADING;
@@ -91,13 +93,13 @@ public class ModLoader
     {
         INSTANCE = this;
         this.loadingModList = FMLLoader.getLoadingModList();
-        this.loadingExceptions = FMLLoader.getLoadingModList().getErrors().stream()
+        this.loadingExceptions = this.loadingModList.getErrors().stream()
                 .flatMap(ModLoadingException::fromEarlyException)
                 .collect(Collectors.toList());
-        this.loadingWarnings = FMLLoader.getLoadingModList().getBrokenFiles().stream()
+        this.loadingWarnings = this.loadingModList.getBrokenFiles().stream()
                 .map(file -> new ModLoadingWarning(null, ModLoadingStage.VALIDATE, InvalidModIdentifier.identifyJarProblem(file.getFilePath()).orElse("fml.modloading.brokenfile"), file.getFileName()))
                 .collect(Collectors.toList());
-        FMLLoader.getLoadingModList().getModFiles().stream()
+        this.loadingModList.getModFiles().stream()
                 .filter(ModFileInfo::missingLicense)
                 .filter(modFileInfo -> modFileInfo.getMods().stream().noneMatch(thisModInfo -> this.loadingExceptions.stream().map(ModLoadingException::getModInfo).anyMatch(otherInfo -> otherInfo == thisModInfo))) //Ignore files where any other mod already encountered an error
                 .map(modFileInfo -> new ModLoadingException(null, ModLoadingStage.VALIDATE, "fml.modloading.missinglicense", null, modFileInfo.getFile()))
@@ -228,10 +230,9 @@ public class ModLoader
         } catch (CompletionException e) {
             loadingStateValid = false;
             Throwable t = e.getCause();
-            final List<Throwable> notModLoading = Arrays.stream(t.getSuppressed())
-                    .filter(obj -> !(obj instanceof ModLoadingException))
-                    .collect(Collectors.toList());
-            if (!notModLoading.isEmpty()) {
+            boolean hasNotModLoadingEx = Arrays.stream(t.getSuppressed())
+                    .anyMatch(obj -> !(obj instanceof ModLoadingException));
+            if (hasNotModLoadingEx) {
                 LOGGER.fatal("Encountered non-modloading exceptions!", e);
                 statusConsumer.ifPresent(c->c.accept("ERROR DURING MOD LOADING"));
                 throw e;
@@ -257,10 +258,10 @@ public class ModLoader
                 .stream()
                 .map(e -> buildModContainerFromTOML(modFile, modInfoMap, e))
                 .filter(Objects::nonNull)
-                .collect(Collectors.toList());
+                .toList();
         if (containers.size() != modInfoMap.size()) {
-            var modIds = modInfoMap.values().stream().map(IModInfo::getModId).sorted().collect(Collectors.toList());
-            var containerIds = containers.stream().map(c -> c != null ? c.getModId() : "(null)").sorted().collect(Collectors.toList());
+            var modIds = modInfoMap.values().stream().map(IModInfo::getModId).sorted().toList();
+            var containerIds = containers.stream().map(c -> c != null ? c.getModId() : "(null)").sorted().toList();
 
             LOGGER.fatal(LOADING,"File {} constructed {} mods: {}, but had {} mods specified: {}",
                     modFile.getFilePath(),
@@ -278,7 +279,7 @@ public class ModLoader
             loadingExceptions.add(new ModLoadingException(null, ModLoadingStage.CONSTRUCT, "fml.modloading.missingclasses", null, modFile.getFilePath()));
         }
         // remove errored mod containers
-        return containers.stream().filter(mc -> mc.modLoadingStage != ModLoadingStage.ERROR).collect(Collectors.toList());
+        return containers.stream().filter(mc -> mc.modLoadingStage != ModLoadingStage.ERROR).toList();
     }
 
     private ModContainer buildModContainerFromTOML(final IModFile modFile, final Map<String, IModInfo> modInfoMap, final Map.Entry<String, ? extends IModLanguageProvider.IModLanguageLoader> idToProviderEntry) {

fmlloader/src/main/java/net/minecraftforge/fml/loading/BackgroundWaiter.java

@@ -11,7 +11,7 @@ import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 
 public class BackgroundWaiter {
-    private static ExecutorService runner = Executors.newSingleThreadExecutor();
+    private static final ExecutorService runner = Executors.newSingleThreadExecutor();
 
     public static void runAndTick(Runnable r, Runnable tick) {
         ImmediateWindowHandler.updateProgress("Loading bootstrap resources");

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/AbstractModProvider.java

@@ -8,6 +8,7 @@ package net.minecraftforge.fml.loading.moddiscovery;
 import com.mojang.logging.LogUtils;
 import cpw.mods.jarhandling.JarMetadata;
 import cpw.mods.jarhandling.SecureJar;
+import net.minecraftforge.fml.loading.EarlyLoadingException;
 import net.minecraftforge.fml.loading.LogMarkers;
 import net.minecraftforge.forgespi.language.IConfigurable;
 import net.minecraftforge.forgespi.language.IModFileInfo;
@@ -50,6 +51,9 @@ public abstract class AbstractModProvider implements IModProvider {
         if (sj.moduleDataProvider().findFile(MODS_TOML).isPresent()) {
             LOGGER.debug(LogMarkers.SCAN, "Found {} mod of type {}: {}", MODS_TOML, type, path);
             mod = new ModFile(sj, this, ModFileParser::modsTomlParser);
+            if (mod.getModFileInfo().getFileProperties().containsKey(ModFileInfo.NOT_A_FORGE_MOD_PROP)) {
+                return new IModLocator.ModFileOrException(null, new ModFileLoadingException("File \"%s\" is not a Forge mod and cannot be loaded. Look for a Forge version of this mod or consider alternative mods.".formatted(mod.getFileName())));
+            }
         } else if (type != null) {
             LOGGER.debug(LogMarkers.SCAN, "Found {} mod of type {}: {}", JarFile.MANIFEST_NAME, type, path);
             mod = new ModFile(sj, this, this::manifestParser, type);
@@ -109,20 +113,20 @@ public abstract class AbstractModProvider implements IModProvider {
             };
         }
 
-        try {
-            Files.walk(root)
+        try (var files = Files.walk(root)) {
+            files
                 .filter(p -> p.toString().endsWith(".class"))
                 .forEach(consumer);
 
             file.setSecurityStatus(holder.value);
-        } catch(IOException e) {
+        } catch (IOException e) {
             e.printStackTrace();
         }
 
         LOGGER.debug(LogMarkers.SCAN, "Scan finished: {}", file);
     }
 
-    private static class Holder<T> {
+    private static final class Holder<T> {
         T value;
     }
 

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/InvalidModIdentifier.java

@@ -12,10 +12,12 @@ import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.Optional;
 import java.util.function.BiPredicate;
+import java.util.stream.Stream;
 import java.util.zip.ZipFile;
 
 import static cpw.mods.modlauncher.api.LamdbaExceptionUtils.*;
 
+// TODO: [FML][Loader] By the time this is reached, most stuff this checks for is already filtered out - needs fixing.
 public enum InvalidModIdentifier {
 
     OLDFORGE(filePresent("mcmod.info")),
@@ -23,9 +25,9 @@ public enum InvalidModIdentifier {
     LITELOADER(filePresent("litemod.json")),
     OPTIFINE(filePresent("optifine/Installer.class")),
     BUKKIT(filePresent("plugin.yml")),
-    INVALIDZIP((f,zf) -> !zf.isPresent());
+    INVALIDZIP((f,zf) -> zf.isEmpty()); // note: only this one INVALIDZIP check is ran until the todo on this class is fixed
 
-    private BiPredicate<Path, Optional<ZipFile>> ident;
+    private final BiPredicate<Path, Optional<ZipFile>> ident;
 
     InvalidModIdentifier(BiPredicate<Path, Optional<ZipFile>> identifier)
     {
@@ -40,7 +42,7 @@ public enum InvalidModIdentifier {
     public static Optional<String> identifyJarProblem(Path path)
     {
         Optional<ZipFile> zfo = optionalFromException(() -> new ZipFile(path.toFile()));
-        Optional<String> result = Arrays.stream(values()).
+        Optional<String> result = Stream.of(INVALIDZIP).
                                          filter(i -> i.ident.test(path, zfo)).
                                          map(InvalidModIdentifier::getReason).
                                          findAny();

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModDiscoverer.java

@@ -23,6 +23,7 @@ import net.minecraftforge.forgespi.locating.IDependencyLocator;
 import net.minecraftforge.forgespi.locating.IModFile;
 import net.minecraftforge.forgespi.locating.IModLocator;
 
+import net.minecraftforge.forgespi.locating.ModFileLoadingException;
 import org.jetbrains.annotations.ApiStatus;
 import org.slf4j.Logger;
 
@@ -71,6 +72,7 @@ public class ModDiscoverer {
         List<EarlyLoadingException.ExceptionData> discoveryErrorData = new ArrayList<>();
         boolean successfullyLoadedMods = true;
         List<IModFileInfo> brokenFiles = new ArrayList<>();
+        List<ModFileLoadingException> modFileLoadingExceptions = new ArrayList<>();
         boolean distIsDedicatedServer = FMLLoader.getDist().isDedicatedServer();
         ImmediateWindowHandler.updateProgress("Discovering mod files");
         //Loop all mod locators to get the prime mods to load from.
@@ -82,7 +84,17 @@ public class ModDiscoverer {
                 var exceptions = candidates.stream().map(IModLocator.ModFileOrException::ex).filter(Objects::nonNull).toList();
                 if (!exceptions.isEmpty()) {
                     LOGGER.debug(LogMarkers.SCAN, "Locator {} found {} invalid mod files", locator, exceptions.size());
-                    brokenFiles.addAll(exceptions.stream().map(e->e instanceof InvalidModFileException ime ? ime.getBrokenFile() : null).filter(Objects::nonNull).toList());
+                    for (ModFileLoadingException ex : exceptions) {
+                        // pipe exception messages through the discoveryErrorData to avoid swallowing some exceptions and improve error messages
+                        // (no longer a generic "Invalid mod file" for all InvalidModExceptions - it actually shows the exception message now)
+                        if (ex instanceof InvalidModFileException invalidModFileEx) {
+                            var modInfo = invalidModFileEx.getBrokenFile();
+                            brokenFiles.add(modInfo);
+                            discoveryErrorData.add(new EarlyLoadingException.ExceptionData(ex.getMessage(), modInfo));
+                        } else {
+                            discoveryErrorData.add(new EarlyLoadingException.ExceptionData(ex.getMessage()));
+                        }
+                    }
                 }
                 var locatedFiles = candidates.stream().map(IModLocator.ModFileOrException::file).filter(Objects::nonNull).collect(Collectors.toList());
 
@@ -173,7 +185,7 @@ public class ModDiscoverer {
 
         //Validate the loading. With a deduplicated list, we can now successfully process the artifacts and load
         //transformer plugins.
-        var validator = new ModValidator(modFilesMap, brokenFiles, discoveryErrorData);
+        var validator = new ModValidator(modFilesMap, brokenFiles, discoveryErrorData, modFileLoadingExceptions);
         validator.stage1Validation();
         return validator;
     }

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModFileInfo.java

@@ -14,6 +14,8 @@ import net.minecraftforge.forgespi.language.IConfigurable;
 import net.minecraftforge.forgespi.language.IModFileInfo;
 import net.minecraftforge.forgespi.language.IModInfo;
 import net.minecraftforge.forgespi.language.MavenVersionAdapter;
+import org.apache.maven.artifact.versioning.ArtifactVersion;
+import org.apache.maven.artifact.versioning.DefaultArtifactVersion;
 import org.slf4j.Logger;
 import javax.security.auth.x500.X500Principal;
 import java.net.URL;
@@ -34,12 +36,17 @@ import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Consumer;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 public class ModFileInfo implements IModFileInfo, IConfigurable
 {
     private static final Logger LOGGER = LogUtils.getLogger();
+    private static final ArtifactVersion NOT_JAVAFML_VER = new DefaultArtifactVersion("2");
+    private static final String JAVAFML = "javafml";
+    public static final String NOT_A_FORGE_MOD_PROP = "__FORGE__not_a_forge_mod";
+
     private final IConfigurable config;
     private final ModFile modFile;
     private final URL issueURL;
@@ -65,7 +72,18 @@ public class ModFileInfo implements IModFileInfo, IConfigurable
                 .map(MavenVersionAdapter::createFromVersionSpec)
                 .orElseThrow(()->new InvalidModFileException("Missing ModLoader version in file", this));
         this.languageSpecs = new ArrayList<>(List.of(new LanguageSpec(modLoader, modLoaderVersion)));
+
         // the remaining properties are optional with sensible defaults
+        boolean notAForgeMod = false;
+        if (modLoader.equals(JAVAFML) && modLoaderVersion.containsVersion(NOT_JAVAFML_VER)) {
+            notAForgeMod = true;
+            this.properties = Map.of(NOT_A_FORGE_MOD_PROP, true);
+        } else {
+            this.properties = config.<Map<String, Object>>getConfigElement("properties")
+                    .orElse(Collections.emptyMap());
+        }
+        this.modFile.setFileProperties(this.properties);
+
         this.license = config.<String>getConfigElement("license")
                 .orElse("");
         this.showAsResourcePack = config.<Boolean>getConfigElement("showAsResourcePack")
@@ -76,12 +94,14 @@ public class ModFileInfo implements IModFileInfo, IConfigurable
                 .orElse(false);
         this.usesServices = config.<List<String>>getConfigElement("services")
                 .orElse(List.of());
-        this.properties = config.<Map<String, Object>>getConfigElement("properties")
-                .orElse(Collections.emptyMap());
-        this.modFile.setFileProperties(this.properties);
         this.issueURL = config.<String>getConfigElement("issueTrackerURL")
                 .map(StringUtils::toURL)
                 .orElse(null);
+
+        if (notAForgeMod) {
+            this.mods = List.of();
+            return;
+        }
         final List<? extends IConfigurable> modConfigs = config.getConfigList("mods");
         if (modConfigs.isEmpty())
         {

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModFileParser.java

@@ -13,6 +13,7 @@ import net.minecraftforge.fml.loading.LogMarkers;
 import net.minecraftforge.forgespi.language.IModFileInfo;
 import net.minecraftforge.forgespi.locating.IModFile;
 import net.minecraftforge.forgespi.locating.ModFileFactory;
+import org.jetbrains.annotations.Nullable;
 import org.slf4j.Logger;
 
 import java.io.IOException;
@@ -30,7 +31,7 @@ public class ModFileParser {
         return parser.build(modFile);
     }
 
-    public static IModFileInfo modsTomlParser(final IModFile imodFile) {
+    public static @Nullable IModFileInfo modsTomlParser(final IModFile imodFile) {
         ModFile modFile = (ModFile) imodFile;
         LOGGER.debug(LogMarkers.LOADING,"Considering mod file candidate {}", modFile.getFilePath());
         final Path modsjson = modFile.findResource("META-INF", "mods.toml");

fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModValidator.java

@@ -17,6 +17,7 @@ import net.minecraftforge.fml.loading.LogMarkers;
 import net.minecraftforge.fml.loading.ModSorter;
 import net.minecraftforge.forgespi.language.IModFileInfo;
 import net.minecraftforge.forgespi.locating.IModFile;
+import net.minecraftforge.forgespi.locating.ModFileLoadingException;
 import org.jetbrains.annotations.NotNull;
 import org.slf4j.Logger;
 
@@ -33,15 +34,21 @@ public class ModValidator {
     private LoadingModList loadingModList;
     private final List<IModFile> brokenFiles = new ArrayList<>();
     private final List<EarlyLoadingException.ExceptionData> discoveryErrorData;
+    private final List<ModFileLoadingException> modFileLoadingExceptions;
 
-    public ModValidator(Map<IModFile.Type, List<ModFile>> modFiles, List<IModFileInfo> brokenFiles, List<EarlyLoadingException.ExceptionData> discoveryErrorData) {
+    public ModValidator(Map<IModFile.Type, List<ModFile>> modFiles, List<IModFileInfo> brokenFiles, List<EarlyLoadingException.ExceptionData> discoveryErrorData, List<ModFileLoadingException> modFileLoadingExceptions) {
         this.candidateMods = lst(modFiles.get(IModFile.Type.MOD));
         this.gameLibraries = lst(modFiles.get(IModFile.Type.GAMELIBRARY));
         this.candidateMods.addAll(this.gameLibraries);
         this.candidatePlugins = lst(modFiles.get(IModFile.Type.LANGPROVIDER));
         this.candidatePlugins.addAll(lst(modFiles.get(IModFile.Type.LIBRARY)));
         this.discoveryErrorData = discoveryErrorData;
-        brokenFiles.stream().map(IModFileInfo::getFile).forEach(this.brokenFiles::add);
+        this.brokenFiles.addAll(brokenFiles.stream().map(IModFileInfo::getFile).toList());
+        this.modFileLoadingExceptions = modFileLoadingExceptions;
+    }
+
+    public ModValidator(Map<IModFile.Type, List<ModFile>> modFiles, List<IModFileInfo> brokenFiles, List<EarlyLoadingException.ExceptionData> discoveryErrorData) {
+        this(modFiles, brokenFiles, discoveryErrorData, List.of());
     }
 
     private static List<ModFile> lst(List<ModFile> files) {