Merge branch 'dev/patch' into dev/feature

Author: APickledWalrus

build.gradle

@@ -4,7 +4,7 @@ import org.apache.tools.ant.filters.ReplaceTokens
 import java.time.LocalTime
 
 plugins {
-	id 'com.gradleup.shadow' version '9.4.2'
+	id 'com.gradleup.shadow' version '9.4.3'
 	id 'maven-publish'
 	id 'java'
 	id 'checkstyle'

gradle.properties

@@ -5,7 +5,7 @@ org.gradle.parallel=true
 
 groupid=ch.njol
 name=skript
-version=2.15.3
+version=2.15.4
 jarName=Skript.jar
 testEnv=java25/paper-26.1.2
 testEnvJavaVersion=25

src/main/java/ch/njol/skript/ScriptLoader.java

@@ -4,7 +4,6 @@
 import ch.njol.skript.config.Node;
 import ch.njol.skript.config.SectionNode;
 import ch.njol.skript.config.SimpleNode;
-import ch.njol.skript.events.bukkit.PreScriptLoadEvent;
 import ch.njol.skript.lang.*;
 import ch.njol.skript.lang.parser.ParserInstance;
 import ch.njol.skript.log.CountingLogHandler;
@@ -21,6 +20,7 @@
 import ch.njol.util.StringUtils;
 import org.bukkit.Bukkit;
 import org.jetbrains.annotations.ApiStatus;
+import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.annotations.UnknownNullability;
 import org.skriptlang.skript.bukkit.text.TextComponentParser;
@@ -111,7 +111,7 @@ private static ParserInstance getParser() {
 	 * All loaded scripts.
 	 */
 	@SuppressWarnings("null")
-	private static final Set<Script> loadedScripts = Collections.synchronizedSortedSet(new TreeSet<>(new Comparator<Script>() {
+	private static final Set<Script> loadedScripts = Collections.synchronizedSortedSet(new TreeSet<>(new Comparator<>() {
 		@Override
 		public int compare(Script s1, Script s2) {
 			File f1 = s1.getConfig().getFile();
@@ -157,6 +157,13 @@ private boolean isSubDir(File directory, File subDir) {
 	public static Script getScript(File file) {
 		if (!file.isFile())
 			throw new IllegalArgumentException("Something other than a file was provided.");
+		try {
+			file = file.getCanonicalFile();
+		} catch (IOException e) {
+			//noinspection ThrowableNotThrown
+			Skript.exception(e, "An exception occurred while trying to get the canonical file of: " + file);
+			return null;
+		}
 		for (Script script : loadedScripts) {
 			if (file.equals(script.getConfig().getFile()))
 				return script;
@@ -284,7 +291,6 @@ public static boolean isParallel() {
 
 	/**
 	 * Returns the executor used for submitting tasks based on the user config.sk settings.
-	 * 
 	 * The thread count will be based on the value of {@link #asyncLoaderSize}.
 	 * <p>
 	 * You may also use class {@link ch.njol.skript.util.Task} and the appropriate constructor
@@ -319,7 +325,7 @@ public static void setAsyncLoaderSize(int size) throws IllegalStateException {
 
 		// Remove threads
 		while (loaderThreads.size() > size) {
-			AsyncLoaderThread thread = loaderThreads.remove(loaderThreads.size() - 1);
+			AsyncLoaderThread thread = loaderThreads.removeLast();
 			thread.cancelExecution();
 		}
 		// Add threads
@@ -334,7 +340,7 @@ public static void setAsyncLoaderSize(int size) throws IllegalStateException {
 			private final AtomicInteger threadId = new AtomicInteger(0);
 
 			@Override
-			public Thread newThread(Runnable runnable) {
+			public Thread newThread(@NotNull Runnable runnable) {
 				Thread thread = new Thread(asyncLoaderThreadGroup, runnable, "Skript async loaders thread " + threadId.incrementAndGet());
 				thread.setDaemon(true);
 				return thread;
@@ -447,6 +453,26 @@ private static <T> CompletableFuture<T> makeFuture(Supplier<T> supplier, OpenClo
 	 * Script Loading Methods
 	 */
 
+	private static final Set<File> trackedFiles = new HashSet<>();
+
+	/**
+	 * Tracks that a file should be considered as loading.
+	 * @param file The file to track.
+	 * @return Whether this file was already being tracked.
+	 */
+	private static boolean track(File file) {
+		return !trackedFiles.add(file);
+	}
+
+	/**
+	 * Stops tracking that a script (its file) should be considered as loading.
+	 * @param script The script to stop tracking.
+	 */
+	private static void untrack(Script script) {
+		// Loaded scripts are created with canonical files
+		trackedFiles.remove(script.getConfig().getFile());
+	}
+
 	/**
 	 * Loads the Script present at the file using {@link #loadScripts(List, OpenCloseable)},
 	 * 	sending info/error messages when done.
@@ -483,15 +509,14 @@ public static CompletableFuture<ScriptInfo> loadScripts(Set<File> files, OpenClo
 	 *  and closed after the {@link Structure#postLoad()} stage.
 	 * @return Info on the loaded scripts.
 	 */
-	@SuppressWarnings("removal")
 	private static CompletableFuture<ScriptInfo> loadScripts(List<Config> configs, OpenCloseable openCloseable) {
 		if (configs.isEmpty()) // Nothing to load
 			return CompletableFuture.completedFuture(new ScriptInfo());
 
 		eventRegistry().events(ScriptPreInitEvent.class)
 				.forEach(event -> event.onPreInit(configs));
-		//noinspection deprecation - we still need to call it
-		Bukkit.getPluginManager().callEvent(new PreScriptLoadEvent(configs));
+		//noinspection removal - we still need to call it
+		Bukkit.getPluginManager().callEvent(new ch.njol.skript.events.bukkit.PreScriptLoadEvent(configs));
 
 		ScriptInfo scriptInfo = new ScriptInfo();
 
@@ -513,7 +538,7 @@ private static CompletableFuture<ScriptInfo> loadScripts(List<Config> configs, O
 		}
 
 		return CompletableFuture.allOf(scriptInfoFutures.toArray(new CompletableFuture[0]))
-			.thenApply(unused -> {
+			.thenApply(ignored -> {
 				// TODO in the future this won't work when parallel loading is fixed
 				// It does now though so let's avoid calling getParser() a bunch.
 				ParserInstance parser = getParser();
@@ -630,28 +655,18 @@ record LoadingStructure (LoadingScriptInfo loadingScriptInfo, Structure structur
 				} finally {
 					parser.setInactive();
 
+					for (LoadingScriptInfo info : scripts) {
+						untrack(info.script);
+					}
+
 					openCloseable.close();
 				}
 			}).exceptionally(t -> {
 				throw Skript.exception(t);
 			});
 	}
 
-	private static class LoadingScriptInfo {
-
-		public final Script script;
-
-		public final List<Structure> structures;
-
-		public final Map<Structure, Node> nodeMap;
-
-		public LoadingScriptInfo(Script script, List<Structure> structures, Map<Structure, Node> nodeMap) {
-			this.script = script;
-			this.structures = structures;
-			this.nodeMap = nodeMap;
-		}
-
-	}
+	private record LoadingScriptInfo(Script script, List<Structure> structures, Map<Structure, Node> nodeMap) { }
 
 	/**
 	 * Creates a script and loads the provided config into it.
@@ -810,6 +825,8 @@ private static Config loadStructure(File file) {
 			return null;
 		}
 
+		track(file);
+
 		try {
 			String name = Skript.getInstance().getDataFolder().toPath().toAbsolutePath()
 					.resolve(Skript.SCRIPTSFOLDER).relativize(file.toPath().toAbsolutePath()).toString();
@@ -857,6 +874,10 @@ private static Config loadStructure(InputStream source, String name) {
 	 *         This data is calculated by using {@link ScriptInfo#add(ScriptInfo)}.
 	 */
 	public static ScriptInfo unloadScripts(Set<Script> scripts) {
+		scripts = scripts.stream()
+			.filter(script -> !track(script.getConfig().getFile()))
+			.collect(Collectors.toSet());
+
 		// ensure unloaded scripts are not being unloaded
 		for (Script script : scripts) {
 			if (!loadedScripts.contains(script))
@@ -920,6 +941,8 @@ record UnloadingStructure (Script script, Structure structure) {}
 			File scriptFile = script.getConfig().getFile();
 			assert scriptFile != null;
 			disabledScripts.add(new File(scriptFile.getParentFile(), DISABLED_SCRIPT_PREFIX + scriptFile.getName()));
+
+			untrack(script);
 		}
 
 		return info;
@@ -1041,6 +1064,7 @@ public static ArrayList<TriggerItem> loadItems(SectionNode node) {
 				items.add(item);
 			} else if (subNode instanceof SectionNode subSection) {
 
+				//noinspection resource - manual management is intentional
 				RetainingLogHandler handler = SkriptLogger.startRetainingLog();
 				find_section:
 				try {
@@ -1177,15 +1201,15 @@ public static Script createDummyScript(String name, @Nullable File file) {
 	 * Any changes to loaded scripts will not be reflected in the returned set.
 	 */
 	public static Set<Script> getLoadedScripts() {
-		return Collections.unmodifiableSet(new HashSet<>(loadedScripts));
+		return Set.copyOf(loadedScripts);
 	}
 
 	/**
 	 * @return An unmodifiable set containing a snapshot of the currently disabled scripts.
 	 * Any changes to disabled scripts will not be reflected in the returned set.
 	 */
 	public static Set<File> getDisabledScripts() {
-		return Collections.unmodifiableSet(new HashSet<>(disabledScripts));
+		return Set.copyOf(disabledScripts);
 	}
 
 	/**
@@ -1316,7 +1340,7 @@ public static File getScriptFromName(String script, File directory) {
 			script = script.replace('/', File.separatorChar).replace('\\', File.separatorChar);
 		} else if (!StringUtils.endsWithIgnoreCase(script, ".sk")) {
 			int dot = script.lastIndexOf('.');
-			if (dot > 0 && !script.substring(dot + 1).equals(""))
+			if (dot > 0 && !script.substring(dot + 1).isEmpty())
 				return null;
 			script = script + ".sk";
 		}

src/main/java/ch/njol/skript/conditions/CondScriptLoaded.java

@@ -62,7 +62,11 @@ public boolean check(Event event) {
 			return ScriptLoader.getLoadedScripts().contains(currentScript) ^ isNegated();
 		return scripts.check(event, scriptName -> {
 			File scriptFile = ScriptLoader.getScriptFromName(scriptName);
-			return scriptFile != null && ScriptLoader.getLoadedScripts().contains(ScriptLoader.getScript(scriptFile));
+			if (scriptFile == null) {
+				return false;
+			}
+			Script script = ScriptLoader.getScript(scriptFile);
+			return script != null && ScriptLoader.getLoadedScripts().contains(script);
 		}, isNegated());
 	}
 

src/main/java/ch/njol/skript/effects/EffScriptFile.java

@@ -1,6 +1,7 @@
 package ch.njol.skript.effects;
 
 import ch.njol.skript.ScriptLoader;
+import ch.njol.skript.ScriptLoader.ScriptInfo;
 import ch.njol.skript.Skript;
 import ch.njol.skript.SkriptCommand;
 import ch.njol.skript.command.ScriptCommand;
@@ -123,7 +124,8 @@ private void handle(@Nullable File scriptFile, @Nullable String name, OpenClosea
 		FileFilter filter = ScriptLoader.getDisabledScriptsFilter();
 		switch (mark) {
 			case ENABLE:
-				if (ScriptLoader.getLoadedScripts().contains(ScriptLoader.getScript(scriptFile)))
+				Script script = ScriptLoader.getScript(scriptFile);
+				if (script != null && ScriptLoader.getLoadedScripts().contains(script))
 					return;
 				if (filter.accept(scriptFile)) {
 					try {
@@ -148,7 +150,9 @@ private void handle(@Nullable File scriptFile, @Nullable String name, OpenClosea
 				if (filter.accept(scriptFile))
 					return;
 
-				this.unloadScripts(scriptFile);
+				if (!this.unloadScripts(scriptFile)) {
+					return;
+				}
 
 				ScriptLoader.loadScripts(scriptFile, openCloseable);
 				break;
@@ -164,7 +168,9 @@ private void handle(@Nullable File scriptFile, @Nullable String name, OpenClosea
 				if (filter.accept(scriptFile))
 					return;
 
-				this.unloadScripts(scriptFile);
+				if (!this.unloadScripts(scriptFile)) {
+					return;
+				}
 
 				try {
 					FileUtils.move(
@@ -183,21 +189,25 @@ private void handle(@Nullable File scriptFile, @Nullable String name, OpenClosea
 		}
 	}
 
-	private void unloadScripts(File file) {
-		Set<Script> loaded = ScriptLoader.getLoadedScripts();
+	private boolean unloadScripts(File file) {
 		if (file.isDirectory()) {
-			Set<Script> scripts = ScriptLoader.getScripts(file);
-			if (scripts.isEmpty())
-				return;
-			scripts.retainAll(loaded); // skip any that are not loaded (avoid throwing error)
-			ScriptLoader.unloadScripts(scripts);
-		} else {
-			Script script = ScriptLoader.getScript(file);
-			if (!loaded.contains(script))
-				return; // don't need to unload if not loaded (avoid throwing error)
-			if (script != null)
-				ScriptLoader.unloadScript(script);
+			ScriptInfo info = ScriptLoader.unloadScripts(ScriptLoader.getScripts(file));
+			if (info.files == 0) {
+				error("Failed to unload one or more scripts within '" + file.getName() + "'. Were they already loading or unloading?");
+				return false;
+			}
+			return true;
+		}
+		Script script = ScriptLoader.getScript(file);
+		if (script == null) {
+			return true; // we want the file to still be marked disabled
 		}
+		ScriptInfo info = ScriptLoader.unloadScript(script);
+		if (info.files == 0) {
+			error("Failed to unload the script '" + script.nameAndPath() + "'. Was it already loading or unloading?");
+			return false;
+		}
+		return true;
 	}
 
 	@Override

src/main/java/org/skriptlang/skript/bukkit/entity/player/elements/effects/EffBan.java

@@ -77,7 +77,7 @@ public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelaye
 
 	@Override
 	protected void execute(Event event) {
-		Component reason = this.reason == null ? null : this.reason.getSingle(event); // don't check for null, just ignore an invalid reason
+		Component reason = this.reason == null ? null : this.reason.getSingle(event);
 		Timespan duration = this.expires == null ? null : this.expires.getSingle(event);
 		Date expires = duration == null ? null : new Date(System.currentTimeMillis() + duration.getAs(TimePeriod.MILLISECOND));
 		for (Object object : players.getArray(event)) {
@@ -91,16 +91,14 @@ protected void execute(Event event) {
 						String ip = address.getAddress().getHostAddress();
 						var banList = Bukkit.getBanList(Type.IP);
 						if (ban) {
-							String legacyReason = TextComponentParser.instance().toLegacyString(reason);
-							banList.addBan(ip, legacyReason, expires, SKRIPT_BAN_SOURCE);
+							banList.addBan(ip, toLegacyString(reason), expires, SKRIPT_BAN_SOURCE);
 						} else {
 							banList.pardon(ip);
 						}
 					} else {
 						var banList = Bukkit.getBanList(Type.NAME);
 						if (ban) {
-							String legacyReason = TextComponentParser.instance().toLegacyString(reason);
-							banList.addBan(player.getName(), legacyReason, expires, SKRIPT_BAN_SOURCE); // FIXME [UUID] ban UUID
+							banList.addBan(player.getName(), toLegacyString(reason), expires, SKRIPT_BAN_SOURCE); // FIXME [UUID] ban UUID
 						} else {
 							banList.pardon(player.getName());
 						}
@@ -116,8 +114,7 @@ protected void execute(Event event) {
 					}
 					var banList = Bukkit.getBanList(Type.NAME);
 					if (ban) {
-						String legacyReason = TextComponentParser.instance().toLegacyString(reason);
-						banList.addBan(name, legacyReason, expires, SKRIPT_BAN_SOURCE);
+						banList.addBan(name, toLegacyString(reason), expires, SKRIPT_BAN_SOURCE);
 					} else {
 						banList.pardon(name);
 					}
@@ -126,7 +123,7 @@ protected void execute(Event event) {
 					var ipBanList = Bukkit.getBanList(Type.IP);
 					var nameBanList = Bukkit.getBanList(Type.NAME);
 					if (ban) {
-						String legacyReason = TextComponentParser.instance().toLegacyString(reason);
+						String legacyReason = toLegacyString(reason);
 						ipBanList.addBan(ip, legacyReason, expires, SKRIPT_BAN_SOURCE);
 						nameBanList.addBan(ip, legacyReason, expires, SKRIPT_BAN_SOURCE);
 					} else {
@@ -151,4 +148,11 @@ public String toString(@Nullable Event event, boolean debug) {
 			.toString();
 	}
 
+	private static @Nullable String toLegacyString(@Nullable Component component) {
+		if (component == null) {
+			return null;
+		}
+		return TextComponentParser.instance().toLegacyString(component);
+	}
+
 }