Fix bad FunctionRegistry#remove logic (#8015)

* fix bad remove logic

* update to pr

Author: Efnilite

src/main/java/ch/njol/skript/lang/function/FunctionRegistry.java

@@ -64,21 +64,34 @@ public static FunctionRegistry getRegistry() {
 
 	/**
 	 * Registers a signature.
+	 * <p>
+	 * Attempting to register a local signature in the global namespace, or a global signature in
+	 * a local namespace, will throw an {@link IllegalArgumentException}.
+	 * </p>
 	 *
 	 * @param namespace The namespace to register the signature in.
 	 *                  If namespace is null, will register this signature globally.
 	 *                  Usually represents the path of the script this signature is registered in.
 	 * @param signature The signature to register.
 	 * @throws SkriptAPIException if a signature with the same name and parameters is already registered
 	 *                            in this namespace.
+	 * @throws IllegalArgumentException if the signature is global and namespace is not null, or
+	 *                                  if the signature is local and namespace is null.
 	 */
 	public void register(@Nullable String namespace, @NotNull Signature<?> signature) {
 		Preconditions.checkNotNull(signature, "signature cannot be null");
+		if (signature.isLocal() && namespace == null) {
+			throw new IllegalArgumentException("Cannot register a local signature in the global namespace");
+		}
+		if (!signature.isLocal() && namespace != null) {
+			throw new IllegalArgumentException("Cannot register a global signature in a local namespace");
+		}
+
 		Skript.debug("Registering signature '%s'", signature.getName());
 
 		// namespace
 		NamespaceIdentifier namespaceId;
-		if (namespace != null && signature.isLocal()) {
+		if (namespace != null) {
 			namespaceId = new NamespaceIdentifier(namespace);
 		} else {
 			namespaceId = GLOBAL_NAMESPACE;
@@ -118,17 +131,29 @@ public void register(@NotNull Function<?> function) {
 
 	/**
 	 * Registers a function.
+	 * <p>
+	 * Attempting to register a local function in the global namespace, or a global function in
+	 * a local namespace, will throw an {@link IllegalArgumentException}.
+	 * </p>
 	 *
 	 * @param namespace The namespace to register the function in.
-	 *                  If namespace is null, will register this function globally.
+	 *                  If namespace is null, will register this function globally, only if the function is global.
 	 *                  Usually represents the path of the script this function is registered in.
 	 * @param function  The function to register.
-	 * @throws SkriptAPIException if the function name is invalid or if
-	 *                            a function with the same name and parameters is already registered
-	 *                            in this namespace.
+	 * @throws SkriptAPIException       if the function name is invalid or if
+	 *                                  a function with the same name and parameters is already registered
+	 *                                  in this namespace.
+	 * @throws IllegalArgumentException if the function is global and namespace is not null, or
+	 *                                  if the function is local and namespace is null.
 	 */
 	public void register(@Nullable String namespace, @NotNull Function<?> function) {
 		Preconditions.checkNotNull(function, "function cannot be null");
+		if (function.getSignature().isLocal() && namespace == null) {
+			throw new IllegalArgumentException("Cannot register a local function in the global namespace");
+		}
+		if (!function.getSignature().isLocal() && namespace != null) {
+			throw new IllegalArgumentException("Cannot register a global function in a local namespace");
+		}
 		Skript.debug("Registering function '%s'", function.getName());
 
 		String name = function.getName();
@@ -138,7 +163,7 @@ public void register(@Nullable String namespace, @NotNull Function<?> function)
 
 		// namespace
 		NamespaceIdentifier namespaceId;
-		if (namespace != null && function.getSignature().isLocal()) {
+		if (namespace != null) {
 			namespaceId = new NamespaceIdentifier(namespace);
 		} else {
 			namespaceId = GLOBAL_NAMESPACE;
@@ -487,8 +512,13 @@ public void remove(@NotNull Signature<?> signature) {
 		String name = signature.getName();
 		FunctionIdentifier identifier = FunctionIdentifier.of(signature);
 
-		Namespace namespace = namespaces.getOrDefault(new NamespaceIdentifier(signature.script),
-			namespaces.get(GLOBAL_NAMESPACE));
+		Namespace namespace;
+		if (signature.isLocal()) {
+			namespace = namespaces.get(new NamespaceIdentifier(signature.script));
+		} else {
+			namespace = namespaces.get(GLOBAL_NAMESPACE);
+		}
+
 		if (namespace == null) {
 			return;
 		}
@@ -532,6 +562,7 @@ private record NamespaceIdentifier(@Nullable String name) {
 
 		/**
 		 * Returns whether this identifier is for local namespaces.
+		 *
 		 * @return Whether this identifier is for local namespaces.
 		 */
 		public boolean local() {

src/main/java/ch/njol/skript/lang/function/Functions.java

@@ -104,7 +104,11 @@ public static JavaFunction<?> registerFunction(JavaFunction<?> function) {
 			namespace.addFunction(function);
 		}
 
-		FunctionRegistry.getRegistry().register(script.getConfig().getFileName(), function);
+		if (function.getSignature().isLocal()) {
+			FunctionRegistry.getRegistry().register(script.getConfig().getFileName(), function);
+		} else {
+			FunctionRegistry.getRegistry().register(null, function);
+		}
 
 		return function;
 	}

src/test/java/ch/njol/skript/lang/function/FunctionRegistryTest.java

@@ -388,4 +388,35 @@ public void testIdentifierSignatureOf() {
 		assertEquals(FunctionIdentifier.of(function2.getSignature()), identifier);
 	}
 
+	// see https://github.com/SkriptLang/Skript/pull/8015
+	@Test
+	public void testRemoveGlobalScriptFunctions8015() {
+		// create empty TEST_SCRIPT namespace such that it is not null
+		registry.register(TEST_SCRIPT, LOCAL_TEST_FUNCTION);
+		registry.remove(LOCAL_TEST_FUNCTION.getSignature());
+
+		assertEquals(RetrievalResult.NOT_REGISTERED, registry.getSignature(TEST_SCRIPT, FUNCTION_NAME).result());
+
+		// construct a global function with a non-null script, which happens in script functions
+		Signature<Boolean> signature = new Signature<>(TEST_SCRIPT, FUNCTION_NAME, new Parameter<?>[0],
+			false, DefaultClasses.BOOLEAN, true, "");
+		SimpleJavaFunction<Boolean> fn = new SimpleJavaFunction<>(signature) {
+			@Override
+			public Boolean @Nullable [] executeSimple(Object[][] params) {
+				return new Boolean[] { true };
+			}
+		};
+
+		// ensure new behaviour
+		assertThrows(IllegalArgumentException.class, () -> registry.register(TEST_SCRIPT, fn));
+
+		registry.register(null, fn);
+
+		assertEquals(RetrievalResult.EXACT, registry.getSignature(null, FUNCTION_NAME).result());
+
+		registry.remove(signature);
+
+		assertEquals(RetrievalResult.NOT_REGISTERED, registry.getSignature(null, FUNCTION_NAME).result());
+	}
+
 }