-
-
Notifications
You must be signed in to change notification settings - Fork 390
Function overloading #7757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev/feature
Are you sure you want to change the base?
Function overloading #7757
Conversation
How? |
src/main/java/ch/njol/skript/lang/function/FunctionRegistry.java
Outdated
Show resolved
Hide resolved
I was thinking literal type clarification but I guess that's only for literals |
You could evaluate at runtime, which could be very flexible but also possibly confusing to users in some cases. |
I think I prefer a more limited system that errors at parse time over one that may cause runtime errors. Wdyt? |
Why not just use ExprValueWithin to clarify the type? |
ahh good point |
I am really hyped for this one ngl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preliminary review
src/main/java/ch/njol/skript/lang/function/DynamicFunctionReference.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/lang/function/FunctionReference.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/lang/function/FunctionReference.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/lang/function/FunctionReference.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/lang/function/FunctionRegistry.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/lang/function/FunctionRegistry.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/lang/function/FunctionRegistry.java
Outdated
Show resolved
Hide resolved
* @param name The name of the function. | ||
* @param args The arguments of the function. | ||
*/ | ||
record FunctionIdentifier(@NotNull String name, boolean local, int minArgCount, Class<?>... args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale behind making this class instead of just using/upgrading Signature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can replace this to use signature in the pr for named function arguments (fun(x:1,y:2)
). changing it now would mean breaking changes for signature now (since hashCode and equals behaviour would differ) and then also for when i add named function arguments, as the methods would then use a different object for arguments/parameters than just Class<?> varargs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would hashcode/equals behavior change due to this PR? Moreover, why would it be a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently, Signature's hash is only generated based on its name. if we want to be able to have multiple functions with the same name but different arguments, then Signature's hash would have to include these argument types. therefore, someone who is adding Signatures to a set may find that they can suddenly add multiple with the same name, which may not be intended behaviour. this is niche, obviously, but still.
after looking at it further, it seems implementing equals wouldn't lead to any issues, nor would implementing named function arguments be a breaking change.
the problem is that constructing a Signature for getting a function requires single
to be known. therefore, you'd need to know whether the function returned a single value before you can search for the function. the same applies to Parameter.
I could make it use Signature, but that'd require just setting the value to false or true.
FunctionIdentifier is basically a representation of the stuff we have available at runtime to find the actual registered Signature. it is therefore an incomplete version of a Signature.
…ature/fun-overloading
Description
Adds function overloading.
Function overloading allows two functions with the same name to be registered, as long as
An example:
FunctionRegistry
Description
To accomplish this, a new
FunctionRegistry
class has been added, which is only visible to thefunction
package. This holds all signatures and functions by a namespace. A namespace is either the global namespace, for global functions, or a script, in which local functions can be registered.Functions and signatures are differentiated by a
FunctionIdentifier
, which holds the function's name and the types of its arguments.Why are the methods in the Functions class not deprecated?
In the future, I plan on adding function parameter specification by name, e.g.
location(x=10,y=10,z=10,world="world")
. When that has been added, there will, in my opinion, be enough future-proofing, thus the FunctionRegistry class can replace the Functions class' functionality. Overloaded functions will not be visible to Functions.Behaviour
When a function has an ambiguous type in one of its arguments, it will attempt to match based on the other arguments.
Therefore,
overloaded(1, {_none})
andoverloaded({_none}, 2)
will still call the correctoverloaded
, as the functionoverloaded(int, int)
is the only one that can be matched here.When a function has multiple implementations, and it can't be decided which one to use (due to variables for example), a parse-time error will be thrown.
Like other languages, functions cannot be differentiated by return type.
Target Minecraft Versions: any
Requirements: none
Related Issues: #1185, #2993